[webkit-reviews] review granted: [Bug 207506] Parent service worker controller should be used for child iframe as per https://w3c.github.io/ServiceWorker/#control-and-use-window-client : [Attachment 390375] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Feb 11 12:47:22 PST 2020


Darin Adler <darin at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 207506: Parent service worker controller should be used for child iframe as
per https://w3c.github.io/ServiceWorker/#control-and-use-window-client
https://bugs.webkit.org/show_bug.cgi?id=207506

Attachment 390375: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 390375
  --> https://bugs.webkit.org/attachment.cgi?id=390375
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:1060
> +// https://w3c.github.io/ServiceWorker/#control-and-use-window-client

The terminology used in the specification is so different from the terminology
we use here. It made hard for me to understand. For example, the specification
doesn’t talk about a "controller" at all.

> Source/WebCore/loader/DocumentLoader.cpp:1061
> +static inline bool isInheritingControllerFromParent(const Document&
document, const Document& parent)

Name should probably be:

    shouldApplyActiveServiceWorkerFromParent

> Source/WebCore/loader/DocumentLoader.cpp:1067
> +    if (document.url().protocolIsInHTTPFamily())
> +	   return false;
> +    if (document.securityOrigin().isUnique())
> +	   return false;
> +    return parent.securityOrigin().canAccess(document.securityOrigin());

I think this might be more readable as !a && !b && c rather than with cascading
if statements.

> Source/WebCore/loader/DocumentLoader.cpp:1098
> +	       } else if (auto* parent = m_frame->document()->parentDocument())
{

m_frame->document()-> is repeated over 10 times in this function! There’s got
to be some refactoring we can do to cut down on that distracting repetition
safely (some day).

>
LayoutTests/http/tests/workers/service/serviceworkerclients-claim.https-expecte
d.txt:3
> +CONSOLE MESSAGE: Origin null is not allowed by Access-Control-Allow-Origin.
> +CONSOLE MESSAGE: Fetch API cannot load https://127.0.0.1:8443/pinkelephant
due to access control checks.
> +CONSOLE MESSAGE: line 1: Unhandled Promise Rejection: TypeError: Origin null
is not allowed by Access-Control-Allow-Origin.

I don’t understand these changes to the results. Nothing in the change log
makes it clear either.


More information about the webkit-reviews mailing list