[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
bugzilla-daemon at webkit.org
Mon Jun 27 21:42:48 PDT 2011
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.
More information about the webkit-reviews
mailing list