Bug 70432 – JS Test Harness: Insert the stylesheet dynamically

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Erik Arvidsson]

JS Test Harness: Insert the stylesheet dynamically

[Erik Arvidsson]

Created attachment 111653 [details]
Patch

[Ojan Vafai]

Comment on attachment 111653 [details]
Patch

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?

[Erik Arvidsson]

Comment on attachment 111653 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111653&action=review

>> 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?

I'll be happy to another follow up patch cleaning more console/description. My goal with the previous patch was to not cause too many change to the expected results.

In this patch I'll update the ones that already need a rebaseline.

>> 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?

Added a FIXME. I'm looking at fixing the tests that depend on this.

> LayoutTests/fast/js/resources/js-test-pre.js:80
> +        var path = findPath();

I added a FIXME here too. We should just user a style element instead.

[Erik Arvidsson]

Committed r97881: <http://trac.webkit.org/changeset/97881>