[Webkit-unassigned] [Bug 79886] ShadowRoot should have activeElement

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=79886


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #130612|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #11 from Ryosuke Niwa <rniwa at webkit.org>  2012-03-07 22:52:35 PST ---
(From update of attachment 130612)
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).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.