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

Favicon is now updated in session after visiting site #5285

Merged
merged 1 commit into from
Oct 31, 2016
Merged

Favicon is now updated in session after visiting site #5285

merged 1 commit into from
Oct 31, 2016

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Oct 31, 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).

Required to make sure we get the latest favicons for the new about:newtab page,
our bookmarks toolbar, and also the about:bookmarks page. Before, the favicon
was only being updated
:

  • when site is bookmarked / rebookmarked
  • when site is pinned

Auditors: @darkdh @cezaraugusto

Fixes #4860

Test Plan:

  1. Import bookmarks from Chrome or HTML... OR manually create a new bookmark (from about:bookmarks)
  2. Show the bookmarks toolbar and notice the favicon is not loaded
  3. Load/visit one of the bookmarks which does NOT have a favicon yet
  4. After visiting the page, notice that the favicon is updated

Fixes #2747

Test Plan:

  1. Open a new tab
  2. Per issue description, visit tokyo.cawaii.media/gisele/
  3. Go back to the empty tab by pressing the back button
  4. Press and hold the forward button to display the pulldown

Fixes #5148

Test Plan:

  1. Open a new tab
  2. Go to a site that is not bookmarked, such as https://brave.com
  3. Click the star icon next to the URL (or push cmd+d / ctrl+d) to quickly bookmark the page (before page finishes loading)
  4. Confirm favicon gets updated when page finishes loading

Required to make sure we get the latest favicons for the new about:newtab page,
our bookmarks toolbar, and also the about:bookmarks page. Before, the favicon
was only being updated:
- when site is bookmarked / rebookmarked
- when site is pinned

Auditors: @darkdh @cezaraugusto

Fixes #4860

Test Plan:
1. Import bookmarks from Chrome or HTML... OR manually create a new bookmark (from about:bookmarks)
2. Show the bookmarks toolbar and notice the favicon is not loaded
3. Load/visit one of the bookmarks which does NOT have a favicon yet
4. After visiting the page, notice that the favicon is updated

Fixes #2747

Test Plan:
1. Open a new tab
2. Per issue description, visit tokyo.cawaii.media/gisele/
3. Go back to the empty tab by pressing the back button
4. Press and hold the forward button to display the pulldown
@@ -770,6 +770,9 @@ const handleAppAction = (action) => {
case AppConstants.APP_DEFAULT_BROWSER_CHECK_COMPLETE:
appState = appState.set('defaultBrowserCheckComplete', {})
break
case WindowConstants.WINDOW_SET_FAVICON:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who dispatches WINDOW_SET_FAVICON? I didn't see it in this PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

already existing windowAction.setFavicon

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. 😛

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes- which is triggered in frame.js, from the page-favicon-updated handler 😄

@bbondy
Copy link
Member

bbondy commented Oct 31, 2016

++

@luixxiul
Copy link
Contributor

luixxiul commented Nov 6, 2016

Note: #4860 will be fixed in 0.12.9

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.

None yet

6 participants