fix chrome.tabs.onUpdated bugs, add browsertest.

This CL addresses a few issues related to the behavior of the onUpdated event.

Issue 1: The page re-entering the load state and the completing was causing multiple 'complete' status events to be fired. We now only report the first 'complete' after the didNavigate message is fired (iframe navigation, for example).

Issue 2: We were initializing the URL when the TabEntry was created, and this caused us to fail to send the url with the first navigation because we thought it wasn't changing.

BUG=27208,37149

Review URL: http://codereview.chromium.org/2111010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@47717 0039d316-1c4b-4281-b951-d872f2087c98

14 files changed, 195 insertions, 83 deletions

diff --git a/chrome/browser/extensions/extension_browser_event_router.cc b/chrome/browser/extensions/extension_browser_event_router.cc
index 52d26da..1932552 100644
--- a/chrome/browser/extensions/extension_browser_event_router.cc
+++ b/chrome/browser/extensions/extension_browser_event_router.cc
@@ -22,61 +22,39 @@ namespace tab_keys = extension_tabs_module_constants;
 namespace page_action_keys = extension_page_actions_module_constants;
 
 ExtensionBrowserEventRouter::TabEntry::TabEntry()
-    : state_(ExtensionTabUtil::TAB_COMPLETE),
-      pending_navigate_(false),
+    : complete_waiting_on_load_(false),
       url_() {
 }
 
-ExtensionBrowserEventRouter::TabEntry::TabEntry(const TabContents* contents)
-    : state_(ExtensionTabUtil::TAB_COMPLETE),
-      pending_navigate_(false),
-      url_(contents->GetURL()) {
-  UpdateLoadState(contents);
-}
-
 DictionaryValue* ExtensionBrowserEventRouter::TabEntry::UpdateLoadState(
     const TabContents* contents) {
-  ExtensionTabUtil::TabStatus old_state = state_;
-  state_ = ExtensionTabUtil::GetTabStatus(contents);
-
-  if (old_state == state_)
-    return false;
-
-  if (state_ == ExtensionTabUtil::TAB_LOADING) {
-    // Do not send "loading" state changed now. Wait for navigate so the new
-    // url is available.
-    pending_navigate_ = true;
+  // The tab may go in & out of loading (for instance if iframes navigate).
+  // We only want to respond to the first change from loading to !loading after
+  // the NAV_ENTRY_COMMITTED was fired.
+  if (!complete_waiting_on_load_ || contents->is_loading())
     return NULL;
 
-  } else if (state_ == ExtensionTabUtil::TAB_COMPLETE) {
-    // Send "complete" state change.
-    DictionaryValue* changed_properties = new DictionaryValue();
-    changed_properties->SetString(tab_keys::kStatusKey,
-        tab_keys::kStatusValueComplete);
-    return changed_properties;
-
-  } else {
-    NOTREACHED();
-    return NULL;
-  }
+  // Send "complete" state change.
+  complete_waiting_on_load_ = false;
+  DictionaryValue* changed_properties = new DictionaryValue();
+  changed_properties->SetString(tab_keys::kStatusKey,
+      tab_keys::kStatusValueComplete);
+  return changed_properties;
 }
 
 DictionaryValue* ExtensionBrowserEventRouter::TabEntry::DidNavigate(
     const TabContents* contents) {
-  if (!pending_navigate_)
-    return NULL;
-
+  // Send "loading" state change.
+  complete_waiting_on_load_ = true;
   DictionaryValue* changed_properties = new DictionaryValue();
   changed_properties->SetString(tab_keys::kStatusKey,
       tab_keys::kStatusValueLoading);
 
-  GURL new_url = contents->GetURL();
-  if (new_url != url_) {
-    url_ = new_url;
+  if (contents->GetURL() != url_) {
+    url_ = contents->GetURL();
     changed_properties->SetString(tab_keys::kUrlKey, url_.spec());
   }
 
-  pending_navigate_ = false;
   return changed_properties;
 }
 
@@ -133,7 +111,7 @@ void ExtensionBrowserEventRouter::Init() {
       for (int i = 0; i < browser->tabstrip_model()->count(); ++i) {
         TabContents* contents = browser->tabstrip_model()->GetTabContentsAt(i);
         int tab_id = ExtensionTabUtil::GetTabId(contents);
-        tab_entries_[tab_id] = TabEntry(contents);
+        tab_entries_[tab_id] = TabEntry();
       }
     }
   }
