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

Created session helper for newTabState #5361

Merged
merged 2 commits into from
Nov 4, 2016
Merged

Created session helper for newTabState #5361

merged 2 commits into from
Nov 4, 2016

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Nov 3, 2016

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

Created session helper for newTabState

  • site will only be added if it's a bookmark or history item
  • site will only be removed if it's a bookmark or history item
  • site moves are now ignored
  • favicon is now updated for the newTabState (which is a different list than AppState.sites)

Fixes #5357
Fixes #5310

Auditors:
@bridiver - check out the new session helper; hope I did this right :)

@darkdh - this fixes the folder delete issue by ignoring folders

@cezaraugusto - move is no longer tracked (which is fine; we don't care about the order of
the items with regard to their position in bookmarks toolbar, which is what moveSite is for).
Also, see the new session helper (which we can use for ordering by # visits)

Test Plan:

  1. Launch Brave and visit brave.com
  2. Open a new tab (about:newtab) and confirm you see brave.com there (favicon should show also)
  3. Visit about:history and delete the visit you just made to brave.com
  4. Confirm it's now gone in about:newtab

@darkdh
Copy link
Member

darkdh commented Nov 3, 2016

lgtm

@cezaraugusto
Copy link
Contributor

awesome! lgtm 👌

@bridiver
Copy link
Collaborator

bridiver commented Nov 3, 2016

nice cleanup! Please add some tests and what do you think about naming it aboutNewTabState? When I first looked at it I thought it was code that applied to any new tab and took me a minute to realize that it was specifically for the about:newtab page.

- site will only be added if it's a bookmark or history item
- site will only be removed if it's a bookmark or history item
- site moves are now ignored
- favicon is now updated for the newTabState (which is a different list than AppState.sites)

Fixes #5357
Fixes #5310

Auditors:
@bridiver - check out the new session helper; hope I did this right :)

@darkdh - this fixes the folder delete issue by ignoring folders

@cezaraugusto - move is no longer tracked (which is fine; we don't care about the order of
the items with regard to their position in bookmarks toolbar, which is what moveSite is for).
Also, see the new session helper (which we can use for ordering by # visits)

Test Plan:
1. Launch Brave and visit brave.com
2. Open a new tab (about:newtab) and confirm you see brave.com there (favicon should show also)
3. Visit about:history and delete the visit you just made to brave.com
4. Confirm it's now gone in about:newtab
@bsclifton
Copy link
Member Author

bsclifton commented Nov 4, 2016

@bridiver renamed and also added test cases 😄 Took a while to get it all sorted, but really glad I added them

While writing the tests, I realized that the remove wasn't working and ended up updating the code to only add the site info, (not the tags). If you look at siteUtil.removeSite, it never actually deletes the entry (unless the tags are empty). By excluding the tags, the remove will always work (instead of keeping the entry for history)

@bridiver
Copy link
Collaborator

bridiver commented Nov 4, 2016

++

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.

delete folder cause stack overflow about:newtab - favicons are not always shown
6 participants