Bug 28897 - SharedWorkers "name" attribute is now optional
Summary: SharedWorkers "name" attribute is now optional
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:
 
Reported: 2009-09-01 16:53 PDT by Andrew Wilson
Modified: 2009-09-22 21:17 PDT (History)
1 user (show)

See Also:


Attachments
proposed patch (13.71 KB, patch)
2009-09-15 17:24 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-01 16:53:15 PDT
There's new language in the spec regarding the name parameter to the SharedWorker constructor:

1) The name param is optional. If omitted, it becomes the empty string ("").
2) If the name param is the empty string, then it is ignored (the URL becomes the sole identifier of the shared worker).
Comment 1 Andrew Wilson 2009-09-15 17:24:47 PDT
Created attachment 39625 [details]
proposed patch
Comment 2 Michael Nordman 2009-09-21 21:25:35 PDT
Hurray for workers identified by the url!

Lonely patch... fwiw, some review comments.

First, Looks Great!

File: LayoutTests/fast/workers/resources/shared-worker-name.js
Maybe add a case for creating a worker with the same name as an existing worker, but a different url (or is that covered in other layout tests?).

60     v8::Handle<v8::String> scriptUrl = args[0]->ToString();
Maybe go straight toWebCoreString as the v8::Stuff<v8::Is> inscrutible :)
Comment 3 Andrew Wilson 2009-09-22 15:24:47 PDT
(In reply to comment #2)
> Hurray for workers identified by the url!
> 
> Lonely patch... fwiw, some review comments.
> 
> First, Looks Great!
> 
> File: LayoutTests/fast/workers/resources/shared-worker-name.js
> Maybe add a case for creating a worker with the same name as an existing
> worker, but a different url (or is that covered in other layout tests?).

That case is already covered by shared-worker-shared.html.

> 
> 60     v8::Handle<v8::String> scriptUrl = args[0]->ToString();
> Maybe go straight toWebCoreString as the v8::Stuff<v8::Is> inscrutible :)

There's some logic further on in that routine that checks for emptiness and aborts the routine which might get broken by calling toWebCoreString earlier. I'd rather not make extraneous changes to the code that might have unexpected side-effects.
Comment 4 David Levin 2009-09-22 16:52:18 PDT
Comment on attachment 39625 [details]
proposed patch

Just a few things that you can fix on landing.

> diff --git a/LayoutTests/fast/workers/resources/shared-worker-name.js b/LayoutTests/fast/workers/resources/shared-worker-name.js
> +var curTest = 0;

Use whole words: currentTest?


> +// Iterates through the tests until none are left

Add .


> +function test1()
> +{
> +    // Make sure we can create a shared worker with no name

Add .

> +function test2()
> +{
> +    // Creating a worker with no name should match an existing worker with no name

Add .



> diff --git a/WebCore/workers/DefaultSharedWorkerRepository.cpp b/WebCore/workers/DefaultSharedWorkerRepository.cpp
> +bool SharedWorkerProxy::matches(const String& name, PassRefPtr<SecurityOrigin> origin, const KURL& urlToMatch) const
> +    // Implements step 5 of section 4.8.3 of the HTML5 spec - if the names are both empty, compares the URLs instead.

(I think it is currently 4.8.3 step 7 substep 5. Regardless...) I really dislike comments that refer to items in the spec using precise numbering since the sections and steps are still changing in the spec, it will be likely incorrect shortly after check in.

Would you find some other way to refer to the spec? Also this is in the Web Workers spec, not the HTML 5 spec.
Comment 5 Andrew Wilson 2009-09-22 21:17:00 PDT
Committed as r48666