Bug 185459 – Download and present System Preview

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Dean Jackson]

Download and present System Preview

[Radar WebKit Bug Importer]

<rdar://problem/40079228>

[Dean Jackson]

Created attachment 339909 [details]
Patch

[Dean Jackson]

Created attachment 340378 [details]
Patch

[Tim Horton]

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?

[Jon Lee]

please fix build errors too!

[Dean Jackson]

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.

[Dean Jackson]

Created attachment 340400 [details]
Patch

[Jon Lee]

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.

[Dean Jackson]

Committed r231795: <https://trac.webkit.org/changeset/231795>

[Dean Jackson]

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?

[Jon Lee]

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