Move has_focus_ tracking from RenderWidget to RenderView.

Move has_focus_ tracking from RenderWidget to RenderView.

This will simplify setting page-level focus for OOPIFs.

BUG=339659
Committed: https://crrev.com/7fac9aeb0d52b0fe573b661c823dbf9f22ae84b3
Cr-Commit-Position: refs/heads/master@{#354625}

[alexmos, 2015-10-14 19:15:49]

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

[alexmos, 2015-10-14 19:15:50]

Charlie, can you please take a look?  This is a straightforward move of the
has_focus_ bit from RW to RV.  Even though it was being for all RenderWidgets,
all callers of has_focus() are currently doing so through RenderView (one case
used RFI::GetRenderWidget, which also just returns the RFI's view).  This should
clear the way for me to send one page-level focus message per process in the
other CL (https://codereview.chromium.org/1394383002/).  Note that as I
mentioned, moving the IPC plumbing over to RV is more involved, as we have some
cases where a non-view RWH can send these focus messages, such as for fullscreen
Flash.

[Charlie Reis, 2015-10-14 19:32:52]

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

[Charlie Reis, 2015-10-14 19:32:52]

Seems good to me, but I've got a question for Daniel and Ken about how Blink
tracks the equivalent notion of focus.

https://codereview.chromium.org/1400003003/diff/60001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/1400003003/diff/60001/content/renderer/render...
content/renderer/render_view_impl.h:235: bool has_focus() const { return
has_focus_; }
Let's document what this means.  It's page-level focus (not frame-level focus),
right?

https://codereview.chromium.org/1400003003/diff/60001/content/renderer/render...
content/renderer/render_view_impl.h:878: // Indicates whether we have been
focused/unfocused by the browser.
s/we have/this page has/

https://codereview.chromium.org/1400003003/diff/60001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1400003003/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1309: webwidget_->setFocus(enable);
Daniel/Ken, can I get a sanity check here?  content/ will now track page-level
focus on the view rather than the widget, since there could be more than one
widget in the page.

This call shows that Blink is still tracking (presumably) page-level focus at
the widget level.  Is that sane?  What happens if there are multiple widgets?

[alexmos, 2015-10-14 20:12:33]

https://codereview.chromium.org/1400003003/diff/60001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/1400003003/diff/60001/content/renderer/render...
content/renderer/render_view_impl.h:235: bool has_focus() const { return
has_focus_; }
On 2015/10/14 19:32:52, Charlie Reis wrote:
> Let's document what this means.  It's page-level focus (not frame-level
focus),
> right?

Done.

https://codereview.chromium.org/1400003003/diff/60001/content/renderer/render...
content/renderer/render_view_impl.h:878: // Indicates whether we have been
focused/unfocused by the browser.
On 2015/10/14 19:32:52, Charlie Reis wrote:
> s/we have/this page has/

Done.

https://codereview.chromium.org/1400003003/diff/60001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1400003003/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1309: webwidget_->setFocus(enable);
On 2015/10/14 19:32:52, Charlie Reis wrote:
> Daniel/Ken, can I get a sanity check here?  content/ will now track page-level
> focus on the view rather than the widget, since there could be more than one
> widget in the page.
> 
> This call shows that Blink is still tracking (presumably) page-level focus at
> the widget level.  Is that sane?  What happens if there are multiple widgets?

Yes, it'd be good to have Daniel or Ken double-check this.  IIUC, Blink doesn't
track this at the widget level but rather at Page level, and all the widget
implementations that handle this proceed to forward it to blink::Page's
FocusController.  If there are multiple widgets due to --site-per-process,
currently only the top one gets this message (that's why we don't have
page-level focus set correctly in subframe processes).

It might be good to eventually remove page-level focus tracking from
FocusController and get it from the embedder instead (i.e., from has_focus_), so
that the two can't become inconsistent.

[kenrb, 2015-10-15 18:57:30]

https://codereview.chromium.org/1400003003/diff/60001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1400003003/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1309: webwidget_->setFocus(enable);
On 2015/10/14 20:12:33, alexmos wrote:
> On 2015/10/14 19:32:52, Charlie Reis wrote:
> > Daniel/Ken, can I get a sanity check here?  content/ will now track
page-level
> > focus on the view rather than the widget, since there could be more than one
> > widget in the page.
> > 
> > This call shows that Blink is still tracking (presumably) page-level focus
at
> > the widget level.  Is that sane?  What happens if there are multiple
widgets?
> 
> Yes, it'd be good to have Daniel or Ken double-check this.  IIUC, Blink
doesn't
> track this at the widget level but rather at Page level, and all the widget
> implementations that handle this proceed to forward it to blink::Page's
> FocusController.  If there are multiple widgets due to --site-per-process,
> currently only the top one gets this message (that's why we don't have
> page-level focus set correctly in subframe processes).
> 
> It might be good to eventually remove page-level focus tracking from
> FocusController and get it from the embedder instead (i.e., from has_focus_),
so
> that the two can't become inconsistent.

It looks like setFocus() could move from from WebWidget to WebView, since indeed
it does simply access the Page from there. It could be cleaner, and would let us
remove the currently duplicated code between WebViewImpl and WebFrameWidget, but
I don't actually know if there is any functional improvement.

Alex, what do you mean by "currently on the top one get the message"? It looks
to me like it wouldn't even matter which WebWidget is used for the setFocus()
call, since it doesn't access any WebWidget members other than what it needs to
access the Page. FocusController should know which Frame has focus regardless of
which WebWidget it is under.

[alexmos, 2015-10-15 20:29:30]

https://codereview.chromium.org/1400003003/diff/60001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1400003003/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1309: webwidget_->setFocus(enable);
On 2015/10/15 18:57:30, kenrb wrote:
> On 2015/10/14 20:12:33, alexmos wrote:
> > On 2015/10/14 19:32:52, Charlie Reis wrote:
> > > Daniel/Ken, can I get a sanity check here?  content/ will now track
> page-level
> > > focus on the view rather than the widget, since there could be more than
one
> > > widget in the page.
> > > 
> > > This call shows that Blink is still tracking (presumably) page-level focus
> at
> > > the widget level.  Is that sane?  What happens if there are multiple
> widgets?
> > 
> > Yes, it'd be good to have Daniel or Ken double-check this.  IIUC, Blink
> doesn't
> > track this at the widget level but rather at Page level, and all the widget
> > implementations that handle this proceed to forward it to blink::Page's
> > FocusController.  If there are multiple widgets due to --site-per-process,
> > currently only the top one gets this message (that's why we don't have
> > page-level focus set correctly in subframe processes).
> > 
> > It might be good to eventually remove page-level focus tracking from
> > FocusController and get it from the embedder instead (i.e., from
has_focus_),
> so
> > that the two can't become inconsistent.
> 
> It looks like setFocus() could move from from WebWidget to WebView, since
indeed
> it does simply access the Page from there. It could be cleaner, and would let
us
> remove the currently duplicated code between WebViewImpl and WebFrameWidget,
but
> I don't actually know if there is any functional improvement.

I think this would be good to do.  It seems that WebFrameWidget's implementation
is actually an imcomplete copy (it doesn't deal with blur), so this would fix
that.  But how would we deal with the WebPagePopupImpl::setFocus, which is the
other implementation?

> Alex, what do you mean by "currently on the top one get the message"? It looks
> to me like it wouldn't even matter which WebWidget is used for the setFocus()
> call, since it doesn't access any WebWidget members other than what it needs
to
> access the Page. FocusController should know which Frame has focus regardless
of
> which WebWidget it is under.

Right.  I was just pointing out that subframe WebWidgets never actually receive
that message today, since the browser process only sends it for the top frame's
WebWidget, but I agree that it wouldn't matter even if they did.

[kenrb, 2015-10-16 20:38:16]

https://codereview.chromium.org/1400003003/diff/60001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1400003003/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1309: webwidget_->setFocus(enable);
On 2015/10/15 20:29:30, alexmos wrote:
> On 2015/10/15 18:57:30, kenrb wrote:
> > On 2015/10/14 20:12:33, alexmos wrote:
> > > On 2015/10/14 19:32:52, Charlie Reis wrote:
> > > > Daniel/Ken, can I get a sanity check here?  content/ will now track
> > page-level
> > > > focus on the view rather than the widget, since there could be more than
> one
> > > > widget in the page.
> > > > 
> > > > This call shows that Blink is still tracking (presumably) page-level
focus
> > at
> > > > the widget level.  Is that sane?  What happens if there are multiple
> > widgets?
> > > 
> > > Yes, it'd be good to have Daniel or Ken double-check this.  IIUC, Blink
> > doesn't
> > > track this at the widget level but rather at Page level, and all the
widget
> > > implementations that handle this proceed to forward it to blink::Page's
> > > FocusController.  If there are multiple widgets due to --site-per-process,
> > > currently only the top one gets this message (that's why we don't have
> > > page-level focus set correctly in subframe processes).
> > > 
> > > It might be good to eventually remove page-level focus tracking from
> > > FocusController and get it from the embedder instead (i.e., from
> has_focus_),
> > so
> > > that the two can't become inconsistent.
> > 
> > It looks like setFocus() could move from from WebWidget to WebView, since
> indeed
> > it does simply access the Page from there. It could be cleaner, and would
let
> us
> > remove the currently duplicated code between WebViewImpl and WebFrameWidget,
> but
> > I don't actually know if there is any functional improvement.
> 
> I think this would be good to do.  It seems that WebFrameWidget's
implementation
> is actually an imcomplete copy (it doesn't deal with blur), so this would fix
> that.  But how would we deal with the WebPagePopupImpl::setFocus, which is the
> other implementation?
> 

That's a good point, and probably a reason to leave it on WebWidget.

[alexmos, 2015-10-16 21:11:13]

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

[commit-bot: I haz the power, 2015-10-16 21:11:22]

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

[Charlie Reis, 2015-10-16 21:56:23]

Thanks for checking.  Unfortunate about WebPagePopupImpl, since otherwise moving
to WebView would have made sense.

LGTM.

[commit-bot: I haz the power, 2015-10-16 22:09:20]

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

[commit-bot: I haz the power, 2015-10-16 22:09:21]

Dry run: This issue passed the CQ dry run.

[alexmos, 2015-10-16 22:30:41]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-10-16 22:34:00]

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

[commit-bot: I haz the power, 2015-10-16 22:39:56]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-16 22:40:31]

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