Bug 237245

<rdar://problem/89525531>

Created attachment 454590 [details]
Patch

Created attachment 454982 [details]
Patch

Comment on attachment 454982 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:3466
> +        auto mimeType = MIMETypeRegistry::mimeTypeForPath(navigation->currentRequest().url().path().toString());

This seems a little light weight.  Don't we even want to look at the mime type of the response?

Comment on attachment 454982 [details]
Patch

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

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3466
>> +        auto mimeType = MIMETypeRegistry::mimeTypeForPath(navigation->currentRequest().url().path().toString());
> 
> This seems a little light weight.  Don't we even want to look at the mime type of the response?

There's no access to the response at this point. Do you have a suggestion of a good place to put this where there is access to the response?

Comment on attachment 454982 [details]
Patch

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

>>> Source/WebKit/UIProcess/WebPageProxy.cpp:3466
>>> +        auto mimeType = MIMETypeRegistry::mimeTypeForPath(navigation->currentRequest().url().path().toString());
>> 
>> This seems a little light weight.  Don't we even want to look at the mime type of the response?
> 
> There's no access to the response at this point. Do you have a suggestion of a good place to put this where there is access to the response?

WebPageProxy::receivedPolicyDecision or WebPageProxy::decidePolicyForResponseShared. This is always frequently where we trigger downloads.

(In reply to Tim Nguyen (:ntim) from comment #5)
> Comment on attachment 454982 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454982&action=review
> 
> >> Source/WebKit/UIProcess/WebPageProxy.cpp:3466
> >> +        auto mimeType = MIMETypeRegistry::mimeTypeForPath(navigation->currentRequest().url().path().toString());
> > 
> > This seems a little light weight.  Don't we even want to look at the mime type of the response?
> 
> There's no access to the response at this point. Do you have a suggestion of
> a good place to put this where there is access to the response?

Purely relying on the URL ending with .pdf is weak and will miss plenty of cases where we're actually loading a PDF. Like Alex says, we need to check the content type in the resource response.

Created attachment 455019 [details]
Patch

Comment on attachment 455019 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:5735
> +        auto captivePortalMode = (policies ? policies->captivePortalModeEnabled() : shouldEnableCaptivePortalMode()) ? WebProcessProxy::CaptivePortalMode::Enabled : WebProcessProxy::CaptivePortalMode::Disabled;
> +        if (captivePortalMode == WebProcessProxy::CaptivePortalMode::Enabled && MIMETypeRegistry::isPDFOrPostScriptMIMEType(navigationResponse->response().mimeType()))

I'm not sure if this works, but I think you can do:

```
if (process->captivePortalMode() == WebProcessProxy::CaptivePortalMode::Enabled)
```

directly, instead of checking through policies. Though I might be wrong :)

Comment on attachment 455019 [details]
Patch

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

r=me with Tim's proposed change.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:5735
>> +        if (captivePortalMode == WebProcessProxy::CaptivePortalMode::Enabled && MIMETypeRegistry::isPDFOrPostScriptMIMEType(navigationResponse->response().mimeType()))
> 
> I'm not sure if this works, but I think you can do:
> 
> ```
> if (process->captivePortalMode() == WebProcessProxy::CaptivePortalMode::Enabled)
> ```
> 
> directly, instead of checking through policies. Though I might be wrong :)

I agree with Tim here. His proposal is simpler and I believe it would work since we would have already switched to a captive-portal-mode process earlier (during navigation policy decision).

Created attachment 455095 [details]
Patch

Comment on attachment 455095 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:5726
> +        process, navigationResponse] (PolicyAction policyAction, API::WebsitePolicies* policies, ProcessSwapRequestedByClient processSwapRequestedByClient, RefPtr<SafeBrowsingWarning>&& safeBrowsingWarning, std::optional<NavigatingToAppBoundDomain> isNavigatingToAppBoundDomain) mutable {

nit: please undo this change, which should no longer be necessary

Created attachment 455096 [details]
Patch for landing

Thank you Tim, Chris, and Alex for the comments and review!

Committed r291491 (248603@main): <https://commits.webkit.org/248603@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455096 [details].