OOPIF: Replicate dynamic window.name updates.

OOPIF: Replicate dynamic window.name updates.

This CL adds the plumbing to replicate window.name updates to the
associated RenderFrameProxies.  This consists of:
- sending a FrameHostMsg_DidChangeName to the browser process whenever
  there's a window.name update in JS
- recording the changed name in the frame's FrameReplicationState
- forwarding the updated name to any of the frame's proxies in other
  renderer processes via FrameMsg_DidUpdateName.

This is a Chromium-only patch since the plumbing for getting
window.name notifications out of Blink was already in place and used
in <webview>.  <webview>'s Chromium-side window.name code is
refactored to use the new plumbing in this CL.

BUG=426512
Committed: https://crrev.com/be2f4c337f17e85533f5a23dc31ee08bf7c0c167
Cr-Commit-Position: refs/heads/master@{#319823}

[alexmos, 2015-03-04 00:34:07]

alexmos@chromium.org changed reviewers:
+ nasko@chromium.org

[alexmos, 2015-03-04 01:04:22]

Nasko, could you please take an initial look?  There's also a question below
about what to do with the existing WebView plumbing for this.

https://codereview.chromium.org/974723002/diff/100001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/974723002/diff/100001/content/renderer/render...
content/renderer/render_frame_impl.cc:2141:
FOR_EACH_OBSERVER(RenderFrameObserver, observers_, DidChangeName(name));
I'm still finalizing what this should be, and it'd be great to get feedback
here.  ExtensionsRenderFrameObserver::DidChangeName  will send another message
ExtensionHostMsg_FrameNameChanged, which is redundant.  I think WebView should
be reusing the more general FrameHostMsg_DidChangeName I'm adding.  We would
then send the message when we're either in --site-per-process, or the WebView
renderer pref is set.  One question, though, is on the browser side, do we have
a way to call something like WebViewGuest::OnFrameNameChanged from a RFH?

https://codereview.chromium.org/974723002/diff/100001/content/test/data/frame...
File content/test/data/frame_tree/top.html (left):

https://codereview.chromium.org/974723002/diff/100001/content/test/data/frame...
content/test/data/frame_tree/top.html:4: window.name = "foo";
I removed this to fix FrameTreeBrowserTest.FrameTreeShape2, which was failing
because it was checking the root's frame_name() and expecting it to be empty,
but with this line it was ending up as "foo" (correctly, I think).  I don't
think the "foo" name is used anywhere else in the tests, so I just removed it
here rather than fix the expectations.  Let me know if this does have a use I
don't know about.

[nasko, 2015-03-05 18:09:28]

This is looking great! Mostly nits.

https://codereview.chromium.org/974723002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/974723002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1193: // Update the frame's
name in the browser process
nit: Comments that explain what the code does when it is mostly obvious
shouldn't be used. Let's remove this one.

https://codereview.chromium.org/974723002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1197:
frame_tree_node()->render_manager()->OnDidUpdateName(name);
Shouldn't we put this code inside the FTN's method that sets the name? RFH
shouldn't care what needs to happen with this name. It is more of a function of
the FTN that it knows it can have proxies (through its RFHM).

https://codereview.chromium.org/974723002/diff/100001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/974723002/diff/100001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:1189: // frame's proxies, so
that the latest frame names can be used in frame
nit: s/used in frame navigations/used in navigations/

https://codereview.chromium.org/974723002/diff/100001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:1199: 
nit: no need for empty line

https://codereview.chromium.org/974723002/diff/100001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:1219:
EXPECT_EQ(root->child_at(0)->frame_name(), "updated-name");
What guarantees that the name has been propagated to the browser process between
ExecuteScript and this line? It looks like this could be timing dependent and
cause the test to be flaky.

https://codereview.chromium.org/974723002/diff/100001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:1241:
"'/cross-site/foo.com/title1.html');"));
Why not use the result of embedded_test_server()->GetURL("foo.com",
"/title1.html") here instead of using the cross-site redirector?

https://codereview.chromium.org/974723002/diff/100001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/974723002/diff/100001/content/renderer/render...
content/renderer/render_frame_impl.cc:2141:
FOR_EACH_OBSERVER(RenderFrameObserver, observers_, DidChangeName(name));
On 2015/03/04 01:04:21, alexmos wrote:
> I'm still finalizing what this should be, and it'd be great to get feedback
> here.  ExtensionsRenderFrameObserver::DidChangeName  will send another message
> ExtensionHostMsg_FrameNameChanged, which is redundant.  I think WebView should
> be reusing the more general FrameHostMsg_DidChangeName I'm adding.  We would
> then send the message when we're either in --site-per-process, or the WebView
> renderer pref is set.  One question, though, is on the browser side, do we
have
> a way to call something like WebViewGuest::OnFrameNameChanged from a RFH?

