Splits sites object in the app state #9979

NejcZdovc · 2017-07-12T17:25:37Z

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.

Auditors: @bbondy @ayumi @diracdeltas @bsclifton @darkdh

Test Plan:

Bookmark

try to add a bookmark

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)

Bookmark link

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

Bookmark manager

try to dnd bookmark into another folder

Bookmark manager

try to dnd multiple bookmarks into another folder

Bookmark manager

try to dnd folder into another folder

Bookmark manager

try to reorder bookmarks in bookmark manager

Bookmark toolbar

try to dnd bookmark into the folder

Bookmark toolbar

try to dnd folder into another folder

Bookmark toolbar

try to to reorder bookmarks with dnd (try it with folders as well)

History manager

check if history is successfully added

History manager

right click on a history item
try to delete it

Pinned tabs

try to pin a site
restart the browser
check if pinned tab is still there

Importer

try to import HTML file

Importer

try to import from chrome

Importer

try to import from FF

Exporter

check if export is working

Url bar suggestions

check if bookmark is shown in suggestion list

Url bar suggestions

check if history site is shown in suggestion list

Ledger

enabled ledger
visit some site
check if site is added to the ledger

New tab

check if top sites are displayed correctly

New tab

check if you can remove top site

New tab

check if you can pin top site

New tab

check if you can bookmark and edit bookmark already bookmarked top site

New tab

check if you can reorder top sites

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-07-12T17:32:33Z

Pinned sites

create utils
create tests for util file
create upgrade script

Default sites

remove from state
refactor newTab to use topSites directly

History sites (current sites object)

Bookmarks sites

Bookmark folder sites

General

Questions

can we remove site from sites when we pin it?
should we be saving keys in string format? Otherwise every time you want to use key in .get(), you need to add toString()
we have last visited time in about:bookmarks. Is this needed? @bradleyrichter if so we need to to update bookmark entry when we add history entry.

Known issues

Tests

Possible fixes (check if this issues are fixed after done)

ayumi · 2017-07-12T18:00:51Z

docs/state.md

@@ -236,7 +236,47 @@ AppStore
    'tabs.switch-to-new-tabs': boolean, // true if newly opened tabs should be focused immediately
    'tabs.tabs-per-page': number // number of tabs per tab page
  },
