Implement the onBeforeRetarget event of the webNavigation API

Implement the onBeforeRetarget event of the webNavigation API

TEST=none
BUG=50943
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72163

[jochen (gone - plz use gerrit), 2011-01-17 08:38:00]

please review

depends on http://codereview.chromium.org/6293007/

api tests will follow shortly

[jochen (gone - plz use gerrit), 2011-01-17 17:18:24]

On 2011/01/17 08:38:00, jochen wrote:
> please review
> 
> depends on http://codereview.chromium.org/6293007/
> 
> api tests will follow shortly

Now with tests!

[Paweł Hajdan Jr., 2011-01-17 18:17:23]

Drive-by with a test comment. Please let me take another look before committing.

http://codereview.chromium.org/6363002/diff/3001/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/webnavigation/navigation1/openTab/a.html
(right):

http://codereview.chromium.org/6363002/diff/3001/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/webnavigation/navigation1/openTab/a.html:1:
<script>window.setTimeout('window.open("b.html")', 500);</script>
setTimeout smells flakily. Why do we need it?

[jochen (gone - plz use gerrit), 2011-01-18 18:25:43]

On 2011/01/17 18:17:23, Paweł Hajdan Jr. wrote:
> Drive-by with a test comment. Please let me take another look before
committing.
> 
>
http://codereview.chromium.org/6363002/diff/3001/chrome/test/data/extensions/...
> File
> chrome/test/data/extensions/api_test/webnavigation/navigation1/openTab/a.html
> (right):
> 
>
http://codereview.chromium.org/6363002/diff/3001/chrome/test/data/extensions/...
>
chrome/test/data/extensions/api_test/webnavigation/navigation1/openTab/a.html:1:
> <script>window.setTimeout('window.open("b.html")', 500);</script>
> setTimeout smells flakily. Why do we need it?

I want to open the new window not during the load (i.e. before the dom content
loaded signal), because the exact ordering of events would be random then.

[Paweł Hajdan Jr., 2011-01-18 18:59:31]

The timeout still looks suspicious to me, I'm not convinced this is the only
solution. Does it make additional race conditions possible?

On Tue, Jan 18, 2011 at 19:25, <jochen@chromium.org> wrote:

> On 2011/01/17 18:17:23, Paweł Hajdan Jr. wrote:
>
>> Drive-by with a test comment. Please let me take another look before
>>
> committing.
>
>
>
>
http://codereview.chromium.org/6363002/diff/3001/chrome/test/data/extensions/...
>
>> File
>>
>> chrome/test/data/extensions/api_test/webnavigation/navigation1/openTab/a.html
>> (right):
>>
>
>
>
>
http://codereview.chromium.org/6363002/diff/3001/chrome/test/data/extensions/...
>
>
>
chrome/test/data/extensions/api_test/webnavigation/navigation1/openTab/a.html:1:
>
>> <script>window.setTimeout('window.open("b.html")', 500);</script>
>> setTimeout smells flakily. Why do we need it?
>>
>
> I want to open the new window not during the load (i.e. before the dom
> content
> loaded signal), because the exact ordering of events would be random then.
>
>
>
> http://codereview.chromium.org/6363002/
>

[Matt Perry, 2011-01-18 20:30:31]

> I want to open the new window not during the load (i.e. before the dom
> content
> loaded signal), because the exact ordering of events would be random then.

Could you put the navigation in a window.onload handler, then?

http://codereview.chromium.org/6363002/diff/3001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_helper.cc (right):

http://codereview.chromium.org/6363002/diff/3001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_helper.cc:231:
Source<TabContents>(host->delegate()->GetAsTabContents()),
I don't think we're guaranteed to use a TabContents for this host, which means
this might return NULL.

[yzshen1, 2011-01-18 21:49:24]

http://codereview.chromium.org/6363002/diff/3001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_webnavigation_api.cc (right):