@@ -225,7 +203,7 @@ void ExtensionBrowserEventRouter::TabInsertedAt(TabContents* contents,
   // If tab is new, send created event.
   int tab_id = ExtensionTabUtil::GetTabId(contents);
   if (tab_entries_.find(tab_id) == tab_entries_.end()) {
-    tab_entries_[tab_id] = TabEntry(contents);
+    tab_entries_[tab_id] = TabEntry();
 
     TabCreatedAt(contents, index, foreground);
     return;
diff --git a/chrome/browser/extensions/extension_browser_event_router.h b/chrome/browser/extensions/extension_browser_event_router.h
index 13c8ead..f9b2367 100644
--- a/chrome/browser/extensions/extension_browser_event_router.h
+++ b/chrome/browser/extensions/extension_browser_event_router.h
@@ -115,13 +115,6 @@ class ExtensionBrowserEventRouter : public TabStripModelObserver,
     // std::map<> by value.
     TabEntry();
 
-    // Create a new tab entry whose initial state is derived from the given
-    // tab contents.
-    explicit TabEntry(const TabContents* contents);
-
-    // Returns the current state of the tab.
-    ExtensionTabUtil::TabStatus state() const { return state_; }
-
     // Update the load state of the tab based on its TabContents.  Returns true
     // if the state changed, false otherwise.  Whether the state has changed or
     // not is used to determine if events needs to be sent to extensions during
@@ -136,13 +129,11 @@ class ExtensionBrowserEventRouter : public TabStripModelObserver,
     DictionaryValue* DidNavigate(const TabContents* contents);
 
    private:
-    // Tab state used for last notification to extensions.
-    ExtensionTabUtil::TabStatus state_;
-
-    // Remember that the LOADING state has been captured, but not yet reported
-    // because it is waiting on the navigation event to know what the
-    // destination url is.
-    bool pending_navigate_;
+    // Whether we are waiting to fire the 'complete' status change. This will
+    // occur the first time the TabContents stops loading after the
+    // NAV_ENTRY_COMMITTED was fired. The tab may go back into and out of the
+    // loading state subsequently, but we will ignore those changes.
+    bool complete_waiting_on_load_;
 
     GURL url_;
   };
diff --git a/chrome/browser/extensions/extension_tabs_apitest.cc b/chrome/browser/extensions/extension_tabs_apitest.cc
index 7da6dad..d271af0 100644
--- a/chrome/browser/extensions/extension_tabs_apitest.cc
+++ b/chrome/browser/extensions/extension_tabs_apitest.cc
@@ -37,3 +37,9 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, CaptureVisibleTab) {
 
   ASSERT_TRUE(RunExtensionTest("tabs/capture_visible_tab")) << message_;
 }
+
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, TabsOnUpdated) {
+  ASSERT_TRUE(StartHTTPServer());
+
+  ASSERT_TRUE(RunExtensionTest("tabs/on_updated")) << message_;
+}
diff --git a/chrome/browser/extensions/extension_tabs_module.cc b/chrome/browser/extensions/extension_tabs_module.cc
index 6912e25..771ad23 100644
--- a/chrome/browser/extensions/extension_tabs_module.cc
+++ b/chrome/browser/extensions/extension_tabs_module.cc
@@ -78,23 +78,8 @@ int ExtensionTabUtil::GetTabId(const TabContents* tab_contents) {
   return tab_contents->controller().session_id().id();
 }
 
