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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cezaraugusto
changed the title
cezaraugusto
changed the title
|
Ready for review |
bsclifton
reviewed
Nov 27, 2016
| @@ -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) => { | |||
There was a problem hiding this comment.
This is a pretty elegant way to do it, which avoids the complexity of promises. Good job 😄
Auditors: @bsclifton Fix #5455 Fixes #2075 Fixes #3648 Test Plan * Automated tests must pass
|
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 |
4 tasks
This was referenced Dec 12, 2016
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
This was referenced Dec 21, 2016
This was referenced Jan 16, 2017
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
git rebase -ito 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):
about:preferencesand setBookmarks Barto the valueFavicons only