Bug 76762 - js-test-pre.js: relay calls to testPassed, testFailed, debug, etc from worker to document
Summary: js-test-pre.js: relay calls to testPassed, testFailed, debug, etc from worker...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Grogan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-20 18:17 PST by David Grogan
Modified: 2012-02-10 13:41 PST (History)
7 users (show)

See Also:


Attachments
Patch (37.59 KB, patch)
2012-01-20 18:41 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (33.66 KB, patch)
2012-02-03 18:53 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (33.71 KB, patch)
2012-02-06 13:10 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (34.61 KB, patch)
2012-02-06 13:27 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (33.61 KB, patch)
2012-02-06 15:48 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch for landing (35.89 KB, patch)
2012-02-06 18:12 PST, David Grogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2012-01-20 18:17:40 PST
Add worker overrides to js-test-pre.js
Comment 1 David Grogan 2012-01-20 18:41:02 PST
Created attachment 123424 [details]
Patch
Comment 2 David Grogan 2012-02-03 18:53:55 PST
Created attachment 125462 [details]
Patch
Comment 3 David Grogan 2012-02-03 19:11:08 PST
Eric and Kinuko, could you take a look at this refactoring and let me know if you have any opinions?
Comment 4 Kinuko Yasuda 2012-02-05 00:38:32 PST
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.
Comment 5 David Grogan 2012-02-05 23:49:16 PST
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?
Comment 6 David Grogan 2012-02-06 13:10:05 PST
Created attachment 125698 [details]
Patch
Comment 7 David Grogan 2012-02-06 13:27:22 PST
Created attachment 125701 [details]
Patch
Comment 8 Eric U. 2012-02-06 13:31:12 PST
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.
Comment 9 David Grogan 2012-02-06 13:44:12 PST
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.
Comment 10 Erik Arvidsson 2012-02-06 14:05:51 PST
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.
Comment 11 Ojan Vafai 2012-02-06 14:08:35 PST
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.
Comment 12 Ojan Vafai 2012-02-06 14:10:06 PST
(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.
Comment 13 Ojan Vafai 2012-02-06 14:10:40 PST
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?
Comment 14 David Grogan 2012-02-06 15:48:50 PST
Created attachment 125719 [details]
Patch
Comment 15 David Grogan 2012-02-06 15:51:03 PST
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.
Comment 16 David Grogan 2012-02-06 18:12:12 PST
Created attachment 125744 [details]
Patch for landing
Comment 17 WebKit Review Bot 2012-02-06 20:05:38 PST
Comment on attachment 125744 [details]
Patch for landing

Clearing flags on attachment: 125744

Committed r106898: <http://trac.webkit.org/changeset/106898>
Comment 18 WebKit Review Bot 2012-02-06 20:05:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Csaba Osztrogonác 2012-02-07 08:19:18 PST
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.
Comment 20 Csaba Osztrogonác 2012-02-07 08:21:16 PST
Hmmm ... it was wrong bug ... Sorry was the noise.
Comment 21 Csaba Osztrogonác 2012-02-07 08:24:25 PST
(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
Comment 22 David Grogan 2012-02-07 10:41:57 PST
Ossy, any idea why this wasn't caught by either the ews bots or the cq?
Comment 23 David Grogan 2012-02-10 13:41:32 PST
I think this is all fine now.