Bug 76762 – js-test-pre.js: relay calls to testPassed, testFailed, debug, etc from worker to document

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

[David Grogan]

Add worker overrides to js-test-pre.js

[David Grogan]

Created attachment 123424 [details]
Patch

[David Grogan]

Created attachment 125462 [details]
Patch

[David Grogan]

Eric and Kinuko, could you take a look at this refactoring and let me know if you have any opinions?

[Kinuko Yasuda]

Comment on attachment 125462 [details]
Patch

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

Looks good refactoring to me, though you'll probably want to hear comments from the author of js-test-pre.js as well.  Thanks for the cleanup!

> LayoutTests/fast/js/resources/js-test-pre.js:92
> +    return typeof document === 'undefined';

Slightly not sure if this is the best way to detect if it's in worker or not.

[David Grogan]

Comment on attachment 125462 [details]
Patch

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

>> LayoutTests/fast/js/resources/js-test-pre.js:92
>> +    return typeof document === 'undefined';
> 
> Slightly not sure if this is the best way to detect if it's in worker or not.

Do you think !!self.importScripts would be better?

[David Grogan]

Created attachment 125698 [details]
Patch

[David Grogan]

Created attachment 125701 [details]
Patch

[Eric U.]

Comment on attachment 125701 [details]
Patch

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

> LayoutTests/fast/js/resources/js-test-pre.js:450
> +

This message-passing system was a quick hack that was good enough for filesystem tests.
I dunno if folks want it escaping into the larger JS test infrastructure environment.
I guess it's all here in one place, so if anyone wants to clean up the protocol, then can do that without breaking anything.

[David Grogan]

Ojan, could you review this patch?

It moves the worker overrides from a few scattered files (fs-worker-test-util.js, fs-worker-common.js, idb-worker-common.js) into js-test-pre.js.  Eric's comment is pertinent though.

[Erik Arvidsson]

I think it would be cleaner to have a js-test-pre-worker.js file that does the overloading instead of adding all these if (isWorker()) throughout the original file.

[Ojan Vafai]

Comment on attachment 125701 [details]
Patch

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

Looks fine to me, I would just like to see the requirement of prefixing with CODE: to go away. Makes writing more generic worker tests easier.

> LayoutTests/fast/js/resources/js-test-pre.js:459
> +            debug("Got invalid message from worker:" + (event.data ? event.data : "null"));

Printing null for the empty string seems wrong to me. We should print "''" or just leave it as the empty string.

Instead of requiring the arbitrary messages be prefixed with MESG:, how about making anything that doesn't start with CODE: get printed to debug.

[Ojan Vafai]

(In reply to comment #10)
> I think it would be cleaner to have a js-test-pre-worker.js file that does the overloading instead of adding all these if (isWorker()) throughout the original file.

Yeah, that makes sense to me too. I guess the downside is that you now have to import both files.

[Ojan Vafai]

Comment on attachment 125701 [details]
Patch

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

> LayoutTests/fast/filesystem/resources/file-writer-write-overlapped.js:2
>      importScripts('fs-worker-common.js');

You should be deleting these imports, no?

[David Grogan]

Created attachment 125719 [details]
Patch

[David Grogan]

Comment on attachment 125701 [details]
Patch

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

Arv, let me know if putting the overrides at the end of js-test-pre is sufficiently clean.

>> LayoutTests/fast/filesystem/resources/file-writer-write-overlapped.js:2
>>      importScripts('fs-worker-common.js');
> 
> You should be deleting these imports, no?

There are still a few filesystem functions in them.

>> LayoutTests/fast/js/resources/js-test-pre.js:459
>> +            debug("Got invalid message from worker:" + (event.data ? event.data : "null"));
> 
> Printing null for the empty string seems wrong to me. We should print "''" or just leave it as the empty string.
> 
> Instead of requiring the arbitrary messages be prefixed with MESG:, how about making anything that doesn't start with CODE: get printed to debug.

Good idea, done.

[David Grogan]

Created attachment 125744 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 125744 [details]
Patch for landing

Clearing flags on attachment: 125744

Committed r106898: <http://trac.webkit.org/changeset/106898>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because a layout test needs to be updated.
Qt and GTK fix landed in http://trac.webkit.org/changeset/106948

I think other ports needs this update too.

[Csaba Osztrogonác]

Hmmm ... it was wrong bug ... Sorry was the noise.

[Csaba Osztrogonác]

(In reply to comment #20)
> Hmmm ... it was wrong bug ... Sorry was the noise.
Ahh, it isn't my day. :)

This change caused it:
http://trac.webkit.org/changeset/106898/trunk/LayoutTests/fast/js/resources/js-test-pre.js

[David Grogan]

Ossy, any idea why this wasn't caught by either the ews bots or the cq?

[David Grogan]

I think this is all fine now.