We don't have such a way and this type of direct call will be a layering
violation. RFH shouldn't know about any of the layers on top of it.

What we can do, though, is expose WebContentsObserver::FrameNameChanged, which
anyone can listen for and take action. I would suggest going that route and
confirming with fsamuel@ it works for them too.

[alexmos, 2015-03-09 18:47:57]

alexmos@chromium.org changed reviewers:
+ fsamuel@chromium.org

[alexmos, 2015-03-09 18:47:58]

Thanks Nasko, responses below.

Adding fsamuel@ to review the WebView changes.  I've refactored WebView's
window.name update plumbing to use the new OOPIF plumbing.  Please let me know
if this approach will work for you guys.  (Also, is there an existing WebView
test for this?)

https://codereview.chromium.org/974723002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/974723002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1193: // Update the frame's
name in the browser process
On 2015/03/05 18:09:27, nasko wrote:
> nit: Comments that explain what the code does when it is mostly obvious
> shouldn't be used. Let's remove this one.

Done.

https://codereview.chromium.org/974723002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1197:
frame_tree_node()->render_manager()->OnDidUpdateName(name);
On 2015/03/05 18:09:27, nasko wrote:
> Shouldn't we put this code inside the FTN's method that sets the name? RFH
> shouldn't care what needs to happen with this name. It is more of a function
of
> the FTN that it knows it can have proxies (through its RFHM).

Great point - done.  Renamed FrameTreeNode::set_frame_name() to SetFrameName,
and moved the call to OnDidUpdateName there.

https://codereview.chromium.org/974723002/diff/100001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/974723002/diff/100001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:1189: // frame's proxies, so
that the latest frame names can be used in frame
On 2015/03/05 18:09:28, nasko wrote:
> nit: s/used in frame navigations/used in navigations/

Done.

https://codereview.chromium.org/974723002/diff/100001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:1199: 
On 2015/03/05 18:09:28, nasko wrote:
> nit: no need for empty line

Done.

https://codereview.chromium.org/974723002/diff/100001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:1219:
EXPECT_EQ(root->child_at(0)->frame_name(), "updated-name");
On 2015/03/05 18:09:27, nasko wrote:
> What guarantees that the name has been propagated to the browser process
between
> ExecuteScript and this line? It looks like this could be timing dependent and
> cause the test to be flaky.

If I understand correctly, the ExecuteScript in 1214 will block until the JS
string finishes executing and returns a value (ignored here), by waiting for a
FrameHostMsg_DomOperationResponse.  On the renderer side, script execution
synchronously triggers a FrameHostMsg_DidChangeName, and then, after the
execution finishes, a FrameHostMsg_DomOperationResponse. So, assuming message
order is preserved, we should have received and processed
FrameHostMsg_DidChangeName before FrameHostMsg_DomOperationResponse, right?  I
used a similar approach in the DynamicSandboxFlags tests, and also ran the test
a bunch of times as a sanity check.

https://codereview.chromium.org/974723002/diff/100001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:1241:
"'/cross-site/foo.com/title1.html');"));
On 2015/03/05 18:09:28, nasko wrote:
> Why not use the result of embedded_test_server()->GetURL("foo.com",
> "/title1.html") here instead of using the cross-site redirector?

Done.

https://codereview.chromium.org/974723002/diff/100001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/974723002/diff/100001/content/renderer/render...
content/renderer/render_frame_impl.cc:2141:
FOR_EACH_OBSERVER(RenderFrameObserver, observers_, DidChangeName(name));
On 2015/03/05 18:09:28, nasko wrote:
> On 2015/03/04 01:04:21, alexmos wrote:
> > I'm still finalizing what this should be, and it'd be great to get feedback
> > here.  ExtensionsRenderFrameObserver::DidChangeName  will send another
message
> > ExtensionHostMsg_FrameNameChanged, which is redundant.  I think WebView
should
> > be reusing the more general FrameHostMsg_DidChangeName I'm adding.  We would
> > then send the message when we're either in --site-per-process, or the
WebView
> > renderer pref is set.  One question, though, is on the browser side, do we
> have
> > a way to call something like WebViewGuest::OnFrameNameChanged from a RFH?
> 
> We don't have such a way and this type of direct call will be a layering
> violation. RFH shouldn't know about any of the layers on top of it.
> 
> What we can do, though, is expose WebContentsObserver::FrameNameChanged, which
> anyone can listen for and take action. I would suggest going that route and
> confirming with fsamuel@ it works for them too.

