Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(628)

Issue 12591008: Trigger a tab updated event if the favicon URL changes. (Closed)

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
Visibility:
Public.

Description

Trigger 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). #

Unified diffs Side-by-side diffs Delta from patch set Stats (+51 lines, -3 lines) Patch
M chrome/browser/extensions/browser_event_router.h View 1 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/extensions/browser_event_router.cc View 1 2 3 5 chunks +29 lines, -0 lines 0 comments Download
M chrome/common/extensions/api/tabs.json View 1 chunk +5 lines, -0 lines 0 comments Download
A + chrome/test/data/extensions/api_test/tabs/on_updated/favicon/a.html View 1 2 1 chunk +1 line, -2 lines 0 comments Download
M chrome/test/data/extensions/api_test/tabs/on_updated/test.js View 1 2 2 chunks +12 lines, -1 line 0 comments Download

Messages

Total messages: 29 (0 generated)
Timo Reimann
Here's my CL for 177330. Please let me know if there's anything missing, particularly with ...
7 years, 9 months ago (2013-03-12 22:49:10 UTC) #1
pkotwicz
browser_event_router.* LGTM Adding asargent for expertise with extensions https://codereview.chromium.org/12591008/diff/1/chrome/browser/extensions/browser_event_router.cc File chrome/browser/extensions/browser_event_router.cc (right): https://codereview.chromium.org/12591008/diff/1/chrome/browser/extensions/browser_event_router.cc#newcode366 chrome/browser/extensions/browser_event_router.cc:366: if ...
7 years, 9 months ago (2013-03-13 17:44:49 UTC) #2
Timo Reimann
https://codereview.chromium.org/12591008/diff/1/chrome/browser/extensions/browser_event_router.cc File chrome/browser/extensions/browser_event_router.cc (right): https://codereview.chromium.org/12591008/diff/1/chrome/browser/extensions/browser_event_router.cc#newcode366 chrome/browser/extensions/browser_event_router.cc:366: if (!icon_url_changed || !*icon_url_changed) On 2013/03/13 17:44:49, pkotwicz wrote: ...
7 years, 9 months ago (2013-03-13 19:42:13 UTC) #3
pkotwicz
Just to double check, favicon.png is a valid PNG image?
7 years, 9 months ago (2013-03-13 19:46:11 UTC) #4
Timo Reimann
On 2013/03/13 19:46:11, pkotwicz wrote: > Just to double check, favicon.png is a valid PNG ...
7 years, 9 months ago (2013-03-13 19:55:49 UTC) #5
asargent_no_longer_on_chrome
This seems good overall, with a couple of small nits. Also, can you change the ...
7 years, 9 months ago (2013-03-13 20:28:30 UTC) #6
Timo Reimann
I replaced the Google favicon by an all black one. Note that the format changed ...
7 years, 9 months ago (2013-03-13 21:36:26 UTC) #7
asargent_no_longer_on_chrome
LGTM My guess is that it's probably ok to just use possibly_invalid_spec. Extensions can already ...
7 years, 9 months ago (2013-03-14 00:15:39 UTC) #8
Timo Reimann
Replaced spec() by possibly_invalid_spec(). Unit tests run fine; I guess this CL is ready for ...
7 years, 9 months ago (2013-03-14 00:30:14 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttr314@googlemail.com/12591008/25001
7 years, 9 months ago (2013-03-14 00:32:08 UTC) #10
commit-bot: I haz the power
Presubmit check for 12591008-25001 failed and returned exit status 1. INFO:root:Found 6 file(s). Running presubmit ...
7 years, 9 months ago (2013-03-14 00:32:15 UTC) #11
Timo Reimann
On 2013/03/14 00:32:15, I haz the power (commit-bot) wrote: > Presubmit check for 12591008-25001 failed ...
7 years, 9 months ago (2013-03-14 00:47:19 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttr314@googlemail.com/12591008/25001
7 years, 9 months ago (2013-03-14 00:51:31 UTC) #13
commit-bot: I haz the power
Presubmit check for 12591008-25001 failed and returned exit status 1. INFO:root:Found 6 file(s). Running presubmit ...
7 years, 9 months ago (2013-03-14 00:51:38 UTC) #14
Timo Reimann
On 2013/03/14 00:51:38, I haz the power (commit-bot) wrote: > Presubmit check for 12591008-25001 failed ...
7 years, 9 months ago (2013-03-14 00:54:33 UTC) #15
Timo Reimann
Hopefully this should work now...
7 years, 9 months ago (2013-03-14 01:01:43 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttr314@googlemail.com/12591008/34001
7 years, 9 months ago (2013-03-14 01:04:43 UTC) #17
commit-bot: I haz the power
Presubmit check for 12591008-34001 failed and returned exit status 1. INFO:root:Found 6 file(s). Running presubmit ...
7 years, 9 months ago (2013-03-14 01:04:59 UTC) #18
Timo Reimann
Last attempt for today.
7 years, 9 months ago (2013-03-14 01:14:35 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttr314@googlemail.com/12591008/40001
7 years, 9 months ago (2013-03-14 01:22:29 UTC) #20
commit-bot: I haz the power
Presubmit check for 12591008-40001 failed and returned exit status 1. INFO:root:Found 6 file(s). Running presubmit ...
7 years, 9 months ago (2013-03-14 01:22:38 UTC) #21
Timo Reimann
7 years, 9 months ago (2013-03-14 08:55:43 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttr314@googlemail.com/12591008/45001
7 years, 9 months ago (2013-03-14 08:59:03 UTC) #23
commit-bot: I haz the power
Presubmit check for 12591008-45001 failed and returned exit status 1. INFO:root:Found 6 file(s). Running presubmit ...
7 years, 9 months ago (2013-03-14 08:59:14 UTC) #24
Timo Reimann
Could someone with sufficient rights commit my patch please? The CQ doesn't support MIME types, ...
7 years, 9 months ago (2013-03-14 14:25:30 UTC) #25
pkotwicz
I committed favicon.ico for you in https://codereview.chromium.org/12769004/ You should be able to proceed and commit ...
7 years, 9 months ago (2013-03-14 17:14:33 UTC) #26
Timo Reimann
Thanks -- this should hopefully do it.
7 years, 9 months ago (2013-03-14 21:33:48 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttr314@googlemail.com/12591008/55001
7 years, 9 months ago (2013-03-14 21:36:45 UTC) #28
commit-bot: I haz the power
7 years, 9 months ago (2013-03-15 07:38:56 UTC) #29
Message was sent while issue was closed.
Change committed as 188289

Powered by Google App Engine
This is Rietveld 408576698