[webkit-reviews] review denied: [Bug 64587] Add support for retrieving an element in TreeScope by id to window.internals object. : [Attachment 101297] ready for review

[bugzilla-daemon at webkit.org]

MORITA Hajime <morrita at google.com> has denied Hayato Ito
<hayato at chromium.org>'s request for review:
Bug 64587: Add support for retrieving an element in TreeScope by id to
window.internals object.
https://bugs.webkit.org/show_bug.cgi?id=64587

Attachment 101297: ready for review
https://bugs.webkit.org/attachment.cgi?id=101297&action=review

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


> LayoutTests/fast/dom/shadow/get-element-by-id-in-shadow.html:11
> +

Please make it clear that this test need internals object and doesn't work
interactively.

> LayoutTests/fast/dom/shadow/get-element-by-id-in-shadow.html:17
> +	   element.setAttribute(name, attributes[name]);

Don't need braces if the block has single statement.
JS in WebKit basically follows our C++ style.

> LayoutTests/fast/dom/shadow/get-element-by-id-in-shadow.html:60
> +

These helper functions look useful. How about to make it separate js file?

> Source/WebCore/testing/Internals.cpp:76
> +	   ec = INVALID_ACCESS_ERR;

If this function is only for ShadowRoot, the name should be
getElementByIdInTreeRoot().

> Source/WebCore/testing/Internals.cpp:79
> +    return static_cast<ShadowRoot*>(treeScope)->getElementById(id);

I guess we have toShadowRoot().