Make document.activeElement work with OOPIF

Make document.activeElement work with OOPIF.

When a subframe is focused, the parent frame's document.activeElement
needs to return the <iframe> element of that subframe.  This CL adds
logic to make this work for cross-process frames.  Specifically, when
the focused frame changes across processes, we now send the new
(remote) focused frame to the other SiteInstances in the same
FrameTree.  This remote frame is then set by the FocusController as
its current focused frame.

The Blink logic used by document.activeElement is in
TreeScope::adjustedFocusedElement().  To make it work properly with
remote focused frames, this CL also introduces a new FocusController
accessor for getting the focused <iframe> element and moves TreeScope's
focusedFrameOwnerElement logic there.

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

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

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

[alexmos, 2015-10-26 22:33:22]

Daniel, can you please take an initial look at the Blink bits?  Turns out if we
plumb remote focused frames, Blink's document.activeElement logic just works,
provided we can feed it with the remote focused frame.

Another thought is that maybe we can move focusedFrameOwnerElement to
FocusController and expose that directly.  That would let us postpone adding an
accessor that can get remote focused frames.

https://codereview.chromium.org/1423053002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/FocusController.h (right):

https://codereview.chromium.org/1423053002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FocusController.h:55: Frame*
focusedFrameEvenIfRemote() const { return m_focusedFrame.get(); }
Name suggestions are very welcome. :)  Now that focusedFrame() only returns
local focused frames, this is intended to be used for special cases that do need
to know about remote focused frames.  Let me know if this is kind of what you
had in mind when we discussed this last week.

[dcheng, 2015-10-28 00:34:15]

https://codereview.chromium.org/1423053002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/FocusController.h (right):

https://codereview.chromium.org/1423053002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FocusController.h:55: Frame*
focusedFrameEvenIfRemote() const { return m_focusedFrame.get(); }
On 2015/10/26 at 22:33:21, alexmos wrote:
> Name suggestions are very welcome. :)  Now that focusedFrame() only returns
local focused frames, this is intended to be used for special cases that do need
to know about remote focused frames.  Let me know if this is kind of what you
had in mind when we discussed this last week.

This is essentially what I had in mind. I can't think of a good name either
though.

https://codereview.chromium.org/1423053002/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/1423053002/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebLocalFrame.h:155: virtual void
clearFocus(WebRemoteFrame*) = 0;
Hm...

It feels a little wonky to have both this and WebView::setFocusedFrame. Could we
consolidate the two cleanly somehow?

[alexmos, 2015-11-02 23:23:51]

