Summary: | Simplify and modernize member functions of FocusNavigationScope | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
Component: | UI Events | Assignee: | Ryosuke Niwa <rniwa> | ||||||
Status: | RESOLVED WONTFIX | ||||||||
Severity: | Normal | CC: | cdumez, darin, kling, koivisto | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 151379 | ||||||||
Attachments: |
|
Description
Ryosuke Niwa
2016-02-24 15:54:15 PST
Created attachment 272162 [details]
Cleanup
Created attachment 272163 [details]
Removed a tab in change log
Comment on attachment 272163 [details] Removed a tab in change log View in context: https://bugs.webkit.org/attachment.cgi?id=272163&action=review > Source/WebCore/page/FocusController.cpp:118 > -static Node* parentInScope(const Node* node) > -{ > - if (node->isShadowRoot()) > - return nullptr; > - > - ContainerNode* parent = node->parentNode(); > - if (parent && parent->shadowRoot()) > - return nullptr; > - > - return parent; > -} > - > -Node* FocusNavigationScope::nextInScope(const Node* node) const > +Node* FocusNavigationScope::nextInScope(const Node& node) const > { > if (Node* next = firstChildInScope(node)) > return next; > - if (Node* next = node->nextSibling()) > - return next; > - const Node* current = node; > - while (current && !current->nextSibling()) > - current = parentInScope(current); > - return current ? current->nextSibling() : nullptr; > + return NodeTraversal::nextSkippingChildren(node); > } This changes behavior of these functions for children of shadow hosts. Why is that ok? Comment on attachment 272163 [details] Removed a tab in change log View in context: https://bugs.webkit.org/attachment.cgi?id=272163&action=review >> Source/WebCore/page/FocusController.cpp:118 >> } > > This changes behavior of these functions for children of shadow hosts. Why is that ok? That's a good point :( Comment on attachment 272163 [details] Removed a tab in change log View in context: https://bugs.webkit.org/attachment.cgi?id=272163&action=review > Source/WebCore/page/FocusController.cpp:85 > + Node* lastLeafNode() const > + { > + if (!m_lastLeafCache) > + findLastLeafNode(); > + return m_lastLeafCache; > + } Does this function body really need to be in the class definition? Makes it harder to read it over. Lets just define this inline function right afterward. >>> Source/WebCore/page/FocusController.cpp:118 >>> } >> >> This changes behavior of these functions for children of shadow hosts. Why is that ok? > > That's a good point :( So marking review- because of this. |