Bug 122431 – Move FocusController from Page to MainFrame

• Component: Page Loading
• Status: RESOLVED INVALID

[Darin Adler]

Move FocusController from Page to MainFrame

[Darin Adler]

Created attachment 213541 [details]
Patch

[Andreas Kling]

Comment on attachment 213541 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213541&action=review

> Source/WebCore/dom/Element.cpp:1959
> +        if (!frame->mainFrame().focus().setFocusedElement(this, *document().frame(), direction))

*document().frame() -> *frame

> Source/WebCore/page/EventHandler.cpp:3319
> -        bool changedFocusedFrame = m_frame.page() && &m_frame != &m_frame.page()->focusController().focusedOrMainFrame();
> +        bool changedFocusedFrame = m_frame.page() && &m_frame != &m_frame.mainFrame().focus().focusedOrMainFrame();

I think we can lose the m_frame.page() check here. I suspect it was just there due to pointer paranoia.

> Source/WebCore/page/EventHandler.cpp:3341
> -    bool changedFocusedFrame = m_frame.page() && &m_frame != &m_frame.page()->focusController().focusedOrMainFrame();
> +    bool changedFocusedFrame = m_frame.page() && &m_frame != &m_frame.mainFrame().focus().focusedOrMainFrame();

Ditto.

> Source/WebCore/page/FocusController.cpp:200
> -    m_focusedFrame = newFrame;
> +    if (oldFrame != &m_mainFrame)
> +        oldFrame->deref();
> +    if (newFrame != &m_mainFrame)
> +        newFrame->ref();
> +    m_focusedFrame = newFrame.get();

This could probably use a comment.

> Source/WebCore/page/FocusController.cpp:357
> +        document.setFocusedElement(0);

nullptr

> Source/WebCore/page/FocusController.cpp:429
> +// @param start The node from which to start searching. The node after this will be focused. May be null.
> +//
> +// @return The focus node that comes after/before start node.

I know you are just moving them, but these comments sure look out-of-place in WebKit.

> Source/WebCore/page/FocusController.cpp:784
> +    Node* focusedNode = (m_focusedFrame && m_focusedFrame->document()) ? m_focusedFrame->document()->focusedElement() : 0;

Looks like this should be an Element*.

> Source/WebCore/page/FocusController.h:86
> +    Frame* m_focusedFrame;

Bug! This is missing from the constructor initializer list.

[Darin Adler]

Created attachment 213548 [details]
Patch

[EFL EWS Bot]

Comment on attachment 213548 [details]
Patch

Attachment 213548 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3363092

[Andreas Kling]

Comment on attachment 213548 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213548&action=review

EWS looks sick, so I'm just dumping some comments for now:

> Source/WebCore/css/SelectorChecker.cpp:414
> +                Frame* frame = element->document().frame();
> +                return !frame || !frame->mainFrame().focus().isActive();

I wonder if we can really find ourselves matching selectors while Document is Frame-less.
isFrameFocused(Element*) in the same file seems to indicate that we could, but I'm not sure.
Our future selves can figure that out.

> Source/WebCore/page/Page.cpp:421
> +    if (!m_mainFrame)
> +        return;

This should not happen. m_mainFrame is initialized by the Page constructor and never cleared.

> Source/WebCore/page/Page.cpp:563
> +    if (!m_mainFrame)
> +        return false;

Same here.

[EFL EWS Bot]

Comment on attachment 213548 [details]
Patch

Attachment 213548 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3521053

[Build Bot]

Comment on attachment 213548 [details]
Patch

Attachment 213548 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3515058

[Build Bot]

Comment on attachment 213548 [details]
Patch

Attachment 213548 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/3521057

[Build Bot]

Comment on attachment 213548 [details]
Patch

Attachment 213548 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3514058

[kov's GTK+ EWS bot]

Comment on attachment 213548 [details]
Patch

Attachment 213548 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/3563013

[Darin Adler]

Created attachment 213640 [details]
Patch

[Build Bot]

Comment on attachment 213640 [details]
Patch

Attachment 213640 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3746009

New failing tests:
compositing/backface-visibility/backface-visibility-hierarchical-transform.html
accessibility/anchor-linked-anonymous-block-crash.html
compositing/absolute-inside-out-of-view-fixed.html
animations/3d/matrix-transform-type-animation.html
animations/3d/state-at-end-event-transform.html
animations/added-while-suspended.html
animations/animation-add-events-in-handler.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
accessibility/accessibility-object-detached.html
compositing/backface-visibility/backface-visibility-3d.html
accessibility/accessibility-node-reparent.html
animations/animation-border-overflow.html
http/tests/appcache/abort-cache-onchecking-resource-404.html
compositing/animation/animation-compositing.html
canvas/philip/tests/2d.canvas.readonly.html
animations/3d/change-transform-in-end-event.html
compositing/absolute-position-changed-in-composited-layer.html
compositing/animation/animated-composited-inside-hidden.html
http/tests/appcache/abort-cache-onchecking-manifest-404.html
canvas/philip/tests/2d.canvas.reference.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
http/tests/appcache/404-resource.html
animations/3d/transform-origin-vs-functions.html
accessibility/accessibility-node-memory-management.html
http/tests/appcache/abort-cache-onchecking.html
accessibility/adjacent-continuations-cause-assertion-failure.html
http/tests/appcache/404-manifest.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[Build Bot]

Created attachment 213644 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[EFL EWS Bot]

Comment on attachment 213640 [details]
Patch

Attachment 213640 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3713086

[Build Bot]

Comment on attachment 213640 [details]
Patch

Attachment 213640 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/3708064

[EFL EWS Bot]

Comment on attachment 213640 [details]
Patch

Attachment 213640 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3746015

[kov's GTK+ EWS bot]

Comment on attachment 213640 [details]
Patch

Attachment 213640 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/3342079

[Build Bot]

Comment on attachment 213640 [details]
Patch

Attachment 213640 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3868020

New failing tests:
compositing/backface-visibility/backface-visibility-hierarchical-transform.html
compositing/absolute-inside-out-of-view-fixed.html
animations/3d/matrix-transform-type-animation.html
animations/3d/state-at-end-event-transform.html
animations/added-while-suspended.html
animations/animation-add-events-in-handler.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
accessibility/accessibility-object-detached.html
compositing/backface-visibility/backface-visibility-3d.html
accessibility/accessibility-node-reparent.html
animations/animation-border-overflow.html
http/tests/appcache/abort-cache-onchecking-resource-404.html
compositing/animation/animation-compositing.html
canvas/philip/tests/2d.canvas.readonly.html
animations/3d/change-transform-in-end-event.html
compositing/animation/busy-indicator.html
compositing/absolute-position-changed-in-composited-layer.html
compositing/animation/animated-composited-inside-hidden.html
http/tests/appcache/abort-cache-onchecking-manifest-404.html
canvas/philip/tests/2d.canvas.reference.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
accessibility/accessibility-node-memory-management.html
compositing/backface-visibility/backface-visibility-image.html
compositing/backface-visibility/backface-visibility-non3d.html
accessibility/adjacent-continuations-cause-assertion-failure.html
http/tests/appcache/404-manifest.html
http/tests/appcache/404-resource.html
compositing/absolute-position-changed-with-composited-parent-layer.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[Build Bot]

Created attachment 213885 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Darin Adler]

Chris removed MainFrame so this idea doesn’t apply any more.