Bug 73269 – js-test-pre: don't try to load stylesheet from a web worker

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

[David Grogan]

js-test-pre: don't try to load stylesheet from a web worker

[David Grogan]

Created attachment 116854 [details]
Patch

[David Grogan]

Ojan, could you review this?

[Ojan Vafai]

Comment on attachment 116854 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        The other DOM-modification functions can be overridden but
> +        insertStyleSheet is called when js-test-pre loads.

I'm fine with adding the logic to make this file work in workers here. No need to override IMO. That will just make it harder to maintain the file since the definitions will be in different places.

> LayoutTests/fast/js/resources/js-test-pre.js:72
> +    if (typeof document !== 'undefined')

I would find this more readable if you made an isWorker helper function that returned this value.

[David Grogan]

(In reply to comment #3)
> (From update of attachment 116854 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116854&action=review
> 
> > LayoutTests/ChangeLog:8
> > +        The other DOM-modification functions can be overridden but
> > +        insertStyleSheet is called when js-test-pre loads.
> 
> I'm fine with adding the logic to make this file work in workers here. No need to override IMO. That will just make it harder to maintain the file since the definitions will be in different places.

Sounds good.

> > LayoutTests/fast/js/resources/js-test-pre.js:72
> > +    if (typeof document !== 'undefined')
> 
> I would find this more readable if you made an isWorker helper function that returned this value.

Done.

[David Grogan]

Created attachment 116871 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 116871 [details]
Patch for landing

Clearing flags on attachment: 116871

Committed r101320: <http://trac.webkit.org/changeset/101320>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Erik Arvidsson]

Comment on attachment 116871 [details]
Patch for landing

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

What tests inserts this in a worker?

> LayoutTests/fast/js/resources/js-test-pre.js:80
>      window.onerror = function(message)

Doesn't the above line fail too?

[David Grogan]

(In reply to comment #8)
> (From update of attachment 116871 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116871&action=review
> 
> What tests inserts this in a worker?

Nothing checked in yet, but I have a few tests for indexeddb on workers locally that do.

> > LayoutTests/fast/js/resources/js-test-pre.js:80
> >      window.onerror = function(message)
> 
> Doesn't the above line fail too?

My tests assign window = self if run from a worker so I didn't run into any problems with it.  But I see the potential problem.