Bug 181884 - Present WKFileUploadPanel on correct ViewController
Summary: Present WKFileUploadPanel on correct ViewController
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: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-19 14:51 PST by Megan Gardner
Modified: 2018-01-19 16:00 PST (History)
5 users (show)

See Also:


Attachments
Patch (4.32 KB, patch)
2018-01-19 14:54 PST, Megan Gardner
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2018-01-19 14:51:36 PST
Present WKFileUploadPanel on correct ViewController
Comment 1 Megan Gardner 2018-01-19 14:54:50 PST
Created attachment 331793 [details]
Patch
Comment 2 Joseph Pecoraro 2018-01-19 15:03:39 PST
Comment on attachment 331793 [details]
Patch

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

Looks good to me.

There are other places that use -[UIViewController _viewControllerForFullScreenPresentationFromView:]. For example:
UIProcess/ios/forms/WKAirPlayRoutePicker.mm

Should they change as well?

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:462
> +    NSLog(@"Failed to find a view controller to show form validation popover");

Accidental log.
Comment 3 Tim Horton 2018-01-19 15:08:08 PST
(In reply to Joseph Pecoraro from comment #2)
> Comment on attachment 331793 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331793&action=review
> 
> Looks good to me.
> 
> There are other places that use -[UIViewController
> _viewControllerForFullScreenPresentationFromView:]. For example:
> UIProcess/ios/forms/WKAirPlayRoutePicker.mm
> 
> Should they change as well?

Seems likely.
 
> > Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:462
> > +    NSLog(@"Failed to find a view controller to show form validation popover");
> 
> Accidental log.

Humorously that’s in the other version of this function.
Comment 4 Tim Horton 2018-01-19 15:11:45 PST
Comment on attachment 331793 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:456
> +static UIViewController *fallbackViewController(UIView *view)

I wonder if we should put this in the UIDelegate implementation if the client returns null, so we don’t have to put it everywhere.

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:471
> +        _presentationViewController = [self.delegate viewControllerForPresentingFileUploadPanel:self];

What if this returns nil? Can it?
Comment 5 Tim Horton 2018-01-19 15:27:15 PST
Comment on attachment 331793 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:456
>> +static UIViewController *fallbackViewController(UIView *view)
> 
> I wonder if we should put this in the UIDelegate implementation if the client returns null, so we don’t have to put it everywhere.

In a followup!

>>> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:462
>>> +    NSLog(@"Failed to find a view controller to show form validation popover");
>> 
>> Accidental log.
> 
> Humorously that’s in the other version of this function.

Maybe RELEASE_LOG instead

>> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:471
>> +        _presentationViewController = [self.delegate viewControllerForPresentingFileUploadPanel:self];
> 
> What if this returns nil? Can it?

(I think you want if !presentationViewController instead of an else)
Comment 6 Megan Gardner 2018-01-19 15:38:50 PST
https://trac.webkit.org/changeset/227247/webkit
Comment 7 Radar WebKit Bug Importer 2018-01-19 15:39:14 PST
<rdar://problem/36673774>
Comment 8 Megan Gardner 2018-01-19 15:58:28 PST
InRadar <rdar://problem/35114892>
Comment 9 Megan Gardner 2018-01-19 16:00:13 PST
<rdar://problem/35114892>