Another attempt at fixing dead frames being tracked by webNavigation

Another attempt at fixing dead frames being tracked by webNavigation

- when commiting a main frame load (non fragment/history state), cancel old iframes
- when a cross process navigation is cancelled, send error events for that RVH

Also in this CL
- don't incorrectly set client_redirect on history state updates

BUG=139117
TEST=ExtensionApiTest.WebNavigationCrossProcess


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150533

[jochen (gone - plz use gerrit), 2012-08-03 22:49:00]

Charlie, plz review


Is it correct that history API and reference fragment navigations won't cancel a
pending cross-process navigation?

[Charlie Reis, 2012-08-04 00:32:33]

On 2012/08/03 22:49:00, jochen wrote:
> Is it correct that history API and reference fragment navigations won't cancel
a
> pending cross-process navigation?

What do you mean by history API?  JavaScript-initiated back/forwards (as opposed
to clicking Back/Forward)?  I think all history navigations are capable of
canceling a pending cross-process navigation.

I just did an experiment that shows in-page navigations like pushState and
reference fragments will cancel pending navigations as well.

Anyway, sorry that I don't understand what this function is trying to do, so I'm
having trouble reviewing the fix.

https://chromiumcodereview.appspot.com/10823169/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

https://chromiumcodereview.appspot.com/10823169/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:390:
SendErrorEvents(web_contents(), render_view_host_, frame_id);
Sorry, I don't understand this function.  Why is it sending error events when a
normal navigation is committing?

[jochen (gone - plz use gerrit), 2012-08-04 19:28:38]

I tested a page with <a href="slow loading site" onclick="history.pushState({},
'', 'foo')">link</a> and while it created a new history entry, it didn't cancel
the cross process navigation.

I haven't tested reference fragment navigations. Maybe I should write a test for
it :-/

http://codereview.chromium.org/10823169/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

http://codereview.chromium.org/10823169/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:390:
SendErrorEvents(web_contents(), render_view_host_, frame_id);
On 2012/08/04 00:32:33, creis wrote:
> Sorry, I don't understand this function.  Why is it sending error events when
a
> normal navigation is committing?

iframes of a previous tab can still navigate while the main frame is navigating.
as soon as the main frame commits, we won't get signals for the old iframes any
more, so we should send error events for still pending navigatios.

[jochen (gone - plz use gerrit), 2012-08-06 14:55:27]

now with tests!

Seems that reference fragment navigations and history updates indeed cancel
pending navigations, but do not cancel ongoing iframes

Adding Dominic as extension expert

[jochen (gone - plz use gerrit), 2012-08-06 17:48:10]

-battre
+mpcomplete to have a better chance for a review until tomorrow :)

[jochen (gone - plz use gerrit), 2012-08-06 18:10:50]

(try results are on patch set 2)

