[webkit-reviews] review denied: [Bug 221909] Add support for non app-bound requests : [Attachment 420505] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied katherine_cheney at apple.com's
request for review:
Bug 221909: Add support for non app-bound requests
https://bugs.webkit.org/show_bug.cgi?id=221909

Attachment 420505: Patch

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




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

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

I think this looks great, but I don't think we want to limit this concept to
iOS-only. r- to fix that, but otherwise this is ready to go.

> Source/WebCore/loader/DocumentLoader.h:423
> +#if PLATFORM(IOS_FAMILY)

I don't think this concept is limited to IOS_FAMILY only, is it?

> Source/WebCore/loader/DocumentLoader.h:666
> +#if PLATFORM(IOS_FAMILY)

Ditto.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1363
> +    parameters.request.setIsAppBound(lastNavigationWasAppBound ==
LastNavigationWasAppBound::Yes ? true : false);

nit: I think you just need "lastNavigationWasAppBound ==
LastNavigationWasAppBound::Yes" (the additional ternary operation is redundant)

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:880
> +	   redirectRequest.setIsAppBound(request.isAppBound());

I wonder if the test is really necessary before setting. Maybe just always set
the app-bound state of the redirect to match the original request, since we
always want the redirect to have the state of the originating request.

> Source/WebKit/Shared/WebPageCreationParameters.cpp:174
> +#if PLATFORM(IOS_FAMILY)

I don't think this is limited to IOS.

> Source/WebKit/Shared/WebPageCreationParameters.h:245
> +#if PLATFORM(IOS_FAMILY)

Ditto.

> Source/WebKit/UIProcess/WebPageProxy.cpp:1322
> +#if PLATFORM(IOS_FAMILY)

I don't think this should be iOS-only

> Source/WebKit/UIProcess/WebPageProxy.cpp:-7880
> -    

Nit: Unneeded whitespace change.

> Source/WebKit/UIProcess/WebPageProxy.h:1842
> +#if PLATFORM(IOS_FAMILY)

I don't thin this should be iOS-only

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:517
> +#if PLATFORM(IOS_FAMILY)

Not iOS-only

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1624
> +#if PLATFORM(IOS_FAMILY)

Ditto

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6427
> +#if PLATFORM(IOS_FAMILY)

Ditto

> Source/WebKit/WebProcess/WebPage/WebPage.h:1411
> +#if PLATFORM(IOS_FAMILY)

Ditto

> Source/WebKit/WebProcess/WebPage/WebPage.h:2236
> +#if PLATFORM(IOS_FAMILY)

Ditto

> Source/WebKit/WebProcess/WebPage/WebPage.messages.in:633
> +#if PLATFORM(IOS_FAMILY)

Ditto

>>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:51
>>> +#define APP_BOUND_REQUEST_ADDITIONS
>> 
>> This isn't needed.
> 
> It will always use the Internal SDK?

Your test says APPLE_INTERNAL_SDK only, so it seems like yes?

> Tools/TestWebKitAPI/cocoa/TestWKWebView.h:70
> +#if PLATFORM(IOS_FAMILY)

Not iOS-only

> Tools/TestWebKitAPI/cocoa/TestWKWebView.mm:242
> +#if PLATFORM(IOS_FAMILY)

Ditto.