Change sites from List to Map #5587

darkdh · 2016-11-14T00:42:52Z

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

Test Plan:

Auditors: @bbondy, @bsclifton, @cezaraugusto

darkdh · 2016-11-14T00:43:46Z

js/stores/appStore.js

@@ -439,10 +439,15 @@ const handleAppAction = (action) => {
          appState = appState.set('sites', siteUtil.addSite(appState.get('sites'), s, action.tag))
        })
      } else {
-        appState = appState.set('sites', siteUtil.addSite(appState.get('sites'), action.siteDetail, action.tag, action.originalSiteDetail))
+        let sites = appState.get('sites')
+        if (!action.siteDetail.get('folderId') && siteUtil.isFolder(action.siteDetail)) {


This is the root cause of last backed out
766722a#diff-23ca389e2bcb77191b5a9c10900eb3a3R443

bsclifton · 2016-11-14T04:44:20Z

Because this can cause issues with migrations, I wanted to propose a new data-structure for us to use in favor of the sites array and ask what everyone thinks?

We could still keep the migration code in there (so that it copies the old values over to the new structure), but instead of: appState.sites we could add a new field appState.siteMap. This way, there would be no impact to users if they downgrade. We'd then want to make sure that wiping history removes both the old/new fields too, of course

bbondy · 2016-11-15T21:39:07Z

This needs to be rebased

bbondy · 2016-11-18T16:27:51Z

Still needs to be rebased, I'll move to 1.0

darkdh · 2016-12-05T04:09:47Z

rebased! but still looking for the cause of multi-select on bookmark manager failure

darkdh · 2016-12-06T09:13:44Z

It's ready for review again

posix4e · 2016-12-21T00:53:08Z

@darkdh please rebase

darkdh · 2016-12-21T02:06:17Z

@posix4e , rebasing complete!

posix4e · 2016-12-21T02:08:27Z

@darkdh sorry to be a bother but how was this tested?

cezaraugusto · 2017-01-18T01:18:13Z

@darkdh when you're done can you provide some bulk bookmark file as your STR suggested? Mine don't reach 300 😭

darkdh · 2017-01-18T01:23:39Z

sent on slack 😄

cezaraugusto

Tested manually and everything worked as expected.
Noticed some delay regarding DnD for topSites on the new tab but root cause may be #6217.

However, webDriver tests didn't pass for me for sessionStore tests.

Update: Intermittent fail. Tested again and it worked.

cezaraugusto

Previous comment pointed to a failing on sessionStore test but tested again several times and seems like it was a one-timer (intermittent maybe). Otherwise LGTM ++

bbondy · 2017-01-26T05:08:14Z

@darkdh I think this needs another rebase if you have time between C56 building.

@bbondy

fix #4879 Auditors: @bbondy, @bsclifton, @cezaraugusto Test Plan: Performance: 1. Import bulk bookmarks (4000+) from other browsers (Don't merge into toolbar) 2. The process should finish instantly 3. Delete Import from XXX folder 4. The process should finish instantly Migration: 1. Make sure session-store-1 contains the sites data before 0.12.10 2. Lauch Brave and Close 3. sites of session-store-1 should change from [] tp {}

darkdh · 2017-01-26T07:42:34Z

@bbondy, rebased 😄

bbondy · 2017-01-28T17:34:21Z

++

luixxiul · 2017-01-28T17:49:27Z

It's nice to see this finally merged 🎉

darkdh added this to the 0.12.10 release milestone Nov 14, 2016

darkdh assigned bbondy, cezaraugusto and bsclifton Nov 14, 2016

darkdh commented Nov 14, 2016

View reviewed changes

bbondy modified the milestones: 0.12.11, 0.12.10 release Nov 15, 2016

bbondy modified the milestones: 1.0.0, 0.12.11, 0.13.2 Nov 18, 2016

darkdh mentioned this pull request Nov 30, 2016

Importing a lot of bookmarks slows down the browser #5899

Closed

darkdh force-pushed the sites-refactor branch from bdecfdb to 0be2e3c Compare December 5, 2016 04:08

luixxiul added the perf label Dec 5, 2016

darkdh force-pushed the sites-refactor branch 2 times, most recently from 2873ec9 to 9555a85 Compare December 6, 2016 09:13

darkdh force-pushed the sites-refactor branch 2 times, most recently from e3f4c76 to 5fdfdcb Compare December 7, 2016 07:11

darkdh modified the milestones: 0.13.1, 0.13.2 Dec 15, 2016

darkdh force-pushed the sites-refactor branch from 5fdfdcb to 07debeb Compare December 21, 2016 02:00

darkdh force-pushed the sites-refactor branch from 07debeb to 2241730 Compare December 21, 2016 02:12

darkdh force-pushed the sites-refactor branch 2 times, most recently from ba6cfde to 2368ac4 Compare January 15, 2017 13:57

luixxiul removed the QA/required label Jan 15, 2017

cezaraugusto suggested changes Jan 18, 2017

View reviewed changes

cezaraugusto approved these changes Jan 18, 2017

View reviewed changes

ayumi mentioned this pull request Jan 19, 2017

Check that browser-laptop sync works with sites refactor brave/sync#42

Closed

luixxiul added the PR/needs-rebase ۞ label Jan 21, 2017

darkdh force-pushed the sites-refactor branch from 2368ac4 to 8a9b6c4 Compare January 26, 2017 07:42

luixxiul added PR/needs-QA-attention ☕ PR/needs-rebase ۞ and removed PR/needs-rebase ۞ PR/work-in-progress ⚒ PR/needs-QA-attention ☕ labels Jan 26, 2017

Merge branch 'master' into sites-refactor

f1eb25c

bbondy removed the PR/needs-rebase ۞ label Jan 28, 2017

bbondy merged commit 6e9eb57 into master Jan 28, 2017

bsclifton deleted the sites-refactor branch January 30, 2017 22:10

darkdh mentioned this pull request Feb 1, 2017

Add field separators to getSiteKey #6938

Merged

4 tasks

This was referenced Feb 3, 2017

Manual Tests for Windows x64 0.13.2 RC2 #7030

Closed

Manual Tests for Windows ia-32 0.13.2 RC3 #7029

Closed

Manual Tests for Linux 0.13.2 RC3 #7028

Closed

Manual Tests for OS X 0.13.2 RC2 #7027

Closed

luixxiul removed the needs-info Another team member needs information from the PR/issue opener. label Feb 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change sites from List to Map #5587

Change sites from List to Map #5587

darkdh commented Nov 14, 2016 •

edited by luixxiul

Loading

darkdh Nov 14, 2016

bsclifton commented Nov 14, 2016 •

edited

Loading

bbondy commented Nov 15, 2016

bbondy commented Nov 18, 2016

darkdh commented Dec 5, 2016

darkdh commented Dec 6, 2016

posix4e commented Dec 21, 2016

darkdh commented Dec 21, 2016

posix4e commented Dec 21, 2016

cezaraugusto commented Jan 18, 2017

darkdh commented Jan 18, 2017

cezaraugusto left a comment •

edited

Loading

cezaraugusto left a comment •

edited

Loading

bbondy commented Jan 26, 2017

darkdh commented Jan 26, 2017

bbondy commented Jan 28, 2017

luixxiul commented Jan 28, 2017

Change sites from List to Map #5587

Change sites from List to Map #5587

Conversation

darkdh commented Nov 14, 2016 • edited by luixxiul Loading

darkdh Nov 14, 2016

Choose a reason for hiding this comment

bsclifton commented Nov 14, 2016 • edited Loading

bbondy commented Nov 15, 2016

bbondy commented Nov 18, 2016

darkdh commented Dec 5, 2016

darkdh commented Dec 6, 2016

posix4e commented Dec 21, 2016

darkdh commented Dec 21, 2016

posix4e commented Dec 21, 2016

cezaraugusto commented Jan 18, 2017

darkdh commented Jan 18, 2017

cezaraugusto left a comment • edited Loading

Choose a reason for hiding this comment

cezaraugusto left a comment • edited Loading

Choose a reason for hiding this comment

bbondy commented Jan 26, 2017

darkdh commented Jan 26, 2017

bbondy commented Jan 28, 2017

luixxiul commented Jan 28, 2017

darkdh commented Nov 14, 2016 •

edited by luixxiul

Loading

bsclifton commented Nov 14, 2016 •

edited

Loading

cezaraugusto left a comment •

edited

Loading

cezaraugusto left a comment •

edited

Loading