-ExtensionTabUtil::TabStatus ExtensionTabUtil::GetTabStatus(
-    const TabContents* tab_contents) {
-  return tab_contents->is_loading() ? TAB_LOADING : TAB_COMPLETE;
-}
-
-std::string ExtensionTabUtil::GetTabStatusText(TabStatus status) {
-  std::string text;
-  switch (status) {
-    case TAB_LOADING:
-      text = keys::kStatusValueLoading;
-      break;
-    case TAB_COMPLETE:
-      text = keys::kStatusValueComplete;
-      break;
-  }
-
-  return text;
+std::string ExtensionTabUtil::GetTabStatusText(bool is_loading) {
+  return is_loading ? keys::kStatusValueLoading : keys::kStatusValueComplete;
 }
 
 int ExtensionTabUtil::GetWindowIdOfTab(const TabContents* tab_contents) {
@@ -130,22 +115,20 @@ ListValue* ExtensionTabUtil::CreateTabList(const Browser* browser) {
 
 DictionaryValue* ExtensionTabUtil::CreateTabValue(
     const TabContents* contents, TabStripModel* tab_strip, int tab_index) {
-  TabStatus status = GetTabStatus(contents);
-
   DictionaryValue* result = new DictionaryValue();
   result->SetInteger(keys::kIdKey, ExtensionTabUtil::GetTabId(contents));
   result->SetInteger(keys::kIndexKey, tab_index);
   result->SetInteger(keys::kWindowIdKey,
                      ExtensionTabUtil::GetWindowIdOfTab(contents));
   result->SetString(keys::kUrlKey, contents->GetURL().spec());
-  result->SetString(keys::kStatusKey, GetTabStatusText(status));
+  result->SetString(keys::kStatusKey, GetTabStatusText(contents->is_loading()));
   result->SetBoolean(keys::kSelectedKey,
                      tab_strip && tab_index == tab_strip->selected_index());
   result->SetString(keys::kTitleKey, UTF16ToWide(contents->GetTitle()));
   result->SetBoolean(keys::kIncognitoKey,
                      contents->profile()->IsOffTheRecord());
 
-  if (status != TAB_LOADING) {
+  if (!contents->is_loading()) {
     NavigationEntry* entry = contents->controller().GetActiveEntry();
     if (entry) {
       if (entry->favicon().is_valid())
diff --git a/chrome/browser/extensions/extension_tabs_module.h b/chrome/browser/extensions/extension_tabs_module.h
index 8e82981..432ee22 100644
--- a/chrome/browser/extensions/extension_tabs_module.h
+++ b/chrome/browser/extensions/extension_tabs_module.h
@@ -21,17 +21,9 @@ class TabStripModel;
 
 class ExtensionTabUtil {
  public:
-  // Possible tab states.  These states are used to calculate the "status"
-  // property of the Tab object that is used in the extension tab API.
-  enum TabStatus {
-    TAB_LOADING,  // Waiting for the DOM to load.
-    TAB_COMPLETE  // Tab loading and rendering is complete.
-  };
-
   static int GetWindowId(const Browser* browser);
   static int GetTabId(const TabContents* tab_contents);
-  static TabStatus GetTabStatus(const TabContents* tab_contents);
-  static std::string GetTabStatusText(TabStatus status);
+  static std::string GetTabStatusText(bool is_loading);
   static int GetWindowIdOfTab(const TabContents* tab_contents);
   static ListValue* CreateTabList(const Browser* browser);
   static DictionaryValue* CreateTabValue(const TabContents* tab_contents);
diff --git a/chrome/test/data/extensions/api_test/tabs/on_updated/browserThenRendererInitiated/a.html b/chrome/test/data/extensions/api_test/tabs/on_updated/browserThenRendererInitiated/a.html
new file mode 100644
index 0000000..e2fa12e
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/tabs/on_updated/browserThenRendererInitiated/a.html
@@ -0,0 +1,7 @@
+<h1>A (browser initiated)</h1>
+
+<script>
+window.onload = function() {
+  location.href = "b.html";
+}
+</script>
\ No newline at end of file
diff --git a/chrome/test/data/extensions/api_test/tabs/on_updated/browserThenRendererInitiated/b.html b/chrome/test/data/extensions/api_test/tabs/on_updated/browserThenRendererInitiated/b.html
new file mode 100644
index 0000000..64cf281
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/tabs/on_updated/browserThenRendererInitiated/b.html
@@ -0,0 +1 @@
+<h1>B (renderer initiated)</h1>
diff --git a/chrome/test/data/extensions/api_test/tabs/on_updated/iframeNavigated/a.html b/chrome/test/data/extensions/api_test/tabs/on_updated/iframeNavigated/a.html
new file mode 100644
index 0000000..3429290
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/tabs/on_updated/iframeNavigated/a.html
@@ -0,0 +1,3 @@
+<h1>iframes Navigated</h1>
+
+<iframe src="iframe1.html"></iframe>
\ No newline at end of file
diff --git a/chrome/test/data/extensions/api_test/tabs/on_updated/iframeNavigated/iframe1.html b/chrome/test/data/extensions/api_test/tabs/on_updated/iframeNavigated/iframe1.html
new file mode 100644
index 0000000..6e87ff4
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/tabs/on_updated/iframeNavigated/iframe1.html
@@ -0,0 +1,7 @@
+<h1>iframe1</h1>
+<script>
+window.onload = function() {
+  // Navigate during the onload so that 'complete' status won't fire.
+  location.href = "iframe2.html";
+}
+</script>
\ No newline at end of file
diff --git a/chrome/test/data/extensions/api_test/tabs/on_updated/iframeNavigated/iframe2.html b/chrome/test/data/extensions/api_test/tabs/on_updated/iframeNavigated/iframe2.html
new file mode 100644
index 0000000..f1fb9a9
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/tabs/on_updated/iframeNavigated/iframe2.html
@@ -0,0 +1,10 @@
+<h1>iframe2</h1>
+
+<script>
+window.onload = function() {
+  // Navigate after onload so that 'complete' status will fire.
+  setTimeout(function() {
+    location.href = "iframe3.html";
+  }, 0);
+}
+</script>
\ No newline at end of file
diff --git a/chrome/test/data/extensions/api_test/tabs/on_updated/iframeNavigated/iframe3.html b/chrome/test/data/extensions/api_test/tabs/on_updated/iframeNavigated/iframe3.html
new file mode 100644
index 0000000..6904d24
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/tabs/on_updated/iframeNavigated/iframe3.html
@@ -0,0 +1 @@
+<h1>iframe3</h1>
\ No newline at end of file
diff --git a/chrome/test/data/extensions/api_test/tabs/on_updated/internalAnchorNavigated/a.html b/chrome/test/data/extensions/api_test/tabs/on_updated/internalAnchorNavigated/a.html
new file mode 100644
index 0000000..ef2b1ef
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/tabs/on_updated/internalAnchorNavigated/a.html
@@ -0,0 +1,14 @@
+<h1>Internal Anchor</h1>
+
+<a href="#b">b</a>
+<div style="height: 1600px">---SPACE---</div>
+<h2 id="b">B Content</h2>
+
+<script>
+window.onload = function() {
+  // Navigate after the onload, so the 'complete' status will fire.
+  setTimeout(function() {
+    location.href = "#b";
+  }, 0);
+}
+</script>
\ No newline at end of file
diff --git a/chrome/test/data/extensions/api_test/tabs/on_updated/manifest.json b/chrome/test/data/extensions/api_test/tabs/on_updated/manifest.json
new file mode 100644
index 0000000..0a5e419
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/tabs/on_updated/manifest.json
@@ -0,0 +1,7 @@
+{
+  "name": "tabs/on_updated",
+  "version": "0.1",
+  "description": "Tests the behavior of the tabs.onUpdated event with different cases of navigation",
+  "background_page": "test.html",
+  "permissions": ["tabs"]
+}
diff --git a/chrome/test/data/extensions/api_test/tabs/on_updated/test.html b/chrome/test/data/extensions/api_test/tabs/on_updated/test.html
new file mode 100644
index 0000000..284c6f7
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/tabs/on_updated/test.html
@@ -0,0 +1,112 @@
+<script>
+// Description
+
+var expectedEventData;
+var capturedEventData;
+var shouldIgnore;
+
+function expect(data, ignoreFunc) {
+  expectedEventData = data;
+  capturedEventData = [];
+  shouldIgnore = ignoreFunc;
+}
+
+function checkExpectations() {
+  if (capturedEventData.length < expectedEventData.length) {
+    return;
+  }
+  chrome.test.assertEq(JSON.stringify(expectedEventData),
+      JSON.stringify(capturedEventData));
+  chrome.test.succeed();
+}
+
+var getURL = chrome.extension.getURL;
+
+chrome.tabs.onUpdated.addListener(function(tabId, info, tab) {
+  console.log('---onUpdated: ' + info.status + ', ' + info.url);
+  if (shouldIgnore && shouldIgnore(info)) {
+    return;
+  }
+  capturedEventData.push(info);
+  checkExpectations();
+});
+
+chrome.test.runTests([
+  function browserThenRendererInitiated() {
+    // Note that a.html will set it's location.href to b.html, creating a
+    // renderer-initiated navigation.
+    expect([
+      { status: 'loading', url: getURL('browserThenRendererInitiated/a.html') },
+      { status: 'complete' },
+      { status: 'loading', url: getURL('browserThenRendererInitiated/b.html') },
+      { status: 'complete' },
+    ]);
+
+    chrome.tabs.create({ url: getURL('browserThenRendererInitiated/a.html') });    
+  },
+
+  function newTab() {
+    // Test for crbug.com/27208.
+    expect([
+      { status: 'loading', url: 'chrome://newtab/' },
+      { status: 'complete' }
+    ]);
+
+    chrome.tabs.create({ url: 'chrome://newtab/' });    
+  },
+
+  /*
+  // TODO(rafaelw) -- This is disabled because this test is flakey.
+  function updateDuringCreateCallback() {
+    // Test for crbug.com/27204.
+    // We have to ignore anything that comes before the about:blank loading
+    // status.
+    var ignore = true;
+    expect([
+      { status: 'loading', url: 'about:blank' },
+      { status: 'complete' }
+    ], function(info) {
+      if (info.status === 'loading' && info.url === 'about:blank') {
+        ignore = false;
+      }
+      return ignore;
+    });
+
+    chrome.tabs.create({ url: 'chrome://newtab/' }, function(tab) {
+      chrome.tabs.update(tab.id, { url: 'about:blank' });
+    });
+  }, */
+
+  function iframeNavigated() {
+    // The sequence of events goes like this:
+    // -a.html starts loading
+    // -while a.html is loading, iframe1.html (in it's onload) navigates to
+    // iframe2.html. This causes the page to continue to be in the loading state
+    //  so the 'complete' status doesn't fire.
+    // -iframe2.html does a setTimeout to navigate itself to iframe3.html. This
+    // allows the page to stop loading and the 'complete' status to fire, but
+    // when the timeout fires, the pages goes back into the loading state
+    // which causes the new status: 'loading' event to fire without having
+    // changed the url.
+    expect([
+      { status: 'loading', url: getURL('iframeNavigated/a.html') },
+      { status: 'complete' },
+      { status: 'loading' },
+      { status: 'complete' },
+    ]);
+
+    chrome.tabs.create({ url: getURL('iframeNavigated/a.html') });    
+  },
+  
+  function internalAnchorNavigated() {
+    expect([
+      { status: 'loading', url: getURL('internalAnchorNavigated/a.html') },
+      { status: 'complete' },
+      { status: 'loading', url: getURL('internalAnchorNavigated/a.html#b') },
+      { status: 'complete' },
+    ]);
+
+    chrome.tabs.create({ url: getURL('internalAnchorNavigated/a.html') });    
+  }
+]);
+</script>
\ No newline at end of file