[webkit-reviews] review granted: [Bug 28897] SharedWorkers "name" attribute is now optional : [Attachment 39625] proposed patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Andrew Wilson
<atwilson at chromium.org>'s request for review:
Bug 28897: SharedWorkers "name" attribute is now optional
https://bugs.webkit.org/show_bug.cgi?id=28897

Attachment 39625: proposed patch
https://bugs.webkit.org/attachment.cgi?id=39625&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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.