chrome.tabs.onUpdated doesn't called with 'loading' twice.

chrome/browser/extensions/api/tabs/tabs_event_router.cc

 // Copyright 2013 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/api/tabs/tabs_event_router.h"
 
 #include "base/json/json_writer.h"
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.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/extension_tab_util.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/extensions/extension_constants.h"
 #include "components/favicon/content/content_favicon_driver.h"
 #include "content/public/browser/favicon_status.h"
 #include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/navigation_details.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"
 
 using base::DictionaryValue;
 using base::ListValue;
 using base::FundamentalValue;
 using content::NavigationController;
 using content::WebContents;
@@ skipping 55 matching lines @@
                                 tabs_constants::kStatusValueLoading);
 
   if (contents->GetURL() != url_) {
     url_ = contents->GetURL();
     changed_properties->SetString(tabs_constants::kUrlKey, url_.spec());
   }
 
   return changed_properties;
 }
 
+base::DictionaryValue* TabsEventRouter::TabEntry::UpdateUrl(
+    const WebContents* contents) {
+  if (contents->GetURL() == url_)
+    return nullptr;
+
+  url_ = contents->GetURL();
+
+  // Send updated URL only.
+  base::DictionaryValue* changed_properties = new base::DictionaryValue();

[not at google - send to devlin, 2015/07/01 15:57:15]

To imply type-safe pointer ownership transfer to the caller, make this a
scoped_ptr<base::DictionaryValue> then return changed_properties.Pass() from
this function.

And sorry for the small amount of churn, but could you change UpdateLoadState
and DidNavigate to do the same?

[limasdf, 2015/07/06 17:14:44]

Done. Yeah they should be.

+  changed_properties->SetString(tabs_constants::kUrlKey, url_.spec());
+
+  return changed_properties;
+}
+
 TabsEventRouter::TabsEventRouter(Profile* profile)
     : profile_(profile), favicon_scoped_observer_(this) {
   DCHECK(!profile->IsOffTheRecord());
 
   BrowserList::AddObserver(this);
 
   // Init() can happen after the browser is running, so catch up with any
   // windows that already exist.
   for (chrome::BrowserIterator it; !it.done(); it.Next()) {
     RegisterForBrowserNotifications(*it);
@@ skipping 24 matching lines @@
   // Start listening to TabStripModel events for this browser.
   TabStripModel* tab_strip = browser->tab_strip_model();
   tab_strip->AddObserver(this);
 
   for (int i = 0; i < tab_strip->count(); ++i) {
     RegisterForTabNotifications(tab_strip->GetWebContentsAt(i));
   }
 }
 
 void TabsEventRouter::RegisterForTabNotifications(WebContents* contents) {
+  // TODO(limasdf): Attach WebContentsObservers to |contents| rather than
+  //                registering for notifications.

[not at google - send to devlin, 2015/07/01 15:57:15]

Typically we don't indent comments like this.

[limasdf, 2015/07/06 17:14:44]

Done.

   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));
@@ skipping 233 matching lines @@
                    new FundamentalValue(to_index));
   args->Append(object_args);
 
   Profile* profile = Profile::FromBrowserContext(contents->GetBrowserContext());
   DispatchEvent(profile,
                 tabs::OnMoved::kEventName,
                 args.Pass(),
                 EventRouter::USER_GESTURE_UNKNOWN);
 }
 
