Trigger a tab updated event if the favicon URL changes.

chrome/browser/extensions/browser_event_router.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/browser_event_router.h"
 
 #include "base/json/json_writer.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/api/extension_action/extension_page_actions_api_constants.h"
 #include "chrome/browser/extensions/api/tabs/tabs_constants.h"
 #include "chrome/browser/extensions/api/tabs/tabs_windows_api.h"
 #include "chrome/browser/extensions/api/tabs/windows_event_router.h"
 #include "chrome/browser/extensions/event_names.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/extensions/window_controller.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_iterator.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/extensions/api/extension_action/action_info.h"
 #include "chrome/common/extensions/extension_constants.h"
+#include "content/public/browser/favicon_status.h"
 #include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/web_contents.h"
 
 namespace events = extensions::event_names;
 namespace tab_keys = extensions::tabs_constants;
 namespace page_actions_keys = extension_page_actions_api_constants;
 
 using content::NavigationController;
 using content::WebContents;
@@ skipping 84 matching lines @@
   registrar_.Add(
       this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
       content::Source<NavigationController>(&contents->GetController()));
 
   // Observing NOTIFICATION_WEB_CONTENTS_DESTROYED is necessary because it's
   // possible for tabs to be created, detached and then destroyed without
   // ever having been re-attached and closed. This happens in the case of
   // a devtools WebContents that is opened in window, docked, then closed.
   registrar_.Add(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
                  content::Source<WebContents>(contents));
+
+  registrar_.Add(this, chrome::NOTIFICATION_FAVICON_UPDATED,
+                 content::Source<WebContents>(contents));
 }
 
 void BrowserEventRouter::UnregisterForTabNotifications(WebContents* contents) {
   registrar_.Remove(this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
       content::Source<NavigationController>(&contents->GetController()));
   registrar_.Remove(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
       content::Source<WebContents>(contents));
+  registrar_.Remove(this, chrome::NOTIFICATION_FAVICON_UPDATED,
+      content::Source<WebContents>(contents));
 }
 
 void BrowserEventRouter::OnBrowserRemoved(Browser* browser) {
   if (!profile_->IsSameProfile(browser->profile()))
     return;
 
   // Stop listening to TabStripModel events for this browser.
   browser->tab_strip_model()->RemoveObserver(this);
 }
 
@@ skipping 197 matching lines @@
 
   if (did_navigate)
     changed_properties.reset(entry->DidNavigate(contents));
   else
     changed_properties.reset(entry->UpdateLoadState(contents));
 
   if (changed_properties)
     DispatchTabUpdatedEvent(contents, changed_properties.Pass());
 }
 
+void BrowserEventRouter::FaviconUrlUpdated(
+  WebContents* contents,
+  const bool* icon_url_changed) {

[asargent_no_longer_on_chrome, 2013/03/13 20:28:30]

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) {

[Timo Reimann, 2013/03/13 21:36:26]

Done.

+    if (!icon_url_changed || !*icon_url_changed)
+      return;
+    content::NavigationEntry* entry =
+        contents->GetController().GetActiveEntry();
+    if (!entry || !entry->GetFavicon().valid)
+      return;
+    scoped_ptr<DictionaryValue> changed_properties(new DictionaryValue());
+    changed_properties->SetString(tab_keys::kFaviconUrlKey,
+                                  entry->GetFavicon().url.spec());

[asargent_no_longer_on_chrome, 2013/03/13 20:28:30]

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)

[Timo Reimann, 2013/03/13 21:36:26]

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.

+    DispatchTabUpdatedEvent(contents, changed_properties.Pass());
+}
+
 void BrowserEventRouter::DispatchEvent(
     Profile* profile,
     const char* event_name,
     scoped_ptr<ListValue> args,
     EventRouter::UserGestureState user_gesture) {
   if (!profile_->IsSameProfile(profile) ||
       !extensions::ExtensionSystem::Get(profile)->event_router())
     return;
 
   scoped_ptr<Event> event(new Event(event_name, args.Pass()));
@@ skipping 95 matching lines @@
     NavigationController* source_controller =
         content::Source<NavigationController>(source).ptr();
     TabUpdated(source_controller->GetWebContents(), true);
   } else if (type == content::NOTIFICATION_WEB_CONTENTS_DESTROYED) {
     // Tab was destroyed after being detached (without being re-attached).
     WebContents* contents = content::Source<WebContents>(source).ptr();
     registrar_.Remove(this, content::NOTIFICATION_NAV_ENTRY_COMMITTED,
         content::Source<NavigationController>(&contents->GetController()));
     registrar_.Remove(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
         content::Source<WebContents>(contents));
+    registrar_.Remove(this, chrome::NOTIFICATION_FAVICON_UPDATED,
+        content::Source<WebContents>(contents));
+  } else if (type == chrome::NOTIFICATION_FAVICON_UPDATED) {
+    WebContents* contents = content::Source<WebContents>(source).ptr();
+    const bool* icon_url_changed = content::Details<bool>(details).ptr();
+    FaviconUrlUpdated(contents, icon_url_changed);
   } else {
     NOTREACHED();
   }
 }
 
 void BrowserEventRouter::TabChangedAt(WebContents* contents,
                                       int index,
                                       TabChangeType change_type) {
   TabUpdated(contents, false);
 }
@@ skipping 128 matching lines @@
 
     DispatchEventToExtension(profile,
                              extension_action.extension_id(),
                              event_name,
                              args.Pass(),
                              EventRouter::USER_GESTURE_ENABLED);
   }
 }
 
 }  // namespace extensions