-  sites: {
+  // TODO sort alphabetically


sorted collections are not easily possible with Immutable.JS right? and we probably don't want to re-sort on each sites mutation

currently i'm working around this by keeping separate sorted lists of siteKeys & scores (redis-sorted-set) in memory, populated at program init and with each ADD/RE/MOVE _SITE event.

this TOD only means that I should reorder this new entities in this md file when all properties are fixed

Regarding sort, I was thinking of creating simple sort object that would have siteKey and order key. This way we can have bookmarks and folders split, but still keep the order that we need for the bookmark toolbar. Something similar we use for tabs as well.

What do you think?

re-sort on every sites mutation has been a source of serious perf issues, so as long as we don't do that it's okay.

i'm actually working on something similar for caching history and topsites. downside is the cache is available in the same process only.

it would be nice if there existed an Immutable.JS sorted map.

ayumi · 2017-07-12T18:04:18Z

what's the timeline on this refactor?
currently i'm fixing the last major reducer perf issue (100–200ms / load) by caching history and topSite siteKeys.

NejcZdovc · 2017-07-12T18:09:35Z

Timeline is probably start of the next week

NejcZdovc · 2017-07-13T10:29:49Z

docs/state.md

+      title: string
+    }
+  },
+  pinnedSites: {


I think that this can be removed and we only use tabs for it.

diracdeltas · 2017-07-13T20:51:36Z

the pinned tab changes won't affect sync, but everything else does. i can help do the sync change

diracdeltas · 2017-07-13T20:53:26Z

app/sessionStore.js

@@ -626,7 +628,8 @@ module.exports.defaultAppState = () => {
      pendingRecords: {}
    },
    locationSiteKeysCache: undefined,
-    sites: getTopSiteMap(),
+    sites: getTopSiteMap(), // TODO remove


what will happen to the top sites? currently they have siteTag DEFAULT so that they don't show up in history/autosuggest until they are actually visited

I found out that we only use top sites in new tab, so top sites will be automatically added to the new tab top sites list inside the newTab component. Top sites will not be part of the state anymore.

NejcZdovc · 2017-07-13T21:27:47Z

@diracdeltas thank you, will need some help with sync for sure. Right now I am still building the big picture, so probably tomorrow I will have cleaner image and path how to do everything.

NejcZdovc · 2017-07-15T17:38:26Z

@ayumi how far are you with your fix?

NejcZdovc · 2017-07-24T06:54:10Z

@luixxiul ok so there could be a problem with an upgrade process. Will take a look

Sites refactor was #9979

Auditors: Test Plan:

NejcZdovc · 2017-07-26T07:29:17Z

Closing in favor of #10136, which is based on this repo and not on my fork.

Sites refactor was #9979

NejcZdovc added the refactoring label Jul 12, 2017

NejcZdovc added this to the 0.19.x (Developer Channel) milestone Jul 12, 2017

NejcZdovc self-assigned this Jul 12, 2017

ayumi reviewed Jul 12, 2017

View reviewed changes

NejcZdovc commented Jul 13, 2017

View reviewed changes

NejcZdovc force-pushed the refactor/#9978-sites branch from 8d60b6c to 1aa22fb Compare July 13, 2017 19:20

diracdeltas reviewed Jul 13, 2017

View reviewed changes

NejcZdovc force-pushed the refactor/#9978-sites branch 10 times, most recently from 0ba3c00 to f96e0b1 Compare July 15, 2017 17:35

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

NejcZdovc force-pushed the refactor/#9978-sites branch from bdc93ce to 28664c7 Compare July 16, 2017 18:49

NejcZdovc added feature/bookmarks feature/history feature/newtab feature/pinned-tabs labels Jul 16, 2017

NejcZdovc force-pushed the refactor/#9978-sites branch from 820542d to 28b71b9 Compare July 17, 2017 10:45

NejcZdovc force-pushed the refactor/#9978-sites branch 18 times, most recently from a87b20d to 52ddddb Compare July 25, 2017 15:30

ayumi added a commit that referenced this pull request Jul 25, 2017

Fix Sync after sites refactor

c5241e1

Sites refactor was #9979

ayumi mentioned this pull request Jul 25, 2017

[wip] Fix Sync after sites refactor NejcZdovc/browser-laptop#1

Closed

4 tasks

NejcZdovc force-pushed the refactor/#9978-sites branch from 52ddddb to 475c457 Compare July 26, 2017 07:20

Splits sites object in the app state

a77e9a8

Auditors: Test Plan:

NejcZdovc force-pushed the refactor/#9978-sites branch from 475c457 to a77e9a8 Compare July 26, 2017 07:27

NejcZdovc closed this Jul 26, 2017

ayumi added a commit that referenced this pull request Jul 26, 2017

Fix Sync after sites refactor

4af7bea

Sites refactor was #9979

ayumi added a commit that referenced this pull request Jul 26, 2017

Fix Sync after sites refactor

6b66042

Sites refactor was #9979

diracdeltas removed this from the 0.20.x (Developer Channel) milestone Aug 7, 2017

luixxiul removed the PR/work-in-progress ⚒ label Oct 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Splits sites object in the app state #9979

Splits sites object in the app state #9979

NejcZdovc commented Jul 12, 2017 •

edited

Loading

NejcZdovc commented Jul 12, 2017 •

edited

Loading

ayumi Jul 12, 2017 •

edited

Loading

NejcZdovc Jul 12, 2017 •

edited

Loading

NejcZdovc Jul 12, 2017

ayumi Jul 12, 2017

ayumi commented Jul 12, 2017

NejcZdovc commented Jul 12, 2017

NejcZdovc Jul 13, 2017

NejcZdovc Jul 15, 2017

diracdeltas commented Jul 13, 2017

diracdeltas Jul 13, 2017

NejcZdovc Jul 15, 2017

NejcZdovc commented Jul 13, 2017

NejcZdovc commented Jul 15, 2017

NejcZdovc commented Jul 24, 2017

NejcZdovc commented Jul 26, 2017

Splits sites object in the app state #9979

Splits sites object in the app state #9979

Conversation

NejcZdovc commented Jul 12, 2017 • edited Loading

NejcZdovc commented Jul 12, 2017 • edited Loading

Pinned sites

Default sites

History sites (current sites object)

Bookmarks sites

Bookmark folder sites

General

Questions

Known issues

Tests

Possible fixes (check if this issues are fixed after done)

ayumi Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

NejcZdovc Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

NejcZdovc Jul 12, 2017

Choose a reason for hiding this comment

ayumi Jul 12, 2017

Choose a reason for hiding this comment

ayumi commented Jul 12, 2017

NejcZdovc commented Jul 12, 2017

NejcZdovc Jul 13, 2017

Choose a reason for hiding this comment

NejcZdovc Jul 15, 2017

Choose a reason for hiding this comment

diracdeltas commented Jul 13, 2017

diracdeltas Jul 13, 2017

Choose a reason for hiding this comment

NejcZdovc Jul 15, 2017

Choose a reason for hiding this comment

NejcZdovc commented Jul 13, 2017

NejcZdovc commented Jul 15, 2017

NejcZdovc commented Jul 24, 2017

NejcZdovc commented Jul 26, 2017

NejcZdovc commented Jul 12, 2017 •

edited

Loading

NejcZdovc commented Jul 12, 2017 •

edited

Loading

ayumi Jul 12, 2017 •

edited

Loading

NejcZdovc Jul 12, 2017 •

edited

Loading