Bug 28959 – Enable SHARED_WORKERS by default

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Andrew Wilson]

Now that SharedWorkers have feature parity with regular dedicated Workers (all of the APIs are implemented except for Database), it's time to turn on ENABLE_SHARED_WORKERS by default.

[Andrew Wilson]

Created attachment 39022 [details]
proposed patch

Renamed previously-disabled tests, cleaned up a couple of instances of bitrot (APIs that changed but weren't updated in disabled SharedWorker code, and turned on ENABLE_SHARED_WORKERS.

[Eric Seidel (no email)]

Comment on attachment 39022 [details]
proposed patch

Some of your sharedWorker tests could just use "shouldThrow" instead of custom exception detection code.  Likewise stuff which shouldn' thorugh could use shouldBeTrue("!!statement");

So there are no edits to these test files?  We're just moving them?  I'm surprised we didn't just have them in the Skipped lists.

Could we turn on SharedWorkers first, in one patch and then enable the tests in a second one? I think that would be easier to read.  Especially if the seocnd patch guarantees no changes to tests, just moving files.

[Andrew Wilson]

Created attachment 39074 [details]
Patch without moving tests

Updated the patch per eric's suggestion - moved the test renamings to a separate patch. Thank god for git.

[Andrew Wilson]

Created attachment 39100 [details]
Updated patch with test fix

When I merged with the latest tip-of-tree one more test started failing (apparently unhandled SharedWorker exceptions weren't being propagated to the console before, but they are now).

This exposed a bug in one of the tests, which I fixed, and also necessitated updating the test expectations.

[Andrew Wilson]

(In reply to comment #2)
> (From update of attachment 39022 [details])
> Some of your sharedWorker tests could just use "shouldThrow" instead of custom
> exception detection code.  Likewise stuff which shouldn' thorugh could use
> shouldBeTrue("!!statement");

Agreed, btw. As I write new tests, I've been using the helper functions in ../js/resources/js-test-pre.js. I haven't bothered going back over earlier tests to convert them as it doesn't seem like there's much benefit.

[David Levin]

Comment on attachment 39100 [details]
Updated patch with test fix

Marked as cq- because I think you wanted to land this yourself to handle possible build breakage.

[Andrew Wilson]

Committed as r48131