OOPIF: Send page-level focus messages to all processes rendering a page.

OOPIF: Send page-level focus messages to all processes rendering a page.

Previously, when a window gains or loses focus (such as when
alt-tabbing to/from it), page-level focus would only be updated in
top-level frame's process.  This caused issues when cross-process
subframes checked page-level focus via FocusController::isFocused(),
for example when processing keyboard events for input fields.

This CL adds logic to replicate the focus update to processes created
for cross-process frames.  This is used to update the focus state
inside each RenderView (has_focus_) as well as
page()->focusController().isFocused() state in each process.  Some
discussion about this is at https://goo.gl/B9S5ou.

Note that the isActive() bit (set by ViewMsg_SetActive) will probably
need to be similarly replicated as well, but this will be done in a
separate CL.

BUG=530663, 339659
Committed: https://crrev.com/3fcd0cae4e0cdd067ef5d8a2f3bd1c3b12da3109
Cr-Commit-Position: refs/heads/master@{#355833}

[alexmos, 2015-10-09 20:23:29]

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

[alexmos, 2015-10-09 20:23:30]

Charlie, can you please take a look?

Ken, I've CC'ed you as well to see if you have thoughts on this, as you
mentioned you might want to do something similar with BeginFrame.

https://codereview.chromium.org/1394383002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1394383002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1592: if (is_being_destroyed_)
For reference, the call stack leading to this is at
http://pastebin.com/Z9h2d80n.  Without this check, some tests crashed doing the
view enumeration below in AddRenderWidgetHostViewToSet, when trying to access
the root's RFH (which is null).

[Charlie Reis, 2015-10-09 21:21:51]

Hmm, I think I'm missing something.  Why do all the widgets want to know the
same focus state?  Is this different from frame-level focus?  (If so, maybe we
can clarify that in the code.)

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

https://codereview.chromium.org/1394383002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2443: if
(!SiteIsolationPolicy::AreCrossProcessFramesPossible())
I think you want AreAllSitesIsolatedForTesting.  For example, the test probably
fails if we pass --isolate-extensions.

https://codereview.chromium.org/1394383002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2466:
NavigationEntryImpl entry(NULL /* instance */, -1 /* page_id */, kUrlB,
nit: nullptr

https://codereview.chromium.org/1394383002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2492: auto
verify_focus_messages = [this, host1, host2](bool enable_focus) {
Wow, first time I've come across lambdas in Chrome.  :)

I'm not terribly familiar with C++ lambdas, but I think this is ok style-wise. 
Google style says to keep them short unless they're named, and this is named. 
Chrome style says not to store them, but I think that means as a member.

Ok, works for me.  (Out of curiosity, do you know if we have precedents
elsewhere in the code?  And is the stack understandable if one of these EXPECTs
fails?)

https://codereview.chromium.org/1394383002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2501:
EXPECT_EQ(enable_focus, base::get<0>(params));
nit: enable_focus -> focus_expected?

https://codereview.chromium.org/1394383002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2511:
EXPECT_EQ(enable_focus, base::get<0>(params));
I'm puzzled.  Why would we tell the subframes that they have focus if the main
frame was focused?  Or does this message mean something else?

https://codereview.chromium.org/1394383002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2532: }
Should we be testing the case where one of the subframes gets focus?  I guess
that wouldn't be able to easily use the lambda, but it seems like we'd want to
verify that the correct frame ended up with focus.

https://codereview.chromium.org/1394383002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1394383002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1592: if (is_being_destroyed_)
On 2015/10/09 20:23:29, alexmos wrote:
> For reference, the call stack leading to this is at
> http://pastebin.com/Z9h2d80n.  Without this check, some tests crashed doing
the
> view enumeration below in AddRenderWidgetHostViewToSet, when trying to access
> the root's RFH (which is null).

Acknowledged.

https://codereview.chromium.org/1394383002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1596: if (!view || view ==
GetRenderWidgetHostView())
Hmm.  It looks like any widget (including subframe widgets) can call this
method, so why would we compare against the main frame's widget here?  That
would make us skip the main frame, as well as duplicate the message to the
widget that called this method.

https://codereview.chromium.org/1394383002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1601: rwh->Send(new
InputMsg_SetFocus(rwh->GetRoutingID(), is_focused));
As in the test, I'm wondering why we want to tell all the other widgets that
some other widget got focus.  It seems like they would want to know that they've
lost focus, if they had it.  (Or at least, which frame has it now.)

[alexmos, 2015-10-09 21:52:14]

On 2015/10/09 21:21:51, Charlie Reis (slow til 10-12) wrote:
> Hmm, I think I'm missing something.  Why do all the widgets want to know the
> same focus state?  Is this different from frame-level focus?  (If so, maybe we
> can clarify that in the code.)

Sorry about the confusion.  The issue is that the renderer keeps track of both
frame-level and page-level focus.  The currently focused frame changes as you
click on different frames, <tab> across input fields, etc.  This CL isn't about
that, but rather it's about the page-level and widget-level focus, which changes
when you alt-tab across windows, for example, and is set/reset via
InputMsg_SetFocus.  When dispatching keyboard events, Blink checks both of
these: it verifies that the current Page has focus (skipping a bunch of
processing otherwise), and then it dispatches the event to the focused frame. 
What this CL is trying to accomplish is when you alt-tab to a window with a page
that has cross-process frames, we want to make sure that all renderers know that
the current Page has gained focus.  Does this make more sense?  I'll reply to
the other comments separately.

[Charlie Reis, 2015-10-09 22:23:06]

Ah, ok.  Should we be tracking that on Page (shared across all widgets)
rather than per-Widget?  Then we only have to send one IPC per process
(e.g., to the top-level frame or proxy), rather than one per widget.

On Fri, Oct 9, 2015 at 2:52 PM, <alexmos@chromium.org> wrote:

> On 2015/10/09 21:21:51, Charlie Reis (slow til 10-12) wrote:
>
>> Hmm, I think I'm missing something.  Why do all the widgets want to know
>> the
>> same focus state?  Is this different from frame-level focus?  (If so,
>> maybe we
>> can clarify that in the code.)
>>
>
> Sorry about the confusion.  The issue is that the renderer keeps track of
> both
> frame-level and page-level focus.  The currently focused frame changes as
> you
> click on different frames, <tab> across input fields, etc.  This CL isn't
> about
> that, but rather it's about the page-level and widget-level focus, which
> changes
> when you alt-tab across windows, for example, and is set/reset via
> InputMsg_SetFocus.  When dispatching keyboard events, Blink checks both of
> these: it verifies that the current Page has focus (skipping a bunch of
> processing otherwise), and then it dispatches the event to the focused
> frame.
> What this CL is trying to accomplish is when you alt-tab to a window with
> a page
> that has cross-process frames, we want to make sure that all renderers
> know that
> the current Page has gained focus.  Does this make more sense?  I'll reply
> to
> the other comments separately.
>
> https://codereview.chromium.org/1394383002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[alexmos, 2015-10-09 22:54:42]

I wanted to do this but didn't, because each RenderWidget also has a has_focus_
bit, which maintains whether the widget itself is focused.  So I left this alone
initially, since this seems to be used by plug-ins for example.  (I'm guessing
because there's no Page to consult in that case?).  E.g., see
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
The case I worried about was when two cross-process subframes each embed a
plug-in. 

However, all references to has_focus() except the above seem to be through
RenderView, so I could try to move the has_focus_ to be tracked by RV, which
would allow us to send one message per process.  On the other hand, RenderView
is going away, so I'm not sure where we want to keep page-level focus state in
content/.  What do you think?


On 2015/10/09 22:23:06, Charlie Reis (slow til 10-12) wrote:
> Ah, ok.  Should we be tracking that on Page (shared across all widgets)
> rather than per-Widget?  Then we only have to send one IPC per process
> (e.g., to the top-level frame or proxy), rather than one per widget.
> 
> On Fri, Oct 9, 2015 at 2:52 PM, <mailto:alexmos@chromium.org> wrote:
> 
> > On 2015/10/09 21:21:51, Charlie Reis (slow til 10-12) wrote:
> >
> >> Hmm, I think I'm missing something.  Why do all the widgets want to know
> >> the
> >> same focus state?  Is this different from frame-level focus?  (If so,
> >> maybe we
> >> can clarify that in the code.)
> >>
> >
> > Sorry about the confusion.  The issue is that the renderer keeps track of
> > both
> > frame-level and page-level focus.  The currently focused frame changes as
> > you
> > click on different frames, <tab> across input fields, etc.  This CL isn't
> > about
> > that, but rather it's about the page-level and widget-level focus, which
> > changes
> > when you alt-tab across windows, for example, and is set/reset via
> > InputMsg_SetFocus.  When dispatching keyboard events, Blink checks both of
> > these: it verifies that the current Page has focus (skipping a bunch of
> > processing otherwise), and then it dispatches the event to the focused
> > frame.
> > What this CL is trying to accomplish is when you alt-tab to a window with
> > a page
> > that has cross-process frames, we want to make sure that all renderers
> > know that
> > the current Page has gained focus.  Does this make more sense?  I'll reply
> > to
> > the other comments separately.
> >
> > https://codereview.chromium.org/1394383002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[alexmos, 2015-10-09 22:57:09]

On 2015/10/09 22:54:42, alexmos wrote:
> I wanted to do this but didn't, because each RenderWidget also has a
has_focus_
> bit, which maintains whether the widget itself is focused.  

Oh, and just to clarify, when processing InputMsg_SetFocus, a RenderWidget would
first update its own has_focus_ value and then pass this on to Blink which will
update the page's focus in FocusController.

> So I left this alone
> initially, since this seems to be used by plug-ins for example.  (I'm guessing
> because there's no Page to consult in that case?).  E.g., see
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
> The case I worried about was when two cross-process subframes each embed a
> plug-in. 
> 
> However, all references to has_focus() except the above seem to be through
> RenderView, so I could try to move the has_focus_ to be tracked by RV, which
> would allow us to send one message per process.  On the other hand, RenderView
> is going away, so I'm not sure where we want to keep page-level focus state in
> content/.  What do you think?
> 
> 
> On 2015/10/09 22:23:06, Charlie Reis (slow til 10-12) wrote:
> > Ah, ok.  Should we be tracking that on Page (shared across all widgets)
> > rather than per-Widget?  Then we only have to send one IPC per process
> > (e.g., to the top-level frame or proxy), rather than one per widget.
> > 
> > On Fri, Oct 9, 2015 at 2:52 PM, <mailto:alexmos@chromium.org> wrote:
> > 
> > > On 2015/10/09 21:21:51, Charlie Reis (slow til 10-12) wrote:
> > >
> > >> Hmm, I think I'm missing something.  Why do all the widgets want to know
> > >> the
> > >> same focus state?  Is this different from frame-level focus?  (If so,
> > >> maybe we
> > >> can clarify that in the code.)
> > >>
> > >
> > > Sorry about the confusion.  The issue is that the renderer keeps track of
> > > both
> > > frame-level and page-level focus.  The currently focused frame changes as
> > > you
> > > click on different frames, <tab> across input fields, etc.  This CL isn't
> > > about
> > > that, but rather it's about the page-level and widget-level focus, which
> > > changes
> > > when you alt-tab across windows, for example, and is set/reset via
> > > InputMsg_SetFocus.  When dispatching keyboard events, Blink checks both of
> > > these: it verifies that the current Page has focus (skipping a bunch of
> > > processing otherwise), and then it dispatches the event to the focused
> > > frame.
> > > What this CL is trying to accomplish is when you alt-tab to a window with
> > > a page
> > > that has cross-process frames, we want to make sure that all renderers
> > > know that
> > > the current Page has gained focus.  Does this make more sense?  I'll reply
> > > to
> > > the other comments separately.
> > >
> > > https://codereview.chromium.org/1394383002/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[Charlie Reis, 2015-10-09 23:30:38]

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

[Charlie Reis, 2015-10-09 23:40:33]

[+dcheng]

Hrm.  It seems like we have too many classes keeping track of focus, each with a
slightly different meaning...  (I remember lamenting this when mlamouri added
yet another earlier this year.)

I don't have a great sense for the plugin case, and maybe keeping this
window-level has_focus_ bit on RenderWidget is a reasonable step for now.

However, I'd like to aim for a future design that is a bit cleaner, if we can. 
I think we're planning to replace RenderView with a content-level class that's
equivalent to Page, and that class could keep track of the window-level focus
instead of RenderWidget.  If we can make the plugin case work, then maybe we can
put window-level focus on RenderView until we create that class?  Daniel, would
it make sense to create that class now, or should we wait?


On 2015/10/09 22:57:09, alexmos wrote:
> On 2015/10/09 22:54:42, alexmos wrote:
> > I wanted to do this but didn't, because each RenderWidget also has a
> has_focus_
> > bit, which maintains whether the widget itself is focused.  
> 
> Oh, and just to clarify, when processing InputMsg_SetFocus, a RenderWidget
would
> first update its own has_focus_ value and then pass this on to Blink which
will
> update the page's focus in FocusController.
> 
> > So I left this alone
> > initially, since this seems to be used by plug-ins for example.  (I'm
guessing
> > because there's no Page to consult in that case?).  E.g., see
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
> > The case I worried about was when two cross-process subframes each embed a
> > plug-in. 
> > 
> > However, all references to has_focus() except the above seem to be through
> > RenderView, so I could try to move the has_focus_ to be tracked by RV, which
> > would allow us to send one message per process.  On the other hand,
RenderView
> > is going away, so I'm not sure where we want to keep page-level focus state
in
> > content/.  What do you think?
> > 
> > 
> > On 2015/10/09 22:23:06, Charlie Reis (slow til 10-12) wrote:
> > > Ah, ok.  Should we be tracking that on Page (shared across all widgets)
> > > rather than per-Widget?  Then we only have to send one IPC per process
> > > (e.g., to the top-level frame or proxy), rather than one per widget.
> > > 
> > > On Fri, Oct 9, 2015 at 2:52 PM, <mailto:alexmos@chromium.org> wrote:
> > > 
> > > > On 2015/10/09 21:21:51, Charlie Reis (slow til 10-12) wrote:
> > > >
> > > >> Hmm, I think I'm missing something.  Why do all the widgets want to
know
> > > >> the
> > > >> same focus state?  Is this different from frame-level focus?  (If so,
> > > >> maybe we
> > > >> can clarify that in the code.)
> > > >>
> > > >
> > > > Sorry about the confusion.  The issue is that the renderer keeps track
of
> > > > both
> > > > frame-level and page-level focus.  The currently focused frame changes
as
> > > > you
> > > > click on different frames, <tab> across input fields, etc.  This CL
isn't
> > > > about
> > > > that, but rather it's about the page-level and widget-level focus, which
> > > > changes
> > > > when you alt-tab across windows, for example, and is set/reset via
> > > > InputMsg_SetFocus.  When dispatching keyboard events, Blink checks both
of
> > > > these: it verifies that the current Page has focus (skipping a bunch of
> > > > processing otherwise), and then it dispatches the event to the focused
> > > > frame.
> > > > What this CL is trying to accomplish is when you alt-tab to a window
with
> > > > a page
> > > > that has cross-process frames, we want to make sure that all renderers
> > > > know that
> > > > the current Page has gained focus.  Does this make more sense?  I'll
reply
> > > > to
> > > > the other comments separately.
> > > >
> > > > https://codereview.chromium.org/1394383002/
> > > >
> > > 
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2015-10-12 17:37:44]

On 2015/10/09 at 23:40:33, creis wrote:
> [+dcheng]
> 
> Hrm.  It seems like we have too many classes keeping track of focus, each with
a slightly different meaning...  (I remember lamenting this when mlamouri added
yet another earlier this year.)
> 
> I don't have a great sense for the plugin case, and maybe keeping this
window-level has_focus_ bit on RenderWidget is a reasonable step for now.
> 
> However, I'd like to aim for a future design that is a bit cleaner, if we can.
 I think we're planning to replace RenderView with a content-level class that's
equivalent to Page, and that class could keep track of the window-level focus
instead of RenderWidget.  If we can make the plugin case work, then maybe we can
put window-level focus on RenderView until we create that class?  Daniel, would
it make sense to create that class now, or should we wait?

I'd be OK with waiting. Let's see if we can make this first.

> 
> 
> On 2015/10/09 22:57:09, alexmos wrote:
> > On 2015/10/09 22:54:42, alexmos wrote:
> > > I wanted to do this but didn't, because each RenderWidget also has a
> > has_focus_
> > > bit, which maintains whether the widget itself is focused.  
> > 
> > Oh, and just to clarify, when processing InputMsg_SetFocus, a RenderWidget
would
> > first update its own has_focus_ value and then pass this on to Blink which
will
> > update the page's focus in FocusController.
> > 
> > > So I left this alone
> > > initially, since this seems to be used by plug-ins for example.  (I'm
guessing
> > > because there's no Page to consult in that case?).  E.g., see
> > >
> >
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
> > > The case I worried about was when two cross-process subframes each embed a
> > > plug-in. 
> > > 
> > > However, all references to has_focus() except the above seem to be through
> > > RenderView, so I could try to move the has_focus_ to be tracked by RV,
which
> > > would allow us to send one message per process.  On the other hand,
RenderView
> > > is going away, so I'm not sure where we want to keep page-level focus
state in
> > > content/.  What do you think?

Per our discussion, I believe the RenderWidget::has_focus() reference in pepper
always uses the RV anyway, so this /shouldn't/ be a blocker.

> > > 
> > > 
> > > On 2015/10/09 22:23:06, Charlie Reis (slow til 10-12) wrote:
> > > > Ah, ok.  Should we be tracking that on Page (shared across all widgets)
> > > > rather than per-Widget?  Then we only have to send one IPC per process
> > > > (e.g., to the top-level frame or proxy), rather than one per widget.
> > > > 
> > > > On Fri, Oct 9, 2015 at 2:52 PM, <mailto:alexmos@chromium.org> wrote:
> > > > 
> > > > > On 2015/10/09 21:21:51, Charlie Reis (slow til 10-12) wrote:
> > > > >
> > > > >> Hmm, I think I'm missing something.  Why do all the widgets want to
know
> > > > >> the
> > > > >> same focus state?  Is this different from frame-level focus?  (If so,
> > > > >> maybe we
> > > > >> can clarify that in the code.)
> > > > >>
> > > > >
> > > > > Sorry about the confusion.  The issue is that the renderer keeps track
of
> > > > > both
> > > > > frame-level and page-level focus.  The currently focused frame changes
as
> > > > > you
> > > > > click on different frames, <tab> across input fields, etc.  This CL
isn't
> > > > > about
> > > > > that, but rather it's about the page-level and widget-level focus,
which
> > > > > changes
> > > > > when you alt-tab across windows, for example, and is set/reset via
> > > > > InputMsg_SetFocus.  When dispatching keyboard events, Blink checks
both of
> > > > > these: it verifies that the current Page has focus (skipping a bunch
of
> > > > > processing otherwise), and then it dispatches the event to the focused
> > > > > frame.
> > > > > What this CL is trying to accomplish is when you alt-tab to a window
with
> > > > > a page
> > > > > that has cross-process frames, we want to make sure that all renderers
> > > > > know that
> > > > > the current Page has gained focus.  Does this make more sense?  I'll
reply
> > > > > to
> > > > > the other comments separately.
> > > > >
> > > > > https://codereview.chromium.org/1394383002/
> > > > >
> > > > 
> > > > To unsubscribe from this group and stop receiving emails from it, send
an
> > > email
> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.

[alexmos, 2015-10-16 16:50:45]

Charlie/Daniel: please take another look.  I switched to send one page focus
message per process, routed through main frame's proxies.  I'm still thinking
about how general some of this should be, and whether to do it here vs a
follow-up CL, so I'm curious to hear your thoughts.  I.e., should
ReplicatePageFocus() be something that can handle generic page-level messages,
and should it also handle sending to main frame in addition to proxies
(following Nasko's recent ideas about this).

This depends on https://codereview.chromium.org/1400003003/ to move has_focus()
from RW to RV, which is what allows me to not send the message to each RW
anymore.

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

https://codereview.chromium.org/1394383002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2443: if
(!SiteIsolationPolicy::AreCrossProcessFramesPossible())
On 2015/10/09 21:21:51, Charlie Reis (slow til 10-12) wrote:
> I think you want AreAllSitesIsolatedForTesting.  For example, the test
probably
> fails if we pass --isolate-extensions.

Indeed.  Done.

https://codereview.chromium.org/1394383002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2466:
NavigationEntryImpl entry(NULL /* instance */, -1 /* page_id */, kUrlB,
On 2015/10/09 21:21:50, Charlie Reis (slow til 10-12) wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/1394383002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2492: auto
verify_focus_messages = [this, host1, host2](bool enable_focus) {
On 2015/10/09 21:21:51, Charlie Reis (slow til 10-12) wrote:
> Wow, first time I've come across lambdas in Chrome.  :)
> 
> I'm not terribly familiar with C++ lambdas, but I think this is ok style-wise.

> Google style says to keep them short unless they're named, and this is named. 
> Chrome style says not to store them, but I think that means as a member.
> 
> Ok, works for me.  (Out of curiosity, do you know if we have precedents
> elsewhere in the code?  And is the stack understandable if one of these
EXPECTs
> fails?)

I saw it used in lazyboy's CL
(https://codereview.chromium.org/1307013004/diff/320001/content/browser/site_p...)
and checked out the style guide to see that they're allowed then.  Looking
around codesearch, I saw a few more uses in various tests (a couple of examples:
https://code.google.com/p/chromium/codesearch#search/&q==%5C%20%5C%5B.*%5C%5D...
).  There were a few uses in content/, including in
RenderFrameHostManager::RenderFrameProxyHostMap::Add/Remove, so I figured why
not. :)

Failed EXPECTs show up normally, and crashes just have a
"...FocusPropagatesToAllRenderWidgets_Test::TestBody()::$_0::operator()()" for
the lambda frame.

https://codereview.chromium.org/1394383002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2501:
EXPECT_EQ(enable_focus, base::get<0>(params));
On 2015/10/09 21:21:51, Charlie Reis (slow til 10-12) wrote:
> nit: enable_focus -> focus_expected?

Changed to expected_focus in the updated test.

https://codereview.chromium.org/1394383002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2511:
EXPECT_EQ(enable_focus, base::get<0>(params));
On 2015/10/09 21:21:51, Charlie Reis (slow til 10-12) wrote:
> I'm puzzled.  Why would we tell the subframes that they have focus if the main
> frame was focused?  Or does this message mean something else?

Yes, this message means that the whole page was focused, not the main frame, so
each Page object in the renderer processes needs to find out about that.

https://codereview.chromium.org/1394383002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2532: }
On 2015/10/09 21:21:51, Charlie Reis (slow til 10-12) wrote:
> Should we be testing the case where one of the subframes gets focus?  I guess
> that wouldn't be able to easily use the lambda, but it seems like we'd want to
> verify that the correct frame ended up with focus.

I added a test for some of that in https://codereview.chromium.org/1370013003/,
but as we discussed, this test is for page-level focus, not frame focus.

https://codereview.chromium.org/1394383002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1394383002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1596: if (!view || view ==
GetRenderWidgetHostView())
On 2015/10/09 21:21:51, Charlie Reis wrote:
> Hmm.  It looks like any widget (including subframe widgets) can call this
> method, so why would we compare against the main frame's widget here?  That
> would make us skip the main frame, as well as duplicate the message to the
> widget that called this method.

This is supposed to be only called from main widgets; RWHI now checks
IsRenderView before calling this, and this check is removed.

https://codereview.chromium.org/1394383002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1601: rwh->Send(new
InputMsg_SetFocus(rwh->GetRoutingID(), is_focused));
On 2015/10/09 21:21:51, Charlie Reis wrote:
> As in the test, I'm wondering why we want to tell all the other widgets that
> some other widget got focus.  It seems like they would want to know that
they've
> lost focus, if they had it.  (Or at least, which frame has it now.)

I think we've cleared this up in earlier comments, but regardless, I'm now
sending this through main frame's proxies, not widgets.

https://codereview.chromium.org/1394383002/diff/120001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1394383002/diff/120001/content/renderer/rende...
content/renderer/render_view_impl.cc:3073: void RenderViewImpl::SetFocus(bool
enable) {
I exposed this so that I could call it from RenderFrameProxy.  I thought about
having the proxy directly call setFocused() through WebWidget, but the plugin
notifications below seemed important to do also.  I'm assuming that if an OOPIF
hosts a plugin, that plugin should get recorded in the swapped-out RV (e.g., in
active_pepper_instances_), though I haven't actually checked how plugins
currently behave in OOPIFs.

[Charlie Reis, 2015-10-16 18:56:09]

LGTM, thanks!

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

https://codereview.chromium.org/1394383002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2492: auto
verify_focus_messages = [this, host1, host2](bool enable_focus) {
On 2015/10/16 16:50:44, alexmos wrote:
> On 2015/10/09 21:21:51, Charlie Reis (slow til 10-12) wrote:
> > Wow, first time I've come across lambdas in Chrome.  :)
> > 
> > I'm not terribly familiar with C++ lambdas, but I think this is ok
style-wise.
> 
> > Google style says to keep them short unless they're named, and this is
named. 
> > Chrome style says not to store them, but I think that means as a member.
> > 
> > Ok, works for me.  (Out of curiosity, do you know if we have precedents
> > elsewhere in the code?  And is the stack understandable if one of these
> EXPECTs
> > fails?)
> 
> I saw it used in lazyboy's CL
>
(https://codereview.chromium.org/1307013004/diff/320001/content/browser/site_p...)
> and checked out the style guide to see that they're allowed then.  Looking
> around codesearch, I saw a few more uses in various tests (a couple of
examples:
>
https://code.google.com/p/chromium/codesearch#search/&q==%5C%20%5C%5B.*%5C%5D...
> ).  There were a few uses in content/, including in
> RenderFrameHostManager::RenderFrameProxyHostMap::Add/Remove, so I figured why
> not. :)
> 
> Failed EXPECTs show up normally, and crashes just have a
> "...FocusPropagatesToAllRenderWidgets_Test::TestBody()::$_0::operator()()" for
> the lambda frame.

Acknowledged.

https://codereview.chromium.org/1394383002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2511:
EXPECT_EQ(enable_focus, base::get<0>(params));
On 2015/10/16 16:50:44, alexmos wrote:
> On 2015/10/09 21:21:51, Charlie Reis (slow til 10-12) wrote:
> > I'm puzzled.  Why would we tell the subframes that they have focus if the
main
> > frame was focused?  Or does this message mean something else?
> 
> Yes, this message means that the whole page was focused, not the main frame,
so
> each Page object in the renderer processes needs to find out about that.

Acknowledged.

https://codereview.chromium.org/1394383002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2532: }
On 2015/10/16 16:50:44, alexmos wrote:
> On 2015/10/09 21:21:51, Charlie Reis (slow til 10-12) wrote:
> > Should we be testing the case where one of the subframes gets focus?  I
guess
> > that wouldn't be able to easily use the lambda, but it seems like we'd want
to
> > verify that the correct frame ended up with focus.
> 
> I added a test for some of that in
https://codereview.chromium.org/1370013003/,
> but as we discussed, this test is for page-level focus, not frame focus.

Acknowledged.

https://codereview.chromium.org/1394383002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1394383002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:87: bool
CollectSiteInstances(std::set<SiteInstance*>* set,
nit: Add comment about what this is useful for (e.g., SiteInstances involved in
rendering the page, which is a subset of those in proxy_hosts_, thanks to
openers).

https://codereview.chromium.org/1394383002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2013: // Main frame
should have proxies in all SiteInstances of other frames in
nit: Notify the proxies for all SiteInstances of other frames in this FrameTree.
 Note that the main frame might also know about proxies in...

https://codereview.chromium.org/1394383002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2016: // can't just
iterate over proxy_hosts_.
This comment should go above line 2005.

https://codereview.chromium.org/1394383002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1394383002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:502: // current
FrameTree, not including the main frame's SiteInstance.  The focus
nit: current main frame's

https://codereview.chromium.org/1394383002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/1394383002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:714: if (IsRenderView()
&& delegate_)
Does this still work with Avi's changes to split RWH/RVH?

https://codereview.chromium.org/1394383002/diff/120001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1394383002/diff/120001/content/renderer/rende...
content/renderer/render_view_impl.cc:3073: void RenderViewImpl::SetFocus(bool
enable) {
On 2015/10/16 16:50:45, alexmos wrote:
> I exposed this so that I could call it from RenderFrameProxy.  I thought about
> having the proxy directly call setFocused() through WebWidget, but the plugin
> notifications below seemed important to do also.  I'm assuming that if an
OOPIF
> hosts a plugin, that plugin should get recorded in the swapped-out RV (e.g.,
in
> active_pepper_instances_), though I haven't actually checked how plugins
> currently behave in OOPIFs.

Acknowledged.

[nasko, 2015-10-16 21:43:43]

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

[nasko, 2015-10-16 21:43:44]

Drive by comment.

https://codereview.chromium.org/1394383002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1394383002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2005: void
RenderFrameHostManager::ReplicatePageFocus(bool is_focused) {
This doesn't seem to logically belong in this class. It is walking the frame
tree and doing update work on it. RenderFrameHostManager is supposed to be state
management for RFHs and RFPHs and cross-process navigations. Therefore it
shouldn't do work for the entire tree. The FrameTree class should be taking care
of these things.

[nasko, 2015-10-16 21:44:21]

https://codereview.chromium.org/1394383002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1394383002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2021: CHECK(proxy);
No need to CHECK here, as it will crash on the next line.

[Charlie Reis, 2015-10-16 22:03:26]

https://codereview.chromium.org/1394383002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1394383002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2005: void
RenderFrameHostManager::ReplicatePageFocus(bool is_focused) {
On 2015/10/16 21:43:44, nasko (slow to review) wrote:
> This doesn't seem to logically belong in this class. It is walking the frame
> tree and doing update work on it. RenderFrameHostManager is supposed to be
state
> management for RFHs and RFPHs and cross-process navigations. Therefore it
> shouldn't do work for the entire tree. The FrameTree class should be taking
care
> of these things.

I could see this going either way.  It's broadcasting to a set of proxies, which
tends to be something that we do from RFHM.

I agree that it could fit well in FrameTree, though, since it's only the proxies
in the frame tree, plus it's about focus.  Works for me.

[alexmos, 2015-10-16 22:56:06]

Thanks!

https://codereview.chromium.org/1394383002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1394383002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:87: bool
CollectSiteInstances(std::set<SiteInstance*>* set,
On 2015/10/16 18:56:09, Charlie Reis wrote:
> nit: Add comment about what this is useful for (e.g., SiteInstances involved
in
> rendering the page, which is a subset of those in proxy_hosts_, thanks to
> openers).

Done.

https://codereview.chromium.org/1394383002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2013: // Main frame
should have proxies in all SiteInstances of other frames in
On 2015/10/16 18:56:09, Charlie Reis wrote:
> nit: Notify the proxies for all SiteInstances of other frames in this
FrameTree.
>  Note that the main frame might also know about proxies in...

Done.

https://codereview.chromium.org/1394383002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2016: // can't just
iterate over proxy_hosts_.
On 2015/10/16 18:56:09, Charlie Reis wrote:
> This comment should go above line 2005.

Done.

https://codereview.chromium.org/1394383002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1394383002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:502: // current
FrameTree, not including the main frame's SiteInstance.  The focus
On 2015/10/16 18:56:09, Charlie Reis wrote:
> nit: current main frame's

Done.

https://codereview.chromium.org/1394383002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/1394383002/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:714: if (IsRenderView()
&& delegate_)
On 2015/10/16 18:56:09, Charlie Reis wrote:
> Does this still work with Avi's changes to split RWH/RVH?

Yes, just confirmed that it does.  RVHI is still a RWHI.

https://codereview.chromium.org/1394383002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1394383002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2005: void
RenderFrameHostManager::ReplicatePageFocus(bool is_focused) {
On 2015/10/16 22:03:26, Charlie Reis wrote:
> On 2015/10/16 21:43:44, nasko (slow to review) wrote:
> > This doesn't seem to logically belong in this class. It is walking the frame
> > tree and doing update work on it. RenderFrameHostManager is supposed to be
> state
> > management for RFHs and RFPHs and cross-process navigations. Therefore it
> > shouldn't do work for the entire tree. The FrameTree class should be taking
> care
> > of these things.
> 
> I could see this going either way.  It's broadcasting to a set of proxies,
which
> tends to be something that we do from RFHM.
> 
> I agree that it could fit well in FrameTree, though, since it's only the
proxies
> in the frame tree, plus it's about focus.  Works for me.

Good suggestion -- moved to FrameTree.

https://codereview.chromium.org/1394383002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2021: CHECK(proxy);
On 2015/10/16 21:44:21, nasko (slow to review) wrote:
> No need to CHECK here, as it will crash on the next line.

Done.

[nasko, 2015-10-16 23:06:30]

Just a couple of things.

https://codereview.chromium.org/1394383002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1394383002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2484: // Check
that when a window is focused/blurred, the message that sets
Shouldn't this test move to frame_tree_unittests.cc if the code moved too?

https://codereview.chromium.org/1394383002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1394383002/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.h:23: class Message;
Why is this forward declared here? I don't see any new code that uses it.

[alexmos, 2015-10-16 23:23:30]

https://codereview.chromium.org/1394383002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1394383002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2484: // Check
that when a window is focused/blurred, the message that sets
On 2015/10/16 23:06:30, nasko (slow to review) wrote:
> Shouldn't this test move to frame_tree_unittests.cc if the code moved too?

I couldn't move it due to a dependency on NavigateToEntry.  As we discussed, I
left a TODO to move the test over once that's moved to a common place.

https://codereview.chromium.org/1394383002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1394383002/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_delegate.h:23: class Message;
On 2015/10/16 23:06:30, nasko (slow to review) wrote:
> Why is this forward declared here? I don't see any new code that uses it.

Oops, removed.  This sneaked through from a previous iteration of the code.

[alexmos, 2015-10-23 05:52:05]

The CQ bit was checked by alexmos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-23 05:52:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394383002/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394383002/230001

[commit-bot: I haz the power, 2015-10-23 09:41:17]

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

[commit-bot: I haz the power, 2015-10-23 09:41:18]

Dry run: This issue passed the CQ dry run.

[alexmos, 2015-10-23 18:08:50]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2015-10-23 18:08: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/1394383002/#ps230001
(title: "Rebase")

[commit-bot: I haz the power, 2015-10-23 18:09:34]

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

[commit-bot: I haz the power, 2015-10-23 18:18:40]

Committed patchset #13 (id:230001)

[commit-bot: I haz the power, 2015-10-23 18:19:32]

Patchset 13 (id:??) landed as
https://crrev.com/3fcd0cae4e0cdd067ef5d8a2f3bd1c3b12da3109
Cr-Commit-Position: refs/heads/master@{#355833}