Reduce redundant siteSort to sort once and used everywhere #8075

darkdh · 2017-04-05T03:47:06Z

fix #7240

Auditors: @bbondy, @bsclifton

Test Plan:

Import bunch of bookmarks
Open bookmarks of menu shouldn't affect performance
Open about:bookmarks shouldn't affect performance

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).

bbondy · 2017-04-17T16:22:41Z

++ please rebase and merge

darkdh · 2017-04-17T18:48:35Z

rebased. please review again.

darkdh · 2017-04-18T02:01:41Z

js/stores/appStore.js

-      const d = diff(lastEmittedState, appState)
+      // diff and patch will make map out of order so toList() is to maintain
+      // sites map order
+      const diffAppState = appState.set('sites', appState.get('sites').toList())


We need a better way to deal with the out of order.
This will cause significant slowness

bsclifton · 2017-04-29T00:16:28Z

@darkdh is this still a WIP? Sorry for not reviewing earlier ☹️

darkdh · 2017-04-29T00:45:57Z

sorry, this is waiting for shared app state change landed

NejcZdovc · 2017-05-30T22:26:54Z

let's wait for tests, but on a first glance it looks good to me

darkdh · 2017-05-31T18:21:15Z

There are two failed test I need to look into

tab tests tab order with pinned tabs sequentially by default
sitesReducerTest APP_MOVE_SITE Moves the specified site

darkdh · 2017-05-31T20:03:27Z

fix sitesReducerTest APP_MOVE_SITE Moves the specified site
and tab tests tab order with pinned tabs sequentially by default runs correctly on my local env

NejcZdovc · 2017-05-31T20:13:36Z

test/unit/app/browser/reducers/sitesReducerTest.js

