[webkit-reviews] review denied: [Bug 62358] Needs a unified way to handle a case when a shadow host is focused. : [Attachment 98704] Patch

[bugzilla-daemon at webkit.org]

MORITA Hajime <morrita at google.com> has denied Hayato Ito
<hayato at chromium.org>'s request for review:
Bug 62358: Needs a unified way to handle a case when a shadow host is focused.
https://bugs.webkit.org/show_bug.cgi?id=62358

Attachment 98704: Patch
https://bugs.webkit.org/attachment.cgi?id=98704&action=review

------- Additional Comments from MORITA Hajime <morrita at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=98704&action=review

Thanks for the update!
I'd like to have one more turn.

> Source/WebCore/ChangeLog:14
> +	   That should be addressed in another patch.

Could you file a bug for this? It would block 59802.

> Source/WebCore/ChangeLog:21
> +	   (WebCore::FocusController::deepFocusableNode):

Could you update the description?

> Source/WebCore/page/FocusController.cpp:159
> +bool FocusController::transferFocusToElementInShadowRoot(Element*
shadowHost, bool restorePreviousSelection)

Could you define a two-value enum instead of bool? It's a recent WebCore trend.


> Source/WebCore/page/FocusController.cpp:162
> +    Node* node = deepFocusableNode(FocusDirectionForward, shadowHost, 0);

I'd love to have more informative name.

> Source/WebCore/page/FocusController.cpp:163
> +    if (shadowHost != node) {

How about to exit  early? It's more preferable in general.

> Source/WebCore/page/FocusController.cpp:186
> +	       if (node->hasTagName(inputTag) || node->hasTagName(textareaTag)
|| node->hasTagName(videoTag) || node->hasTagName(audioTag))

I might be better to extract this conditional to a function like
hasCustomFocusLogic or something.
Then we can use it for an assertion.