Unify frame IDs with RenderFrameHost routing IDs.

Unify frame IDs with RenderFrameHost routing IDs.

This eliminates a lot of unnecessary code around FrameTreeNode::frame_id_, which was also incorrect if two frames had the same frame ID in different processes.

BUG=314791
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252370

[dcheng, 2014-02-19 23:55:36]

Would it make sense to get rid of the remaining calls to WebFrame::create() in
your current CL?
As far as I can tell, there are three places that still depend on this:
android_webview/renderer/print_web_view_helper.cc
chrome/renderer/printing/print_web_view_helper.cc
components/plugins/renderer/webview_plugin.cc

Then it just requires a blink-side CL to finish this change.

[Charlie Reis, 2014-02-20 00:01:46]

On 2014/02/19 23:55:36, dcheng wrote:
> Would it make sense to get rid of the remaining calls to WebFrame::create() in
> your current CL?
> As far as I can tell, there are three places that still depend on this:
> android_webview/renderer/print_web_view_helper.cc
> chrome/renderer/printing/print_web_view_helper.cc
> components/plugins/renderer/webview_plugin.cc
> 
> Then it just requires a blink-side CL to finish this change.

I definitely like that idea, but those sites don't have RenderFrames.  What ID
would I pass?  Do we know if the ID gets used at all in those cases?

I suppose the answer to that question matters regardless, because we'll end up
with conflicts between generateEmbedderIdentifier() and RenderFrame routing IDs
if we leave it as is (e.g., a PrintWebViewHelper and RenderFrameHost could have
the same frame->identifier() if they somehow shared a process).

[Charlie Reis, 2014-02-20 00:20:22]

On 2014/02/20 00:01:46, Charlie Reis wrote:
> On 2014/02/19 23:55:36, dcheng wrote:
> > Would it make sense to get rid of the remaining calls to WebFrame::create()
in
> > your current CL?
> > As far as I can tell, there are three places that still depend on this:
> > android_webview/renderer/print_web_view_helper.cc
> > chrome/renderer/printing/print_web_view_helper.cc
> > components/plugins/renderer/webview_plugin.cc
> > 
> > Then it just requires a blink-side CL to finish this change.
> 
> I definitely like that idea, but those sites don't have RenderFrames.  What ID
> would I pass?  Do we know if the ID gets used at all in those cases?
> 
> I suppose the answer to that question matters regardless, because we'll end up
> with conflicts between generateEmbedderIdentifier() and RenderFrame routing
IDs
> if we leave it as is (e.g., a PrintWebViewHelper and RenderFrameHost could
have
> the same frame->identifier() if they somehow shared a process).

Doesn't look like those classes use the identifier, so I'm just passing 0 for it
in patch set 3.  That should let us remove the API from Blink after this lands.

[awong, 2014-02-20 00:25:29]

OMG this is so so sooo much nicer.

Have a couple of comments. Mostly superficial.

https://codereview.chromium.org/172063002/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/172063002/diff/90001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.h:73: void SetFrameUrl(int routing_id, int
process_id, const GURL& url);
We have APIs for add, remove, and edit (SetFrameURL) all of which require
identifying a position in the tree.

For Add/Remove, we use the FrameTreeNode*.  For SetFrameUrl, we use the
routing_id/process_id.  Can we unify?

https://codereview.chromium.org/172063002/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/172063002/diff/90001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:321: // TODO(creis): Use
frame routing IDs instead.  Currently equivalent.
Do you mean to rename it basically?  The TODO comment doesn't quite make sense.

https://codereview.chromium.org/172063002/diff/90001/content/browser/site_per...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/172063002/diff/90001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:501: // TODO(creis): Can we
check something else, like the URL?
This is confusing since the code below is actually checking a url...

https://codereview.chromium.org/172063002/diff/90001/content/test/test_render...
File content/test/test_render_view_host.cc (right):