http://codereview.chromium.org/6363002/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_webnavigation_api.cc:298: if (frame_id == 0
|| !navigation_state_.CanSendEvents(frame_id))
I think it is okay to allow this notification even if
navigation_state_.CanSendEvents(frame_id) == false, since it is not part of a
navigation event sequence within the frame that frame_id refers to.

http://codereview.chromium.org/6363002/diff/3001/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/webnavigation/navigation1/test.html
(right):

http://codereview.chromium.org/6363002/diff/3001/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/webnavigation/navigation1/test.html:451:
sourceUrl: getURL('openTab/a.html'),
[Nice to have, optional]
Test the scenario that a subframe opens the new window/tab, in which case
sourceUrl should be the URL of the subframe.

[jochen (gone - plz use gerrit), 2011-01-20 22:46:27]

On 2011/01/18 20:30:31, Matt Perry wrote:
> > I want to open the new window not during the load (i.e. before the dom
> > content
> > loaded signal), because the exact ordering of events would be random then.
> 
> Could you put the navigation in a window.onload handler, then?
> 

The race will just begin later.

If the window.open is within a <script> tag, the window is opened before the DOM
content is loaded. If it's opened in onload, it's opened after the DOM content
is loaded (but there's still the onCompleted signal pending).

I guess the right way to do this would be to not compare against a golden copy,
but define which signals need to come and define a partial order on these
events, then record the actual events and check that the partial order holds and
all events arrived.

Pawel, this extension API's test suite is full of 500ms delays. Given that this
would require a bigger rewrite, is it ok to first land this CL so it's in before
the branch point, and refactor the test suite in a separate CL?

>
http://codereview.chromium.org/6363002/diff/3001/chrome/browser/renderer_host...
> File chrome/browser/renderer_host/render_widget_helper.cc (right):
> 
>
http://codereview.chromium.org/6363002/diff/3001/chrome/browser/renderer_host...
> chrome/browser/renderer_host/render_widget_helper.cc:231:
> Source<TabContents>(host->delegate()->GetAsTabContents()),
> I don't think we're guaranteed to use a TabContents for this host, which means
> this might return NULL.

[Matt Perry, 2011-01-20 22:53:56]

http://codereview.chromium.org/6363002/diff/3001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_helper.cc (right):

http://codereview.chromium.org/6363002/diff/3001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_helper.cc:231:
Source<TabContents>(host->delegate()->GetAsTabContents()),
On 2011/01/18 20:30:31, Matt Perry wrote:
> I don't think we're guaranteed to use a TabContents for this host, which means
> this might return NULL.

Don't forget to handle a NULL return value here. It's probably OK to just ignore
the event in that case.

[jochen (gone - plz use gerrit), 2011-01-20 23:14:45]

On 2011/01/20 22:53:56, Matt Perry wrote:
>
http://codereview.chromium.org/6363002/diff/3001/chrome/browser/renderer_host...
> File chrome/browser/renderer_host/render_widget_helper.cc (right):
> 
>
http://codereview.chromium.org/6363002/diff/3001/chrome/browser/renderer_host...
> chrome/browser/renderer_host/render_widget_helper.cc:231:
> Source<TabContents>(host->delegate()->GetAsTabContents()),
> On 2011/01/18 20:30:31, Matt Perry wrote:
> > I don't think we're guaranteed to use a TabContents for this host, which
means
> > this might return NULL.
> 
> Don't forget to handle a NULL return value here. It's probably OK to just
ignore
> the event in that case.

updated, incl. the additional test Yuzhu wanted

[Matt Perry, 2011-01-20 23:16:04]

LGTM

[yzshen1, 2011-01-21 01:46:57]

LGTM

[Paweł Hajdan Jr., 2011-01-21 09:12:01]

On Thu, Jan 20, 2011 at 23:46, <jochen@chromium.org> wrote:

> Pawel, this extension API's test suite is full of 500ms delays. Given that
> this
> would require a bigger rewrite, is it ok to first land this CL so it's in
> before
> the branch point, and refactor the test suite in a separate CL?


Hmm, okay. (OK)