Bug 154660 – Simplify and modernize member functions of FocusNavigationScope

• Status: RESOLVED
• Resolution: WONTFIX
• Component: UI Events

[Ryosuke Niwa]

Use NodeTraversal for example

[Ryosuke Niwa]

Created attachment 272162 [details]
Cleanup

[Ryosuke Niwa]

Created attachment 272163 [details]
Removed a tab in change log

[Antti Koivisto]

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?

[Ryosuke Niwa]

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 :(

[Darin Adler]

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.