[webkit-reviews] review granted: [Bug 175237] Implement most of ServiceWorkerContainer::addRegistration : [Attachment 317401] Patch

[bugzilla-daemon at webkit.org]

Andy Estes <aestes at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 175237: Implement most of ServiceWorkerContainer::addRegistration
https://bugs.webkit.org/show_bug.cgi?id=175237

Attachment 317401: Patch

https://bugs.webkit.org/attachment.cgi?id=317401&action=review




--- Comment #31 from Andy Estes <aestes at apple.com> ---
Comment on attachment 317401
  --> https://bugs.webkit.org/attachment.cgi?id=317401
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317401&action=review

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:27513
> +				51F174FE1F35899200C74950 /* WorkerType.h in
Headers */,

This list looked sorted before you added WorkerType.h.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:27602
> +				51F174FF1F35899700C74950 /*
ServiceWorkerUpdateViaCache.h in Headers */,

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:28551
> +				51F175061F358BF700C74950 /* JSWorkerType.h in
Headers */,

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:28834
> +				51F175031F358B3B00C74950 /*
JSServiceWorkerUpdateViaCache.h in Headers */,

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:32210
> +				51F175071F358BF900C74950 /* JSWorkerType.cpp in
Sources */,

Ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:33848
> +				51F175021F358B3B00C74950 /*
JSServiceWorkerUpdateViaCache.cpp in Sources */,

Ditto.

> Source/WebCore/page/NavigatorBase.h:66
> -    std::unique_ptr<ServiceWorkerContainer> m_serviceWorkerContainer;
> +    UniqueRef<ServiceWorkerContainer> m_serviceWorkerContainer;

Should this be guarded with an #if ENABLE(SERVICE_WORKER)?

> Source/WebCore/workers/ServiceWorkerContainer.cpp:63
> +    promise->reject(Exception(UnknownError,
ASCIILiteral("serviceWorker.ready() is not yet implemented")));

I like how you use brace initialization for the Exception in addRegistration().

> Source/WebCore/workers/ServiceWorkerContainer.cpp:75
> +	   promise->reject(Exception { TypeError,
ASCIILiteral("serviceWorker.register() cannot be called with a null script
URL") });

You say "null script URL" in the error message, but you check for an empty or
null URL.