Move FocusNavigationScope from FocusController.h to FocusController.cpp
Created attachment 272077 [details] Cleanup
Created attachment 272085 [details] Fixed builds
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
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
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
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
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
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
Created attachment 272091 [details] Fixed builds for real
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?
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
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
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
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
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
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
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.
Created attachment 272105 [details] Patch for landing
Comment on attachment 272105 [details] Patch for landing Clearing flags on attachment: 272105 Committed r197021: <http://trac.webkit.org/changeset/197021>
All reviewed patches have been landed. Closing bug.
(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.
Should be fixed with <https://trac.webkit.org/changeset/197032>