OOPIF: Clear old focused frame when focus moves to a cross-process frame.

OOPIF: Clear old focused frame when focus moves to a cross-process frame.

Previously, clicking on a cross-process frame would not tell the old
frame's renderer about the focus change.  This prevented blur events
from firing in the old frame and led to inconsistencies such as
multiple focused text fields with multiple blinking carets being
visible at the same time.  This CL fixes these issues by adding a new
IPC that is sent to clear focus when the focused frame is changed
cross-process.

BUG=530663, 339659, 341918
Committed: https://crrev.com/ca2c6ba98bb00e7b6d301053b8dbc1a45c068134
Cr-Commit-Position: refs/heads/master@{#351897}

[alexmos, 2015-09-29 23:37:12]

alexmos@chromium.org changed reviewers:
+ creis@chromium.org, dcheng@chromium.org

[alexmos, 2015-09-29 23:37:13]

Charlie - can you please review content/?
Daniel - can you please review Blink changes and frame_messages.h?

This is a step towards making our focus behavior more sane and enabling keyboard
input in OOPIFs later on.  Please let me know what you think.

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

https://codereview.chromium.org/1370013003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1821: Send(new
FrameMsg_ClearFocus(routing_id_));
I was debating whether this should pass the new focused frame (as a proxy
routing ID), but I haven't found a use case that needs this, so went with just
clearing focus in the old renderer.  I wrote up some discussion on this in my
doc (https://goo.gl/B9S5ou).

[Charlie Reis, 2015-09-29 23:50:44]

Nice.  content/ LGTM with nits.

https://codereview.chromium.org/1370013003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1370013003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:260:
node->set_last_focus_time(base::TimeTicks::Now());
nit: Move this below with the actual assignment.

https://codereview.chromium.org/1370013003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:266: if
(SiteIsolationPolicy::AreCrossProcessFramesPossible() && oldFocusedFrame &&
nit: We can probably just make AreCrossProcessFramesPossible() be a DCHECK
inside the block, right?  I don't think two frames from the same tree can be in
different SiteInstances otherwise.

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

https://codereview.chromium.org/1370013003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1821: Send(new
FrameMsg_ClearFocus(routing_id_));
On 2015/09/29 23:37:13, alexmos wrote:
> I was debating whether this should pass the new focused frame (as a proxy
> routing ID), but I haven't found a use case that needs this, so went with just
> clearing focus in the old renderer.  I wrote up some discussion on this in my
> doc (https://goo.gl/B9S5ou).

Acknowledged.

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

https://codereview.chromium.org/1370013003/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3479: // Focus the main frame's
text field.  The return value "focused"
"input-focus"?

https://codereview.chromium.org/1370013003/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3499:
rwh_child->ForwardMouseEvent(mouse_event);
Sanity check: Is this the right way to send input events now that we use
Surfaces?

[alexmos, 2015-09-30 16:48:20]

https://codereview.chromium.org/1370013003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1370013003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:260:
node->set_last_focus_time(base::TimeTicks::Now());
On 2015/09/29 23:50:44, Charlie Reis wrote:
> nit: Move this below with the actual assignment.

Done.

https://codereview.chromium.org/1370013003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:266: if
(SiteIsolationPolicy::AreCrossProcessFramesPossible() && oldFocusedFrame &&
On 2015/09/29 23:50:43, Charlie Reis wrote:
> nit: We can probably just make AreCrossProcessFramesPossible() be a DCHECK
> inside the block, right?  I don't think two frames from the same tree can be
in
> different SiteInstances otherwise.

Indeed - done.

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

https://codereview.chromium.org/1370013003/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3479: // Focus the main frame's
text field.  The return value "focused"
On 2015/09/29 23:50:44, Charlie Reis wrote:
> "input-focus"?

Done.

https://codereview.chromium.org/1370013003/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3499:
rwh_child->ForwardMouseEvent(mouse_event);
On 2015/09/29 23:50:44, Charlie Reis wrote:
> Sanity check: Is this the right way to send input events now that we use
> Surfaces?

I've double-checked with Ken that this should be fine.  I also see various other
tests using this.

Interestingly, I noticed there's a SimulateMouseClickAt in browser_test_utils.h,
but that takes in a WebContents, so doesn't work for subframes.  It'd be nice to
refactor that so it can take in RFHs too at some point.  I'll put this on my
todo list.

[dcheng, 2015-09-30 21:38:34]

https://codereview.chromium.org/1370013003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1370013003/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:265:
oldFocusedFrame->current_frame_host()->GetSiteInstance() !=
This almost feels like a check that should have a helper.
RenderFrameHostManager::CreateRenderFrame has something very similar, but not
quite the same.

https://codereview.chromium.org/1370013003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1370013003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1365: if (!frame()->page())
What's the path for this to be called on a detached frame?

[alexmos, 2015-10-01 00:53:48]

https://codereview.chromium.org/1370013003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1370013003/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:265:
oldFocusedFrame->current_frame_host()->GetSiteInstance() !=
On 2015/09/30 21:38:34, dcheng wrote:
> This almost feels like a check that should have a helper.
> RenderFrameHostManager::CreateRenderFrame has something very similar, but not
> quite the same.

Yeah, I agree they're similar, though I don't quite see how to make a generic
helper for both.  We do have RenderFrameHostImpl::IsCrossProcessSubframe for the
parent case; maybe we could create a more generic version of that for any two
frames?  E.g., static AreFramesCrossProcess(f1, f2) on FTN or RFHI?  Though the
check in CreateRenderFrame wouldn't be able to use that, since the new RFH isn't
created yet.  I'm also not sure we'd want the CHECK below in that helper. 

Charlie, what do you think?

https://codereview.chromium.org/1370013003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1370013003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1365: if (!frame()->page())
On 2015/09/30 21:38:34, dcheng wrote:
> What's the path for this to be called on a detached frame?

I didn't actually hit a path that needed this; I mostly assumed this is needed
because many other places check this before accessing page()->focusController().
 But as we discussed, it shouldn't be possible to be processing this IPC after a
frame has been detached but before it's destroyed.  So I've removed this check
here, and if we crash, we'll hopefully learn why it's needed.

[dcheng, 2015-10-01 05:05:03]

lgtm

[Charlie Reis, 2015-10-01 18:42:28]

https://codereview.chromium.org/1370013003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1370013003/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:265:
oldFocusedFrame->current_frame_host()->GetSiteInstance() !=
On 2015/10/01 00:53:48, alexmos wrote:
> On 2015/09/30 21:38:34, dcheng wrote:
> > This almost feels like a check that should have a helper.
> > RenderFrameHostManager::CreateRenderFrame has something very similar, but
not
> > quite the same.
> 
> Yeah, I agree they're similar, though I don't quite see how to make a generic
> helper for both.  We do have RenderFrameHostImpl::IsCrossProcessSubframe for
the
> parent case; maybe we could create a more generic version of that for any two
> frames?  E.g., static AreFramesCrossProcess(f1, f2) on FTN or RFHI?  Though
the
> check in CreateRenderFrame wouldn't be able to use that, since the new RFH
isn't
> created yet.  I'm also not sure we'd want the CHECK below in that helper. 
> 
> Charlie, what do you think?

I'd rather not.  Comparing SiteInstances is what we want to encourage people to
do, and each case might have its own complexities (which parts need null checks,
etc).

It's also not necessarily "cross process," since two different SiteInstances can
share the same process.  Comparing SiteInstances is still the right thing to do,
since that's what determines if you're interacting with proxies, etc.

LGTM as is.

[alexmos, 2015-10-01 19:02:40]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-10-01 19:05:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1370013003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1370013003/80001

[commit-bot: I haz the power, 2015-10-01 19:13:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-01 19:13:33]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[alexmos, 2015-10-01 21:02:05]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-10-01 21:02:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1370013003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1370013003/80001

[commit-bot: I haz the power, 2015-10-01 21:52:32]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-01 21:53:22]

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