Improves perf of bookmark hanger #9710

NejcZdovc · 2017-06-26T08:09:11Z

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.

Resolves #9674
Possible fix for #9602
Fix #5314
Fix #5508
Fix #4978

Auditors: @bsclifton

Bookmark

try to add a bookmark
try to add a bookmark, but into the second level folder

Bookmark folder

try to add a bookmark folder

Bookmark

try to edit a bookmark

Bookmark folder

try to add a bookmark folder

Bookmark

try to remove a bookmark

Bookmark folder

try to remove a bookmark folder

Bookmark

try to add a bookmark into a folder (with a right click on a folder)
folder should be preselected

Bookmark folder

try to add a bookmark folder into a folder (with a right click on a folder)
folder should be preselected

Navigation bar

try to bookmark a page via navigation bar star

Navigation bar

try to edit a bookmark via navigation bar star

Bookmark manager

try to add a bookmark folder from a bookmark manager (list, folder tree and button)

Bookmark manager

try to add a bookmark from a bookmark manager (list, folder tree and button)

Bookmark manager

try to edit a bookmark from a bookmark manager (list and folder tree)

Bookmark manager

try to edit a bookmark folder from a bookmark manager (list and folder tree)

Perf

have a lot of bookmarks (1k +)
try to add a bookmark with clicking on the star

Perf

have a lot of bookmarks (1k +)
right click on bookmarks toolbar and click Add folder

Bookmark link

try to bookmark a link (right click on a link in the page)

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

NejcZdovc · 2017-06-26T08:15:47Z

Some comparison of the merge props compute time with @bsclifton's exported file:

Before this PR:

After this PR:

bsclifton

This is better for sure- but one of the main problems seems to be that every keydown is triggering a window action (which I think is unnecessary). Also, doing the punycode on each keystroke is expensive

NejcZdovc · 2017-06-30T19:18:27Z

@bsclifton what would you suggest?

We have couple of options.

debounce
blur event
save only when you click on save button
...

bsclifton · 2017-06-30T22:12:28Z

@NejcZdovc the save (or remove) when you click the button is the best option, IMO. But we'd still need a handler for if the button should be enabled or not (ex: is the length > 0 for title)

NejcZdovc · 2017-07-07T14:27:41Z

@bsclifton ok, now this PR is finally finished. I did complete rewrite of adding/editing bookmarks.

NejcZdovc · 2017-07-11T06:51:50Z

js/state/siteUtil.js