-void TabsEventRouter::TabUpdated(WebContents* contents, bool did_navigate) {
+void TabsEventRouter::TabUpdated(WebContents* contents,
+                                 api::tabs::TabStatus status) {
   TabEntry* entry = GetTabEntry(contents);
   scoped_ptr<base::DictionaryValue> changed_properties;
 
   if (!entry)
     return;
 
-  if (did_navigate)
-    changed_properties.reset(entry->DidNavigate(contents));
-  else
-    changed_properties.reset(entry->UpdateLoadState(contents));
+  switch (status) {
+    case api::tabs::TAB_STATUS_LOADING:
+      changed_properties.reset(entry->DidNavigate(contents));

[not at google - send to devlin, 2015/07/01 15:57:15]

and then, following on from my comment above, these would simply be:
  changed_properties = entry->DidNavigate(contents);

[limasdf, 2015/07/06 17:14:44]

Done.

+      break;
+    case api::tabs::TAB_STATUS_COMPLETE:
+      changed_properties.reset(entry->UpdateLoadState(contents));
+      break;
+    case api::tabs::TAB_STATUS_NONE:
+      changed_properties.reset(entry->UpdateUrl(contents));

[not at google - send to devlin, 2015/07/01 15:57:15]

It's actually not clear to me - or it wouldn't be outside the context of this CL
- why a NONE status implies the URL was updated. In other words, why the
*current* tab status (which in the case of NONE is very general) implies
something about the *change to the URL* (which is very specific).

For example - take the bug about the title changing -
https://code.google.com/p/chromium/issues/detail?id=458839. How would that fit
in here? You'd need to pass the |status| in as NONE but then how do you know if
it's a URL change, versus a title change?

I think the problem is that we're trying to distinguish between 2 different
kinds of updates, where really we shouldn't. Instead of those 3 methods
DidNavigate/UpdateLoadState/UpdateUrl we should just have an "Update" method and
a "Reset" method. "Reset" would reset all the state to its default and set
change values for anything that changed. "Update" would detect what has changed
by holding onto the previous values, in this case, the load state
(api::tabs::TabStatus) and a URL:

scoped_ptr<DictionaryValue> changed(new DictionaryValue);
if (status_ == 'loading' && !contents->IsLoading()) {
  // load status has changed from 'loading' to 'complete'.
  changed->SetString('status', status_);
  entry->tab_status = 'complete';
}
GURL new_url = contents->GetCommittedURL();
if (url_ != new_url) {
  // URL has changed. This may be because the loading state changed,
  // or it may be an in-page navigation like a #-navigation or
  // history.push/replaceState.
  changed->SetString('url', new_url);
  url_ = new_url;
}
// Then later, you or I or somebody else can add:
std::string new_title = ...;
if (title_ -> new_title) {
  changed->SetString('title', new_title);
  title_ = new_title;
}
return changed.Pass();

Does that make sense? Would it work? Sorry for this big change!

[limasdf, 2015/07/06 17:14:44]

Agree. I tried to have "Update" method instead of 3 method, but it's impossible.
because "UpdateLoadState" is called from TabChangedAt which is called
frequently.

[not at google - send to devlin, 2015/07/06 19:33:19]

Why does that make it impossible?

[limasdf, 2015/07/21 18:28:54]

I tried to make single "Update" method(See patch set #4). 'complete' and 'url'
are possible. but 'loading' is difficult.

I have tried below

condition#1 :  if (status_ != "loading" && contents->IsLoading() ) 
 - FAIL. because IsLoading is changed frequently. that result in  loading,
complete, loading, complete. (example: chrome://newtab)

condition#2:  if (url_ != contents->GetCommittedURL() && status_ != "loading" &&
contents->IsLoading() ) 
 - FAIL. refresh(F5) doesn't fire onUpdate.

+      break;
+    default:
+      NOTREACHED();
+  }
 
   if (changed_properties)
     DispatchTabUpdatedEvent(contents, changed_properties.Pass());
 }
 
 void TabsEventRouter::FaviconUrlUpdated(WebContents* contents) {
     content::NavigationEntry* entry =
         contents->GetController().GetVisibleEntry();
     if (!entry || !entry->GetFavicon().valid)
       return;
@@ skipping 71 matching lines @@
   std::map<int, TabEntry>::iterator i = tab_entries_.find(tab_id);
   if (tab_entries_.end() == i)
     return NULL;
   return &i->second;
 }
 
 void TabsEventRouter::Observe(int type,
                               const content::NotificationSource& source,
                               const content::NotificationDetails& details) {
   if (type == content::NOTIFICATION_NAV_ENTRY_COMMITTED) {
+    content::LoadCommittedDetails* commit =
+        content::Details<content::LoadCommittedDetails>(details).ptr();
     NavigationController* source_controller =
         content::Source<NavigationController>(source).ptr();
-    TabUpdated(source_controller->GetWebContents(), true);
+    if (commit->type == content::NavigationType::NAVIGATION_TYPE_IN_PAGE) {

[not at google - send to devlin, 2015/07/01 15:57:15]

It may be simpler to just do:

if (commit->is_in_page) {
  ...
} else {
  ...
}

+      // 'History.ReplaceState()' falls here. Just update url only.

[not at google - send to devlin, 2015/07/01 15:57:15]

It's not just that, it's also hash fragment navigation? I'd make the comment
more like "In-page navigation includes #-navigation and
history.push/replaceState.

(and yep I see that you said that pushState already "works" - which is
surprising to me)

+      TabUpdated(source_controller->GetWebContents(),
+                 api::tabs::TAB_STATUS_NONE);
+    } else {
+      TabUpdated(source_controller->GetWebContents(),
+                 api::tabs::TAB_STATUS_LOADING);
+    }
   } 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));
     favicon_scoped_observer_.Remove(
         favicon::ContentFaviconDriver::FromWebContents(contents));
   } else {
     NOTREACHED();
   }
 }
 
 void TabsEventRouter::TabChangedAt(WebContents* contents,
                                    int index,
                                    TabChangeType change_type) {
-  TabUpdated(contents, false);
+  TabUpdated(contents, api::tabs::TAB_STATUS_COMPLETE);
 }
 
 void TabsEventRouter::TabReplacedAt(TabStripModel* tab_strip_model,
                                     WebContents* old_contents,
                                     WebContents* new_contents,
                                     int index) {
   // Notify listeners that the next tabs closing or being added are due to
   // WebContents being swapped.
   const int new_tab_id = ExtensionTabUtil::GetTabId(new_contents);
   const int old_tab_id = ExtensionTabUtil::GetTabId(old_contents);
@@ skipping 62 matching lines @@
 void TabsEventRouter::OnFaviconUpdated(favicon::FaviconDriver* favicon_driver,
                                        bool icon_url_changed) {
   if (icon_url_changed) {
     favicon::ContentFaviconDriver* content_favicon_driver =
         static_cast<favicon::ContentFaviconDriver*>(favicon_driver);
     FaviconUrlUpdated(content_favicon_driver->web_contents());
   }
 }
 
 }  // namespace extensions