[webkit-reviews] review denied: [Bug 208026] App-bound domains should have separate Network Sessions : [Attachment 391339] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied katherine_cheney at apple.com's
request for review:
Bug 208026: App-bound domains should have separate Network Sessions
https://bugs.webkit.org/show_bug.cgi?id=208026

Attachment 391339: Patch

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




--- Comment #4 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 391339
  --> https://bugs.webkit.org/attachment.cgi?id=391339
Patch

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

This looks very good! I have a few minor things I'd like addressed.

> Source/WebKit/NetworkProcess/NetworkDataTask.cpp:53
> +    return NetworkDataTaskCocoa::create(session, client, parameters.request,
parameters.webFrameID, parameters.webPageID,
parameters.storedCredentialsPolicy, parameters.contentSniffingPolicy,
parameters.contentEncodingSniffingPolicy,
parameters.shouldClearReferrerOnHTTPSToHTTPRedirect,
parameters.shouldPreconnectOnly, parameters.isMainFrameNavigation,
parameters.isMainResourceNavigationForAnyFrame,
parameters.networkActivityTracker, parameters.isNavigatingToAppBoundDomain);

At some point, it seems like we should just past the parameters object to the
'create' method! :-)

> Source/WebKit/NetworkProcess/NetworkLoadParameters.h:64
> +    bool isNavigatingToAppBoundDomain { true };

It seems like the default state should be false here (most traffic is likely
not app-bound).

> Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.cpp:124
> +

Extra whitespace.

> Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.h:77
> +    bool isNavigatingToAppBoundDomain;

Whitespace, and should have a default argument.

> Source/WebKit/Shared/LoadParameters.h:72
> +    bool isNavigatingToAppBoundDomain;

Please provide a default initializer (false).

> Source/WebKit/UIProcess/WebPageProxy.cpp:3064
> +    void send(PolicyDecision&& policyDecision)

Oops! :-)

> Source/WebKit/UIProcess/WebPageProxy.cpp:3083
> +bool WebPageProxy::isAppBoundDomain(WebCore::RegistrableDomain domain)

Could this be "const WebCore::RegistrableDomain&" ?

> Source/WebKit/UIProcess/WebPageProxy.h:2259
> +    bool isAppBoundDomain(WebCore::RegistrableDomain);

Could this be const? Also seems like the argument (which is effectively a
String) would benefit from being passed by reference.

> Source/WebKit/UIProcess/WebPageProxy.h:2261
> +    bool isNavigatingToAppBoundDomain() { return
m_isNavigatingToAppBoundDomain; }

This should be "bool isNavigatingToAppBoundDomain() const"

> Source/WebKit/WebProcess/WebPage/WebPage.h:1290
> +    bool isNavigatingToAppBoundDomain() { return
m_isNavigatingToAppBoundDomain; }

Should be const.

> Source/WebKit/WebProcess/WebPage/WebPage.h:2073
> +    bool m_isNavigatingToAppBoundDomain { true };

I think this should default to 'false'.