Move Document pointer from Frame to LocalFrame.

Move Document pointer from Frame to LocalFrame.

RemoteFrames do not need Document members, so this belongs in the
LocalFrame class and clients who access Document need to ensure that
they are handling a LocalFrame.

R=dcheng@chromium.org, eseidel@chromium.org
BUG=346764
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171584

[kenrb, 2014-04-11 22:58:36]

Daniel or Eric, can either of you have a look at this please?

[dcheng, 2014-04-14 19:25:57]

https://codereview.chromium.org/235553006/diff/1/Source/core/frame/LocalFrame...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/235553006/diff/1/Source/core/frame/LocalFrame...
Source/core/frame/LocalFrame.cpp:620: doc->topDocument().clearAXObjectCache();
Do you know what accessibility is planning on doing here? Right now the AXObject
cache always lives on the document of the top-most frame, but some/all of that
state moves to the browser, we can devirtualize this again right?

https://codereview.chromium.org/235553006/diff/1/Source/core/page/FocusContro...
File Source/core/page/FocusController.cpp (left):

https://codereview.chromium.org/235553006/diff/1/Source/core/page/FocusContro...
Source/core/page/FocusController.cpp:831: if (HTMLFrameOwnerElement*
frameElement = frameOwnerElement(focusCandidate)) {
Why was this if removed?

https://codereview.chromium.org/235553006/diff/1/Source/core/page/FocusContro...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/235553006/diff/1/Source/core/page/FocusContro...
Source/core/page/FocusController.cpp:300: if (!owner->contentFrame() ||
!owner->contentFrame()->isLocalFrame())
This means we'll stop searching as soon as we hit a remote frame right? I expect
this will change when browser is helping with focus as well?

https://codereview.chromium.org/235553006/diff/1/Source/core/page/FocusContro...
Source/core/page/FocusController.cpp:656: newDocument =
toLocalFrame(newFocusedFrame.get())->document();
Do you happen to know if these codepaths like this and on line 319-321 are
covered by layout tests? I expect these to break temporarily until we hoist part
of focus into chrome/browser, but it'd be good to be able to verify our
solutions.

https://codereview.chromium.org/235553006/diff/1/Source/core/page/FocusContro...
Source/core/page/FocusController.cpp:787: Element* focusedElement =
(focusedFrame() && toLocalFrame(focusedFrame())->document()) ?
toLocalFrame(focusedFrame())->document()->focusedElement() : 0;
Are we skipping the isLocalFrame() check here because we assume that this code
will only be hit when focusedFrame() is a local frame with the browser-side
focus work?

https://codereview.chromium.org/235553006/diff/1/Source/core/page/FocusContro...
Source/core/page/FocusController.cpp:864: Element* focusedElement =
toLocalFrame(focusedOrMainFrame())->document()->focusedElement();
Does this need to be protected by an isLocalFrame() check?

[kenrb, 2014-04-14 20:09:51]

https://codereview.chromium.org/235553006/diff/1/Source/core/frame/LocalFrame...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/235553006/diff/1/Source/core/frame/LocalFrame...
Source/core/frame/LocalFrame.cpp:620: doc->topDocument().clearAXObjectCache();
On 2014/04/14 19:25:57, dcheng wrote:
> Do you know what accessibility is planning on doing here? Right now the
AXObject
> cache always lives on the document of the top-most frame, but some/all of that
> state moves to the browser, we can devirtualize this again right?

I don't know much about accessibility, so I have just been trying to preserve
existing behavior.

https://codereview.chromium.org/235553006/diff/1/Source/core/page/FocusContro...
File Source/core/page/FocusController.cpp (left):

https://codereview.chromium.org/235553006/diff/1/Source/core/page/FocusContro...
Source/core/page/FocusController.cpp:831: if (HTMLFrameOwnerElement*
frameElement = frameOwnerElement(focusCandidate)) {
On 2014/04/14 19:25:57, dcheng wrote:
> Why was this if removed?

It wasn't, it was just moved lower -- line 839 in the new file. Just the ASSERT
had to be moved outside of the if body because we need to do that before
checking isLocalFrame().

https://codereview.chromium.org/235553006/diff/1/Source/core/page/FocusContro...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/235553006/diff/1/Source/core/page/FocusContro...
Source/core/page/FocusController.cpp:300: if (!owner->contentFrame() ||
!owner->contentFrame()->isLocalFrame())
On 2014/04/14 19:25:57, dcheng wrote:
> This means we'll stop searching as soon as we hit a remote frame right? I
expect
> this will change when browser is helping with focus as well?

Yes we are treating the remote frame as a focused element for now. Inter-process
node traversal for focus changing is part of the FocusController refactoring we
have in our work queue.

https://codereview.chromium.org/235553006/diff/1/Source/core/page/FocusContro...
Source/core/page/FocusController.cpp:656: newDocument =
toLocalFrame(newFocusedFrame.get())->document();
On 2014/04/14 19:25:57, dcheng wrote:
> Do you happen to know if these codepaths like this and on line 319-321 are
> covered by layout tests? I expect these to break temporarily until we hoist
part
> of focus into chrome/browser, but it'd be good to be able to verify our
> solutions.

This code only changes behavior for cross-site iframes. I am not aware of any
layout test coverage that would explicitly hit that.

https://codereview.chromium.org/235553006/diff/1/Source/core/page/FocusContro...
Source/core/page/FocusController.cpp:787: Element* focusedElement =
(focusedFrame() && toLocalFrame(focusedFrame())->document()) ?
toLocalFrame(focusedFrame())->document()->focusedElement() : 0;
On 2014/04/14 19:25:57, dcheng wrote:
> Are we skipping the isLocalFrame() check here because we assume that this code
> will only be hit when focusedFrame() is a local frame with the browser-side
> focus work?

I think that assumption is correct, but I put a bailout in
advanceFocusDirectionally() just in case. Directional focus navigation won't
work with remote frames until the refactor so we already have that returning 0
if a RemoteFrame currently holds focus.

https://codereview.chromium.org/235553006/diff/1/Source/core/page/FocusContro...
Source/core/page/FocusController.cpp:864: Element* focusedElement =
toLocalFrame(focusedOrMainFrame())->document()->focusedElement();
On 2014/04/14 19:25:57, dcheng wrote:
> Does this need to be protected by an isLocalFrame() check?

Same comment as above re: directional focus advancement.

[dcheng, 2014-04-14 21:09:29]

https://codereview.chromium.org/235553006/diff/1/Source/core/page/FocusContro...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/235553006/diff/1/Source/core/page/FocusContro...
Source/core/page/FocusController.cpp:787: Element* focusedElement =
(focusedFrame() && toLocalFrame(focusedFrame())->document()) ?
toLocalFrame(focusedFrame())->document()->focusedElement() : 0;
On 2014/04/14 20:09:52, kenrb wrote:
> On 2014/04/14 19:25:57, dcheng wrote:
> > Are we skipping the isLocalFrame() check here because we assume that this
code
> > will only be hit when focusedFrame() is a local frame with the browser-side
> > focus work?
> 
> I think that assumption is correct, but I put a bailout in
> advanceFocusDirectionally() just in case. Directional focus navigation won't
> work with remote frames until the refactor so we already have that returning 0
> if a RemoteFrame currently holds focus.

I don't see any changes, so maybe you didn't upload the latest patchset?
That being said, it's probably OK to leave as is. I'd prefer the code crash when
we incorrectly handle RemoteFrames so we know what to fix up.

[kenrb, 2014-04-15 14:20:43]

https://codereview.chromium.org/235553006/diff/1/Source/core/page/FocusContro...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/235553006/diff/1/Source/core/page/FocusContro...
Source/core/page/FocusController.cpp:787: Element* focusedElement =
(focusedFrame() && toLocalFrame(focusedFrame())->document()) ?
toLocalFrame(focusedFrame())->document()->focusedElement() : 0;
On 2014/04/14 21:09:29, dcheng wrote:
> On 2014/04/14 20:09:52, kenrb wrote:
> > On 2014/04/14 19:25:57, dcheng wrote:
> > > Are we skipping the isLocalFrame() check here because we assume that this
> code
> > > will only be hit when focusedFrame() is a local frame with the
browser-side
> > > focus work?
> > 
> > I think that assumption is correct, but I put a bailout in
> > advanceFocusDirectionally() just in case. Directional focus navigation won't
> > work with remote frames until the refactor so we already have that returning
0
> > if a RemoteFrame currently holds focus.
> 
> I don't see any changes, so maybe you didn't upload the latest patchset?
> That being said, it's probably OK to leave as is. I'd prefer the code crash
when
> we incorrectly handle RemoteFrames so we know what to fix up.

Sorry if I wasn't clear. The check already exists, I added it in a previous CL.
See FocusController::advanceFocusDirectionally().

[dcheng, 2014-04-15 16:42:56]

lgtm

[kenrb, 2014-04-15 17:23:52]

The CQ bit was checked by kenrb@chromium.org

[commit-bot: I haz the power, 2014-04-15 17:24:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenrb@chromium.org/235553006/1

[commit-bot: I haz the power, 2014-04-15 17:35:46]

Change committed as 171584