Skip to content
This repository was archived by the owner on Dec 11, 2019. It is now read-only.

Optimize top site order lookups #9582

Merged
merged 1 commit into from
Jun 20, 2017
Merged

Optimize top site order lookups #9582

merged 1 commit into from
Jun 20, 2017

Conversation

bbondy
Copy link
Member

@bbondy bbondy commented Jun 19, 2017 •
edited by bsclifton
Loading

Fix #9581

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@bbondy bbondy added this to the 0.17.x (Beta Channel) milestone Jun 19, 2017
@bbondy bbondy self-assigned this Jun 19, 2017
bsclifton
bsclifton reviewed Jun 19, 2017
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbondy can you fill in a test plan? or if not needed, we can label as automated-tests and qa/no-qa-needed

test/unit/app/common/data/topSitesTest.js
assert.equal(getSiteOrder('calendar.google.com'), 4)
})
it('orders unknown sites as max int', function () {
assert.equal(getSiteOrder('bradhatesprimes.com'), 9007199254740991)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use Number.MAX_SAFE_INTEGER?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER

jonathansampson
jonathansampson approved these changes Jun 19, 2017
View reviewed changes
Copy link
Collaborator

@jonathansampson jonathansampson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to Clifton's comment on using MAX_SAFE

bsclifton
bsclifton approved these changes Jun 20, 2017
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides nit comment, looks good! 😄 Merging

@bsclifton bsclifton merged commit c43bfa4 into master Jun 20, 2017
@bsclifton bsclifton deleted the 9581 branch June 20, 2017 06:09
bsclifton added a commit that referenced this pull request Jun 20, 2017
@bsclifton
Merge pull request #9582 from brave/9581
b6d7b37 
Optimize top site order lookups
bsclifton added a commit that referenced this pull request Jun 20, 2017
@bsclifton
Merge pull request #9582 from brave/9581
a7bc130 
Optimize top site order lookups
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonathansampson jonathansampson jonathansampson approved these changes

@bsclifton bsclifton bsclifton approved these changes

Assignees

@bbondy bbondy

Labels
None yet
Projects
None yet
Milestone
0.17.13 (Release Channel)
Development

Successfully merging this pull request may close these issues.
3 participants
@bbondy @jonathansampson @bsclifton