https://codereview.chromium.org/172063002/diff/90001/content/test/test_render...
content/test/test_render_view_host.cc:27: namespace {
This namespace is empty now. Remove?

[Charlie Reis, 2014-02-20 00:43:52]

Thanks!  PTAL.

https://codereview.chromium.org/172063002/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/172063002/diff/90001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.h:73: void SetFrameUrl(int routing_id, int
process_id, const GURL& url);
On 2014/02/20 00:25:30, awong wrote:
> We have APIs for add, remove, and edit (SetFrameURL) all of which require
> identifying a position in the tree.
> 
> For Add/Remove, we use the FrameTreeNode*.  For SetFrameUrl, we use the
> routing_id/process_id.  Can we unify?

Sure.  Even better, that just leaves a call to set_current_url, which we can do
directly on the node.  I've removed SetFrameUrl entirely.

https://codereview.chromium.org/172063002/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/172063002/diff/90001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:321: // TODO(creis): Use
frame routing IDs instead.  Currently equivalent.
On 2014/02/20 00:25:30, awong wrote:
> Do you mean to rename it basically?  The TODO comment doesn't quite make
sense.

Yeah, there's no need for this TODO.  There's still a bunch of other places that
say frame_id that we can rename eventually.  Now that the concepts are
equivalent, there's no rush.

https://codereview.chromium.org/172063002/diff/90001/content/browser/site_per...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/172063002/diff/90001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:501: // TODO(creis): Can we
check something else, like the URL?
On 2014/02/20 00:25:30, awong wrote:
> This is confusing since the code below is actually checking a url...

Oops!  I was fixing my own TODO from patch set 1 and forgot to remove the TODO.

https://codereview.chromium.org/172063002/diff/90001/content/test/test_render...
File content/test/test_render_view_host.cc (right):

https://codereview.chromium.org/172063002/diff/90001/content/test/test_render...
content/test/test_render_view_host.cc:27: namespace {
On 2014/02/20 00:25:30, awong wrote:
> This namespace is empty now. Remove?

Done.

[awong, 2014-02-20 00:49:15]

LGTM

I have one comment, but fixing is seems like it's beyond this CL.

https://codereview.chromium.org/172063002/diff/280001/android_webview/rendere...
File android_webview/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/172063002/diff/280001/android_webview/rendere...
android_webview/renderer/print_web_view_helper.cc:659:
web_view->setMainFrame(blink::WebFrame::create(this, 0));
Are we going to document the "0 means don't care" in the Blink layer?

This seems like an odd API semantic though perhaps it's unavoidable?

[Charlie Reis, 2014-02-20 00:51:32]

https://codereview.chromium.org/172063002/diff/280001/android_webview/rendere...
File android_webview/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/172063002/diff/280001/android_webview/rendere...
android_webview/renderer/print_web_view_helper.cc:659:
web_view->setMainFrame(blink::WebFrame::create(this, 0));
On 2014/02/20 00:49:16, awong wrote:
> Are we going to document the "0 means don't care" in the Blink layer?
> 
> This seems like an odd API semantic though perhaps it's unavoidable?

Daniel can correct me if I'm wrong, but I think we'd like to eventually find a
way to not have other creators of WebFrames.  Maybe these classes can hold
RenderFrames, for example.

Anyway, he's working on a Blink CL to remove generateEmbedderIdentifier(), so
perhaps he can add a comment like that.

[Charlie Reis, 2014-02-20 00:59:59]

Time for owners reviews...

mkosiba: Can you review android_webview/renderer/print_web_view_helper.cc?
dpapad: Can you review chrome/renderer/printing/print_web_view_helper.cc?
jochen: Can you review components/plugins/renderer/webview_plugin.cc?

In all of these cases, we no longer want to use WebFrame::create(client) because
it calls generateEmbedderIdentifier().  These cases don't seem to need a frame
ID, so we're just passing 0 for it instead.  (Elsewhere we've used the
RenderFrame's routing ID for the frame ID, and we're going to remove
generateEmbedderIdentifier() from Blink.)  Thanks!

dcheng: Can you review content/common/frame_messages.h for IPC owners?  Feel
free to check over the rest as well.