+  for (let i = 0; i < resultSize; i++) {
+    const site = results.get(i)
+    if (site.get('folderId') === folderId) {
+      return


Please change this from return to continue

bsclifton · 2017-07-11T07:02:11Z

app/browser/reducers/sitesReducer.js

+    case appConstants.APP_ADD_BOOKMARK:
+    case appConstants.APP_EDIT_BOOKMARK:
+      {
+        const isSyncEnabled = syncEnabled()


nitpick for this section- it has enough logic and branches (if/else) that it would be nice to pull it out to a function so that it can be easily tested

This section is fully covered with tests. I wouldn't move it into the function that is exportable. What we can do is move this block into the new function but keep it in the same file for internal use.

bsclifton · 2017-07-11T07:05:17Z

app/renderer/components/bookmarks/addEditBookmarkHanger.js

  }

  onClose () {
-    windowActions.setBookmarkDetail()
+    windowActions.onBookmarkClose()


So much nicer! Thanks for taking the time to properly name these 😄

bsclifton · 2017-07-11T07:06:25Z

docs/appActions.md

@@ -171,7 +171,7 @@ Notifies that a tab has been closed



-### addSite(siteDetail, tag, originalSiteDetail, destinationIsParent, skipSync) 
+### addSite(siteDetail, tag, skipSync) 


bsclifton · 2017-07-11T07:08:16Z

js/state/siteUtil.js

+  for (let i = 0; i < resultSize; i++) {
+    const site = results.get(i)
+    if (site.get('folderId') === folderId) {
+      return


@bsclifton

Resolves brave#9674 Auditors: @bsclifton Test Plan: - have a lot of bookmarks (1k +) - try to add a bookmark with clicking on the star

bsclifton

I found a few bugs which you had fixed @NejcZdovc 😄 Great job on this PR. Much needed overhaul on some of the older code and it makes a huge positive impact when users have a decent number of bookmarks. ++++!

NejcZdovc · 2017-07-11T10:08:46Z

Thank you very much for this late night review 😃

NejcZdovc added feature/bookmarks perf labels Jun 26, 2017

NejcZdovc added this to the 0.18.x (Developer Channel) milestone Jun 26, 2017

NejcZdovc self-assigned this Jun 26, 2017

NejcZdovc requested a review from bsclifton June 26, 2017 08:09

bsclifton force-pushed the 0.18.x branch from d59dda1 to 7aa5171 Compare June 28, 2017 06:11

bsclifton reviewed Jun 30, 2017

View reviewed changes

NejcZdovc mentioned this pull request Jul 3, 2017

Removes non-primitive types from NavigationBar #9792

Closed

8 tasks

NejcZdovc force-pushed the hotfix/#9674-bookmark branch from a432b54 to 6ed3238 Compare July 4, 2017 13:28

NejcZdovc changed the base branch from 0.18.x to master July 4, 2017 13:32

NejcZdovc modified the milestones: 0.19.x (Developer Channel), 0.18.x (Beta Channel) Jul 4, 2017

NejcZdovc force-pushed the hotfix/#9674-bookmark branch from 6ed3238 to 0bb5b9c Compare July 4, 2017 13:36

NejcZdovc added the PR/work-in-progress ⚒ label Jul 4, 2017

NejcZdovc force-pushed the hotfix/#9674-bookmark branch 13 times, most recently from 3f0712f to c27d482 Compare July 6, 2017 08:35

NejcZdovc force-pushed the hotfix/#9674-bookmark branch from 590acc4 to 7b60afd Compare July 7, 2017 14:25

NejcZdovc removed the PR/work-in-progress ⚒ label Jul 7, 2017

NejcZdovc requested a review from bsclifton July 7, 2017 14:26

NejcZdovc force-pushed the hotfix/#9674-bookmark branch 5 times, most recently from c0372f1 to 21e2b13 Compare July 11, 2017 06:15

NejcZdovc commented Jul 11, 2017

View reviewed changes

bsclifton mentioned this pull request Jul 11, 2017

Editing a bookmark (or folder) locks up the browser #9674

Closed

bsclifton force-pushed the hotfix/#9674-bookmark branch from bc05c17 to 8d2f527 Compare July 11, 2017 06:54

bsclifton reviewed Jul 11, 2017

View reviewed changes

NejcZdovc force-pushed the hotfix/#9674-bookmark branch 2 times, most recently from 40dbd62 to 8a38142 Compare July 11, 2017 07:28

Imporves perf of bookmark hanger

4538db1

Resolves brave#9674 Auditors: @bsclifton Test Plan: - have a lot of bookmarks (1k +) - try to add a bookmark with clicking on the star

NejcZdovc force-pushed the hotfix/#9674-bookmark branch from 8a38142 to 4538db1 Compare July 11, 2017 07:37

bsclifton approved these changes Jul 11, 2017

View reviewed changes

bsclifton added QA/test-plan-specified release-notes/include labels Jul 11, 2017

bsclifton merged commit c8e92a1 into brave:master Jul 11, 2017

luixxiul mentioned this pull request Jul 11, 2017

Refactor navigationBar with Aphrodite - master #9851

Closed

8 tasks

luixxiul mentioned this pull request Jul 22, 2017

Edited bookmark bugs tracking issue #9120

Closed

7 tasks

NejcZdovc mentioned this pull request Aug 10, 2017

Overall Bookmark Manager Improvement Tracking Issue #9312

Closed

15 tasks

NejcZdovc mentioned this pull request Sep 7, 2017

Bookmark toolbar is not hidden if you disable it #10828

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improves perf of bookmark hanger #9710

Improves perf of bookmark hanger #9710

NejcZdovc commented Jun 26, 2017 •

edited by luixxiul

Loading

NejcZdovc commented Jun 26, 2017 •

edited

Loading

bsclifton left a comment

NejcZdovc commented Jun 30, 2017

bsclifton commented Jun 30, 2017

NejcZdovc commented Jul 7, 2017

NejcZdovc Jul 11, 2017 •

edited

Loading

bsclifton Jul 11, 2017

bsclifton Jul 11, 2017

NejcZdovc Jul 11, 2017

bsclifton Jul 11, 2017

bsclifton Jul 11, 2017

bsclifton Jul 11, 2017

bsclifton left a comment

NejcZdovc commented Jul 11, 2017

		@@ -171,7 +171,7 @@ Notifies that a tab has been closed



		### addSite(siteDetail, tag, originalSiteDetail, destinationIsParent, skipSync)
		### addSite(siteDetail, tag, skipSync)

Improves perf of bookmark hanger #9710

Improves perf of bookmark hanger #9710

Conversation

NejcZdovc commented Jun 26, 2017 • edited by luixxiul Loading

NejcZdovc commented Jun 26, 2017 • edited Loading

bsclifton left a comment

Choose a reason for hiding this comment

NejcZdovc commented Jun 30, 2017

bsclifton commented Jun 30, 2017

NejcZdovc commented Jul 7, 2017

NejcZdovc Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

bsclifton Jul 11, 2017

Choose a reason for hiding this comment

bsclifton Jul 11, 2017

Choose a reason for hiding this comment

NejcZdovc Jul 11, 2017

Choose a reason for hiding this comment

bsclifton Jul 11, 2017

Choose a reason for hiding this comment

bsclifton Jul 11, 2017

Choose a reason for hiding this comment

bsclifton Jul 11, 2017

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

NejcZdovc commented Jul 11, 2017

NejcZdovc commented Jun 26, 2017 •

edited by luixxiul

Loading

NejcZdovc commented Jun 26, 2017 •

edited

Loading

NejcZdovc Jul 11, 2017 •

edited

Loading