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

Splits sites object in the app state #10136

Merged
merged 7 commits into from
Aug 4, 2017
Merged

Splits sites object in the app state #10136

merged 7 commits into from
Aug 4, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Jul 26, 2017

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 #1646 (Wrong favicon is shown for bookmark)
Resolves #1856 (Removing sites from history - siteUtil functionality)
Resolves #2655 (Bookmark name of a file does not appear in the autocomplete list)
Resolves #2771 (Bookmark folder is created left of bookmark item, not right)
Resolves #3646 (Bookmarks title changes when navigating around the page)
Resolves #3694 (Add automated test for making sure bookmark custom title appears on bookmark toolbar)
Resolves #4224 (Hidden history entries appear by deleting the latest entry)
Resolves #4260 (Edited information of bookmark does not appear on bookmark dialog via star icon)
Resolves #4833 (Bookmark title dynamically changed)
Resolves #4868 (Rebookmarking from context menu does not reset bookmark title)
Resolves #4929 (Bookmark title automatically changes on the bookmark form)
Resolves #5072 (about:history omits recent history)
Resolves #5382 (clicking entries on about:history is laggy)
Resolves #5699 (Last Visited on Bookmark Manager is not recorded for newly created bookmarks)
Resolves #6104 (Address bar should autocomplete from custom bookmark titles)
Resolves #6108 (Bookmark title on bookmark toolbar is renamed)
Resolves #6585 (Remove customTitle from siteDetail object)
Resolves #8022 (Bookmark moved to a folder on the toolbar cannot be removed via the star button)
Resolves #9301 (Changing bookmark location causes duplicate bookmarks)
Resolves #9326 (Custom bookmark title is not being displayed in suggestions)
Resolves #9978 (Split sites into separate entities)
Resolves #10026 (Bookmarks toolbar : imported sorted bookmarks are represented in wacky order)

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

Test Plan:
Run general regression tests first: https://github.com/brave/browser-laptop/wiki/Manual-Tests

  1. Bookmark
  • try to add a bookmark
  1. Bookmark
  • try to add a bookmark, but into the second level folder
  1. Bookmark folder
  • try to add a bookmark folder
  1. Bookmark
  • try to edit a bookmark
  1. Bookmark folder
  • try to add a bookmark folder
  1. Bookmark
  • try to remove a bookmark
  1. Bookmark folder
  • try to remove a bookmark folder
  1. Bookmark
  • try to add a bookmark into a folder (with a right click on a folder)
  • folder should be preselected
  1. Bookmark folder
  • try to add a bookmark folder into a folder (with a right click on a folder)
  • folder should be preselected
  1. Navigation bar
  • try to bookmark a page via navigation bar star
  1. Navigation bar
  • try to edit a bookmark via navigation bar star
  1. Bookmark manager
  • try to add a bookmark folder from a bookmark manager (list, folder tree and button)
  1. Bookmark manager
  • try to add a bookmark from a bookmark manager (list, folder tree and button)
  1. Bookmark manager
  • try to edit a bookmark from a bookmark manager (list and folder tree)
  1. Bookmark manager
  • try to edit a bookmark folder from a bookmark manager (list and folder tree)
  1. Bookmark link
  • try to bookmark a link (right click on a link in the page)
  1. Bookmark manager
  • try to dnd bookmark into another folder
  1. Bookmark manager
  • try to dnd multiple bookmarks into another folder
  1. Bookmark manager
  • try to dnd folder into another folder
  1. Bookmark manager
  • try to reorder bookmarks in bookmark manager
  1. Bookmark toolbar
  • try to dnd bookmark into the folder
  1. Bookmark toolbar
  • try to dnd folder into another folder
  1. Bookmark toolbar
  • try to to reorder bookmarks with dnd (try it with folders as well)
  1. History manager
  • check if history is successfully added
  1. History manager
  • right click on a history item
  • try to delete it
  1. Pinned tabs
  • try to pin a site
  • restart the browser
  • check if pinned tab is still there
  1. Importer
  • try to import HTML file
  1. Importer
  • try to import from chrome
  1. Importer
  • try to import from FF
  1. Exporter
  • check if export is working
  1. Url bar suggestions
  • check if bookmark is shown in suggestion list
  1. Url bar suggestions
  • check if history site is shown in suggestion list
  1. Ledger
  • enabled ledger
  • visit some site
  • check if site is added to the ledger
  1. New tab
  • check if top sites are displayed correctly
  1. New tab
  • check if you can remove top site
  1. New tab
  • check if you can pin top site
  1. New tab
  • check if you can bookmark and edit bookmark already bookmarked top site
  1. 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 NejcZdovc added this to the 0.20.x (Nightly Channel) milestone Jul 26, 2017
