Move Document pointer from Frame to LocalFrame.

Source/core/page/FocusController.cpp

 /*
  * Copyright (C) 2006, 2007 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nuanti Ltd.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 102 matching lines @@
 }
 
 FocusNavigationScope FocusNavigationScope::ownedByShadowHost(Node* node)
 {
     ASSERT(isShadowHost(node));
     return FocusNavigationScope(toElement(node)->shadow()->youngestShadowRoot());
 }
 
 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)
 {
     ASSERT(isShadowInsertionPointFocusScopeOwner(*shadowInsertionPoint));
     return FocusNavigationScope(shadowInsertionPoint->olderShadowRoot());
 }
 
 static inline void dispatchEventsOnWindowAndFocusedNode(Document* document, bool focused)
 {
@@ skipping 155 matching lines @@
     }
 }
 
 Node* FocusController::findFocusableNodeDecendingDownIntoFrameDocument(FocusType type, Node* node)
 {
     // The node we found might be a HTMLFrameOwnerElement, so descend down the tree until we find either:
     // 1) a focusable node, or
     // 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)
             break;
         ASSERT(node != foundNode);
         node = foundNode;
     }
     return node;
 }
 
 bool FocusController::setInitialFocus(FocusType type)
 {
     bool didAdvanceFocus = advanceFocus(type, true);
 
     // 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;
 }
 
 bool FocusController::advanceFocus(FocusType type, bool initialFocus)
 {
     switch (type) {
     case FocusTypeForward:
     case FocusTypeBackward:
         return advanceFocusInDocumentOrder(type, initialFocus);
@@ skipping 312 matching lines @@
 
     // FIXME: Might want to disable this check for caretBrowsing
     if (oldFocusedElement && oldFocusedElement->isRootEditableElement() && !relinquishesEditingFocus(oldFocusedElement))
         return false;
 
     m_page->chrome().client().willSetInputMethodState();
 
     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;
 
     clearSelectionIfNeeded(oldFocusedFrame.get(), toLocalFrame(newFocusedFrame.get()), element);
 
     if (oldDocument && oldDocument != newDocument)
         oldDocument->setFocusedElement(nullptr);
 
     if (newFocusedFrame && !newFocusedFrame->page()) {
@@ skipping 110 matching lines @@
             closest = candidate;
         return;
     }
 
     if (candidate.alignment > closest.alignment)
         closest = candidate;
 }
 
 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;
     current.rect = startingRect;
     current.focusableNode = focusedElement;
     current.visibleNode = focusedElement;
 
     for (; element; element = (element->isFrameOwnerElement() || canScrollInDirection(element, type))
         ? ElementTraversal::nextSkippingChildren(*element, &container)
         : ElementTraversal::next(*element, &container)) {
@@ skipping 26 matching lines @@
     FocusCandidate focusCandidate;
     findFocusCandidateInContainer(*container, newStartingRect, type, focusCandidate);
 
     if (focusCandidate.isNull()) {
         // Nothing to focus, scroll if possible.
         // NOTE: If no scrolling is performed (i.e. scrollInDirection returns false), the
         // spatial navigation algorithm will skip this container.
         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);
         }
         return true;
     }
 
     if (canScrollInDirection(focusCandidate.visibleNode, type)) {
         if (focusCandidate.isOffscreenAfterScrolling) {
             scrollInDirection(focusCandidate.visibleNode, type);
             return true;
         }
         // 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);
     }
     if (focusCandidate.isOffscreenAfterScrolling) {
         Node* container = focusCandidate.enclosingScrollableBox;
         scrollInDirection(container, type);
         return true;
     }
 
@@ skipping 42 matching lines @@
         startingRect = nodeRectInAbsoluteCoordinates(container, true /* ignore border */);
         container = scrollableEnclosingBoxOrParentFrameForNodeInDirection(type, container);
         if (container && container->isDocumentNode())
             toDocument(container)->updateLayoutIgnorePendingStylesheets();
     } while (!consumed && container);
 
     return consumed;
 }
 
 } // namespace WebCore