|
|
Created:
7 years, 9 months ago by Timo Reimann Modified:
7 years, 9 months ago CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTrigger a tab updated event if the favicon URL changes.
Have BrowserEventRouter listen for NOTIFICATION_FAVICON_UPDATED. When the favicon URL changes, a tabs onUpdated event is triggered and the new URL provided within the |changeInfo| parameter in chrome.tabs.onUpdated as |favIconUrl| property.
BUG=177330
Contributed by Timo Reimann <ttr314@googlemail.com>
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188289
Patch Set 1 #
Total comments: 10
Patch Set 2 : Apply review improvements. #
Total comments: 4
Patch Set 3 : Apply review improvements for patch set 2. #Patch Set 4 : Use GURL::possibly_invalid_spec() instead of GURL::spec(). #Patch Set 5 : Set correct MIME type for favicon.ico to please presubmit check. #Patch Set 6 : Another try to get the MIME type right. #Patch Set 7 : Remove and re-add favicon.ico to trigger SVN auto-props (hopefully). #Patch Set 8 : Remove favicon.ico (was committed separately). #
Messages
Total messages: 29 (0 generated)
Here's my CL for 177330. Please let me know if there's anything missing, particularly with regards to the JS unit tests. Thanks!
browser_event_router.* LGTM Adding asargent for expertise with extensions https://codereview.chromium.org/12591008/diff/1/chrome/browser/extensions/bro... File chrome/browser/extensions/browser_event_router.cc (right): https://codereview.chromium.org/12591008/diff/1/chrome/browser/extensions/bro... chrome/browser/extensions/browser_event_router.cc:366: if (!icon_url_changed || !*icon_url_changed) Nit: 2 spaces of indent for the function body https://codereview.chromium.org/12591008/diff/1/chrome/browser/extensions/bro... chrome/browser/extensions/browser_event_router.cc:368: scoped_ptr<DictionaryValue> changed_properties(new DictionaryValue()); Nit: Move the creation of |changed_properties| after checking entry->GetFavicon().valid. https://codereview.chromium.org/12591008/diff/1/chrome/browser/extensions/bro... chrome/browser/extensions/browser_event_router.cc:370: contents->GetController().GetActiveEntry(); Nit: 4 spaces of indent for the wrapped line https://codereview.chromium.org/12591008/diff/1/chrome/browser/extensions/bro... chrome/browser/extensions/browser_event_router.cc:498: content::Source<WebContents>(contents)); Style Nit: Check type == chrome::NOTIFICATION_FAVICON_UPDATED last to match the order that we registered listening for the notifications. https://codereview.chromium.org/12591008/diff/1/chrome/browser/extensions/bro... File chrome/browser/extensions/browser_event_router.h (right): https://codereview.chromium.org/12591008/diff/1/chrome/browser/extensions/bro... chrome/browser/extensions/browser_event_router.h:102: bool* icon_url_changed); Nit: const bool*
https://codereview.chromium.org/12591008/diff/1/chrome/browser/extensions/bro... File chrome/browser/extensions/browser_event_router.cc (right): https://codereview.chromium.org/12591008/diff/1/chrome/browser/extensions/bro... chrome/browser/extensions/browser_event_router.cc:366: if (!icon_url_changed || !*icon_url_changed) On 2013/03/13 17:44:49, pkotwicz wrote: > Nit: 2 spaces of indent for the function body Done. https://codereview.chromium.org/12591008/diff/1/chrome/browser/extensions/bro... chrome/browser/extensions/browser_event_router.cc:368: scoped_ptr<DictionaryValue> changed_properties(new DictionaryValue()); On 2013/03/13 17:44:49, pkotwicz wrote: > Nit: Move the creation of |changed_properties| after checking > entry->GetFavicon().valid. Done. https://codereview.chromium.org/12591008/diff/1/chrome/browser/extensions/bro... chrome/browser/extensions/browser_event_router.cc:370: contents->GetController().GetActiveEntry(); On 2013/03/13 17:44:49, pkotwicz wrote: > Nit: 4 spaces of indent for the wrapped line Done. https://codereview.chromium.org/12591008/diff/1/chrome/browser/extensions/bro... chrome/browser/extensions/browser_event_router.cc:498: content::Source<WebContents>(contents)); On 2013/03/13 17:44:49, pkotwicz wrote: > Style Nit: Check type == chrome::NOTIFICATION_FAVICON_UPDATED last to match the > order that we registered listening for the notifications. Done. https://codereview.chromium.org/12591008/diff/1/chrome/browser/extensions/bro... File chrome/browser/extensions/browser_event_router.h (right): https://codereview.chromium.org/12591008/diff/1/chrome/browser/extensions/bro... chrome/browser/extensions/browser_event_router.h:102: bool* icon_url_changed); On 2013/03/13 17:44:49, pkotwicz wrote: > Nit: const bool* Done. Also added const qualifier at call site for consistency reasons.
Just to double check, favicon.png is a valid PNG image?
On 2013/03/13 19:46:11, pkotwicz wrote: > Just to double check, favicon.png is a valid PNG image? Yes, it's the Google favicon. Hope that is OK.
This seems good overall, with a couple of small nits. Also, can you change the favicon to just be an all-black 16x16 png or something, instead of using the google favicon? I'd rather not have to go and find out if using the google one is technically allowed or not. https://codereview.chromium.org/12591008/diff/7001/chrome/browser/extensions/... File chrome/browser/extensions/browser_event_router.cc (right): https://codereview.chromium.org/12591008/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/browser_event_router.cc:365: const bool* icon_url_changed) { style nit: If you can make it fit under 80 chars, I prefer to see these 3 lines only take up 2 lines by indenting like this: void BrowserEventRouter::FaviconUrlUpdated(WebContents* contents, const bool* icon_url_changed) { https://codereview.chromium.org/12591008/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/browser_event_router.cc:374: entry->GetFavicon().url.spec()); Is it possible that the favicon url could be invalid? If so then you should use .possibly_invalid_spec() instead of .spec() (which will cause a browser crash due to CHECK failure on invalid urls)
I replaced the Google favicon by an all black one. Note that the format changed from PNG to ICO simply because it was easier for me to create a black ICO. I updated favicon/a.html and the unit test accordingly. https://codereview.chromium.org/12591008/diff/7001/chrome/browser/extensions/... File chrome/browser/extensions/browser_event_router.cc (right): https://codereview.chromium.org/12591008/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/browser_event_router.cc:365: const bool* icon_url_changed) { On 2013/03/13 20:28:30, Antony Sargent wrote: > style nit: If you can make it fit under 80 chars, I prefer to see these 3 lines > only take up 2 lines by indenting like this: > > void BrowserEventRouter::FaviconUrlUpdated(WebContents* contents, > const bool* icon_url_changed) { > Done. https://codereview.chromium.org/12591008/diff/7001/chrome/browser/extensions/... chrome/browser/extensions/browser_event_router.cc:374: entry->GetFavicon().url.spec()); On 2013/03/13 20:28:30, Antony Sargent wrote: > Is it possible that the favicon url could be invalid? If so then you should use > .possibly_invalid_spec() instead of .spec() (which will cause a browser crash > due to CHECK failure on invalid urls) > I'm not perfectly sure on this one. Peter probably has a better understanding of the code and may be able to comment. I haven't changed the call from spec() to possibly_invalid_spec() yet because its in-code documentation states that the "spec MUST NOT be [...] sent over the network". Technically, passing an invalid spec to an extension doesn't qualify as network transmission but indirectly the extension itself could do so. Just wanted to make sure this would be OK. Also, if there was a chance that the URL is invalid we possibly would not want to expose a broken URL/error message to the extension for user experience reasons. I could imagine though that Peter accounted for in situation backend-wise and doesn't send out NOTIFICATION_FAVICON_UPDATED in this case. Please let me know how to resolve this one.
LGTM My guess is that it's probably ok to just use possibly_invalid_spec. Extensions can already pass invalid URLs to XHR, img src, etc.
Replaced spec() by possibly_invalid_spec(). Unit tests run fine; I guess this CL is ready for landing.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttr314@googlemail.com/12591008/25001
Presubmit check for 12591008-25001 failed and returned exit status 1. INFO:root:Found 6 file(s). Running presubmit commit checks ... Running /b/commit-queue/workdir/chromium/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py ** Presubmit ERRORS ** Run the command: svn pset svn:mime-type image/vnd.microsoft.icon \ chrome/test/data/extensions/api_test/tabs/on_updated/favicon/favicon.ico Presubmit checks took 5.2s to calculate.
On 2013/03/14 00:32:15, I haz the power (commit-bot) wrote: > Presubmit check for 12591008-25001 failed and returned exit status 1. > > INFO:root:Found 6 file(s). > > Running presubmit commit checks ... > Running /b/commit-queue/workdir/chromium/PRESUBMIT.py > Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py > Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py > Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py > > ** Presubmit ERRORS ** > Run the command: svn pset svn:mime-type image/vnd.microsoft.icon \ > chrome/test/data/extensions/api_test/tabs/on_updated/favicon/favicon.ico > > Presubmit checks took 5.2s to calculate. Not quite sure how to apply that SVN command as I'm using Git...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttr314@googlemail.com/12591008/25001
Presubmit check for 12591008-25001 failed and returned exit status 1. INFO:root:Found 6 file(s). Running presubmit commit checks ... Running /b/commit-queue/workdir/chromium/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py ** Presubmit ERRORS ** Run the command: svn pset svn:mime-type image/vnd.microsoft.icon \ chrome/test/data/extensions/api_test/tabs/on_updated/favicon/favicon.ico Presubmit checks took 5.8s to calculate.
On 2013/03/14 00:51:38, I haz the power (commit-bot) wrote: > Presubmit check for 12591008-25001 failed and returned exit status 1. > > INFO:root:Found 6 file(s). > > Running presubmit commit checks ... > Running /b/commit-queue/workdir/chromium/PRESUBMIT.py > Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py > Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py > Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py > > ** Presubmit ERRORS ** > Run the command: svn pset svn:mime-type image/vnd.microsoft.icon \ > chrome/test/data/extensions/api_test/tabs/on_updated/favicon/favicon.ico > > Presubmit checks took 5.8s to calculate. I'll set it via .subversion/config. Waiting for the tree to reopen...
Hopefully this should work now...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttr314@googlemail.com/12591008/34001
Presubmit check for 12591008-34001 failed and returned exit status 1. INFO:root:Found 6 file(s). Running presubmit commit checks ... Running /b/commit-queue/workdir/chromium/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py ** Presubmit ERRORS ** Run the command: svn pset svn:mime-type image/vnd.microsoft.icon \ chrome/test/data/extensions/api_test/tabs/on_updated/favicon/favicon.ico Presubmit checks took 10.9s to calculate.
Last attempt for today.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttr314@googlemail.com/12591008/40001
Presubmit check for 12591008-40001 failed and returned exit status 1. INFO:root:Found 6 file(s). Running presubmit commit checks ... Running /b/commit-queue/workdir/chromium/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py ** Presubmit ERRORS ** Run the command: svn pset svn:mime-type image/vnd.microsoft.icon \ chrome/test/data/extensions/api_test/tabs/on_updated/favicon/favicon.ico Presubmit checks took 6.2s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttr314@googlemail.com/12591008/45001
Presubmit check for 12591008-45001 failed and returned exit status 1. INFO:root:Found 6 file(s). Running presubmit commit checks ... Running /b/commit-queue/workdir/chromium/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py ** Presubmit ERRORS ** Run the command: svn pset svn:mime-type image/vnd.microsoft.icon \ chrome/test/data/extensions/api_test/tabs/on_updated/favicon/favicon.ico Presubmit checks took 7.6s to calculate.
Could someone with sufficient rights commit my patch please? The CQ doesn't support MIME types, see https://code.google.com/p/chromium/issues/detail?id=23608 and https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/rhw2UGHCYvQ. Thanks!
I committed favicon.ico for you in https://codereview.chromium.org/12769004/ You should be able to proceed and commit the remainder of the CL via the commit queue
Thanks -- this should hopefully do it.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttr314@googlemail.com/12591008/55001
Message was sent while issue was closed.
Change committed as 188289 |