[Matt Perry, 2012-08-06 20:00:51]

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:386: if
(is_main_frame && render_view_host_ == render_view_host) {
this branch confuses to me. how could the current rvh have a main frame
navigation? shouldn't it have been pending first?

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:389: if
(pending_render_view_host_) {
is it ever possible for pending_rvh_ to be the same as rvh_ here?

http://codereview.chromium.org/10823169/diff/12001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/webnavigation/crossProcess/b.html
(right):

http://codereview.chromium.org/10823169/diff/12001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/webnavigation/crossProcess/b.html:1:
<html></html>
nit: it would be easier to follow the logic of these tests if this file was
named something like "final.html" or "empty.html".

[Charlie Reis, 2012-08-06 20:04:13]

On 2012/08/06 14:55:27, jochen wrote:
> Seems that reference fragment navigations and history updates indeed cancel
> pending navigations, but do not cancel ongoing iframes

Please update the CL description for this, since it still says they won't cancel
cross-process navigations.

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:387: if
(!is_reference_fragment_navigation && !is_history_navigation)
Please add comments here, since this code is difficult to understand without
them.  Are you saying that we don't need to notify others about subframes in
these two cases because those loads will proceed?  That makes sense to me for
reference fragments but not history navigations.  And how does pushState fit
into this?

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
(right):

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:55:
CallMethodOnLoad(const std::string& method,
This is really just an arbitrary string to execute, right?  Maybe the name
should be ExecuteJavascriptOnLoad, since it's not limited to methods.

Also, ...OnLoad makes it sound like the JavaScript onload event, which happens
after the load commits.  Perhaps we should call it ExecuteJavascriptOnLoadStart.

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:85: if
(validated_url != url_ || !rvh_)
This is an unexpected condition to me.  It looks like rvh_ isn't set until
commit, so this ExecuteJavascript call doesn't happen until the second page load
starts.  Is that correct?  Maybe that should be made clearer in the class-level
comment.

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:400:
CallMethodOnLoad call_method("navigate2()", test_server()->GetURL("slow?30"));
When running this test manually, can you check whether this slow?30 URL actually
runs for 30 seconds, even if the test finishes earlier?  Depending on how the
slow URL works, we could end up blocking for 93 seconds.  (Or the sleeping
process could be alive that long.)

(I've seen this in practice, which is why I wanted to bring it up.  It affected
subsequent tests that tried to use the HTTP server, I believe.)

http://codereview.chromium.org/10823169/diff/12001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/webnavigation/app/manifest.json
(right):

http://codereview.chromium.org/10823169/diff/12001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/webnavigation/app/manifest.json:13:
"http://www.a.com/server-redirect"
Why is this the web extent for the app?  It doesn't include the launch URL,
which should be illegal (http://crbug.com/106123).

http://codereview.chromium.org/10823169/diff/12001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/webnavigation/test_crossProcess.js
(right):

http://codereview.chromium.org/10823169/diff/12001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/webnavigation/test_crossProcess.js:93: //
Redirects through an app extend, should cause two process switches.
nit: extend -> extent

[jochen (gone - plz use gerrit), 2012-08-06 21:08:39]

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:386: if
(is_main_frame && render_view_host_ == render_view_host) {
On 2012/08/06 20:00:51, Matt Perry wrote:
> this branch confuses to me. how could the current rvh have a main frame
> navigation? shouldn't it have been pending first?

pending means that it's in a different process. This mirrors the naming of the
RenderViewHostManager

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:387: if
(!is_reference_fragment_navigation && !is_history_navigation)
On 2012/08/06 20:04:13, creis wrote:
> Please add comments here, since this code is difficult to understand without
> them.  Are you saying that we don't need to notify others about subframes in
> these two cases because those loads will proceed?  That makes sense to me for
> reference fragments but not history navigations.  And how does pushState fit
> into this?

I added a comment and clarified the variable name: if you add a history entry
via history.pushState or replaceState, no navigation is started (there's just a
commit signal, but no load start signal), and iframe navigations aren't
cancelled either.

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:389: if
(pending_render_view_host_) {
On 2012/08/06 20:00:51, Matt Perry wrote:
> is it ever possible for pending_rvh_ to be the same as rvh_ here?

No (see above)

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
(right):

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:55:
CallMethodOnLoad(const std::string& method,
On 2012/08/06 20:04:13, creis wrote:
> This is really just an arbitrary string to execute, right?  Maybe the name
> should be ExecuteJavascriptOnLoad, since it's not limited to methods.
> 
> Also, ...OnLoad makes it sound like the JavaScript onload event, which happens
> after the load commits.  Perhaps we should call it
ExecuteJavascriptOnLoadStart.

Done.

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:85: if
(validated_url != url_ || !rvh_)
On 2012/08/06 20:04:13, creis wrote:
> This is an unexpected condition to me.  It looks like rvh_ isn't set until
> commit, so this ExecuteJavascript call doesn't happen until the second page
load
> starts.  Is that correct?  Maybe that should be made clearer in the
class-level
> comment.

Done.

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:400:
CallMethodOnLoad call_method("navigate2()", test_server()->GetURL("slow?30"));
On 2012/08/06 20:04:13, creis wrote:
> When running this test manually, can you check whether this slow?30 URL
actually
> runs for 30 seconds, even if the test finishes earlier?  Depending on how the
> slow URL works, we could end up blocking for 93 seconds.  (Or the sleeping
> process could be alive that long.)
> 
> (I've seen this in practice, which is why I wanted to bring it up.  It
affected
> subsequent tests that tried to use the HTTP server, I believe.)

Chrome cancels the request immediately, so the test server keeps running and 
throws a "broken pipe" exception, but Chrome immediately stops. I guess that's
ok from a test point of view, as each test starts its own new server?

http://codereview.chromium.org/10823169/diff/12001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/webnavigation/app/manifest.json
(right):

http://codereview.chromium.org/10823169/diff/12001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/webnavigation/app/manifest.json:13:
"http://www.a.com/server-redirect"
On 2012/08/06 20:04:13, creis wrote:
> Why is this the web extent for the app?  It doesn't include the launch URL,
> which should be illegal (http://crbug.com/106123).

I made this the launch URL.

http://codereview.chromium.org/10823169/diff/12001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/webnavigation/crossProcess/b.html
(right):

http://codereview.chromium.org/10823169/diff/12001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/webnavigation/crossProcess/b.html:1:
<html></html>
On 2012/08/06 20:00:51, Matt Perry wrote:
> nit: it would be easier to follow the logic of these tests if this file was
> named something like "final.html" or "empty.html".

Done.

http://codereview.chromium.org/10823169/diff/12001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/webnavigation/test_crossProcess.js
(right):

http://codereview.chromium.org/10823169/diff/12001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/webnavigation/test_crossProcess.js:93: //
Redirects through an app extend, should cause two process switches.
On 2012/08/06 20:04:13, creis wrote:
> nit: extend -> extent

Done.

[Matt Perry, 2012-08-06 21:16:26]

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:386: if
(is_main_frame && render_view_host_ == render_view_host) {
On 2012/08/06 21:08:39, jochen wrote:
> On 2012/08/06 20:00:51, Matt Perry wrote:
> > this branch confuses to me. how could the current rvh have a main frame
> > navigation? shouldn't it have been pending first?
> 
> pending means that it's in a different process. This mirrors the naming of the
> RenderViewHostManager

Actually, the RVHM uses "pending" to refer to a navigation that has begun but
not been committed yet. Navigations are always pending before they commit.

Your logic in AboutToNavigateRenderView seems wrong in this regard. It should
always assign to pending_render_view_host_.

Maybe this confusion is why you and creis seem to disagree on when navigations
are cancelled.

[jochen (gone - plz use gerrit), 2012-08-06 21:21:40]

On 2012/08/06 21:16:26, Matt Perry wrote:
>
http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
> File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
(right):
> 
>
http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
> chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:386: if
> (is_main_frame && render_view_host_ == render_view_host) {
> On 2012/08/06 21:08:39, jochen wrote:
> > On 2012/08/06 20:00:51, Matt Perry wrote:
> > > this branch confuses to me. how could the current rvh have a main frame
> > > navigation? shouldn't it have been pending first?
> > 
> > pending means that it's in a different process. This mirrors the naming of
the
> > RenderViewHostManager
> 
> Actually, the RVHM uses "pending" to refer to a navigation that has begun but
> not been committed yet. Navigations are always pending before they commit.

Sorry, I meant cross site not cross process. If the current RVH navigates,
there's no pending RVH.

> 
> Your logic in AboutToNavigateRenderView seems wrong in this regard. It should
> always assign to pending_render_view_host_.
> 
> Maybe this confusion is why you and creis seem to disagree on when navigations
> are cancelled.

[Charlie Reis, 2012-08-06 21:30:26]

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:386: if
(is_main_frame && render_view_host_ == render_view_host) {
On 2012/08/06 21:16:26, Matt Perry wrote:
> On 2012/08/06 21:08:39, jochen wrote:
> > On 2012/08/06 20:00:51, Matt Perry wrote:
> > > this branch confuses to me. how could the current rvh have a main frame
> > > navigation? shouldn't it have been pending first?
> > 
> > pending means that it's in a different process. This mirrors the naming of
the
> > RenderViewHostManager
> 
> Actually, the RVHM uses "pending" to refer to a navigation that has begun but
> not been committed yet. Navigations are always pending before they commit.

Pending is ambiguous here.  NavigationController has "pending entries" for all
browser-initiated navigations (cross-process or same-process), but RVHM only has
"pending RVHs" for cross-process navigations.  In that sense, Jochen's name
agrees with RVHM.

> Your logic in AboutToNavigateRenderView seems wrong in this regard. It should
> always assign to pending_render_view_host_.
> 
> Maybe this confusion is why you and creis seem to disagree on when navigations
> are cancelled.

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
(right):

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:400:
CallMethodOnLoad call_method("navigate2()", test_server()->GetURL("slow?30"));
On 2012/08/06 21:08:39, jochen wrote:
> On 2012/08/06 20:04:13, creis wrote:
> > When running this test manually, can you check whether this slow?30 URL
> actually
> > runs for 30 seconds, even if the test finishes earlier?  Depending on how
the
> > slow URL works, we could end up blocking for 93 seconds.  (Or the sleeping
> > process could be alive that long.)
> > 
> > (I've seen this in practice, which is why I wanted to bring it up.  It
> affected
> > subsequent tests that tried to use the HTTP server, I believe.)
> 
> Chrome cancels the request immediately, so the test server keeps running and 
> throws a "broken pipe" exception, but Chrome immediately stops. I guess that's
> ok from a test point of view, as each test starts its own new server?

I'd prefer you check with a Chrome trooper about this before landing.  I've run
into trouble before with having lots of old test server processes waiting for
long delays.  Your idea of using a ResourceThrottle that pauses the response for
tests seems much less flaky and less inclined to cause problems on the bots.

If there's time pressure and if they're ok with a TODO and a bug to later
improve the approach, then I'm ok with it as well.

http://codereview.chromium.org/10823169/diff/12001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/webnavigation/app/manifest.json
(right):

http://codereview.chromium.org/10823169/diff/12001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/webnavigation/app/manifest.json:13:
"http://www.a.com/server-redirect"
On 2012/08/06 21:08:39, jochen wrote:
> On 2012/08/06 20:04:13, creis wrote:
> > Why is this the web extent for the app?  It doesn't include the launch URL,
> > which should be illegal (http://crbug.com/106123).
> 
> I made this the launch URL.

Now there's no extent, so we'll never enter the app's process.  I think you just
had them reversed.  The extent should probably be http://www.a.com, right?

[jochen (gone - plz use gerrit), 2012-08-06 21:42:32]

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:386: if
(is_main_frame && render_view_host_ == render_view_host) {
On 2012/08/06 21:30:26, creis wrote:
> On 2012/08/06 21:16:26, Matt Perry wrote:
> > On 2012/08/06 21:08:39, jochen wrote:
> > > On 2012/08/06 20:00:51, Matt Perry wrote:
> > > > this branch confuses to me. how could the current rvh have a main frame
> > > > navigation? shouldn't it have been pending first?
> > > 
> > > pending means that it's in a different process. This mirrors the naming of
> the
> > > RenderViewHostManager
> > 
> > Actually, the RVHM uses "pending" to refer to a navigation that has begun
but
> > not been committed yet. Navigations are always pending before they commit.
> 
> Pending is ambiguous here.  NavigationController has "pending entries" for all
> browser-initiated navigations (cross-process or same-process), but RVHM only
has
> "pending RVHs" for cross-process navigations.  In that sense, Jochen's name
> agrees with RVHM.

I added a comment to the defintion in the header.

> 
> > Your logic in AboutToNavigateRenderView seems wrong in this regard. It
should
> > always assign to pending_render_view_host_.
> > 
> > Maybe this confusion is why you and creis seem to disagree on when
navigations
> > are cancelled.

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
(right):

http://codereview.chromium.org/10823169/diff/12001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:400:
CallMethodOnLoad call_method("navigate2()", test_server()->GetURL("slow?30"));
On 2012/08/06 21:30:26, creis wrote:
> On 2012/08/06 21:08:39, jochen wrote:
> > On 2012/08/06 20:04:13, creis wrote:
> > > When running this test manually, can you check whether this slow?30 URL
> > actually
> > > runs for 30 seconds, even if the test finishes earlier?  Depending on how
> the
> > > slow URL works, we could end up blocking for 93 seconds.  (Or the sleeping
> > > process could be alive that long.)
> > > 
> > > (I've seen this in practice, which is why I wanted to bring it up.  It
> > affected
> > > subsequent tests that tried to use the HTTP server, I believe.)
> > 
> > Chrome cancels the request immediately, so the test server keeps running and

> > throws a "broken pipe" exception, but Chrome immediately stops. I guess
that's
> > ok from a test point of view, as each test starts its own new server?
> 
> I'd prefer you check with a Chrome trooper about this before landing.  I've
run
> into trouble before with having lots of old test server processes waiting for
> long delays.  Your idea of using a ResourceThrottle that pauses the response
for
> tests seems much less flaky and less inclined to cause problems on the bots.
> 
> If there's time pressure and if they're ok with a TODO and a bug to later
> improve the approach, then I'm ok with it as well.

I talked with Nicolas and he pointed out that while it shouldn't affect
following tests, it'll make the bots hang if this happens to be the last test. I
guess I'll give the resource throttle a try

http://codereview.chromium.org/10823169/diff/12001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/webnavigation/app/manifest.json
(right):

http://codereview.chromium.org/10823169/diff/12001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/webnavigation/app/manifest.json:13:
"http://www.a.com/server-redirect"
On 2012/08/06 21:30:26, creis wrote:
> On 2012/08/06 21:08:39, jochen wrote:
> > On 2012/08/06 20:04:13, creis wrote:
> > > Why is this the web extent for the app?  It doesn't include the launch
URL,
> > > which should be illegal (http://crbug.com/106123).
> > 
> > I made this the launch URL.
> 
> Now there's no extent, so we'll never enter the app's process.  I think you
just
> had them reversed.  The extent should probably be http://www.a.com, right?

Without an extent, the launch url's host + /* is used as web extent by default

[jochen (gone - plz use gerrit), 2012-08-07 12:06:56]

ok, so here's a version that uses a resource throttle to delay the loads. A side
effect is that I can continue the load when I need to, so I was able to add
tests with slow loading iframes as well.

[jochen (gone - plz use gerrit), 2012-08-07 12:15:06]

+jam for the addition of the test navigation listener in
chrome/browser/renderer_host (and chrome/ OWNERS approval)

[battre, 2012-08-07 13:34:39]

http://codereview.chromium.org/10823169/diff/4006/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
(right):

http://codereview.chromium.org/10823169/diff/4006/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:56: //
commits.
Add comment, that |script| is supposed to trigger a new navigation to
|util_url|?

http://codereview.chromium.org/10823169/diff/4006/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:65: const
std::string& script)
opt: change the order to delay_url -> script -> until_url to reflect order of
events?

http://codereview.chromium.org/10823169/diff/4006/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/test_navigation_listener.cc (right):

http://codereview.chromium.org/10823169/diff/4006/chrome/browser/renderer_hos...
chrome/browser/renderer_host/test_navigation_listener.cc:31: {
nit: { in previous line

http://codereview.chromium.org/10823169/diff/4006/chrome/browser/renderer_hos...
chrome/browser/renderer_host/test_navigation_listener.cc:43: {
nit: { in previous line

[jochen (gone - plz use gerrit), 2012-08-07 13:41:10]

all done

[jam, 2012-08-07 16:14:11]

do you really need to add testing code to ChromeResourceDispatcherHostDelegate
and to add a getter to BrowserProcess? can't this be done by overriding
ContentBrowserClient in the test and having it give a CRDHD-derived class?

[jochen (gone - plz use gerrit), 2012-08-07 17:46:05]

On 2012/08/07 16:14:11, John Abd-El-Malek wrote:
> do you really need to add testing code to ChromeResourceDispatcherHostDelegate
> and to add a getter to BrowserProcess? can't this be done by overriding
> ContentBrowserClient in the test and having it give a CRDHD-derived class?

ContentBrowserClient does not provide the RDHDelegate, but just notifies with
the g_browser_process that the RDH singleton is now available.

Overriding CBC to not forward this notification and instead setting my own class
seemed more hacky than this approach, because it relies on BrowserProcessImpl
never touching the CRDHD it would own otherwise.

[jam, 2012-08-07 18:01:08]

On 2012/08/07 17:46:05, jochen wrote:
> On 2012/08/07 16:14:11, John Abd-El-Malek wrote:
> > do you really need to add testing code to
ChromeResourceDispatcherHostDelegate
> > and to add a getter to BrowserProcess? can't this be done by overriding
> > ContentBrowserClient in the test and having it give a CRDHD-derived class?
> 
> ContentBrowserClient does not provide the RDHDelegate, but just notifies with
> the g_browser_process that the RDH singleton is now available.
> 
> Overriding CBC to not forward this notification and instead setting my own
class
> seemed more hacky than this approach, because it relies on BrowserProcessImpl
> never touching the CRDHD it would own otherwise.

BrowserProcessImpl doesn't use the CRDHD, it just stores it since it's a
convenient place as a browser process singleton to hold this object which is
also a singleton. 
I would much rather us not modify CRDHD and BrowserProcess for a test if it can
be written otherwise.

[jochen (gone - plz use gerrit), 2012-08-07 20:29:49]

ok, here's a version that replaces ChromeContentBrowserClient to install it's
own ChromeResourceDispatcherHostDelegate. It also register
ChromeBrowserMainExtraParts that manage the lifetime of the RDHDelegate

[jam, 2012-08-07 20:31:32]

test looks good to me, thanks

http://codereview.chromium.org/10823169/diff/6025/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
(right):

http://codereview.chromium.org/10823169/diff/6025/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:303: :
chrome::ChromeContentBrowserClient(),
nit: why?

http://codereview.chromium.org/10823169/diff/6025/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:310: //
It would notify BrowserProcessImpl which would create a CRDHD and other
nit: ChromeResourceDispatcherHostDelegate since not everyone would know the
acronym

[jam, 2012-08-07 20:31:52]

lgtm

[Matt Perry, 2012-08-07 20:36:46]

lgtm

[Charlie Reis, 2012-08-07 21:53:04]

Sorry, took me a bit to understand what test_crossProcess.js was doing, but I
think I follow it now.  LGTM.

http://codereview.chromium.org/10823169/diff/6025/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
(right):

http://codereview.chromium.org/10823169/diff/6025/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:148: //
commits. The idea is that |script| triggers the load of |until_url|.
nit: s/The idea is that |script| triggers/This class expects |script| to
trigger/.

[jochen (gone - plz use gerrit), 2012-08-08 08:31:51]

http://codereview.chromium.org/10823169/diff/6025/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
(right):

http://codereview.chromium.org/10823169/diff/6025/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:148: //
commits. The idea is that |script| triggers the load of |until_url|.
On 2012/08/07 21:53:05, creis wrote:
> nit: s/The idea is that |script| triggers/This class expects |script| to
> trigger/.

Done.

http://codereview.chromium.org/10823169/diff/6025/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:303: :
chrome::ChromeContentBrowserClient(),
On 2012/08/07 20:31:32, John Abd-El-Malek wrote:
> nit: why?

Done.

http://codereview.chromium.org/10823169/diff/6025/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:310: //
It would notify BrowserProcessImpl which would create a CRDHD and other
On 2012/08/07 20:31:32, John Abd-El-Malek wrote:
> nit: ChromeResourceDispatcherHostDelegate since not everyone would know the
> acronym

Done.

[commit-bot: I haz the power, 2012-08-08 08:32:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/10823169/1014

[commit-bot: I haz the power, 2012-08-08 10:46:44]

Change committed as 150533