[webkit-reviews] review denied: [Bug 79886] ShadowRoot should have activeElement : [Attachment 130612] Updated Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Kaustubh Atrawalkar
<kaustubh at motorola.com>'s request for review:
Bug 79886: ShadowRoot should have activeElement
https://bugs.webkit.org/show_bug.cgi?id=79886

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=130612&action=review


> Source/WebCore/dom/ShadowRoot.cpp:162
> +    if (this->document()->isHTMLDocument())

Why do we need this->?

> Source/WebCore/dom/TreeScope.cpp:177
> +    Node* treeScope = node->treeScope()->rootNode();

Why are you calling the root node a tree scope? That makes very little sense.
r- because of this.

> Source/WebCore/dom/TreeScope.cpp:178
> +    while (treeScope != this->rootNode()) {

Why do we need this->?	Also, you can just use m_rootNode instead.

> Source/WebCore/dom/TreeScope.cpp:181
> +	   if (treeScope == document)
> +	       break;

You can merge this condition with treeScope != this->rootNode().

> LayoutTests/fast/dom/shadow/shadow-root-activeElement-expected.txt:9
> +p1 is focused

Please don't use two-letter variables like p1. You should give a descriptive
name (e.g. firstElementInShadowDOM).

> LayoutTests/fast/dom/shadow/shadow-root-activeElement.html:20
> +shadowRoot = new WebKitShadowRoot(shadowHost);

We definitely need a test case where we have a shadow DOM inside another shadow
DOM.

> LayoutTests/fast/dom/shadow/shadow-root-activeElement.html:32
> +debug("Check if activeElement attribute is defined");

This debug output isn't useful at all. It repeats what the code says.

> LayoutTests/fast/dom/shadow/shadow-root-activeElement.html:37
> +debug("p1 is focused");
> +p1.focus();
> +shouldBe("shadowRoot.activeElement", "p1");

Instead of saying that p1 is focused, it's much more clear to just do:
shouldBe("p1.focus();shadowRoot.activeElement", "p1");

(of course, after giving p1 a more descriptive name).