fix chrome.tabs.onUpdated bugs, add browsertest

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
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=47717

[Matt Perry, 2010-05-18 23:36:32]

Mostly LG. Some comments.

http://codereview.chromium.org/2111010/diff/1/3
File chrome/browser/extensions/extension_browser_event_router.h (right):

http://codereview.chromium.org/2111010/diff/1/3#newcode134
chrome/browser/extensions/extension_browser_event_router.h:134: //
NAV_ENTRY_COMMITTED, and is always set to true on the first subsequent
set to false?

http://codereview.chromium.org/2111010/diff/1/9
File chrome/test/data/extensions/api_test/tabs/on_updated/content_script.js
(right):

http://codereview.chromium.org/2111010/diff/1/9#newcode1
chrome/test/data/extensions/api_test/tabs/on_updated/content_script.js:1: var
scriptMessageEvent = document.createEvent("Event");
Is this file used at all? I don't understand what it's trying to do.

http://codereview.chromium.org/2111010/diff/1/15
File chrome/test/data/extensions/api_test/tabs/on_updated/test.html (right):

http://codereview.chromium.org/2111010/diff/1/15#newcode80
chrome/test/data/extensions/api_test/tabs/on_updated/test.html:80: //
TODO(rafaelw) -- This is failing on linux/views.
Doesn't chromeos use linux/views? If so, I think you'll have to disable this, at
least for chromeos.

http://codereview.chromium.org/2111010/diff/1/15#newcode88
chrome/test/data/extensions/api_test/tabs/on_updated/test.html:88: { status:
'loading' },
I'm confused. From your description, I thought iframe navigations after the
first load wouldn't count as load events.

[rafaelw, 2010-05-18 23:57:04]

http://codereview.chromium.org/2111010/diff/1/3
File chrome/browser/extensions/extension_browser_event_router.h (right):

http://codereview.chromium.org/2111010/diff/1/3#newcode134
chrome/browser/extensions/extension_browser_event_router.h:134: //
NAV_ENTRY_COMMITTED, and is always set to true on the first subsequent
Yes. thank you.

On 2010/05/18 23:36:32, Matt Perry wrote:
> set to false?

http://codereview.chromium.org/2111010/diff/1/9
File chrome/test/data/extensions/api_test/tabs/on_updated/content_script.js
(right):

http://codereview.chromium.org/2111010/diff/1/9#newcode1
chrome/test/data/extensions/api_test/tabs/on_updated/content_script.js:1: var
scriptMessageEvent = document.createEvent("Event");
Woops. copy error. thanks for the catch.

On 2010/05/18 23:36:32, Matt Perry wrote:
> Is this file used at all? I don't understand what it's trying to do.

http://codereview.chromium.org/2111010/diff/1/15
File chrome/test/data/extensions/api_test/tabs/on_updated/test.html (right):

http://codereview.chromium.org/2111010/diff/1/15#newcode80
chrome/test/data/extensions/api_test/tabs/on_updated/test.html:80: //
TODO(rafaelw) -- This is failing on linux/views.
Damn. slopy today. Sorry. This was failing, but I figured out why. Just missed
removing the comment when I commented it back in.

On 2010/05/18 23:36:32, Matt Perry wrote:
> Doesn't chromeos use linux/views? If so, I think you'll have to disable this,
at
> least for chromeos.

http://codereview.chromium.org/2111010/diff/1/15#newcode88
chrome/test/data/extensions/api_test/tabs/on_updated/test.html:88: { status:
'loading' },
It depends on whether the navigation of the iframe happens during it's onload
handler (as it turns out). I've expanded the test case to include both during &
after onload navigation and added comments explaining it.

On 2010/05/18 23:36:32, Matt Perry wrote:
> I'm confused. From your description, I thought iframe navigations after the
> first load wouldn't count as load events.

[Matt Perry, 2010-05-19 00:03:21]

LGTM with comments addressed.

http://codereview.chromium.org/2111010/diff/7001/8001
File chrome/browser/extensions/extension_browser_event_router.cc (right):

http://codereview.chromium.org/2111010/diff/7001/8001#newcode32
chrome/browser/extensions/extension_browser_event_router.cc:32: // We only want
to respond to the first change from loading to !loading.
This comment is slightly misleading given the behavior. We respond to subsequent
changes from loading -> !loading if the previous event was "loading". Right?

http://codereview.chromium.org/2111010/diff/7001/8009
File
chrome/test/data/extensions/api_test/tabs/on_updated/iframeNavigated/iframe1.html
(right):

http://codereview.chromium.org/2111010/diff/7001/8009#newcode4
chrome/test/data/extensions/api_test/tabs/on_updated/iframeNavigated/iframe1.html:4:
// Navigate during the onload that 'complete' status won't fire.
nit: "so that"

http://codereview.chromium.org/2111010/diff/7001/8014
File chrome/test/data/extensions/api_test/tabs/on_updated/test.html (right):

http://codereview.chromium.org/2111010/diff/7001/8014#newcode86
chrome/test/data/extensions/api_test/tabs/on_updated/test.html:86: //
-iframe2.html does a setTimeout to navigate itself to iframe3.html. This
This seems like it might be flaky, if the timeout happens to run before the
'complete' event is fired? Maybe not. Just an FYI to watch for it.

[rafaelw, 2010-05-19 00:21:50]

http://codereview.chromium.org/2111010/diff/7001/8001
File chrome/browser/extensions/extension_browser_event_router.cc (right):

http://codereview.chromium.org/2111010/diff/7001/8001#newcode32
chrome/browser/extensions/extension_browser_event_router.cc:32: // We only want
to respond to the first change from loading to !loading.
Further clarified and member renamed.

On 2010/05/19 00:03:22, Matt Perry wrote:
> This comment is slightly misleading given the behavior. We respond to
subsequent
> changes from loading -> !loading if the previous event was "loading". Right?

http://codereview.chromium.org/2111010/diff/7001/8009
File
chrome/test/data/extensions/api_test/tabs/on_updated/iframeNavigated/iframe1.html
(right):

http://codereview.chromium.org/2111010/diff/7001/8009#newcode4
chrome/test/data/extensions/api_test/tabs/on_updated/iframeNavigated/iframe1.html:4:
// Navigate during the onload that 'complete' status won't fire.
On 2010/05/19 00:03:22, Matt Perry wrote:
> nit: "so that"

Done.

http://codereview.chromium.org/2111010/diff/7001/8014
File chrome/test/data/extensions/api_test/tabs/on_updated/test.html (right):

http://codereview.chromium.org/2111010/diff/7001/8014#newcode86
chrome/test/data/extensions/api_test/tabs/on_updated/test.html:86: //
-iframe2.html does a setTimeout to navigate itself to iframe3.html. This
Ok. I'll be careful....

On 2010/05/19 00:03:22, Matt Perry wrote:
> This seems like it might be flaky, if the timeout happens to run before the
> 'complete' event is fired? Maybe not. Just an FYI to watch for it.