Bug 154630 – Move FocusNavigationScope into FocusController.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

Move FocusNavigationScope from FocusController.h to FocusController.cpp

[Ryosuke Niwa]

Created attachment 272077 [details]
Cleanup

[Ryosuke Niwa]

Created attachment 272085 [details]
Fixed builds

[Build Bot]

Comment on attachment 272085 [details]
Fixed builds

Attachment 272085 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/874778

New failing tests:
fast/events/frame-tab-focus.html
fast/html/tab-order.html
fast/forms/focus-option-control-on-page.html
svg/custom/tabindex-order.html

[Build Bot]

Created attachment 272087 [details]
Archive of layout-test-results from ews101 for mac-yosemite

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

[Build Bot]

Comment on attachment 272085 [details]
Fixed builds

Attachment 272085 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/874833

New failing tests:
fast/events/frame-tab-focus.html
fast/forms/focus-option-control-on-page.html
fast/html/tab-order.html
svg/custom/tabindex-order.html

[Build Bot]

Created attachment 272088 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

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

[Build Bot]

Comment on attachment 272085 [details]
Fixed builds

Attachment 272085 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/874834

New failing tests:
fast/events/frame-tab-focus.html
fast/html/tab-order.html
fast/forms/focus-option-control-on-page.html
svg/custom/tabindex-order.html

[Build Bot]

Created attachment 272089 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Ryosuke Niwa]

Created attachment 272091 [details]
Fixed builds for real

[Darin Adler]

Comment on attachment 272091 [details]
Fixed builds for real

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

Looks OK, but this needs some work.

> Source/WebCore/bindings/objc/DOM.mm:524
> +    Ref<KeyboardEvent> key = KeyboardEvent::createForDummy();
> +    return kit(page->focusController().nextFocusableElement(*core(self), key.get()));

Would look better as a one-liner. Also, do we really need the dummy keyboard event? Can we leave the argument as a pointer and support null instead? These are the only places that KeyboardEvent::createForDummy are used.

> Source/WebCore/bindings/objc/DOM.mm:536
> +    Ref<KeyboardEvent> key = KeyboardEvent::createForDummy();
> +    return kit(page->focusController().previousFocusableElement(*core(self), key.get()));

Ditto.

> Source/WebCore/page/FocusController.cpp:70
> +

I’d prefer we not add these additional blank lines.

> Source/WebCore/page/FocusController.cpp:73
> +    ContainerNode* rootNode() const;

Should return a reference, not a pointer.

> Source/WebCore/page/FocusController.cpp:75
> +    WEBCORE_EXPORT static FocusNavigationScope focusNavigationScopeOf(Node*);

Argument should be a reference, not a pointer.

> Source/WebCore/page/FocusController.cpp:76
> +    static FocusNavigationScope focusNavigationScopeOwnedByShadowHost(Node*);

Argument should be a reference, not a pointer.

> Source/WebCore/page/FocusController.cpp:77
> +    static FocusNavigationScope focusNavigationScopeOwnedByIFrame(HTMLFrameOwnerElement*);

Argument should be a reference, not a pointer.

> Source/WebCore/page/FocusController.cpp:79
> +    Node* nextInScope(const Node*) const;

Looks like this should be a static member function, not a const member function. The implementation doesn’t look at m_rootTreeScope. Also the argument should be a reference, not a pointer.

> Source/WebCore/page/FocusController.cpp:80
> +    Node* previousInScope(const Node*) const;

Looks like this should be a static member function, not a const member function. The implementation doesn’t look at m_rootTreeScope. Also the argument should be a reference, not a pointer.

> Source/WebCore/page/FocusController.cpp:81
> +    Node* lastChildInScope(const Node*) const;

Looks like this should be a static member function, not a const member function. The implementation doesn’t look at m_rootTreeScope. Also the argument should be a reference, not a pointer.

> Source/WebCore/page/FocusController.cpp:84
> +    Node* firstChildInScope(const Node*) const;

