[webkit-reviews] review denied: [Bug 222635] Service worker loads are not marked as app-bound : [Attachment 422726] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied  review:
Bug 222635: Service worker loads are not marked as app-bound
https://bugs.webkit.org/show_bug.cgi?id=222635

Attachment 422726: Patch

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




--- Comment #7 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 422726
  --> https://bugs.webkit.org/attachment.cgi?id=422726
Patch

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

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:129
> +    m_document->loader()->setLastNavigationWasAppBound(wasAppBound);

Shouldn't we null check the DocumentLoader?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1562
> +static void runTest(ResponseType responseType, bool appBound)

bool appBound would look better at call site if we used an enum class.

enum class IsAppBound : bool { No, Yes };

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1567
> +    __block bool removedAnyExistingData = false;

Seems like you could use the existing isDone above.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1625
> +	   NSMutableURLRequest *nonAppBoundRequest = [request mutableCopy];

This seems to be leaking. If you call mutableCopy, you should adoptNS() right
away.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1639
> +    bool expectingAppBoundRequests = appBound ? true : false;

Why aren't you using appBound directly? What's the point of this new boolean
that means exactly the same thing?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1645
> +    }];

Aren't you missing this right after this call?

TestWebKitAPI::Util::run(&isDone);