Done.  Refactored WebViewGuest to use WebContentsObserver::FrameNameChanged, and
removed the old plumbing and message.

[Fady Samuel, 2015-03-09 19:48:25]

BTW, adding name attribute sync'ing regressed our <webview> performance in
SunSpider. This might be an issue here:

https://code.google.com/p/chromium/issues/detail?id=169110&can=1&q=Cr%3DPlatf...

We didn't care about sunspider performance for webview but I suspect you do for
OOPIFs.

[Fady Samuel, 2015-03-09 21:38:08]

After some offline discussions, lgtm. We decided that this is currently
unavoidable and we shouldn't be too concerned about microbenchmarks.

[nasko, 2015-03-09 21:43:40]

LGTM with a couple of nits.

https://codereview.chromium.org/974723002/diff/100001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/974723002/diff/100001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:1219:
EXPECT_EQ(root->child_at(0)->frame_name(), "updated-name");
On 2015/03/09 18:47:58, alexmos wrote:
> On 2015/03/05 18:09:27, nasko wrote:
> > What guarantees that the name has been propagated to the browser process
> between
> > ExecuteScript and this line? It looks like this could be timing dependent
and
> > cause the test to be flaky.
> 
> If I understand correctly, the ExecuteScript in 1214 will block until the JS
> string finishes executing and returns a value (ignored here), by waiting for a
> FrameHostMsg_DomOperationResponse.  On the renderer side, script execution
> synchronously triggers a FrameHostMsg_DidChangeName, and then, after the
> execution finishes, a FrameHostMsg_DomOperationResponse. So, assuming message
> order is preserved, we should have received and processed
> FrameHostMsg_DidChangeName before FrameHostMsg_DomOperationResponse, right?  I
> used a similar approach in the DynamicSandboxFlags tests, and also ran the
test
> a bunch of times as a sanity check.

Acknowledged.

https://codereview.chromium.org/974723002/diff/160001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/974723002/diff/160001/content/renderer/render...
content/renderer/render_frame_impl.cc:2134: // |report_frame_name_changes| is
set (used by WebView).  This can be
nit: "WebView" here has a very ambiguous meaning. Let's use <webview> to ensure
we mean the tag, not the Blink public WebView API.

https://codereview.chromium.org/974723002/diff/160001/content/renderer/render...
content/renderer/render_frame_impl.cc:2140:
render_view_->renderer_preferences_.report_frame_name_changes)
nit: This is a two line if statement, {} need to be used.

[alexmos, 2015-03-09 21:53:00]

Thanks for reviewing!

https://codereview.chromium.org/974723002/diff/160001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/974723002/diff/160001/content/renderer/render...
content/renderer/render_frame_impl.cc:2134: // |report_frame_name_changes| is
set (used by WebView).  This can be
On 2015/03/09 21:43:40, nasko wrote:
> nit: "WebView" here has a very ambiguous meaning. Let's use <webview> to
ensure
> we mean the tag, not the Blink public WebView API.

Done.  Also updated comment slightly to mention another possible optimization.

https://codereview.chromium.org/974723002/diff/160001/content/renderer/render...
content/renderer/render_frame_impl.cc:2140:
render_view_->renderer_preferences_.report_frame_name_changes)
On 2015/03/09 21:43:40, nasko wrote:
> nit: This is a two line if statement, {} need to be used.

Done.

[alexmos, 2015-03-09 22:05:59]

alexmos@chromium.org changed reviewers:
+ kalman@chromium.org

[alexmos, 2015-03-09 22:06:00]

kalman@: could you please provide an owner's review for extensions/renderer/*?

[not at google - send to devlin, 2015-03-09 22:08:50]

extensions lgtm

[alexmos, 2015-03-09 22:35:48]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2015-03-09 22:35:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/974723002/#ps180001
(title: "Fixed Nasko's nits")

[commit-bot: I haz the power, 2015-03-09 22:36:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/974723002/180001

[alexmos, 2015-03-09 22:41:26]

The CQ bit was unchecked by alexmos@chromium.org

[alexmos, 2015-03-10 00:43:25]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-03-10 00:44:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/974723002/180001

[commit-bot: I haz the power, 2015-03-10 02:30:31]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-03-10 02:31:23]

Patchset 10 (id:??) landed as
https://crrev.com/be2f4c337f17e85533f5a23dc31ee08bf7c0c167
Cr-Commit-Position: refs/heads/master@{#319823}