Daniel, can you please take another look?  Some explanations below.  Plumbing
which remote frames are focused should also fix test flakiness in my keyboard CL
(http://crrev.com/1405293002).

https://codereview.chromium.org/1423053002/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/1423053002/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebLocalFrame.h:155: virtual void
clearFocus(WebRemoteFrame*) = 0;
On 2015/10/28 00:34:15, dcheng wrote:
> Hm...
> 
> It feels a little wonky to have both this and WebView::setFocusedFrame. Could
we
> consolidate the two cleanly somehow?

After also investigating what it'd take to support cross-process window.focus(),
I've changed the approach a little.  I removed clearFocus from WebLocalFrame,
and instead I plumb through WebView, which makes more sense anyway for
page-level FocusController stuff.

I also realized that calling setFocusedElement for this was actually not
entirely correct, as it cleared the focused element, which it didn't need to do.
 The better thing to call was FocusController::focusDocumentView, which fires
the same events without clearing the focused element in old focused frame.  This
is what I exposed from WebViewImpl.

I've changed FrameMsg_ClearFocus to FrameMsg_SetFocusedFrame, which is now sent
by the frame or proxy that needs to become focused.  So in a page A-embed-B, if
subframe B becomes focused, its proxy in A will receive this message, and we
will set the corresponding remote frame as focused in A.  This message will also
support window.focus (I've got this working in a separate CL).

https://codereview.chromium.org/1423053002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebView.h (right):

https://codereview.chromium.org/1423053002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebView.h:173: virtual void
focusDocumentView(WebFrame*) = 0;
Instead of this, I could add a bool flag to setFocusedFrame about whether or not
it should fire these events.  A separate function seemed more readable though,
and it matches what FocusController already exposes.

[dcheng, 2015-11-04 01:46:21]

https://codereview.chromium.org/1423053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.h (right):

https://codereview.chromium.org/1423053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.h:55: Frame*
focusedFrameEvenIfRemote() const { return m_focusedFrame.get(); }
Crazy idea: can we move the logic for determining the focused frame owner
element completely into FocusController so we don't need to expose the fact that
a RemoteFrame as the focused frame?

https://codereview.chromium.org/1423053002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebView.h (right):

https://codereview.chromium.org/1423053002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebView.h:173: virtual void
focusDocumentView(WebFrame*) = 0;
On 2015/11/02 at 23:23:51, alexmos wrote:
> Instead of this, I could add a bool flag to setFocusedFrame about whether or
not it should fire these events.  A separate function seemed more readable
though, and it matches what FocusController already exposes.

I think this is fine: it's pretty subtle though. Maybe something to keep in mind
for potential future naming improvements.

[alexmos, 2015-11-04 18:08:12]

https://codereview.chromium.org/1423053002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.h (right):

https://codereview.chromium.org/1423053002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.h:55: Frame*
focusedFrameEvenIfRemote() const { return m_focusedFrame.get(); }
On 2015/11/04 01:46:21, dcheng wrote:
> Crazy idea: can we move the logic for determining the focused frame owner
> element completely into FocusController so we don't need to expose the fact
that
> a RemoteFrame as the focused frame?

I like this - done!  Please take a look.

[dcheng, 2015-11-04 19:16:47]

third_party/WebKit lgtm

https://codereview.chromium.org/1423053002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.h (right):

https://codereview.chromium.org/1423053002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.h:56: Element*
focusedFrameOwnerElement(LocalFrame* currentFrame) const;
Nit: LocalFrame& since this should never be null.

Also, let's comment this function.

[alexmos, 2015-11-04 21:33:41]

https://codereview.chromium.org/1423053002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.h (right):

https://codereview.chromium.org/1423053002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.h:56: Element*
focusedFrameOwnerElement(LocalFrame* currentFrame) const;
On 2015/11/04 19:16:47, dcheng wrote:
> Nit: LocalFrame& since this should never be null.
> 
> Also, let's comment this function.

Done.  Also, changed the return type to HTMLFrameOwnerElement, which is what
this function actually returns.

[alexmos, 2015-11-04 21:53:58]

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

[alexmos, 2015-11-04 21:53:59]

Charlie, can you please review the content/ side?  

The overall goal here is to propagate information about which remote frame is
focused to all SiteInstances in a FrameTree.  Previously, on cross-process frame
focus changes, we only cleared the focused frame in the old focused frame's
renderer, but didn't actually send information about which remote frame becomes
focused.

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

https://codereview.chromium.org/1423053002/diff/120001/content/browser/frame_...
content/browser/frame_host/frame_tree.cc:281: for (const auto& instance :
frame_tree_site_instances) {
The last part of my test illustrates why it's now necessary to update the remote
focused frame in all SiteInstances in the tree: if you have A-embed-B-embed-C,
and switch focus from A to C, B needs to find out about that, since its
document.activeElement should now return the <iframe> element for C.  There are
probably optimizations for this (i.e., we may not need to notify leaf nodes),
but I just did this for simplicity.

https://codereview.chromium.org/1423053002/diff/120001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1423053002/diff/120001/content/common/frame_m...
content/common/frame_messages.h:659:
IPC_MESSAGE_ROUTED0(FrameMsg_SetFocusedFrame);
Currently, this is only called on proxies, but in my window.focus() CL, this
will also be called on RenderFrames to set them as focused.  I'll be adding the
RFH/RF plumbing to handle this message in that CL.

[Charlie Reis, 2015-11-04 22:23:42]

Thanks for the explanation!  document.activeElement is kind of unfortunate for
us (in terms of the extra IPCs required), but this looks like it will work.

content/ LGTM.

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

https://codereview.chromium.org/1423053002/diff/120001/content/browser/frame_...
content/browser/frame_host/frame_tree.cc:281: for (const auto& instance :
frame_tree_site_instances) {
On 2015/11/04 21:53:59, alexmos wrote:
> The last part of my test illustrates why it's now necessary to update the
remote
> focused frame in all SiteInstances in the tree: if you have A-embed-B-embed-C,
> and switch focus from A to C, B needs to find out about that, since its
> document.activeElement should now return the <iframe> element for C.  There
are
> probably optimizations for this (i.e., we may not need to notify leaf nodes),
> but I just did this for simplicity.

Acknowledged.

https://codereview.chromium.org/1423053002/diff/120001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1423053002/diff/120001/content/common/frame_m...
content/common/frame_messages.h:659:
IPC_MESSAGE_ROUTED0(FrameMsg_SetFocusedFrame);
On 2015/11/04 21:53:59, alexmos wrote:
> Currently, this is only called on proxies, but in my window.focus() CL, this
> will also be called on RenderFrames to set them as focused.  I'll be adding
the
> RFH/RF plumbing to handle this message in that CL.

Acknowledged.

[alexmos, 2015-11-04 23:29:11]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2015-11-04 23:29:12]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1423053002/#ps120001
(title: "Remove some plumbing that should instead be introduced in the
window.focus() CL.")

[commit-bot: I haz the power, 2015-11-04 23:30:20]

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

[commit-bot: I haz the power, 2015-11-05 00:59:27]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-05 01:00:46]

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