Bug 183526 - Allow NavigationState to intercept requests and send them to SystemPreviewController
Summary: Allow NavigationState to intercept requests and send them to SystemPreviewCon...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-09 14:32 PST by Dean Jackson
Modified: 2018-03-09 15:39 PST (History)
4 users (show)

See Also:


Attachments
Patch (9.94 KB, patch)
2018-03-09 14:40 PST, Dean Jackson
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2018-03-09 14:32:59 PST
Allow NavigationState to intercept requests and send them to SystemPreviewController
Comment 1 Dean Jackson 2018-03-09 14:33:35 PST
<rdar://problem/37801140>
Comment 2 Dean Jackson 2018-03-09 14:40:58 PST
Created attachment 335466 [details]
Patch
Comment 3 EWS Watchlist 2018-03-09 14:43:18 PST
Attachment 335466 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/SystemPreviewController.h:33:  _WKPreviewControllerDataSource is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/SystemPreviewController.h:34:  _WKPreviewControllerDelegate is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Tim Horton 2018-03-09 15:24:03 PST
Comment on attachment 335466 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:658
> +            if (page.systemPreviewController()->canPreview(navigationResponse->response().mimeType()))
> +                page.systemPreviewController()->showPreview(navigationResponse->response().url());

It seems a bit weird to take over if the client explicitly wants to download, and also weird to then go ahead with the download?
Comment 5 Dean Jackson 2018-03-09 15:28:20 PST
Comment on attachment 335466 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:658
>> +                page.systemPreviewController()->showPreview(navigationResponse->response().url());
> 
> It seems a bit weird to take over if the client explicitly wants to download, and also weird to then go ahead with the download?

I agree. But we can't nicely cancel the download at this point. Ultimately, we'll have a separate download client or logic that doesn't navigate.
Comment 6 Tim Horton 2018-03-09 15:30:07 PST
Comment on attachment 335466 [details]
Patch

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

>>> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:658
>>> +                page.systemPreviewController()->showPreview(navigationResponse->response().url());
>> 
>> It seems a bit weird to take over if the client explicitly wants to download, and also weird to then go ahead with the download?
> 
> I agree. But we can't nicely cancel the download at this point. Ultimately, we'll have a separate download client or logic that doesn't navigate.

Or a separate policy type?
Comment 7 Dean Jackson 2018-03-09 15:32:36 PST
Comment on attachment 335466 [details]
Patch

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

>>>> Source/WebKit/UIProcess/Cocoa/NavigationState.mm:658
>>>> +                page.systemPreviewController()->showPreview(navigationResponse->response().url());
>>> 
>>> It seems a bit weird to take over if the client explicitly wants to download, and also weird to then go ahead with the download?
>> 
>> I agree. But we can't nicely cancel the download at this point. Ultimately, we'll have a separate download client or logic that doesn't navigate.
> 
> Or a separate policy type?

Actually, it sort of works if I ignore the download!

I originally considered a new policy type, but this one is actually set from Safari. I'll have to do something else for WKWebView clients.
Comment 8 Dean Jackson 2018-03-09 15:39:40 PST
Committed r229488: <https://trac.webkit.org/changeset/229488>