Add worker overrides to js-test-pre.js
Created attachment 123424 [details] Patch
Created attachment 125462 [details] Patch
Eric and Kinuko, could you take a look at this refactoring and let me know if you have any opinions?
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 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?
Created attachment 125698 [details] Patch
Created attachment 125701 [details] Patch
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.
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.
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 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.
(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 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?
Created attachment 125719 [details] Patch
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.
Created attachment 125744 [details] Patch for landing
Comment on attachment 125744 [details] Patch for landing Clearing flags on attachment: 125744 Committed r106898: <http://trac.webkit.org/changeset/106898>
All reviewed patches have been landed. Closing bug.
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.
Hmmm ... it was wrong bug ... Sorry was the noise.
(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
Ossy, any idea why this wasn't caught by either the ews bots or the cq?
I think this is all fine now.