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

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

Index: chrome/browser/extensions/api/tabs/tabs_event_router.cc
diff --git a/chrome/browser/extensions/api/tabs/tabs_event_router.cc b/chrome/browser/extensions/api/tabs/tabs_event_router.cc
index 6f5d5207b3a717a48fc2ac4b6d0551bd44d27064..b2fc108aab36b064dd2620f5c77bd8087dbbf3cb 100644
--- a/chrome/browser/extensions/api/tabs/tabs_event_router.cc
+++ b/chrome/browser/extensions/api/tabs/tabs_event_router.cc
@@ -20,6 +20,7 @@
 #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"
@@ -95,6 +96,20 @@ base::DictionaryValue* TabsEventRouter::TabEntry::DidNavigate(
   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());
@@ -139,6 +154,8 @@ void TabsEventRouter::RegisterForBrowserNotifications(Browser* browser) {
 }
 
 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()));
@@ -392,17 +409,27 @@ void TabsEventRouter::TabMoved(WebContents* contents,
                 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());
@@ -494,9 +521,18 @@ 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();
@@ -514,7 +550,7 @@ void TabsEventRouter::Observe(int type,
 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,