Expose whether a frame is focused to the browser process

Expose whether a frame is focused to the browser process

A frame is considered focused if its associated RenderWidgetHost is
focused and it is the focused frame inside its frame tree.

BUG=450634
Committed: https://crrev.com/a85eb3fe83825fdc46a01ec37616e10687772884
Cr-Commit-Position: refs/heads/master@{#313085}

[mlamouri (slow - plz ping), 2015-01-21 16:29:13]

mlamouri@chromium.org changed reviewers:
+ creis@chromium.org

[mlamouri (slow - plz ping), 2015-01-21 17:29:36]

https://codereview.chromium.org/860393004/diff/1/content/public/browser/rende...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/860393004/diff/1/content/public/browser/rende...
content/public/browser/render_widget_host.h:160: virtual bool IsFocused() const
= 0;
If preferred, I could expose that to RenderWidgetHostImpl instead.

[Charlie Reis, 2015-01-21 17:55:32]

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

[Charlie Reis, 2015-01-21 17:55:33]

[+kenrb for RWH knowledge]

Ken's probably a better reviewer than me here.  It's not clear to me that it's
safe to maintain this state in RWH, for a few reasons.

1) It's confusing to me that focus is handled by widgets when it's a frame-level
property.  Maybe that's not going away, though.

2) I don't understand the comment in the CL description about focus when a frame
tree node isn't "activated."  (What is an activated frame?)

3) It wasn't obvious to me that RWHI will always be notified when the focus
changes, so the new bool might get stale.

Ken would probably have a better sense for whether these concerns are valid.

https://codereview.chromium.org/860393004/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/860393004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.cc:659: is_focused_ =
false;
Is this always called when another widget grabs focus, even when it's in the
renderer process?  I'm not sure if it's safe to maintain a focus state here.

[mlamouri (slow - plz ping), 2015-01-21 18:15:33]

On 2015/01/21 at 17:55:33, creis wrote:
> [+kenrb for RWH knowledge]
> 
> Ken's probably a better reviewer than me here.  It's not clear to me that it's
safe to maintain this state in RWH, for a few reasons.
> 
> 1) It's confusing to me that focus is handled by widgets when it's a
frame-level property.  Maybe that's not going away, though.

The frame knows whether it is focused in the frame tree but the frame tree will
say that a given frame is focused even though the tab is not.

> 2) I don't understand the comment in the CL description about focus when a
frame tree node isn't "activated."  (What is an activated frame?)

What I meant is that the frame tree will have a focused frame regardless of the
frame being in the active tab.

> 3) It wasn't obvious to me that RWHI will always be notified when the focus
changes, so the new bool might get stale.

I wasn't able to find a case where the RWHI wouldn't be notified but I might
have missed something though.

[kenrb, 2015-01-21 18:31:46]

We are talking about tab-level focus and not frame-level focus, right? i.e. when
I press a key, which WebContents does it go to?

I think this is okay, if I understand the intent correctly.

On 2015/01/21 18:15:33, Mounir Lamouri wrote:
> > 3) It wasn't obvious to me that RWHI will always be notified when the focus
> changes, so the new bool might get stale.
> 
> I wasn't able to find a case where the RWHI wouldn't be notified but I might
> have missed something though.

This is the part I am not 100% sure about either, but it seems like it should be
fine. When the focused tab changes, I would expect the RenderWidgetHost of the
previously focused tab should get a blur event, regardless of how what caused
the change.

[mlamouri (slow - plz ping), 2015-01-21 21:33:16]

On 2015/01/21 at 18:31:46, kenrb wrote:
> We are talking about tab-level focus and not frame-level focus, right? i.e.
when I press a key, which WebContents does it go to?
> 
> I think this is okay, if I understand the intent correctly.

That's the intent of RWH::IsFocused(), then I will use
WebContents::GetFocusedFrame() to find the focused frame.
Basically, given a RenderFrameHost, I will check if it is the focused frame for
its WebContents and if its RWH is also focused, if both are focused, I consider
it as focused for ServiceWorker's WindowClient.

