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

Add default icon for url that has a broken file #5826

Merged
merged 1 commit into from
Nov 27, 2016
Merged

Add default icon for url that has a broken file #5826

merged 1 commit into from
Nov 27, 2016

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Nov 23, 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).

Auditors: @bsclifton
/cc @bbondy

Fix #5455

Also fixes:
#2075 (bookmark is invisible if favicon.ico is not found)
#3648 (Favicons Only Behavior Now Leaves Blanks when there is no favicon)

Test Plan:

  • Automated tests must pass (see below)
    For bookmarks toolbar
    npm run test -- --grep="display favicon on bookmarks toolbar"

    For bookmarks manager
    npm run test -- --grep="display favicon on bookmarks manager"

    For tabs
    npm run test -- --grep="Fallback to default icon when no one is specified"

    For new tab page
    npm run test -- --grep="shows favicon image for topSites"
    npm run test -- --grep="replace topSites favicon images with a letter when no icon is found"

  • Manual tests (results can be compared to 0.12.10 to confirm fix):

    • Test adding page which doesn't have favicon
    • Test favicon only mode
      • bookmark a few sites
      • visit https://www.oediscountparts.com/ (which doesn't have a favicon) and bookmark the site
      • Go to about:preferences and set Bookmarks Bar to the value Favicons only

@cezaraugusto cezaraugusto added this to the 0.13.0 milestone Nov 23, 2016
@cezaraugusto cezaraugusto changed the title Add onError handler for favicons inside topSites [WIP] Add onError handler for favicons inside topSites Nov 23, 2016
@cezaraugusto cezaraugusto changed the title [WIP] Add onError handler for favicons inside topSites Add default icon for url that has a broken file Nov 27, 2016
@cezaraugusto
Copy link
Contributor Author

Ready for review

@@ -819,7 +820,9 @@ class Frame extends ImmutableComponent {
})
this.webview.addEventListener('page-favicon-updated', (e) => {
if (e.favicons && e.favicons.length > 0) {
windowActions.setFavicon(this.frame, e.favicons[0])
imageUtil.getWorkingImageUrl(e.favicons[0], (imageFound) => {
Copy link
Member

Choose a reason for hiding this comment

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

This is a pretty elegant way to do it, which avoids the complexity of promises. Good job 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🎉

Auditors: @bsclifton

Fix #5455

Fixes #2075
Fixes #3648

Test Plan
* Automated tests must pass
@bsclifton
Copy link
Member

I pulled this in, ran some manual tests (documented in the steps above)... also ran the automated tests (great job with those, by the way!! 😄 )

Changes look great! I noticed that this fixes a few other issues too (and I confirmed those, added notes to test plan). I'll mark as qa-steps-specified and merge! Awesome work! 😄

@bsclifton bsclifton merged commit 325ea80 into brave:master Nov 27, 2016
@bsclifton bsclifton mentioned this pull request Dec 1, 2016
4 tasks
bbondy pushed a commit that referenced this pull request Dec 13, 2016
Fixes #2703

Also includes
- small refactor in js/components/urlBar.js which should make future changes easier (and opens this up to unit testing)
- removes permafail webdriver test that should have been removed with #5826 (I missed the fact CI failed on this when reviewing)

Auditors: @bbondy, @cezaraugusto

Test Plan:
1. Launch Brave and open Preferences
2. Set Yandex as your default search engine
3. Search in the omnibox, confirm it shows results on yandex.com
4. Check the URL that was created; client ID (clid=) should be:
  - 2274777 on Windows
  - 2274776 on macOS
  - 2274778 on linux
5. Try the search engine shortcut, :ya and do a search in the omnibox
@cezaraugusto cezaraugusto deleted the feature/newtab/5455 branch July 25, 2017 07:26
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.

Favicons need onerror handler (as seen in about:newtab)
3 participants