Bug 28897 – SharedWorkers "name" attribute is now optional

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Andrew Wilson]

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).

[Andrew Wilson]

Created attachment 39625 [details]
proposed patch

[Michael Nordman]

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 :)

[Andrew Wilson]

(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.

[David Levin]

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.

[Andrew Wilson]

Committed as r48666