> On 2015/01/21 18:15:33, Mounir Lamouri wrote:
> > > 3) It wasn't obvious to me that RWHI will always be notified when the
focus
> > changes, so the new bool might get stale.
> > 
> > I wasn't able to find a case where the RWHI wouldn't be notified but I might
> > have missed something though.
> 
> This is the part I am not 100% sure about either, but it seems like it should
be fine. When the focused tab changes, I would expect the RenderWidgetHost of
the previously focused tab should get a blur event, regardless of how what
caused the change.

That is also my understanding.

[mlamouri (slow - plz ping), 2015-01-21 21:33:27]

creis@, PTAL?

https://codereview.chromium.org/860393004/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/860393004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.cc:659: is_focused_ =
false;
On 2015/01/21 at 17:55:33, Charlie Reis wrote:
> Is this always called when another widget grabs focus, even when it's in the
renderer process?  I'm not sure if it's safe to maintain a focus state here.

As far as I can tell, yes. I also did a couple of tests with window.open().
Though, I might have missed some edge cases.

[Charlie Reis, 2015-01-21 22:37:07]

On 2015/01/21 21:33:16, Mounir Lamouri wrote:
> On 2015/01/21 at 18:31:46, kenrb wrote:
> > We are talking about tab-level focus and not frame-level focus, right? i.e.
> when I press a key, which WebContents does it go to?
> > 
> > I think this is okay, if I understand the intent correctly.
> 
> That's the intent of RWH::IsFocused(), then I will use
> WebContents::GetFocusedFrame() to find the focused frame.
> Basically, given a RenderFrameHost, I will check if it is the focused frame
for
> its WebContents and if its RWH is also focused, if both are focused, I
consider
> it as focused for ServiceWorker's WindowClient.

If that's the intent, then it seems more direct to add a
RenderFrameHost::IsFocused() which does this for you (i.e., compares itself to
the focused frame of the FrameTree and asks its widget if it has focus).

That would be the public interface, which would use a non-public
RenderWidgetHostImpl::is_focused() internally.  The reason I suggest it is so
that callers outside content/ don't need to worry about these two different
meanings of the word "focus."

[mlamouri (slow - plz ping), 2015-01-21 23:05:37]

Comments applied. PTAL :)

[Charlie Reis, 2015-01-21 23:38:51]

Thanks.  Since it's not just a trivial bool, can you add a test for
RFH::IsFocused?  That will help prevent regression.

Looks good apart from that.

https://codereview.chromium.org/860393004/diff/20001/content/public/browser/r...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/860393004/diff/20001/content/public/browser/r...
content/public/browser/render_frame_host.h:105: virtual bool IsFocused() const =
0;
Per the Content API guidelines, no const in public interfaces:
http://www.chromium.org/developers/content-module/content-api

"don't add the const identifier to interfaces. For interfaces implemented by the
embedder, we can't make assumptions about what the embedder needs to implement
it. For interfaces implemented by content, the implementation details doesn't
have to be exposed."

[mlamouri (slow - plz ping), 2015-01-22 11:33:31]

I've added some tests and removed the const marker.

PTAL.

[Charlie Reis, 2015-01-22 17:46:05]

Great, much appreciated.  LGTM with nits, but only if you're going to use this
outside content/ right away.  Otherwise we should keep it in RFHI.

https://codereview.chromium.org/860393004/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl_browsertest.cc (right):

https://codereview.chromium.org/860393004/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl_browsertest.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/860393004/diff/60001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/860393004/diff/60001/content/content_tests.gy...
content/content_tests.gypi:209:
'browser/frame_host/render_frame_host_impl_browsertest.cc',
nit: Alphabetize

https://codereview.chromium.org/860393004/diff/60001/content/public/browser/r...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/860393004/diff/60001/content/public/browser/r...
content/public/browser/render_frame_host.h:103: // is the focused frame with in
its frame tree and its associated
nit: within

https://codereview.chromium.org/860393004/diff/60001/content/public/browser/r...
content/public/browser/render_frame_host.h:105: virtual bool IsFocused() = 0;
Sanity check: Do you have a CL ready to go that uses this outside content/?  I
remembered that we generally don't introduce public methods unless they have a
caller outside content:

http://www.chromium.org/developers/content-module/content-api
"only expose methods in the public API that embedders need. If a method is only
used by other code in content, it belongs in foo_impl.h and not foo.h."

[mlamouri (slow - plz ping), 2015-01-22 18:09:41]

