Bug 185459

Summary: Download and present System Preview
Product: WebKit Reporter: Dean Jackson <dino>
Component: WebGLAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, dino, esprehn+autocc, ews-watchlist, gyuyoung.kim, jonlee, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
thorton: review+
Patch none

Description Dean Jackson 2018-05-08 18:21:42 PDT
Download and present System Preview
Comment 1 Radar WebKit Bug Importer 2018-05-08 18:23:06 PDT
<rdar://problem/40079228>
Comment 2 Dean Jackson 2018-05-08 18:28:43 PDT
Created attachment 339909 [details]
Patch
Comment 3 Dean Jackson 2018-05-14 16:54:22 PDT
Created attachment 340378 [details]
Patch
Comment 4 Tim Horton 2018-05-14 17:08:11 PDT
Comment on attachment 340378 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/DownloadClient.mm:88
> +#if USE(SYSTEM_PREVIEW)

I’m sure you could use an early return and avoid conditionalizing quite so much chaos here.

> Source/WebKit/UIProcess/Cocoa/DownloadClient.mm:96
> +    if (downloadProxy.isSystemPreviewDownload()) {

I wonder if in all of these we could hide the logic away in some System Preview class instead, make it conform to the normal delegate protocol, and just change where we send the messages if it’s a system preview download? Can you get enough information back out of the objects handed to the delegate?

> Source/WebKit/UIProcess/Cocoa/DownloadClient.mm:121
> +        if (m_delegateMethods.downloadDidReceiveData)

I still think these should all be early returns

> Source/WebKit/UIProcess/Cocoa/DownloadClient.mm:208
> +#if USE(SYSTEM_PREVIEW)

Why is this in the else? If the delegate implements that method we should skip the system preview path??

> Source/WebKit/UIProcess/Cocoa/SystemPreviewControllerCocoa.mm:86
> +        [_itemProvider registerItemForTypeIdentifier:contentType loadHandler:^(NSItemProviderCompletionHandler _Null_unspecified completionHandler, Class _Null_unspecified __unsafe_unretained expectedValueClass, NSDictionary * _Null_unspecified options) {

Get those _Null_unspecifieds out of there?

> Source/WebKit/UIProcess/Cocoa/SystemPreviewControllerCocoa.mm:171
> +        [m_qlPreviewController.get() dismissViewControllerAnimated:(BOOL)YES completion:nullptr];

(BOOL)YES WHAT?
Comment 5 Jon Lee 2018-05-14 17:49:07 PDT
please fix build errors too!
Comment 6 Dean Jackson 2018-05-15 01:47:14 PDT
Comment on attachment 340378 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/DownloadClient.mm:88
>> +#if USE(SYSTEM_PREVIEW)
> 
> I’m sure you could use an early return and avoid conditionalizing quite so much chaos here.

I'm not sure. It definitely is a mess, but I can't think of a better way to do it.

Well, the best way would be if I could install another DownloadClient just for this task. I should probably investigate that. A special client that handles these types of downloads.
https://bugs.webkit.org/show_bug.cgi?id=185646

>> Source/WebKit/UIProcess/Cocoa/DownloadClient.mm:96
>> +    if (downloadProxy.isSystemPreviewDownload()) {
> 
> I wonder if in all of these we could hide the logic away in some System Preview class instead, make it conform to the normal delegate protocol, and just change where we send the messages if it’s a system preview download? Can you get enough information back out of the objects handed to the delegate?

Yes, I think this is a good idea. I'll put that as an option in the bug above.

I'm not sure it will make things cleaner, since there still will be only one official client, and one official delegate. So something needs to do a test on the type of download to decide to use a different delegate or different client.

>> Source/WebKit/UIProcess/Cocoa/DownloadClient.mm:121
>> +        if (m_delegateMethods.downloadDidReceiveData)
> 
> I still think these should all be early returns

Yeah, I'll return after system preview to remove one level of braces (and one ugly #if)

>> Source/WebKit/UIProcess/Cocoa/DownloadClient.mm:208
>> +#if USE(SYSTEM_PREVIEW)
> 
> Why is this in the else? If the delegate implements that method we should skip the system preview path??

This was a mistake.

>> Source/WebKit/UIProcess/Cocoa/SystemPreviewControllerCocoa.mm:86
>> +        [_itemProvider registerItemForTypeIdentifier:contentType loadHandler:^(NSItemProviderCompletionHandler _Null_unspecified completionHandler, Class _Null_unspecified __unsafe_unretained expectedValueClass, NSDictionary * _Null_unspecified options) {
> 
> Get those _Null_unspecifieds out of there?

Killed!

>> Source/WebKit/UIProcess/Cocoa/SystemPreviewControllerCocoa.mm:171
>> +        [m_qlPreviewController.get() dismissViewControllerAnimated:(BOOL)YES completion:nullptr];
> 
> (BOOL)YES WHAT?

Oops. Copy and paste.
Comment 7 Dean Jackson 2018-05-15 02:18:15 PDT
Created attachment 340400 [details]
Patch
Comment 8 Jon Lee 2018-05-15 02:40:52 PDT
Comment on attachment 340400 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/DownloadClient.h:85
> +#if PLATFORM(IOS) && USE(SYSTEM_PREVIEW)

This variable is used in a few places that could potentially cause build errors because the code is not wrapped up in this specific conditional.

> Source/WebKit/UIProcess/Cocoa/SystemPreviewControllerCocoa.mm:77
> +    if (!_item) {

Consider reversing this with an early return.
Comment 9 Dean Jackson 2018-05-15 02:50:43 PDT
Committed r231795: <https://trac.webkit.org/changeset/231795>
Comment 10 Dean Jackson 2018-05-15 03:01:21 PDT
Comment on attachment 340400 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/DownloadClient.h:85
>> +#if PLATFORM(IOS) && USE(SYSTEM_PREVIEW)
> 
> This variable is used in a few places that could potentially cause build errors because the code is not wrapped up in this specific conditional.

You're right. It works because SYSTEM_PREVIEW is only IOS at the moment.

I'll add IOS around the places that use it.

>> Source/WebKit/UIProcess/Cocoa/SystemPreviewControllerCocoa.mm:77
>> +    if (!_item) {
> 
> Consider reversing this with an early return.

But the function always returns _item.get()? This is basically initialize-if-you-don't-yet-have-it.

Or am I missing something?
Comment 11 Jon Lee 2018-05-15 09:44:04 PDT
(In reply to Dean Jackson from comment #10)
> Comment on attachment 340400 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340400&action=review
> 
> >> Source/WebKit/UIProcess/Cocoa/DownloadClient.h:85
> >> +#if PLATFORM(IOS) && USE(SYSTEM_PREVIEW)
> > 
> > This variable is used in a few places that could potentially cause build errors because the code is not wrapped up in this specific conditional.
> 
> You're right. It works because SYSTEM_PREVIEW is only IOS at the moment.
> 
> I'll add IOS around the places that use it.
> 
> >> Source/WebKit/UIProcess/Cocoa/SystemPreviewControllerCocoa.mm:77
> >> +    if (!_item) {
> > 
> > Consider reversing this with an early return.
> 
> But the function always returns _item.get()? This is basically
> initialize-if-you-don't-yet-have-it.
> 
> Or am I missing something?
There would be one repeated line of code. But the entire function is essentially inside the then clause, so doing the reverse would reduce indentation, get rid of a clause, and improve readability.