[webkit-reviews] review denied: [Bug 78472] ShadowRoot needs applyAuthorStyles : [Attachment 137719] Patch

Sat Apr 21 18:26:41 PDT 2012

Ryosuke Niwa <rniwa at webkit.org> has denied Takashi Sakamoto
<tasak at google.com>'s request for review:
Bug 78472: ShadowRoot needs applyAuthorStyles
https://bugs.webkit.org/show_bug.cgi?id=78472

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

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

> Source/WebCore/ChangeLog:12
> +	   seems to work in the same way as applyAuthorStyles, just renamed all
applyAuthorSheets and
> +	   added applyAuthorStyles to ShadowRoot.idl.

Nit: renamed all applyAuthorSheets to applyAuthorStyles, and added
applyAuthorStyles to ShadowRoot.idl.

> LayoutTests/fast/dom/shadow/shadow-root-applyAuthorStyles-expected.html:4
> +<head>
> +</head>

We don't need head for this, do we?

> LayoutTests/fast/dom/shadow/shadow-root-applyAuthorStyles-expected.html:7
> +<div id="apply-author-style"><span style="margin:2px;
border:solid"></span></div>
> +<div id="no-apply-author-style"><span></span></div>

Can we add more test cases like font-size, color, background-color, etc...?
Also, you should definitely test cases where the author style is overridden by
inline style declaration or
cases where it overrides UA rules or when it's overridden by important UA
rules.
Furthermore, you should test a case where you change the value of
applyAuthorStyles dynamically and
verify that the change takes place dynamically as expected.

r- due to lack of adequate tests.

> LayoutTests/fast/dom/shadow/shadow-root-applyAuthorStyles.html:27
> +    var sr = new WebKitShadowRoot(div);

Please don't use abbreviations like sr. Also, does this constructor work on
ports that don't enable shadow root support yet?

> LayoutTests/fast/dom/shadow/shadow-root-applyAuthorStyles.html:31
> +    var span = document.createElement('span');
> +    sr.appendChild(span);

Since you're not using span elsewhere, you can just do:
sr.appendChild(document.createElement('span'));