[dcheng, 2014-02-20 01:28:03]

Would it make sense to just remove this concept from Blink completely and only
store it in RenderFrameImpl?

It complicates lookup in Chrome when we only have a WebFrame*, since we'll need
to use the map of WebFrame* to RenderFrameImpl*... but this also gets us cleaner
code in several places:
1) Blink doesn't really seem to care about the frame ID at all... mostly.
There's a weird idiosyncrasy with child frame history items that I'm trying to
understand still.
2) Frame ID isn't stored twice (in RenderFrameImpl and in WebCore::FrameInit).
3) Places that don't care about embedder ID don't have to pass in a placeholder
value.

https://codereview.chromium.org/172063002/diff/280001/android_webview/rendere...
File android_webview/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/172063002/diff/280001/android_webview/rendere...
android_webview/renderer/print_web_view_helper.cc:659:
web_view->setMainFrame(blink::WebFrame::create(this, 0));
On 2014/02/20 00:51:32, Charlie Reis wrote:
> On 2014/02/20 00:49:16, awong wrote:
> > Are we going to document the "0 means don't care" in the Blink layer?
> > 
> > This seems like an odd API semantic though perhaps it's unavoidable?
> 
> Daniel can correct me if I'm wrong, but I think we'd like to eventually find a
> way to not have other creators of WebFrames.  Maybe these classes can hold
> RenderFrames, for example.
> 
> Anyway, he's working on a Blink CL to remove generateEmbedderIdentifier(), so
> perhaps he can add a comment like that.

I've done that in https://codereview.chromium.org/172543005/

https://codereview.chromium.org/172063002/diff/280001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/172063002/diff/280001/content/common/frame_me...
content/common/frame_messages.h:302: int /* new_routing_id */)
Nit: I think it'd look a little better we used identical typenames to refer to
both routing IDs here.

https://codereview.chromium.org/172063002/diff/280001/content/common/frame_me...
content/common/frame_messages.h:310: int64 /* frame_id */,
These are just routing IDs too now right? Do we want to make our terminology
(and typenames) consistent? For example, should WebFrame::identifier() return a
32-bit integer now?

[Lei Zhang, 2014-02-20 01:37:28]

chrome/ lgtm, given it's identical to android_view/

[Charlie Reis, 2014-02-20 01:49:08]

On 2014/02/20 01:28:03, dcheng wrote:
> Would it make sense to just remove this concept from Blink completely and only
> store it in RenderFrameImpl?
> 
> It complicates lookup in Chrome when we only have a WebFrame*, since we'll
need
> to use the map of WebFrame* to RenderFrameImpl*... but this also gets us
cleaner
> code in several places:
> 1) Blink doesn't really seem to care about the frame ID at all... mostly.
> There's a weird idiosyncrasy with child frame history items that I'm trying to
> understand still.
> 2) Frame ID isn't stored twice (in RenderFrameImpl and in WebCore::FrameInit).
> 3) Places that don't care about embedder ID don't have to pass in a
placeholder
> value.
> 

I think that's the logical way to go.  We need to plan out the change, though,
since there's moving pieces in both repositories.  Here's one approach:

1) Land this CL, removing any callers of generateEmbedderIdentifier.  We still
have to use the two-argument WebFrame::create, since the one-argument version
calls generateEmbedderIdentifier.
2) Land another Chrome-side CL, replacing any callers of WebFrame::identifier()
with the map you mentioned.  (This is a somewhat independent task from the
current CL, so it might be nice not to grow this one further if possible.)
3) Remove both generateEmbedderIdentifier() and identifier() from Blink.  The
two-argument WebFrame::create now ignores the id and forwards to a simpler
one-argument WebFrame::create.
4) Transition Chrome to use the simpler one-argument WebFrame::create.
5) Remove the two-argument WebFrame::create from Blink.

Sound right?

https://codereview.chromium.org/172063002/diff/280001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/172063002/diff/280001/content/common/frame_me...
content/common/frame_messages.h:302: int /* new_routing_id */)
On 2014/02/20 01:28:03, dcheng wrote:
> Nit: I think it'd look a little better we used identical typenames to refer to
> both routing IDs here.

