Bug 237245 – Trigger PDF download in captive portal mode instead of using PDF viewer

• Component: PDF
• Status: RESOLVED FIXED

[Tim Nguyen (:ntim)]

I think that was the decision taken since PDF.js is not in a good enough state atm.

[Radar WebKit Bug Importer]

<rdar://problem/89525531>

[Tim Nguyen (:ntim)]

Created attachment 454590 [details]
Patch

[[e-mail]]

Created attachment 454982 [details]
Patch

[Alex Christensen]

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?

[Tim Nguyen (:ntim)]

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?

[Chris Dumez]

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.

[Chris Dumez]

(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.

[[e-mail]]

Created attachment 455019 [details]
Patch

[Tim Nguyen (:ntim)]

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 :)

[Chris Dumez]

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).

[[e-mail]]

Created attachment 455095 [details]
Patch

[Tim Nguyen (:ntim)]

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

[[e-mail]]

Created attachment 455096 [details]
Patch for landing

[[e-mail]]

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

[EWS]

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].