[webkit-reviews] review granted: [Bug 70432] JS Test Harness: Insert the stylesheet dynamically : [Attachment 111653] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Erik Arvidsson <arv at chromium.org>'s
request for review:
Bug 70432: JS Test Harness: Insert the stylesheet dynamically
https://bugs.webkit.org/show_bug.cgi?id=70432

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=111653&action=review


YAY!

> LayoutTests/fast/css/counters/2displays-expected.txt:9
>  TEST COMPLETE
> +PASS layoutTestController.counterValueForElementById('testView') is '1000
1000'

It's a little unfortunate that test complete is printed before the test is
done, but this is a stupid limitation of the js-test infrastructure that should
be fixed in a separate patch. So, I think this is fine for now.

> LayoutTests/fast/dom/nodesFromRect-inner-documents.html:57
> +  <div id="console"></div>

Nit: do we need the console element here? Won't it be injected by js-test-pre?

> LayoutTests/fast/forms/file/file-input-change-event.html:12
> +<p id="description">
>  This tests the condition that triggers a 'change' event on file input forms.

>  </p>
> -</div>
> +<div id="console"></div>

nit: Should this use description() and exclude both these elements?

> LayoutTests/fast/forms/input-search-press-escape-key.html:16
> -<div id="console">
> -<p>
> +<p id="description">
>  This tests if the value in a search input form is cleared
>  and a 'search' event is triggered, when we press the Escape key.
>  To run (a part of) this test manually,
>  type some text in the search form and then press the Escape key.
>  If the text is cleared, then the test passes.
>  </p>
> -</div>
> +

Should this use description()?

> LayoutTests/fast/forms/spin-button-gets-disabled-or-readonly.html:21
> +<div id="console"></div>

It this element needed?

> LayoutTests/fast/js/resources/js-test-pre.js:53
> +	   var re = /^(.*resources\/)js-test-pre\.js/;

s/re/regexp/ ?

> LayoutTests/fast/js/resources/js-test-pre.js:56
> +	       var m;

s/m/match/ ?

> LayoutTests/fast/js/resources/js-test-pre.js:60
> +	       if (src && (m = re.exec(src))) {
> +		   return m[1];
> +	       }

nit: webkit style is to not have the curly-braces here.

> LayoutTests/fast/js/resources/js-test-pre.js:65
> +    function hasStyleSheet() {

are there cases where you haven't deleted the stylesheet? if not, we should
just get rid of this. if yes, maybe add a FIXME to get rid of all those cases?