Update routing id of pending resource requests for reparented iframes.

Update routing id of pending resource requests for reparented iframes.

BUG=55200
TEST=MagicIframeBrowserTest.TransferIframeCloseWindow

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

[Dmitry Titov, 2011-08-13 01:06:00]

Another iteration of updating the routing ids for reparented iframes. This time,
without storing additional Ids.

The WebKit side of this change consists of 2 patches:
https://bugs.webkit.org/show_bug.cgi?id=66165
https://bugs.webkit.org/show_bug.cgi?id=66167

[jam, 2011-08-15 16:31:02]

http://codereview.chromium.org/7647003/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7647003/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/resource_dispatcher_host.cc:648:
TransferRequestToNewPage(filter_->child_id(), msg.routing_id(), request_id);
nit: no need to call out from message dispatcher to another function, just move
the body here. we did that for CancelRequest since it's called by other objects

http://codereview.chromium.org/7647003/diff/1/content/common/resource_dispatc...
File content/common/resource_dispatcher.cc (right):

http://codereview.chromium.org/7647003/diff/1/content/common/resource_dispatc...
content/common/resource_dispatcher.cc:236: new
ResourceHostMsg_TransferRequestToNewPage(new_routing_id,
these two values are the same, so just send one only in a control message

http://codereview.chromium.org/7647003/diff/1/content/common/resource_messages.h
File content/common/resource_messages.h (right):

http://codereview.chromium.org/7647003/diff/1/content/common/resource_message...
content/common/resource_messages.h:166:
IPC_MESSAGE_ROUTED1(ResourceHostMsg_TransferRequestToNewPage,
I don't see a need for this to be a routed message (most of the existing
messages should probably be control as well, but that's for another time).
routed messages are used when there are multiple instances of an object on the
other side dispatching the same message type. the routing id is used to
determine which object a message goes to. in this case, there's only one RDH in
the browser process.

http://codereview.chromium.org/7647003/diff/1/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7647003/diff/1/content/renderer/render_view.cc...
content/renderer/render_view.cc:2823: webkit_glue::WebURLLoaderImpl* loaderImpl
=
nit: loader_impl

http://codereview.chromium.org/7647003/diff/1/content/renderer/render_view.h
File content/renderer/render_view.h (right):

http://codereview.chromium.org/7647003/diff/1/content/renderer/render_view.h#...
content/renderer/render_view.h:551: virtual void
didAdoptURLLoader(WebKit::WebURLLoader*);
nit: chrome style is to list the parameter name

[Dmitry Titov, 2011-08-15 20:50:10]

Thanks for review! Addressed feedback, will launch combined patch for
WebKit+Chromium changes to try servers shortly.

http://codereview.chromium.org/7647003/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7647003/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/resource_dispatcher_host.cc:648:
TransferRequestToNewPage(filter_->child_id(), msg.routing_id(), request_id);
On 2011/08/15 16:31:02, John Abd-El-Malek wrote:
> nit: no need to call out from message dispatcher to another function, just
move
> the body here. we did that for CancelRequest since it's called by other
objects

Done.

http://codereview.chromium.org/7647003/diff/1/content/common/resource_dispatc...
File content/common/resource_dispatcher.cc (right):

http://codereview.chromium.org/7647003/diff/1/content/common/resource_dispatc...
content/common/resource_dispatcher.cc:236: new
ResourceHostMsg_TransferRequestToNewPage(new_routing_id,
On 2011/08/15 16:31:02, John Abd-El-Malek wrote:
> these two values are the same, so just send one only in a control message

I switched to control message and pass 2 values - the new routing_id and a
request_id.

http://codereview.chromium.org/7647003/diff/1/content/common/resource_messages.h
File content/common/resource_messages.h (right):

http://codereview.chromium.org/7647003/diff/1/content/common/resource_message...
content/common/resource_messages.h:166:
IPC_MESSAGE_ROUTED1(ResourceHostMsg_TransferRequestToNewPage,
On 2011/08/15 16:31:02, John Abd-El-Malek wrote:
> I don't see a need for this to be a routed message (most of the existing
> messages should probably be control as well, but that's for another time).
> routed messages are used when there are multiple instances of an object on the
> other side dispatching the same message type. the routing id is used to
> determine which object a message goes to. in this case, there's only one RDH
in
> the browser process.

Done.

http://codereview.chromium.org/7647003/diff/1/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7647003/diff/1/content/renderer/render_view.cc...
content/renderer/render_view.cc:2823: webkit_glue::WebURLLoaderImpl* loaderImpl
=
On 2011/08/15 16:31:02, John Abd-El-Malek wrote:
> nit: loader_impl

Done.

http://codereview.chromium.org/7647003/diff/1/content/renderer/render_view.h
File content/renderer/render_view.h (right):

http://codereview.chromium.org/7647003/diff/1/content/renderer/render_view.h#...
content/renderer/render_view.h:551: virtual void
didAdoptURLLoader(WebKit::WebURLLoader*);
On 2011/08/15 16:31:02, John Abd-El-Malek wrote:
> nit: chrome style is to list the parameter name

Done.

[jam, 2011-08-15 22:07:07]

http://codereview.chromium.org/7647003/diff/1/content/common/resource_dispatc...
File content/common/resource_dispatcher.cc (right):

http://codereview.chromium.org/7647003/diff/1/content/common/resource_dispatc...
content/common/resource_dispatcher.cc:236: new
ResourceHostMsg_TransferRequestToNewPage(new_routing_id,
On 2011/08/15 20:50:10, Dmitry Titov wrote:
> On 2011/08/15 16:31:02, John Abd-El-Malek wrote:
> > these two values are the same, so just send one only in a control message
> 
> I switched to control message and pass 2 values - the new routing_id and a
> request_id.

what I meant is that these two values are always the same, since request_id_ is
set to new_routing_id before sending the message

[Dmitry Titov, 2011-08-15 22:33:26]

I'm sorry, I fail to see what you mean. Where is the request_id is overwritten
with a new routing_id? It would be wrong to do that, imo - they are different
ids, routing_id is the id of WebFrame and request_id is the id of request... I
am updating the routing_id copy in the bridge, but request_id stays the same.
Confused :-)

[jam, 2011-08-15 23:02:55]

oops, sorry I was misreading that! lgtm

[commit-bot: I haz the power, 2011-08-16 20:36:35]

Presubmit check for 7647003-1013 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2011 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.\n
Found a bad license header in these files:
  webkit/glue/weburlloader_impl.h

Presubmit checks took 3.1s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!