Bug 154630

Summary:

Move FocusNavigationScope into FocusController.cpp

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

DOM

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, cdumez, commit-queue, darin, koivisto, ossy, rniwa, ryanhaddad

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

151379

Attachments:

Description	Flags
Cleanup	none
Fixed builds	none
Archive of layout-test-results from ews101 for mac-yosemite	none
Archive of layout-test-results from ews105 for mac-yosemite-wk2	none
Archive of layout-test-results from ews113 for mac-yosemite	none
Fixed builds for real	none
Archive of layout-test-results from ews101 for mac-yosemite	none
Archive of layout-test-results from ews106 for mac-yosemite-wk2	none
Archive of layout-test-results from ews114 for mac-yosemite	none
Patch for landing	none

Ryosuke Niwa

Reported 2016-02-23 19:36:50 PST

Move FocusNavigationScope from FocusController.h to FocusController.cpp

Attachments
Cleanup (15.87 KB, patch) 2016-02-23 19:45 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Fixed builds (15.87 KB, patch) 2016-02-23 20:56 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews101 for mac-yosemite (874.81 KB, application/zip) 2016-02-23 21:24 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1015.94 KB, application/zip) 2016-02-23 21:48 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews113 for mac-yosemite (842.68 KB, application/zip) 2016-02-23 21:53 PST, Build Bot	no flags	Details
Fixed builds for real (20.17 KB, patch) 2016-02-23 22:11 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews101 for mac-yosemite (825.00 KB, application/zip) 2016-02-23 22:59 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (815.35 KB, application/zip) 2016-02-23 23:02 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews114 for mac-yosemite (836.09 KB, application/zip) 2016-02-23 23:09 PST, Build Bot	no flags	Details
Patch for landing (19.68 KB, patch) 2016-02-24 00:37 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2016-02-23 19:45:12 PST

Created attachment 272077 [details] Cleanup

Ryosuke Niwa

Comment 2 2016-02-23 20:56:46 PST

Created attachment 272085 [details] Fixed builds

Build Bot

Comment 3 2016-02-23 21:24:04 PST

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

Comment 4 2016-02-23 21:24:07 PST

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 5 2016-02-23 21:48:01 PST

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

Comment 6 2016-02-23 21:48:04 PST

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 7 2016-02-23 21:53:05 PST

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

Comment 8 2016-02-23 21:53:08 PST

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

Comment 9 2016-02-23 22:11:22 PST

Created attachment 272091 [details] Fixed builds for real

Darin Adler

Comment 10 2016-02-23 22:26:54 PST

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 11 2016-02-23 22:59:39 PST

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

Comment 12 2016-02-23 22:59:42 PST

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 13 2016-02-23 23:02:34 PST

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

Comment 14 2016-02-23 23:02:37 PST

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 15 2016-02-23 23:08:57 PST

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

Comment 16 2016-02-23 23:09:01 PST

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 17 2016-02-23 23:10:08 PST

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

Comment 18 2016-02-24 00:37:09 PST

Created attachment 272105 [details] Patch for landing

WebKit Commit Bot

Comment 19 2016-02-24 01:36:09 PST

Comment on attachment 272105 [details] Patch for landing Clearing flags on attachment: 272105 Committed r197021: <http://trac.webkit.org/changeset/197021>

WebKit Commit Bot

Comment 20 2016-02-24 01:36:14 PST

All reviewed patches have been landed. Closing bug.

Csaba Osztrogonác

Comment 21 2016-02-24 01:57:13 PST

(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

Comment 22 2016-02-24 09:38:20 PST

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

Note You need to log in before you can comment on or make changes to this bug.