@NejcZdovc NejcZdovc self-assigned this Jul 26, 2017
@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Jul 26, 2017

Pinned sites

  • create utils
  • create upgrade script

Default sites

  • remove from state
  • refactor newTab to use topSites directly

History sites (current sites object)

  • create utils
  • create upgrade script
  • fix about:history

Bookmarks sites

  • create utils
  • remove customTitle
  • create index-order table
  • create upgrade script
  • fix about:bookmarks
  • try to add a bookmark (star icon) while page is still loading
  • new tab bookmark is not working
  • changing bookmark folder location is not working for already bookmarked item
  • right click on a bookmark and clicking add site deletes all bookmarks

Bookmark folder sites

  • create utils
  • remove customTitle
  • create index-order table
  • create upgrade script

General

  • check how sites (no tags) array is working in other places
  • update sync related code, ask for help from @ayumi @diracdeltas
  • update import related code, ask for help from @darkdh
  • update suggestions related code, ask for help from @bbondy
  • alphabetize state docs
  • keep only general functions in js/state/siteUtil.js
  • remove app/common/state/siteState.js
  • remove app/browser/reducers/sitesReducer.js
  • make sure that import order is preserved
  • go through all getSiteKey calls and see where we can remove them, because key is now inside bookmarks object

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

  • improve import, because now it's taking too long to finish (maybe the problem is that we are calling action inside action)
  • if you add a site via dialog, we need to add favicon somehow if it's not in the history
  • import from chrome is not working
  • sync is not working
  • site cache is broken
  • if you delete a history item and then bookmark active frame you will lose favicon and theme color
  • when importing from chrome last accessed time is not imported correctly
  • add sort values for upgrade process
  • bookmarks don't get favicon if you bookmark top sites on a fresh profile
  • bookmark star status is not displayed correctly on a new tab
  • fix dnd for top sites in new tab
  • more bookmarks doesn't work on bookmark toolbar

Tests 10/20

  • app/browser/reducers/bookmarkFoldersReducer.js
  • app/browser/reducers/bookmarksReducer.js
  • app/browser/reducers/historyReducer.js
  • app/browser/reducers/pinnedSitesReducer.js
  • app/browser/reducers/sitesReducer.js (this file was removed, move things around)
  • app/common/cache/bookmarkLocationCache.js (before it was siteCache.js)
  • app/common/cache/bookmarkOrderCache.js
  • app/common/lib/bookmarkFoldersUtil.js
  • app/common/lib/bookmarkUtil.js
  • js/state/siteUtil.js (test file needs to be updated)
  • app/common/lib/historyUtil.js
  • app/common/lib/pinnedSitesUtil.js
  • app/common/state/aboutHistoryState.js
  • app/common/state/aboutNewTabState.js
  • app/common/state/bookmarkFoldersState.js
  • app/common/state/bookmarksState.js
  • app/common/state/historyState.js
  • app/common/state/pinnedSitesState.js
  • app/common/state/tabState.js
  • js/stores/windowStore.js

@NejcZdovc NejcZdovc force-pushed the refactor/sites-split branch 3 times, most recently from cea2dff to 0b6e7be Compare August 4, 2017 14:45
NejcZdovc and others added 6 commits August 4, 2017 09:58
Resolves #1646
Resolves #1856
Resolves #2655
Resolves #2771
Resolves #3646
Resolves #3694
Resolves #4224
Resolves #4260
Resolves #4833
Resolves #4868
Resolves #4929
Resolves #5072
Resolves #5699
Resolves #5382
Resolves #6104
Resolves #6108
Resolves #6585
Resolves #8022
Resolves #9301
Resolves #9326
Resolves #9978
Resolves #10026

Auditors:

Test Plan:
@NejcZdovc NejcZdovc force-pushed the refactor/sites-split branch from 0b6e7be to 9354d6d Compare August 4, 2017 15:03
@NejcZdovc NejcZdovc mentioned this pull request Aug 4, 2017
6 tasks
@NejcZdovc NejcZdovc force-pushed the refactor/sites-split branch 2 times, most recently from c12cadb to 9934c2d Compare August 4, 2017 15:33
@ayumi
Copy link
Contributor

ayumi commented Aug 4, 2017

👍

* Fix bookmark importer by making bookmarkList immutable
* Fix importer to import in order
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.

++++! ❤️

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants