Move Document pointer from Frame to LocalFrame.

Source/core/page/FocusController.cpp

Index: Source/core/page/FocusController.cpp
diff --git a/Source/core/page/FocusController.cpp b/Source/core/page/FocusController.cpp
index aa083b5815785588fb729980527d7ca5f403bc01..a75d57d3133212b33f5bc2a06b9c2ae0bed5029f 100644
--- a/Source/core/page/FocusController.cpp
+++ b/Source/core/page/FocusController.cpp
@@ -120,8 +120,8 @@ FocusNavigationScope FocusNavigationScope::ownedByShadowHost(Node* node)
 
 FocusNavigationScope FocusNavigationScope::ownedByIFrame(HTMLFrameOwnerElement* frame)
 {
-    ASSERT(frame && frame->contentFrame());
-    return FocusNavigationScope(frame->contentFrame()->document());
+    ASSERT(frame && frame->contentFrame() && frame->contentFrame()->isLocalFrame());
+    return FocusNavigationScope(toLocalFrame(frame->contentFrame())->document());
 }
 
 FocusNavigationScope FocusNavigationScope::ownedByShadowInsertionPoint(HTMLShadowElement* shadowInsertionPoint)
@@ -297,7 +297,7 @@ Node* FocusController::findFocusableNodeDecendingDownIntoFrameDocument(FocusType
     // 2) the deepest-nested HTMLFrameOwnerElement.
     while (node && node->isFrameOwnerElement()) {
         HTMLFrameOwnerElement* owner = toHTMLFrameOwnerElement(node);
-        if (!owner->contentFrame())
+        if (!owner->contentFrame() || !owner->contentFrame()->isLocalFrame())

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

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?

[kenrb, 2014/04/14 20:09:52]

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.

             break;
         Node* foundNode = findFocusableNode(type, FocusNavigationScope::ownedByIFrame(owner), 0);
         if (!foundNode)
@@ -315,8 +315,11 @@ bool FocusController::setInitialFocus(FocusType type)
     // If focus is being set initially, accessibility needs to be informed that system focus has moved
     // into the web area again, even if focus did not change within WebCore. PostNotification is called instead
     // of handleFocusedUIElementChanged, because this will send the notification even if the element is the same.
-    if (AXObjectCache* cache = focusedOrMainFrame()->document()->existingAXObjectCache())
-        cache->postNotification(focusedOrMainFrame()->document(), AXObjectCache::AXFocusedUIElementChanged, true);
+    if (focusedOrMainFrame()->isLocalFrame()) {
+        Document* document = toLocalFrame(focusedOrMainFrame())->document();
+        if (AXObjectCache* cache = document->existingAXObjectCache())
+            cache->postNotification(document, AXObjectCache::AXFocusedUIElementChanged, true);
+    }
 
     return didAdvanceFocus;
 }
@@ -649,8 +652,8 @@ bool FocusController::setFocusedElement(Element* element, PassRefPtr<Frame> newF
     RefPtr<Document> newDocument;
     if (element)
         newDocument = &element->document();
-    else if (newFocusedFrame)
-        newDocument = newFocusedFrame->document();
+    else if (newFocusedFrame && newFocusedFrame->isLocalFrame())
+        newDocument = toLocalFrame(newFocusedFrame.get())->document();

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

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.

[kenrb, 2014/04/14 20:09:52]

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

 
     if (newDocument && oldDocument == newDocument && newDocument->focusedElement() == element)
         return true;
@@ -781,7 +784,7 @@ static void updateFocusCandidateIfNeeded(FocusType type, const FocusCandidate& c
 
 void FocusController::findFocusCandidateInContainer(Node& container, const LayoutRect& startingRect, FocusType type, FocusCandidate& closest)
 {
-    Element* focusedElement = (focusedFrame() && focusedFrame()->document()) ? focusedFrame()->document()->focusedElement() : 0;
+    Element* focusedElement = (focusedFrame() && toLocalFrame(focusedFrame())->document()) ? toLocalFrame(focusedFrame())->document()->focusedElement() : 0;

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

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?

[kenrb, 2014/04/14 20:09:52]

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.

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

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:44]

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

 
     Element* element = ElementTraversal::firstWithin(container);
     FocusCandidate current;
@@ -828,23 +831,23 @@ bool FocusController::advanceFocusDirectionallyInContainer(Node* container, cons
         return scrollInDirection(container, type);
     }
 
-    if (HTMLFrameOwnerElement* frameElement = frameOwnerElement(focusCandidate)) {

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

Why was this if removed?

[kenrb, 2014/04/14 20:09:52]

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().

-        // If we have an iframe without the src attribute, it will not have a contentFrame().
-        // We ASSERT here to make sure that
-        // updateFocusCandidateIfNeeded() will never consider such an iframe as a candidate.
-        ASSERT(frameElement->contentFrame());
-
+    HTMLFrameOwnerElement* frameElement = frameOwnerElement(focusCandidate);
+    // If we have an iframe without the src attribute, it will not have a contentFrame().
+    // We ASSERT here to make sure that
+    // updateFocusCandidateIfNeeded() will never consider such an iframe as a candidate.
+    ASSERT(!frameElement || frameElement->contentFrame());
+    if (frameElement && frameElement->contentFrame()->isLocalFrame()) {
         if (focusCandidate.isOffscreenAfterScrolling) {
             scrollInDirection(&focusCandidate.visibleNode->document(), type);
             return true;
         }
         // Navigate into a new frame.
         LayoutRect rect;
-        Element* focusedElement = focusedOrMainFrame()->document()->focusedElement();
+        Element* focusedElement = toLocalFrame(focusedOrMainFrame())->document()->focusedElement();
         if (focusedElement && !hasOffscreenRect(focusedElement))
             rect = nodeRectInAbsoluteCoordinates(focusedElement, true /* ignore border */);
-        frameElement->contentFrame()->document()->updateLayoutIgnorePendingStylesheets();
-        if (!advanceFocusDirectionallyInContainer(frameElement->contentFrame()->document(), rect, type)) {
+        toLocalFrame(frameElement->contentFrame())->document()->updateLayoutIgnorePendingStylesheets();
+        if (!advanceFocusDirectionallyInContainer(toLocalFrame(frameElement->contentFrame())->document(), rect, type)) {
             // The new frame had nothing interesting, need to find another candidate.
             return advanceFocusDirectionallyInContainer(container, nodeRectInAbsoluteCoordinates(focusCandidate.visibleNode, true), type);
         }
@@ -858,7 +861,7 @@ bool FocusController::advanceFocusDirectionallyInContainer(Node* container, cons
         }
         // Navigate into a new scrollable container.
         LayoutRect startingRect;
-        Element* focusedElement = focusedOrMainFrame()->document()->focusedElement();
+        Element* focusedElement = toLocalFrame(focusedOrMainFrame())->document()->focusedElement();

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

Does this need to be protected by an isLocalFrame() check?

[kenrb, 2014/04/14 20:09:52]

Same comment as above re: directional focus advancement.

         if (focusedElement && !hasOffscreenRect(focusedElement))
             startingRect = nodeRectInAbsoluteCoordinates(focusedElement, true);
         return advanceFocusDirectionallyInContainer(focusCandidate.visibleNode, startingRect, type);