Looks like this should be a static member function, not a const member function. The implementation doesn’t look at m_rootTreeScope. Also the argument should be a reference, not a pointer.

> Source/WebCore/page/FocusController.cpp:86
> +    explicit FocusNavigationScope(TreeScope*);

Argument should be a reference, not  a pointer.

> Source/WebCore/page/FocusController.cpp:87
> +    TreeScope* m_rootTreeScope;

This should be a reference, not  a pointer.

> Source/WebCore/page/FocusController.cpp:296
> +

I’d prefer we not add these additional blank lines.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2327
> +    Ref<KeyboardEvent> key = KeyboardEvent::createForBindings();

Really peculiar that this code calls KeyboardEvent::createForBindings and not createForDummy. This is another case of creating a dummy event; can we just make it pass null?

[Build Bot]

Comment on attachment 272091 [details]
Fixed builds for real

Attachment 272091 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/875054

New failing tests:
fast/events/frame-tab-focus.html
fast/forms/focus-option-control-on-page.html
fast/html/tab-order.html
svg/custom/tabindex-order.html

[Build Bot]

Created attachment 272099 [details]
Archive of layout-test-results from ews101 for mac-yosemite

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

[Build Bot]

Comment on attachment 272091 [details]
Fixed builds for real

Attachment 272091 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/875053

New failing tests:
fast/events/frame-tab-focus.html
fast/html/tab-order.html
fast/forms/focus-option-control-on-page.html
svg/custom/tabindex-order.html

[Build Bot]

Created attachment 272100 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

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

[Build Bot]

Comment on attachment 272091 [details]
Fixed builds for real

Attachment 272091 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/875051

New failing tests:
fast/events/frame-tab-focus.html
fast/html/tab-order.html
fast/forms/focus-option-control-on-page.html
svg/custom/tabindex-order.html

[Build Bot]

Created attachment 272102 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Ryosuke Niwa]

Comment on attachment 272091 [details]
Fixed builds for real

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

>> Source/WebCore/bindings/objc/DOM.mm:524
>> +    return kit(page->focusController().nextFocusableElement(*core(self), key.get()));
> 
> Would look better as a one-liner. Also, do we really need the dummy keyboard event? Can we leave the argument as a pointer and support null instead? These are the only places that KeyboardEvent::createForDummy are used.

Moved the creation of the dummy event into nextFocusableElement since supporting nullptr there is quite hairy.

>> Source/WebCore/bindings/objc/DOM.mm:536
>> +    return kit(page->focusController().previousFocusableElement(*core(self), key.get()));
> 
> Ditto.

Ditto.

>> Source/WebCore/page/FocusController.cpp:70
>> +
> 
> I’d prefer we not add these additional blank lines.

Removed.

>> Source/WebCore/page/FocusController.cpp:73
>> +    ContainerNode* rootNode() const;
> 
> Should return a reference, not a pointer.

Let me make that and other pointer -> reference changes in a follow up patch to avoid making this change too big since that turned out be a pretty large change.

>> Source/WebCore/page/FocusController.cpp:296
>> +
> 
> I’d prefer we not add these additional blank lines.

Removed.

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2327
>> +    Ref<KeyboardEvent> key = KeyboardEvent::createForBindings();
> 
> Really peculiar that this code calls KeyboardEvent::createForBindings and not createForDummy. This is another case of creating a dummy event; can we just make it pass null?

Moved the creation of the dummy event into nextFocusableElement/previousFocusableElement.

[Ryosuke Niwa]

Created attachment 272105 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 272105 [details]
Patch for landing

Clearing flags on attachment: 272105

Committed r197021: <http://trac.webkit.org/changeset/197021>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #19)
> Comment on attachment 272105 [details]
> Patch for landing
> 
> Clearing flags on attachment: 272105
> 
> Committed r197021: <http://trac.webkit.org/changeset/197021>

It broke the iOS build, see build.webkit.org for details.

[Ryan Haddad]

Should be fixed with <https://trac.webkit.org/changeset/197032>