Review comments applied.

https://codereview.chromium.org/860393004/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl_browsertest.cc (right):

https://codereview.chromium.org/860393004/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl_browsertest.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2015/01/22 at 17:46:04, Charlie Reis (slow until 1-29) wrote:
> 2015

Done.

https://codereview.chromium.org/860393004/diff/60001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/860393004/diff/60001/content/content_tests.gy...
content/content_tests.gypi:209:
'browser/frame_host/render_frame_host_impl_browsertest.cc',
On 2015/01/22 at 17:46:04, Charlie Reis (slow until 1-29) wrote:
> nit: Alphabetize

Done.

https://codereview.chromium.org/860393004/diff/60001/content/public/browser/r...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/860393004/diff/60001/content/public/browser/r...
content/public/browser/render_frame_host.h:103: // is the focused frame with in
its frame tree and its associated
On 2015/01/22 at 17:46:04, Charlie Reis (slow until 1-29) wrote:
> nit: within

Done.

https://codereview.chromium.org/860393004/diff/60001/content/public/browser/r...
content/public/browser/render_frame_host.h:105: virtual bool IsFocused() = 0;
On 2015/01/22 at 17:46:04, Charlie Reis (slow until 1-29) wrote:
> Sanity check: Do you have a CL ready to go that uses this outside content/?  I
remembered that we generally don't introduce public methods unless they have a
caller outside content:
> 
> http://www.chromium.org/developers/content-module/content-api
> "only expose methods in the public API that embedders need. If a method is
only used by other code in content, it belongs in foo_impl.h and not foo.h."

Moved to RFHI and updated the tests.

[mlamouri (slow - plz ping), 2015-01-22 18:09:58]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-01-22 18:11:33]

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

[commit-bot: I haz the power, 2015-01-22 19:35:18]

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

[commit-bot: I haz the power, 2015-01-22 19:35:19]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mlamouri (slow - plz ping), 2015-01-23 12:18:13]

My CL has been broken by https://codereview.chromium.org/616133002/.

kenrb@ and creis@, could you take a look at the workaround I have now in
RFH::IsFocused(). I would be glad to change ::GetRenderWidgetHost() but I would
bet I am missing something obvious here.

[mlamouri (slow - plz ping), 2015-01-23 17:50:45]

creis@ and kenrb@, I've updated this CL to match the recent changes in RFH and
its relationship with RWH. It's now using RWHV. I've also updated the
implementation to consider a RFHI as focused if it is a parent of a focused
frame. This matches the Web behaviour better.

PTAL.

[kenrb, 2015-01-23 18:40:12]

One comment, but otherwise lgtm (non-owner).

https://codereview.chromium.org/860393004/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/860393004/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:409: // is the parent chain
of the focused frame within the frame tree and its
nit: "it is *in* the parent chain"

More generally, I find this comment hard to follow. I think the distinction
between focused frame and focused RenderWidgetHost is too subtle for this to be
easy to parse.

[mlamouri (slow - plz ping), 2015-01-26 14:02:18]

https://codereview.chromium.org/860393004/diff/200001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/860393004/diff/200001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:409: // is the parent chain
of the focused frame within the frame tree and its
On 2015/01/23 at 18:40:12, kenrb wrote:
> nit: "it is *in* the parent chain"
> 
> More generally, I find this comment hard to follow. I think the distinction
between focused frame and focused RenderWidgetHost is too subtle for this to be
easy to parse.

I re-worded a bit.

[jochen (gone - plz use gerrit), 2015-01-26 16:00:30]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-01-26 16:00:31]

rubberstamp lgtm

[mlamouri (slow - plz ping), 2015-01-26 16:02:03]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-01-26 16:02:38]

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

[commit-bot: I haz the power, 2015-01-26 17:37:55]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-01-26 17:42:36]

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

[leviw_travelin_and_unemployed, 2015-01-27 20:08:51]

On 2015/01/26 at 17:42:36, commit-bot wrote:
> Patchset 12 (id:??) landed as
https://crrev.com/a85eb3fe83825fdc46a01ec37616e10687772884
> Cr-Commit-Position: refs/heads/master@{#313085}

the IsFocused_Change test is failing on the webkit waterfall. I'm going to
disable it:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=Ren...