Preserve page-level focus for subframe cross-process navigations.

Preserve page-level focus for subframe cross-process navigations.

Currently, CommitPending contains logic to remember if the old RFH's
RWHV was focused, and if so, focus the new frame's RWHV.  In the case
of subframes, however, this logic doesn't work, as
RWHVChildFrame::HasFocus always returned false.  This CL adds the
logic to properly retrieve page focus for subframes and propagate it
to the subframe's renderer, building on logic introduced in my earlier
page-level focus CL (https://codereview.chromium.org/1394383002).

BUG=530663, 339659
Committed: https://crrev.com/0d7e0b09bb13053bf480cc22a1cbfff1b47e8fee
Cr-Commit-Position: refs/heads/master@{#356961}

[alexmos, 2015-10-22 21:33:39]

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

[alexmos, 2015-10-22 21:33:40]

Charlie, can you please take a look?  This CL fixes the scenario where if A
embeds a subframe that navigates to B; B needs to be initialized with the
current page-level focus.  The earlier page-level focus CL on which this depends
(https://codereview.chromium.org/1394383002, hasn't landed yet) only addresses
the alt-tabbing cases, but not this case.

Also adding Ken who may have thoughts about this, especially the RWHVCF
question.

https://codereview.chromium.org/1406103005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (left):

https://codereview.chromium.org/1406103005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2166:
render_frame_host_->GetView()->HasFocus();
For subframes, this routed to RWHVChildFrame, which always returns false for
HasFocus.  I changed this to just ask the root RVHW, which should know correct
page focus.  Alternatively, it might be reasonable to change RWHVCF::HasFocus to
ask the root view (via
CrossProcessFrameConnector::GetRootRenderWidgetHostView?).  Thoughts?

[Charlie Reis, 2015-10-23 00:05:35]

Ken, can you take a first look?  You're probably a better reviewer for this than
me, but I'm happy to do a separate owners review.

[Charlie Reis, 2015-10-23 21:41:53]

I went ahead and reviewed, and it seems good to me.  I think it's probably worth
doing the change in RWHVCF, though.

Thanks for fixing this!

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

https://codereview.chromium.org/1406103005/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:439: // Currently, this is only used to
set page-level focus in cross-process
nit: Drop "Currently"

https://codereview.chromium.org/1406103005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (left):

https://codereview.chromium.org/1406103005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2166:
render_frame_host_->GetView()->HasFocus();
On 2015/10/22 21:33:40, alexmos wrote:
> For subframes, this routed to RWHVChildFrame, which always returns false for
> HasFocus.  I changed this to just ask the root RVHW, which should know correct
> page focus.  Alternatively, it might be reasonable to change RWHVCF::HasFocus
to
> ask the root view (via
> CrossProcessFrameConnector::GetRootRenderWidgetHostView?).  Thoughts?

That sounds like a better change to me, since anyone who asks RWHVCF (via
GetView) will get the wrong answer today.  Sound good, Ken?

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

https://codereview.chromium.org/1406103005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2601: // is
moved to a common place.
Is there a bug on file for this move?

[alexmos, 2015-10-24 00:41:43]

Thanks for reviewing!  (Sorry, a rebase sneaked into the PS where I addressed
your comments.)

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

https://codereview.chromium.org/1406103005/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:439: // Currently, this is only used to
set page-level focus in cross-process
On 2015/10/23 21:41:52, Charlie Reis wrote:
> nit: Drop "Currently"

Done.

https://codereview.chromium.org/1406103005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (left):

https://codereview.chromium.org/1406103005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2166:
render_frame_host_->GetView()->HasFocus();
On 2015/10/23 21:41:52, Charlie Reis wrote:
> On 2015/10/22 21:33:40, alexmos wrote:
> > For subframes, this routed to RWHVChildFrame, which always returns false for
> > HasFocus.  I changed this to just ask the root RVHW, which should know
correct
> > page focus.  Alternatively, it might be reasonable to change
RWHVCF::HasFocus
> to
> > ask the root view (via
> > CrossProcessFrameConnector::GetRootRenderWidgetHostView?).  Thoughts?
> 
> That sounds like a better change to me, since anyone who asks RWHVCF (via
> GetView) will get the wrong answer today.  Sound good, Ken?

I went ahead and did that to see what it'll look like, pending Ken's approval.

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

https://codereview.chromium.org/1406103005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2601: // is
moved to a common place.
On 2015/10/23 21:41:53, Charlie Reis wrote:
> Is there a bug on file for this move?

Nope, so I added one.

[Charlie Reis, 2015-10-27 20:34:33]

Thanks, LGTM.  Ken, do you agree?

https://codereview.chromium.org/1406103005/diff/60001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/1406103005/diff/60001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:102: bool HasFocus();
nit: Add comment.

[kenrb, 2015-10-29 16:41:41]

Yes, this is the right approach. lgtm

[alexmos, 2015-10-29 16:42:42]

https://codereview.chromium.org/1406103005/diff/60001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/1406103005/diff/60001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:102: bool HasFocus();
On 2015/10/27 20:34:32, Charlie Reis wrote:
> nit: Add comment.

Done.

[alexmos, 2015-10-29 16:42:50]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2015-10-29 16:42:50]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1406103005/#ps80001
(title: "Add comment")

[commit-bot: I haz the power, 2015-10-29 16:43:12]

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

[alexmos, 2015-10-29 16:57:33]

The CQ bit was unchecked by alexmos@chromium.org

[alexmos, 2015-10-29 18:07:38]

Argh, I can't get a working run for this on linux_site_isolation due to the
compile issues there, but I suspect that landing this will break
WebViewTest.InterstitialTeardown and
WebViewTest.InterstitialTeardownOnBrowserShutdown due to
https://crbug.com/548912, which Ken's already said he's working on.  Should I
wait for the fix there, or still land this and disable these two tests on the
FYI bots in the meantime (which will allow me to land more of the dependent
keyboard stuff)?

[Charlie Reis, 2015-10-29 18:34:39]

On 2015/10/29 18:07:38, alexmos wrote:
> Argh, I can't get a working run for this on linux_site_isolation due to the
> compile issues there, but I suspect that landing this will break
> WebViewTest.InterstitialTeardown and
> WebViewTest.InterstitialTeardownOnBrowserShutdown due to
> https://crbug.com/548912, which Ken's already said he's working on.  Should I
> wait for the fix there, or still land this and disable these two tests on the
> FYI bots in the meantime (which will allow me to land more of the dependent
> keyboard stuff)?

If the issue only affects <webview>, I'd disable the tests until Ken's fix is in
and land this.

[alexmos, 2015-10-29 21:12:00]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-10-29 21:12:36]

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

[alexmos, 2015-10-29 21:20:02]

On 2015/10/29 18:34:39, Charlie Reis wrote:
> On 2015/10/29 18:07:38, alexmos wrote:
> > Argh, I can't get a working run for this on linux_site_isolation due to the
> > compile issues there, but I suspect that landing this will break
> > WebViewTest.InterstitialTeardown and
> > WebViewTest.InterstitialTeardownOnBrowserShutdown due to
> > https://crbug.com/548912, which Ken's already said he's working on.  Should
I
> > wait for the fix there, or still land this and disable these two tests on
the
> > FYI bots in the meantime (which will allow me to land more of the dependent
> > keyboard stuff)?
> 
> If the issue only affects <webview>, I'd disable the tests until Ken's fix is
in
> and land this.

Yes, it should only affect <webview>.  I put up a CL to disable the tests in
https://codereview.chromium.org/1421733004/ and will land this.

[commit-bot: I haz the power, 2015-10-29 22:11:57]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-29 22:12:52]

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