Bug 28959 - Enable SHARED_WORKERS by default
Summary: Enable SHARED_WORKERS by default
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Andrew Wilson
URL:
Keywords:
Depends on:
Blocks: 28977
  Show dependency treegraph
 
Reported: 2009-09-03 17:12 PDT by Andrew Wilson
Modified: 2009-09-07 12:54 PDT (History)
2 users (show)

See Also:


Attachments
proposed patch (77.22 KB, patch)
2009-09-03 17:24 PDT, Andrew Wilson
no flags Details | Formatted Diff | Diff
Patch without moving tests (33.29 KB, patch)
2009-09-04 10:19 PDT, Andrew Wilson
no flags Details | Formatted Diff | Diff
Updated patch with test fix (35.81 KB, patch)
2009-09-04 17:03 PDT, Andrew Wilson
levin: review+
levin: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Wilson 2009-09-03 17:12:43 PDT
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.
Comment 1 Andrew Wilson 2009-09-03 17:24:57 PDT
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.
Comment 2 Eric Seidel (no email) 2009-09-04 00:22:38 PDT
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.
Comment 3 Andrew Wilson 2009-09-04 10:19:20 PDT
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.
Comment 4 Andrew Wilson 2009-09-04 17:03:22 PDT
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.
Comment 5 Andrew Wilson 2009-09-04 18:09:35 PDT
(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.
Comment 6 David Levin 2009-09-06 16:13:12 PDT
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.
Comment 7 Andrew Wilson 2009-09-07 12:54:15 PDT
Committed as r48131