@@ -149,8 +149,8 @@ describe('sitesReducerTest', function () {
      moveAction.prepend = true
      newState = sitesReducer(Immutable.fromJS(newState), moveAction).toJS()
      assert.equal(Object.keys(newState.sites).length, 3)
-      assert.equal(Object.values(newState.sites)[2].location, url)
-      assert.equal(Object.values(newState.sites)[2].order, 1)
+      assert.equal(Object.values(newState.sites)[1].location, url)


Is this correct change, because the comment at the top states that site should be at the third position (2), but you changed this value to 1

Yes, we will sort sites after ADD, REMOVE, MOVE in this PR

If this is correct then I would add some description about it beside this comment, otherwise it could be confusing

comment added

bbondy · 2017-06-11T04:57:53Z

Please rebase again @darkdh
Thanks!

darkdh · 2017-06-11T19:32:46Z

@bbondy, rebased!

@bbondy

fix #7240 Auditors: @bbondy, @bsclifton Test Plan: 1. Import bunch of bookmarks 2. Open bookmarks of menu shouldn't affect performance 3. Open about:bookmarks shouldn't affect performance

Reduce redundant siteSort to sort once and used everywhere

@bbondy

Auditors: @bbondy, @NejcZdovc

@bbondy

Auditors: @bbondy, @NejcZdovc

@bbondy

Auditors: @bbondy, @NejcZdovc

@bbondy

Auditors: @bbondy, @NejcZdovc

darkdh added feature/bookmarks perf labels Apr 5, 2017

darkdh self-assigned this Apr 5, 2017

darkdh requested review from bbondy and bsclifton April 5, 2017 03:47

cezaraugusto added the PR/needs-rebase ۞ label Apr 16, 2017

darkdh force-pushed the 7240 branch from 463524a to 8328d05 Compare April 17, 2017 18:47

luixxiul removed the PR/needs-rebase ۞ label Apr 17, 2017

NejcZdovc added the PR/needs-rebase ۞ label Apr 17, 2017

darkdh force-pushed the 7240 branch 3 times, most recently from e595fa3 to 302a543 Compare April 18, 2017 01:22

darkdh commented Apr 18, 2017

View reviewed changes

darkdh added PR/work-in-progress ⚒ and removed PR/needs-rebase ۞ labels Apr 18, 2017

bbondy force-pushed the master branch from f0df59d to 70e5d77 Compare April 23, 2017 04:21

srirambv mentioned this pull request Apr 29, 2017

Browser performance reduces when large amount of bookmarks are imported #7240

Closed

darkdh added PR/blocked and removed PR/work-in-progress ⚒ labels May 2, 2017

luixxiul added the PR/needs-rebase ۞ label May 6, 2017

darkdh force-pushed the 7240 branch from 302a543 to c631b5d Compare May 9, 2017 03:30

darkdh added PR/work-in-progress ⚒ and removed PR/blocked PR/needs-rebase ۞ labels May 9, 2017

darkdh force-pushed the 7240 branch 2 times, most recently from 299a076 to 4900648 Compare May 11, 2017 17:30

darkdh added PR/needs-QA-attention ☕ and removed PR/needs-rebase ۞ labels May 30, 2017

darkdh requested a review from NejcZdovc May 30, 2017 21:57

darkdh added PR/work-in-progress ⚒ and removed PR/needs-QA-attention ☕ labels May 31, 2017

darkdh force-pushed the 7240 branch from 4b19059 to 33f3622 Compare May 31, 2017 20:02

darkdh removed the PR/work-in-progress ⚒ label May 31, 2017

NejcZdovc reviewed May 31, 2017

View reviewed changes

darkdh force-pushed the 7240 branch from 33f3622 to 57ff66e Compare June 1, 2017 03:49

darkdh added the PR/needs-QA-attention ☕ label Jun 1, 2017

luixxiul added the PR/needs-rebase ۞ label Jun 4, 2017

darkdh force-pushed the 7240 branch from 57ff66e to 565ed03 Compare June 11, 2017 19:32

darkdh removed the PR/needs-rebase ۞ label Jun 11, 2017

Reduce redundant siteSort to sort once and used everywhere

8538025

fix #7240 Auditors: @bbondy, @bsclifton Test Plan: 1. Import bunch of bookmarks 2. Open bookmarks of menu shouldn't affect performance 3. Open about:bookmarks shouldn't affect performance

darkdh force-pushed the 7240 branch from 565ed03 to 8538025 Compare June 13, 2017 05:12

bbondy approved these changes Jun 13, 2017

View reviewed changes

bbondy merged commit 7ce06ee into master Jun 13, 2017

bbondy added a commit that referenced this pull request Jun 13, 2017

Merge pull request #8075 from brave/7240

8da1304

Reduce redundant siteSort to sort once and used everywhere

bbondy added a commit that referenced this pull request Jun 13, 2017

Merge pull request #8075 from brave/7240

83a50a3

Reduce redundant siteSort to sort once and used everywhere

darkdh added a commit that referenced this pull request Jun 13, 2017

Fix unit test failed introduced by #8075

5e16c50

Auditors: @bbondy, @NejcZdovc

NejcZdovc pushed a commit that referenced this pull request Jun 13, 2017

Fix unit test failed introduced by #8075

9ac39cd

Auditors: @bbondy, @NejcZdovc

NejcZdovc pushed a commit that referenced this pull request Jun 13, 2017

Fix unit test failed introduced by #8075

41353e6

Auditors: @bbondy, @NejcZdovc

bsclifton deleted the 7240 branch June 14, 2017 05:41

bridiver pushed a commit that referenced this pull request Jun 14, 2017

Fix unit test failed introduced by #8075

9f33dd1

Auditors: @bbondy, @NejcZdovc

NejcZdovc mentioned this pull request Jun 22, 2017

Converts bookmarksToolbar and bookmarkToolbarButton into redux #9030

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce redundant siteSort to sort once and used everywhere #8075

Reduce redundant siteSort to sort once and used everywhere #8075

darkdh commented Apr 5, 2017 •

edited

Loading

bbondy commented Apr 17, 2017

darkdh commented Apr 17, 2017

darkdh Apr 18, 2017 •

edited by NejcZdovc

Loading

bsclifton Apr 19, 2017

bsclifton commented Apr 29, 2017

darkdh commented Apr 29, 2017

NejcZdovc commented May 30, 2017

darkdh commented May 31, 2017

darkdh commented May 31, 2017

NejcZdovc May 31, 2017

darkdh May 31, 2017

NejcZdovc May 31, 2017

darkdh Jun 1, 2017

bbondy commented Jun 11, 2017

darkdh commented Jun 11, 2017

Reduce redundant siteSort to sort once and used everywhere #8075

Reduce redundant siteSort to sort once and used everywhere #8075

Conversation

darkdh commented Apr 5, 2017 • edited Loading

bbondy commented Apr 17, 2017

darkdh commented Apr 17, 2017

darkdh Apr 18, 2017 • edited by NejcZdovc Loading

Choose a reason for hiding this comment

bsclifton Apr 19, 2017

Choose a reason for hiding this comment

bsclifton commented Apr 29, 2017

darkdh commented Apr 29, 2017

NejcZdovc commented May 30, 2017

darkdh commented May 31, 2017

darkdh commented May 31, 2017

NejcZdovc May 31, 2017

Choose a reason for hiding this comment

darkdh May 31, 2017

Choose a reason for hiding this comment

NejcZdovc May 31, 2017

Choose a reason for hiding this comment

darkdh Jun 1, 2017

Choose a reason for hiding this comment

bbondy commented Jun 11, 2017

darkdh commented Jun 11, 2017

darkdh commented Apr 5, 2017 •

edited

Loading

darkdh Apr 18, 2017 •

edited by NejcZdovc

Loading