Done.

https://codereview.chromium.org/172063002/diff/280001/content/common/frame_me...
content/common/frame_messages.h:310: int64 /* frame_id */,
On 2014/02/20 01:28:03, dcheng wrote:
> These are just routing IDs too now right? Do we want to make our terminology
> (and typenames) consistent? For example, should WebFrame::identifier() return
a
> 32-bit integer now?

Technically yes, but I'd rather take another pass and just remove all the
frame_ids from FrameHostMsgs, since they're now redundant with the routing ID on
the message.

[jochen (gone - plz use gerrit), 2014-02-20 08:02:28]

components/ lgtm

[dcheng, 2014-02-20 10:44:42]

On 2014/02/20 01:49:08, Charlie Reis wrote:
> On 2014/02/20 01:28:03, dcheng wrote:
> > Would it make sense to just remove this concept from Blink completely and
only
> > store it in RenderFrameImpl?
> > 
> > It complicates lookup in Chrome when we only have a WebFrame*, since we'll
> need
> > to use the map of WebFrame* to RenderFrameImpl*... but this also gets us
> cleaner
> > code in several places:
> > 1) Blink doesn't really seem to care about the frame ID at all... mostly.
> > There's a weird idiosyncrasy with child frame history items that I'm trying
to
> > understand still.
> > 2) Frame ID isn't stored twice (in RenderFrameImpl and in
WebCore::FrameInit).
> > 3) Places that don't care about embedder ID don't have to pass in a
> placeholder
> > value.
> > 
> 
> I think that's the logical way to go.  We need to plan out the change, though,
> since there's moving pieces in both repositories.  Here's one approach:
> 
> 1) Land this CL, removing any callers of generateEmbedderIdentifier.  We still
> have to use the two-argument WebFrame::create, since the one-argument version
> calls generateEmbedderIdentifier.
> 2) Land another Chrome-side CL, replacing any callers of
WebFrame::identifier()
> with the map you mentioned.  (This is a somewhat independent task from the
> current CL, so it might be nice not to grow this one further if possible.)
> 3) Remove both generateEmbedderIdentifier() and identifier() from Blink.  The
> two-argument WebFrame::create now ignores the id and forwards to a simpler
> one-argument WebFrame::create.
> 4) Transition Chrome to use the simpler one-argument WebFrame::create.
> 5) Remove the two-argument WebFrame::create from Blink.
> 
> Sound right?
> 
>
https://codereview.chromium.org/172063002/diff/280001/content/common/frame_me...
> File content/common/frame_messages.h (right):
> 
>
https://codereview.chromium.org/172063002/diff/280001/content/common/frame_me...
> content/common/frame_messages.h:302: int /* new_routing_id */)
> On 2014/02/20 01:28:03, dcheng wrote:
> > Nit: I think it'd look a little better we used identical typenames to refer
to
> > both routing IDs here.
> 
> Done.
> 
>
https://codereview.chromium.org/172063002/diff/280001/content/common/frame_me...
> content/common/frame_messages.h:310: int64 /* frame_id */,
> On 2014/02/20 01:28:03, dcheng wrote:
> > These are just routing IDs too now right? Do we want to make our terminology
> > (and typenames) consistent? For example, should WebFrame::identifier()
return
> a
> > 32-bit integer now?
> 
> Technically yes, but I'd rather take another pass and just remove all the
> frame_ids from FrameHostMsgs, since they're now redundant with the routing ID
on
> the message.

content/common/frame_messages.h LGTM
Your proposed approach seems reasonable to me, though I think we can just
sequence (3) to happen with (5), so we don't need to wait for a Blink roll
before (4).

[mkosiba (inactive), 2014-02-20 11:55:30]

android_webview/ LGTM

[Charlie Reis, 2014-02-20 17:41:02]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2014-02-20 17:44:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/172063002/540001

[commit-bot: I haz the power, 2014-02-20 21:04:03]

Change committed as 252370