Bug 137759 – [Mac] Add API for open panel handling to WKWebView

• Component: WebKit2
• Status: RESOLVED FIXED

[Brian Michel]

When attempting to upload files by way of an <input> tag there is no action taken by WebKit on behalf of the user allowing them to input files to upload. The root cause of this seems to be that the "handleRunOpenPanel" is just unimplemented for OS X clients.

[Brian Michel]

Created attachment 239909 [details]
Patch

[WebKit Commit Bot]

Attachment 239909 [details] did not pass style-queue:


ERROR: Source/WebKit2/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebKit2/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebKit2/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebKit2/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebKit2/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 5 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brian Michel]

Created attachment 239911 [details]
Patch

[Brian Michel]

I changed this to a P1/Blocker since this will break any WKWebView developer attempting to use file input. This is a regression from previously released versions of WebKit, so I think the prioritization is correct. 

Please correct as needed.

[Alexey Proskuryakov]

Anders, is this how it is supposed to work?

[Anders Carlsson]

(In reply to comment #5)
> Anders, is this how it is supposed to work?

I think what we want is to call out to a UI delegate method, and perhaps have the default implementation show an NSOpenPanel.

[Brian Michel]

(In reply to comment #6)
> (In reply to comment #5)
> > Anders, is this how it is supposed to work?
> 
> I think what we want is to call out to a UI delegate method, and perhaps
> have the default implementation show an NSOpenPanel.

I was going to go this route first, but wanted to keep it simple first and see which way to go. I will update the WKUIDelegate to have a new optional methods similar to the ones that exist currently on WebUIDelegate for handling file buttons.

[Brian Michel]

Created attachment 241272 [details]
Patch

[WebKit Commit Bot]

Attachment 241272 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:98:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brian Michel]

Created attachment 241273 [details]
Patch

[Brian Michel]

(In reply to comment #6)
> (In reply to comment #5)
> > Anders, is this how it is supposed to work?
> 
> I think what we want is to call out to a UI delegate method, and perhaps
> have the default implementation show an NSOpenPanel.

I implemented calling out to the UI delegate, but did not include a default implementation as none of the other WKUIDelegate methods provide default implementations. If this is considered very desirable, it will be very easy to add.

[Anders Carlsson]

(In reply to comment #11)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Anders, is this how it is supposed to work?
> > 
> > I think what we want is to call out to a UI delegate method, and perhaps
> > have the default implementation show an NSOpenPanel.
> 
> I implemented calling out to the UI delegate, but did not include a default
> implementation as none of the other WKUIDelegate methods provide default
> implementations. If this is considered very desirable, it will be very easy
> to add.

Great, I really appreciate you working on this! I'll try to take a look at it sometime this week - we still need to figure out how this should interact with iOS as well.

[Brian Michel]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > Anders, is this how it is supposed to work?
> > > 
> > > I think what we want is to call out to a UI delegate method, and perhaps
> > > have the default implementation show an NSOpenPanel.
> > 
> > I implemented calling out to the UI delegate, but did not include a default
> > implementation as none of the other WKUIDelegate methods provide default
> > implementations. If this is considered very desirable, it will be very easy
> > to add.
> 
> Great, I really appreciate you working on this! I'll try to take a look at
> it sometime this week - we still need to figure out how this should interact
> with iOS as well.

Thanks Anders! Should this interact at all with iOS? Looking at the code the implementation of input tag handling is all private. If there's a way to expose this delegate method to OS X only, I'd be more than happy to take a look at going down that route.

[Alexey Proskuryakov]

*** Bug 139745 has been marked as a duplicate of this bug. ***

[Brian Michel]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #6)
> > > > (In reply to comment #5)
> > > > > Anders, is this how it is supposed to work?
> > > > 
> > > > I think what we want is to call out to a UI delegate method, and perhaps
> > > > have the default implementation show an NSOpenPanel.
> > > 
> > > I implemented calling out to the UI delegate, but did not include a default
> > > implementation as none of the other WKUIDelegate methods provide default
> > > implementations. If this is considered very desirable, it will be very easy
> > > to add.
> > 
> > Great, I really appreciate you working on this! I'll try to take a look at
> > it sometime this week - we still need to figure out how this should interact
> > with iOS as well.
> 
> Thanks Anders! Should this interact at all with iOS? Looking at the code the
> implementation of input tag handling is all private. If there's a way to
> expose this delegate method to OS X only, I'd be more than happy to take a
> look at going down that route.

What is the path forward here? Is there more I should add, or can I petition someone to land this for me?

[Justin Schneck]

Can we get some update on this, its pretty critical for Mac OS 10.10 apps

[Brian Michel]

(In reply to comment #16)
> Can we get some update on this, its pretty critical for Mac OS 10.10 apps

Justin, I'm going to drop into the IRC channel later today and see what I can do to get some more traction.

[Jason Murray]

Still eager for this to go through.

[Justin Schneck]

This is starting to cause pain in our company's product. Is there any news on this being put through?

[Justin Caldicott]

Any update on this?

Our (OS X) app has an <input type="file"> to allow upload of photos, and it seems there's no way to handle adding files.  Drag and drop works fine and we can get the file blob directly in Javascript.

The only workaround I see is to make a request to our native api that opens a file picker and return the contents of the file in the response.  This will perform poorly though as the files can be quite large.

I guess an ideal implementation would open a file picker like normal.  I'd settle for a handler that we can implement and open the dialog manually and somehow return the file blobs back.

[Brian Michel]

Created attachment 267946 [details]
Patch

[Brian Michel]

Updated the diff to land clean to trunk.

[Brian Michel]

I manually tested and verified that this code works. I attempted to write automated tests, but there is no clear documentation on how to trigger such events programmatically within WebKit. 

No existing test cases mimic this behavior, and from what I can tell there is no way to trigger this directly through JS.

[Brian Michel]

Created attachment 267951 [details]
Patch

[Brian Michel]

I figured out a way to add a test for this specific case. Please let me know if this is an acceptable approach.

[Justin Schneck]

What is left for us to land this? Can we get additional help on getting this pulled in? WebKit1 javascript is painfully slow on 10.10 + and we cant move to WK2 until this gets landed.

[Brian Michel]

(In reply to comment #26)
> What is left for us to land this? Can we get additional help on getting this
> pulled in? WebKit1 javascript is painfully slow on 10.10 + and we cant move
> to WK2 until this gets landed.

Justin I am waiting for a reviewer to review this patch. Since it was recently updated it should have emailed the mailing lists requesting a review.

If you'd like to help speed this along perhaps drop a note into the #webkit channel on IRC and maybe someone will snatch up the review.

Believe me I want this to get landed too :)

[Alexey Proskuryakov]

Comment on attachment 267951 [details]
Patch

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

Added a few comments. I only did a very shallow review pass though.

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:40
> +/*! A class conforming to WKOpenPanelResultListener provides methods methods

"methods methods"

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:253
> +    if (!m_uiDelegate.m_webView) {
> +        listener->invalidate();
> +        return false;
> +    }
> +
> +    NSWindow *window = [m_uiDelegate.m_webView window];

An explicit null check for m_uiDelegate.m_webView is not needed, please remove it. In Objective-C, sending a message to a nil target is supported, and results in a nil pointer in this case.

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:262
> +    // If we have no delegate, invalide the listener and return false.

How is this possible, given that m_uiDelegate.m_delegateMethods.webViewRunOpenPanelWithResultListenerAllowsMultipleFiles is true?

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:268
> +    [((id <WKUIDelegate>) delegate) webView:m_uiDelegate.m_webView runOpenPanelWithResultListener:(id <WKOpenPanelResultListener>)adoptNS([[WKConcreteOpenPanelResultListener alloc] initWithListenerProxy:listener]) allowsMultipleFiles:parameters->allowMultipleFiles()];

It shouldn't be necessary to typecast delegate here, m_delegate is "id <WKUIDelegate>" already.

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:452
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090

Please remove this check, trunk WebKit doesn't support 10.9.

[Brian Michel]

Created attachment 268008 [details]
Patch

[Brian Michel]

(In reply to comment #28)
> Comment on attachment 267951 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267951&action=review
> 
> Added a few comments. I only did a very shallow review pass though.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:40
> > +/*! A class conforming to WKOpenPanelResultListener provides methods methods
> 
> "methods methods"
> 
> > Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:253
> > +    if (!m_uiDelegate.m_webView) {
> > +        listener->invalidate();
> > +        return false;
> > +    }
> > +
> > +    NSWindow *window = [m_uiDelegate.m_webView window];
> 
> An explicit null check for m_uiDelegate.m_webView is not needed, please
> remove it. In Objective-C, sending a message to a nil target is supported,
> and results in a nil pointer in this case.
> 
> > Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:262
> > +    // If we have no delegate, invalide the listener and return false.
> 
> How is this possible, given that
> m_uiDelegate.m_delegateMethods.
> webViewRunOpenPanelWithResultListenerAllowsMultipleFiles is true?
> 
> > Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:268
> > +    [((id <WKUIDelegate>) delegate) webView:m_uiDelegate.m_webView runOpenPanelWithResultListener:(id <WKOpenPanelResultListener>)adoptNS([[WKConcreteOpenPanelResultListener alloc] initWithListenerProxy:listener]) allowsMultipleFiles:parameters->allowMultipleFiles()];
> 
> It shouldn't be necessary to typecast delegate here, m_delegate is "id
> <WKUIDelegate>" already.
> 
> > Tools/MiniBrowser/mac/WK2BrowserWindowController.m:452
> > +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090
> 
> Please remove this check, trunk WebKit doesn't support 10.9.

Thank you so much for your feedback Alexey, I've made the changes asked for, and tests on my local seem to be passing, but waiting for the bots to say so.

[Darin Adler]

Anders, are you OK with this new API?

[Darin Adler]

Confused about the "WebKit1 javascript is painfully slow on 10.10 +" comment above. On OS X, JavaScript code should be nearly identical in speed in Legacy WebKit and Modern WebKit. If it’s slow then there’s some other bug that needs to be investigated.

[Anders Carlsson]

This is a good start, thanks for tackling it. Instead of having "allowsMultipleFiles", I'd like to see a WKOpenPanelParameters object that has an allowsMultipleFiles flag. This will let us extend the open panel if needed.

[Brian Michel]

(In reply to comment #33)
> This is a good start, thanks for tackling it. Instead of having
> "allowsMultipleFiles", I'd like to see a WKOpenPanelParameters object that
> has an allowsMultipleFiles flag. This will let us extend the open panel if
> needed.

Thanks Anders I will work on that today and update the patch!

[Brian Michel]

(In reply to comment #33)
> This is a good start, thanks for tackling it. Instead of having
> "allowsMultipleFiles", I'd like to see a WKOpenPanelParameters object that
> has an allowsMultipleFiles flag. This will let us extend the open panel if
> needed.

There is already a file with WKOpenPanelOptions.h as the name, how would you recommend naming the Objective-C wrapping class to not conflict?

[Brian Michel]

Created attachment 269635 [details]
Patch

[Brian Michel]

(In reply to comment #35)
> (In reply to comment #33)
> > This is a good start, thanks for tackling it. Instead of having
> > "allowsMultipleFiles", I'd like to see a WKOpenPanelParameters object that
> > has an allowsMultipleFiles flag. This will let us extend the open panel if
> > needed.
> 
> There is already a file with WKOpenPanelOptions.h as the name, how would you
> recommend naming the Objective-C wrapping class to not conflict?

I took a first stab at the parameters object you mentioned. Please let me know what you'd like to see changed!

[Brian Michel]

Created attachment 269672 [details]
Patch

[Brian Michel]

Just checking the build log, and I see a crash in some stream api

Regressions: Unexpected crashes (1)
  imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection.html [ Crash ]

I don't think I modified any of this code, but I did merge trunk in, any way this could be a part of that?

[Alexey Proskuryakov]

This test is flaky, see bug 152436. The failure is unrelated to this patch, and EWS is now green.

[Brian Michel]

(In reply to comment #40)
> This test is flaky, see bug 152436. The failure is unrelated to this patch,
> and EWS is now green.

Ahh Wonderful!

[Darin Adler]

Comment on attachment 269672 [details]
Patch

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

Looks like good work. Thanks for tackling this!

I have many small comments, some are important, others are less important details but still worth improving.

review- mainly because of the most serious problem, the lifetime mistake in WKConcreteOpenPanelResultListener where it doesn’t ref/deref the underlying listener proxy C++ object.

> Source/WebKit2/ChangeLog:3
> +        Handle Open Panel Functions Are Unimplemented

We don’t capitalize every word of bug titles; even though they are “titles”, “title case” is not called for.

> Source/WebKit2/ChangeLog:23
> +        * Shared/API/Cocoa/WebKit.h:
> +        * UIProcess/API/Cocoa/WKUIDelegate.h:
> +        * UIProcess/API/Cocoa/WKUIOpenPanelParameters.h: Copied from Source/WebKit2/Shared/API/Cocoa/WebKit.h.
> +        * UIProcess/API/Cocoa/WKUIOpenPanelParameters.m: Copied from Source/WebKit2/Shared/API/Cocoa/WebKit.h.
> +        (-[WKUIOpenPanelParameters _setAllowsMultipleFiles:]):
> +        * UIProcess/API/Cocoa/WKUIOpenPanelParametersPrivate.h: Copied from Source/WebKit2/Shared/API/Cocoa/WebKit.h.
> +        * UIProcess/Cocoa/UIDelegate.h:
> +        * UIProcess/Cocoa/UIDelegate.mm:
> +        (WebKit::UIDelegate::setDelegate):
> +        (WebKit::UIDelegate::UIClient::runOpenPanel):
> +        * UIProcess/Cocoa/WKConcreteOpenPanelResultListener.h: Copied from Source/WebKit2/Shared/API/Cocoa/WebKit.h.
> +        * UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m: Copied from Source/WebKit2/Shared/API/Cocoa/WebKit.h.
> +        (-[WKConcreteOpenPanelResultListener initWithListenerProxy:]):
> +        (-[WKConcreteOpenPanelResultListener chooseFiles:]):
> +        (-[WKConcreteOpenPanelResultListener cancel]):
> +        * WebKit2.xcodeproj/project.pbxproj:

A good quality change log should have comments about what each change was, and should only list function names when there is a change in the function.

All those "Copied from Source/WebKit2/Shared/API/Cocoa/WebKit.h" also look incorrect. The script generated incorrect stuff there, and you need to replace those mistakes with a useful change log entry.

> Source/WebKit2/ChangeLog:108
> -        (because createWebPage, which usually brings it up, hasn't happened yet), and we 
> +        (because createWebPage, which usually brings it up, hasn't happened yet), and we

Trimming all the trailing whitespace from the ChangeLog is not typically something we want to do; makes the patch bigger for no good reason.

I suppose we could add a pre-commit hook to prevent people from landing such whitespace in the first place to avoid this in the future.

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:39
> +@class WKWebView;
> +@class WKUIOpenPanelParameters;

We normally keep these alphabetically, so please sort these in instead  of adding them at the bottom.

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:43
> +/*! A class conforming to WKOpenPanelResultListener provides methods
> +for interacting with files.
> +*/

This seems a bit vague and oblique. I looked at Apple documentation to see how they write these and it would be something more like:

/*! The WKOpenPanelResultListener protocol defines the methods that a file open panel implementation must invokes when the open interaction is complete.
*/

One critical question is whether we have a threading rule about this class or not. I think we would want these methods to be callable on any thread. And we also want it to be safe to deallocate this object on any thread. That’s tricky to get right.

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:46
> +/*! @abstract passes an array of file urls to the web view.

Should be capitalized: “Passes”.

“file URLs”, not “file urls”.

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:47
> + @param fileURLs The array of file urls to pass to the web view.

Ditto.

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:48
> + @discussion passing nil, or an empty array will have the effect of calling the cancel selector.

There should not be a comma after the word "nil".

But more importantly in a new API we should not be talking about passing nil, since we are not marking this as a nullable argument. Callers should pass @[] rather than nil.

I also don’t think the wording “will have the effect of calling the cancel selector” is quite right. I would write something a little more like this:

@discussion Passing an empty array cancels the file open interaction as the cancel method does.

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:50
> +- (void)chooseFiles:(NSArray *)fileURLs;

The type here should be NSArray<NSURL *> *, which will make things nicer both for programmers using Objective-C and Swift. Not sure exactly how we are handling using that feature in WebKit API at this time; there may be some compatibility issue for older versions of clang. If nobody else knows, Anders at least should know.

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:139
> + @param listener The listener object to be called when selection has completed
> + or cancelled.

I’d put this all on one line. No need to wrap this. Even though some of the comments above are wrapped like this.

Wording not quite right here. I don’t think we would say that "selection has completed". We don’t "call" an object, we invoke one of its methods. And a selection doesn’t complete nor does it cancel. So I think better wording would be more like:

    The listener object with methods to be invoked when the user makes a selection or cancels.

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:140
> + @param parameters Parameters to configure the open panel with as specificed by the webpage.

typo: "specificed"

> Source/WebKit2/UIProcess/API/Cocoa/WKUIOpenPanelParameters.h:32
> +/*! A class to pass configuration flags to an open panel based on options specified in the webpage.

Comment should be more like the ones on other classes in this directory. Here’s a rough cut:

    A WKUIOpenPanelParameters object is a collection of options specified in a webpage to customize an open panel.

My wording isn’t quite right either, though.

Not sure that “parameters” is the right name for this class. There aren’t any other WebKit classes that use the name parameters like this. I looked for other similar ones.

> Source/WebKit2/UIProcess/API/Cocoa/WKUIOpenPanelParameters.h:37
> +/*! @abstract Whether or not multiple files are allowed.
> + */

Looking at other comments, it seems this one should be more like this:

/*! @abstract A Boolean value indicating whether or not the open panel should allow selection of more than one file.
*/

> Source/WebKit2/UIProcess/API/Cocoa/WKUIOpenPanelParameters.m:33
> +- (void)_setAllowsMultipleFiles:(BOOL)allowsMultipleFiles {

WebKit coding style puts the brace on a separate line.

> Source/WebKit2/UIProcess/API/Cocoa/WKUIOpenPanelParametersPrivate.h:30
> +@interface WKUIOpenPanelParameters (WKPrivate)

This should be Internal, not Private. Both the category name and the file name. Things marked Internal are for use only within the WebKit2 framework. Things marked Private are SPI, so can be used outside the framework. This is Internal.

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:104
> +#if PLATFORM(MAC)
> +    m_delegateMethods.webViewRunOpenPanelWithResultListenerParameters = [delegate respondsToSelector:@selector(webView:runOpenPanelWithResultListener:parameters:)];
> +#endif

Would be nice to put the conditional ones after the unconditional ones. I see that PLATFORM(IOS) ones are above, but I suggest putting this down at the bottom, after the ENABLE(CONTEXT_MENUS) ones.

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:241
> +bool UIDelegate::UIClient::runOpenPanel(WebKit::WebPageProxy *page, WebKit::WebFrameProxy *frame, WebKit::WebOpenPanelParameters *parameters, WebKit::WebOpenPanelResultListenerProxy *listener)

Formatting here is wrong for WebKit coding style (should be "WebPageProxy*" with the * next to the type name), and the explicit WebKit prefixes are not needed.

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:247
> +    // If our delegate does not support this method, invalidate the listener and return false.
> +    if (!m_uiDelegate.m_delegateMethods.webViewRunOpenPanelWithResultListenerParameters) {
> +        listener->invalidate();
> +        return false;
> +    }

In WebKit code we try to avoid writing comments that say the same thing the code does. This is an example of that anti-pattern, so please omit the comment.

You should write a comment when you need to explain *why* the code does what it does.

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:254
> +    NSWindow *window = [m_uiDelegate.m_webView window];
> +
> +    if (!window) {
> +        listener->invalidate();
> +        return false;
> +    }

I don’t understand why this code is here. Whether to do this without a window seems like a decision for the delegate, not to be decided by WebKit. Please don’t enforce a "no window, no call to delegate" policy here, unless there is a compelling reason to do so.

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:261
> +    [delegate webView:m_uiDelegate.m_webView runOpenPanelWithResultListener:(id <WKOpenPanelResultListener>)adoptNS([[WKConcreteOpenPanelResultListener alloc] initWithListenerProxy:listener]) parameters:(WKUIOpenPanelParameters *)delegateParameters];

This should be using "adoptNS([[WKConcreteOpenPanelResultListener alloc] initWithListenerProxy:listener]).get()" rather than "(id <WKOpenPanelResultListener>)adoptNS([[WKConcreteOpenPanelResultListener alloc] initWithListenerProxy:listener])". Might also want to use a local variable so this line isn’t so super-long.

This should be using "delegateParameters.get()" rather than "(WKUIOpenPanelParameters *)delegateParameters".

> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.h:2
> + * Copyright (C) 2015 Brian Michel ([e-mail]). All rights reserved.

2016

> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.h:37
> +- (instancetype)initWithListenerProxy:(WebKit::WebOpenPanelResultListenerProxy *)proxy;

In WebKit coding style there should not be a space before "*" on this line.

I suggest naming this method initWithProxy: rather than initWithListenerProxy: because since this object is a listener, it stands to reason the proxy would be a listener proxy.

> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:41
> +    WebOpenPanelResultListenerProxy *m_listener;

This object needs to ref the listener. Otherwise, this listener can outlive the proxy. However, using RefPtr for this would be dangerous because it will make the dealloc method call deref on whatever thread the object is released on. Instead we probably want to use callOnMainThread in the dealloc method to make sure the deref happens on the main thread.

> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:44
> +- (instancetype)initWithListenerProxy:(WebOpenPanelResultListenerProxy *)proxy {

WebKit coding style puts the open brace on a separate line.

> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:53
> +#pragma mark - WKOpenPanelResultListener

Not sure we really need this mark. This is a small simple class.

> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:55
> +- (void)chooseFiles:(NSArray *)fileURLs {

To make it safe to call this on any thread, I think we need to use callOnMainThread in this method and put the actual work inside the block or lambda that we call on the main thread. We also need to capture self (and retain it here and release it in the block or lambda).

Also would be nice to make it safe to call this method multiple times. I think the way to do that would be to null out m_listener after this is called the first time, and then make sure it’s a no-op if m_listener is already null. As with the work I mentioned above and the deref in the dealloc function, the work has to be done on the main thread.

WebKit coding style puts the open brace on a separate line.

> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:57
> +    NSUInteger count = [fileURLs count];
> +    if (!count)

No reason to put this into a local variable since we only use it once. I’d write this:

    if (!fileURLs.count)

> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:60
> +        Vector<RefPtr<API::Object>> urls;

I personally would dodge the ugly "urls" name by naming the vector something else, like "files".

> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:64
> +        for (NSURL *fileURL in fileURLs)
> +            urls.uncheckedAppend(adoptRef(toImpl(WKURLCreateWithCFURL((CFURLRef)fileURL))));

We should consider throwing an Objective-C exception if any of the elements in the array are not NSURL objects. That would make it easier for programmers to figure out what they did wrong than the crash they might otherwise see. Maybe not, though. Doesn’t seem like other WebKit API code is taking this approach, so I guess it’s OK to leave as is for now.

> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:67
> +        RefPtr<API::Array> fileURLsRef = API::Array::create(std::move(urls));
> +        m_listener->chooseFiles(fileURLsRef.get());

No need for the local variable. In WebKit code, should use WTFMove instead of std::move, so should be:

    m_listener->chooseFiles(API::Array::create(WTFMove(urls)).get());

> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:71
> +- (void)cancel {

Same threading issues apply here as in chooseFiles: and I think we should probably make these share code. Simplest way is probably to just implement this as [self chooseFiles:@[]] unless we change the semantics of chooseFiles to make choosing zero files something distinct from cancelling. In that case I’m sure we can still find a way to share the tricky main thread code.

WebKit coding style puts the open brace on a separate line.

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:1082
> +		72C06CFF1C531D1A001291DE /* WKUIOpenPanelParametersPrivate.h in Headers */ = {isa = PBXBuildFile; fileRef = 72C06CFE1C531D1A001291DE /* WKUIOpenPanelParametersPrivate.h */; settings = {ATTRIBUTES = (Private, ); }; };

We don’t want this visible outside the framework, so it should be Internal, not private. The ATTRIBUTES = (Private, ) here is wrong.

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:1084
> +		72FCB6B41C307C2B00E8A012 /* WKConcreteOpenPanelResultListener.h in Headers */ = {isa = PBXBuildFile; fileRef = 72FCB6B11C307C2B00E8A012 /* WKConcreteOpenPanelResultListener.h */; settings = {ATTRIBUTES = (Private, ); }; };

We don’t want this visible outside the framework, so it should be Internal, not private. The ATTRIBUTES = (Private, ) here is wrong.

> Tools/ChangeLog:15
> +        * MiniBrowser/mac/WK2BrowserWindowController.m:
> +        (-[WK2BrowserWindowController webView:runOpenPanelWithResultListener:parameters:]):
> +        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
> +        * TestWebKitAPI/Tests/WebKit2/basic-input-element.html: Added.
> +        * TestWebKitAPI/Tests/WebKit2ObjC/WKUIDelegateOpenPanelTest.mm: Added.
> +        (-[WKOpenPanelTestDelegate webView:runOpenPanelWithResultListener:parameters:]):
> +        (-[WKOpenPanelTestDelegate webView:didFinishNavigation:]):
> +        (TestWebKitAPI::TEST):

As above, a good quality change log should have comments about what each change was. Per-function comments help people understand the change. If you need an example, I suggest looking at change log entries I have written in the past to see what I am asking for.

> Tools/ChangeLog:549
> -        I always forgot to provide a --jsc argument), I always found it easier to 
> +        I always forgot to provide a --jsc argument), I always found it easier to

As above, trimming all the trailing whitespace from the ChangeLog is not typically something we want to do; makes the patch bigger for no good reason.

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:452
> +    [openPanel setAllowsMultipleSelection:parameters.allowsMultipleFiles];

I would have written:

    openPanel.allowsMultipleSelection = parameters.allowsMultipleFiles;

> Tools/TestWebKitAPI/Tests/WebKit2ObjC/WKUIDelegateOpenPanelTest.mm:26
> +#include "config.h"

Should be #import.

> Tools/TestWebKitAPI/Tests/WebKit2ObjC/WKUIDelegateOpenPanelTest.mm:31
> +#include <wtf/RetainPtr.h>

Should be #import. Should be in the same paragraph with the other includes just above.

> Tools/TestWebKitAPI/Tests/WebKit2ObjC/WKUIDelegateOpenPanelTest.mm:33
> +#if WK_API_ENABLED && !PLATFORM(IOS)

Should use the same #if as the code. Since the code is in PLATFORM(MAC), then this should be PLATFORM(MAC), as opposed to !PLATFORM(IOS).

> Tools/TestWebKitAPI/Tests/WebKit2ObjC/WKUIDelegateOpenPanelTest.mm:36
> +static bool doneOpening;
> +static bool doneLoading;

It might be cleaner to use public fields in WKOpenPanelTestDelegate instead of globals. Then we wouldn’t have to clear them on the way out of the test. Not sure we need to do that anyway. But since it’s just a test, I think it’s OK either wya.

[Darin Adler]

Comment on attachment 269672 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKUIOpenPanelParameters.h:34
> +@interface WKUIOpenPanelParameters : NSObject

This class shouldn’t have a “UI” prefix after the “WK” prefix. In “WKUIDelegate” the “UI” is part of the name of the “UI delegate”. Here there is no reason to include “UI”.

[Brian Michel]

(In reply to comment #42)
> Comment on attachment 269672 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269672&action=review
> 
> Looks like good work. Thanks for tackling this!
> 
> I have many small comments, some are important, others are less important
> details but still worth improving.
> 
> review- mainly because of the most serious problem, the lifetime mistake in
> WKConcreteOpenPanelResultListener where it doesn’t ref/deref the underlying
> listener proxy C++ object.
> 
> > Source/WebKit2/ChangeLog:3
> > +        Handle Open Panel Functions Are Unimplemented
> 
> We don’t capitalize every word of bug titles; even though they are “titles”,
> “title case” is not called for.
> 
> > Source/WebKit2/ChangeLog:23
> > +        * Shared/API/Cocoa/WebKit.h:
> > +        * UIProcess/API/Cocoa/WKUIDelegate.h:
> > +        * UIProcess/API/Cocoa/WKUIOpenPanelParameters.h: Copied from Source/WebKit2/Shared/API/Cocoa/WebKit.h.
> > +        * UIProcess/API/Cocoa/WKUIOpenPanelParameters.m: Copied from Source/WebKit2/Shared/API/Cocoa/WebKit.h.
> > +        (-[WKUIOpenPanelParameters _setAllowsMultipleFiles:]):
> > +        * UIProcess/API/Cocoa/WKUIOpenPanelParametersPrivate.h: Copied from Source/WebKit2/Shared/API/Cocoa/WebKit.h.
> > +        * UIProcess/Cocoa/UIDelegate.h:
> > +        * UIProcess/Cocoa/UIDelegate.mm:
> > +        (WebKit::UIDelegate::setDelegate):
> > +        (WebKit::UIDelegate::UIClient::runOpenPanel):
> > +        * UIProcess/Cocoa/WKConcreteOpenPanelResultListener.h: Copied from Source/WebKit2/Shared/API/Cocoa/WebKit.h.
> > +        * UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m: Copied from Source/WebKit2/Shared/API/Cocoa/WebKit.h.
> > +        (-[WKConcreteOpenPanelResultListener initWithListenerProxy:]):
> > +        (-[WKConcreteOpenPanelResultListener chooseFiles:]):
> > +        (-[WKConcreteOpenPanelResultListener cancel]):
> > +        * WebKit2.xcodeproj/project.pbxproj:
> 
> A good quality change log should have comments about what each change was,
> and should only list function names when there is a change in the function.
> 
> All those "Copied from Source/WebKit2/Shared/API/Cocoa/WebKit.h" also look
> incorrect. The script generated incorrect stuff there, and you need to
> replace those mistakes with a useful change log entry.
> 
> > Source/WebKit2/ChangeLog:108
> > -        (because createWebPage, which usually brings it up, hasn't happened yet), and we 
> > +        (because createWebPage, which usually brings it up, hasn't happened yet), and we
> 
> Trimming all the trailing whitespace from the ChangeLog is not typically
> something we want to do; makes the patch bigger for no good reason.
> 
> I suppose we could add a pre-commit hook to prevent people from landing such
> whitespace in the first place to avoid this in the future.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:39
> > +@class WKWebView;
> > +@class WKUIOpenPanelParameters;
> 
> We normally keep these alphabetically, so please sort these in instead  of
> adding them at the bottom.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:43
> > +/*! A class conforming to WKOpenPanelResultListener provides methods
> > +for interacting with files.
> > +*/
> 
> This seems a bit vague and oblique. I looked at Apple documentation to see
> how they write these and it would be something more like:
> 
> /*! The WKOpenPanelResultListener protocol defines the methods that a file
> open panel implementation must invokes when the open interaction is complete.
> */
> 
> One critical question is whether we have a threading rule about this class
> or not. I think we would want these methods to be callable on any thread.
> And we also want it to be safe to deallocate this object on any thread.
> That’s tricky to get right.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:46
> > +/*! @abstract passes an array of file urls to the web view.
> 
> Should be capitalized: “Passes”.
> 
> “file URLs”, not “file urls”.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:47
> > + @param fileURLs The array of file urls to pass to the web view.
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:48
> > + @discussion passing nil, or an empty array will have the effect of calling the cancel selector.
> 
> There should not be a comma after the word "nil".
> 
> But more importantly in a new API we should not be talking about passing
> nil, since we are not marking this as a nullable argument. Callers should
> pass @[] rather than nil.
> 
> I also don’t think the wording “will have the effect of calling the cancel
> selector” is quite right. I would write something a little more like this:
> 
> @discussion Passing an empty array cancels the file open interaction as the
> cancel method does.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:50
> > +- (void)chooseFiles:(NSArray *)fileURLs;
> 
> The type here should be NSArray<NSURL *> *, which will make things nicer
> both for programmers using Objective-C and Swift. Not sure exactly how we
> are handling using that feature in WebKit API at this time; there may be
> some compatibility issue for older versions of clang. If nobody else knows,
> Anders at least should know.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:139
> > + @param listener The listener object to be called when selection has completed
> > + or cancelled.
> 
> I’d put this all on one line. No need to wrap this. Even though some of the
> comments above are wrapped like this.
> 
> Wording not quite right here. I don’t think we would say that "selection has
> completed". We don’t "call" an object, we invoke one of its methods. And a
> selection doesn’t complete nor does it cancel. So I think better wording
> would be more like:
> 
>     The listener object with methods to be invoked when the user makes a
> selection or cancels.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:140
> > + @param parameters Parameters to configure the open panel with as specificed by the webpage.
> 
> typo: "specificed"
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUIOpenPanelParameters.h:32
> > +/*! A class to pass configuration flags to an open panel based on options specified in the webpage.
> 
> Comment should be more like the ones on other classes in this directory.
> Here’s a rough cut:
> 
>     A WKUIOpenPanelParameters object is a collection of options specified in
> a webpage to customize an open panel.
> 
> My wording isn’t quite right either, though.
> 
> Not sure that “parameters” is the right name for this class. There aren’t
> any other WebKit classes that use the name parameters like this. I looked
> for other similar ones.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUIOpenPanelParameters.h:37
> > +/*! @abstract Whether or not multiple files are allowed.
> > + */
> 
> Looking at other comments, it seems this one should be more like this:
> 
> /*! @abstract A Boolean value indicating whether or not the open panel
> should allow selection of more than one file.
> */
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUIOpenPanelParameters.m:33
> > +- (void)_setAllowsMultipleFiles:(BOOL)allowsMultipleFiles {
> 
> WebKit coding style puts the brace on a separate line.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUIOpenPanelParametersPrivate.h:30
> > +@interface WKUIOpenPanelParameters (WKPrivate)
> 
> This should be Internal, not Private. Both the category name and the file
> name. Things marked Internal are for use only within the WebKit2 framework.
> Things marked Private are SPI, so can be used outside the framework. This is
> Internal.
> 
> > Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:104
> > +#if PLATFORM(MAC)
> > +    m_delegateMethods.webViewRunOpenPanelWithResultListenerParameters = [delegate respondsToSelector:@selector(webView:runOpenPanelWithResultListener:parameters:)];
> > +#endif
> 
> Would be nice to put the conditional ones after the unconditional ones. I
> see that PLATFORM(IOS) ones are above, but I suggest putting this down at
> the bottom, after the ENABLE(CONTEXT_MENUS) ones.
> 
> > Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:241
> > +bool UIDelegate::UIClient::runOpenPanel(WebKit::WebPageProxy *page, WebKit::WebFrameProxy *frame, WebKit::WebOpenPanelParameters *parameters, WebKit::WebOpenPanelResultListenerProxy *listener)
> 
> Formatting here is wrong for WebKit coding style (should be "WebPageProxy*"
> with the * next to the type name), and the explicit WebKit prefixes are not
> needed.
> 
> > Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:247
> > +    // If our delegate does not support this method, invalidate the listener and return false.
> > +    if (!m_uiDelegate.m_delegateMethods.webViewRunOpenPanelWithResultListenerParameters) {
> > +        listener->invalidate();
> > +        return false;
> > +    }
> 
> In WebKit code we try to avoid writing comments that say the same thing the
> code does. This is an example of that anti-pattern, so please omit the
> comment.
> 
> You should write a comment when you need to explain *why* the code does what
> it does.
> 
> > Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:254
> > +    NSWindow *window = [m_uiDelegate.m_webView window];
> > +
> > +    if (!window) {
> > +        listener->invalidate();
> > +        return false;
> > +    }
> 
> I don’t understand why this code is here. Whether to do this without a
> window seems like a decision for the delegate, not to be decided by WebKit.
> Please don’t enforce a "no window, no call to delegate" policy here, unless
> there is a compelling reason to do so.
> 
> > Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:261
> > +    [delegate webView:m_uiDelegate.m_webView runOpenPanelWithResultListener:(id <WKOpenPanelResultListener>)adoptNS([[WKConcreteOpenPanelResultListener alloc] initWithListenerProxy:listener]) parameters:(WKUIOpenPanelParameters *)delegateParameters];
> 
> This should be using "adoptNS([[WKConcreteOpenPanelResultListener alloc]
> initWithListenerProxy:listener]).get()" rather than "(id
> <WKOpenPanelResultListener>)adoptNS([[WKConcreteOpenPanelResultListener
> alloc] initWithListenerProxy:listener])". Might also want to use a local
> variable so this line isn’t so super-long.
> 
> This should be using "delegateParameters.get()" rather than
> "(WKUIOpenPanelParameters *)delegateParameters".
> 
> > Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.h:2
> > + * Copyright (C) 2015 Brian Michel ([e-mail]). All rights reserved.
> 
> 2016
> 
> > Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.h:37
> > +- (instancetype)initWithListenerProxy:(WebKit::WebOpenPanelResultListenerProxy *)proxy;
> 
> In WebKit coding style there should not be a space before "*" on this line.
> 
> I suggest naming this method initWithProxy: rather than
> initWithListenerProxy: because since this object is a listener, it stands to
> reason the proxy would be a listener proxy.
> 
> > Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:41
> > +    WebOpenPanelResultListenerProxy *m_listener;
> 
> This object needs to ref the listener. Otherwise, this listener can outlive
> the proxy. However, using RefPtr for this would be dangerous because it will
> make the dealloc method call deref on whatever thread the object is released
> on. Instead we probably want to use callOnMainThread in the dealloc method
> to make sure the deref happens on the main thread.
> 
> > Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:44
> > +- (instancetype)initWithListenerProxy:(WebOpenPanelResultListenerProxy *)proxy {
> 
> WebKit coding style puts the open brace on a separate line.
> 
> > Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:53
> > +#pragma mark - WKOpenPanelResultListener
> 
> Not sure we really need this mark. This is a small simple class.
> 
> > Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:55
> > +- (void)chooseFiles:(NSArray *)fileURLs {
> 
> To make it safe to call this on any thread, I think we need to use
> callOnMainThread in this method and put the actual work inside the block or
> lambda that we call on the main thread. We also need to capture self (and
> retain it here and release it in the block or lambda).
> 
> Also would be nice to make it safe to call this method multiple times. I
> think the way to do that would be to null out m_listener after this is
> called the first time, and then make sure it’s a no-op if m_listener is
> already null. As with the work I mentioned above and the deref in the
> dealloc function, the work has to be done on the main thread.
> 
> WebKit coding style puts the open brace on a separate line.
> 
> > Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:57
> > +    NSUInteger count = [fileURLs count];
> > +    if (!count)
> 
> No reason to put this into a local variable since we only use it once. I’d
> write this:
> 
>     if (!fileURLs.count)
> 
> > Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:60
> > +        Vector<RefPtr<API::Object>> urls;
> 
> I personally would dodge the ugly "urls" name by naming the vector something
> else, like "files".
> 
> > Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:64
> > +        for (NSURL *fileURL in fileURLs)
> > +            urls.uncheckedAppend(adoptRef(toImpl(WKURLCreateWithCFURL((CFURLRef)fileURL))));
> 
> We should consider throwing an Objective-C exception if any of the elements
> in the array are not NSURL objects. That would make it easier for
> programmers to figure out what they did wrong than the crash they might
> otherwise see. Maybe not, though. Doesn’t seem like other WebKit API code is
> taking this approach, so I guess it’s OK to leave as is for now.
> 
> > Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:67
> > +        RefPtr<API::Array> fileURLsRef = API::Array::create(std::move(urls));
> > +        m_listener->chooseFiles(fileURLsRef.get());
> 
> No need for the local variable. In WebKit code, should use WTFMove instead
> of std::move, so should be:
> 
>     m_listener->chooseFiles(API::Array::create(WTFMove(urls)).get());
> 
> > Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:71
> > +- (void)cancel {
> 
> Same threading issues apply here as in chooseFiles: and I think we should
> probably make these share code. Simplest way is probably to just implement
> this as [self chooseFiles:@[]] unless we change the semantics of chooseFiles
> to make choosing zero files something distinct from cancelling. In that case
> I’m sure we can still find a way to share the tricky main thread code.
> 
> WebKit coding style puts the open brace on a separate line.
> 
> > Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:1082
> > +		72C06CFF1C531D1A001291DE /* WKUIOpenPanelParametersPrivate.h in Headers */ = {isa = PBXBuildFile; fileRef = 72C06CFE1C531D1A001291DE /* WKUIOpenPanelParametersPrivate.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> We don’t want this visible outside the framework, so it should be Internal,
> not private. The ATTRIBUTES = (Private, ) here is wrong.
> 
> > Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:1084
> > +		72FCB6B41C307C2B00E8A012 /* WKConcreteOpenPanelResultListener.h in Headers */ = {isa = PBXBuildFile; fileRef = 72FCB6B11C307C2B00E8A012 /* WKConcreteOpenPanelResultListener.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> We don’t want this visible outside the framework, so it should be Internal,
> not private. The ATTRIBUTES = (Private, ) here is wrong.
> 
> > Tools/ChangeLog:15
> > +        * MiniBrowser/mac/WK2BrowserWindowController.m:
> > +        (-[WK2BrowserWindowController webView:runOpenPanelWithResultListener:parameters:]):
> > +        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
> > +        * TestWebKitAPI/Tests/WebKit2/basic-input-element.html: Added.
> > +        * TestWebKitAPI/Tests/WebKit2ObjC/WKUIDelegateOpenPanelTest.mm: Added.
> > +        (-[WKOpenPanelTestDelegate webView:runOpenPanelWithResultListener:parameters:]):
> > +        (-[WKOpenPanelTestDelegate webView:didFinishNavigation:]):
> > +        (TestWebKitAPI::TEST):
> 
> As above, a good quality change log should have comments about what each
> change was. Per-function comments help people understand the change. If you
> need an example, I suggest looking at change log entries I have written in
> the past to see what I am asking for.
> 
> > Tools/ChangeLog:549
> > -        I always forgot to provide a --jsc argument), I always found it easier to 
> > +        I always forgot to provide a --jsc argument), I always found it easier to
> 
> As above, trimming all the trailing whitespace from the ChangeLog is not
> typically something we want to do; makes the patch bigger for no good reason.
> 
> > Tools/MiniBrowser/mac/WK2BrowserWindowController.m:452
> > +    [openPanel setAllowsMultipleSelection:parameters.allowsMultipleFiles];
> 
> I would have written:
> 
>     openPanel.allowsMultipleSelection = parameters.allowsMultipleFiles;
> 
> > Tools/TestWebKitAPI/Tests/WebKit2ObjC/WKUIDelegateOpenPanelTest.mm:26
> > +#include "config.h"
> 
> Should be #import.
> 
> > Tools/TestWebKitAPI/Tests/WebKit2ObjC/WKUIDelegateOpenPanelTest.mm:31
> > +#include <wtf/RetainPtr.h>
> 
> Should be #import. Should be in the same paragraph with the other includes
> just above.
> 
> > Tools/TestWebKitAPI/Tests/WebKit2ObjC/WKUIDelegateOpenPanelTest.mm:33
> > +#if WK_API_ENABLED && !PLATFORM(IOS)
> 
> Should use the same #if as the code. Since the code is in PLATFORM(MAC),
> then this should be PLATFORM(MAC), as opposed to !PLATFORM(IOS).
> 
> > Tools/TestWebKitAPI/Tests/WebKit2ObjC/WKUIDelegateOpenPanelTest.mm:36
> > +static bool doneOpening;
> > +static bool doneLoading;
> 
> It might be cleaner to use public fields in WKOpenPanelTestDelegate instead
> of globals. Then we wouldn’t have to clear them on the way out of the test.
> Not sure we need to do that anyway. But since it’s just a test, I think it’s
> OK either wya.

Darin, thank you so much for the feedback, I'll work on these changes and get them submitted by the end of the week!

[Anders Carlsson]

Created attachment 276410 [details]
Patch

[WebKit Commit Bot]

Attachment 276410 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:275:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 276410 [details]
Patch

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

> Source/WebKit2/UIProcess/API/APIOpenPanelParameters.h:31
> +#include <wtf/Vector.h>

No reason to include this.

> Source/WebKit2/UIProcess/API/APIOpenPanelParameters.h:32
> +#include <wtf/text/WTFString.h>

No reason to include this.

> Source/WebKit2/UIProcess/API/APIOpenPanelParameters.h:40
> +    static PassRefPtr<OpenPanelParameters> create(const WebCore::FileChooserSettings&);

How about Ref<> instead of PassRefPtr?

> Source/WebKit2/UIProcess/API/APIOpenPanelParameters.h:41
> +    ~OpenPanelParameters();

How about not defining this?

[Anders Carlsson]

(In reply to comment #47)
> Comment on attachment 276410 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276410&action=review
> 
> > Source/WebKit2/UIProcess/API/APIOpenPanelParameters.h:31
> > +#include <wtf/Vector.h>
> 
> No reason to include this.
> 
> > Source/WebKit2/UIProcess/API/APIOpenPanelParameters.h:32
> > +#include <wtf/text/WTFString.h>
> 
> No reason to include this.
> 
> > Source/WebKit2/UIProcess/API/APIOpenPanelParameters.h:40
> > +    static PassRefPtr<OpenPanelParameters> create(const WebCore::FileChooserSettings&);
> 
> How about Ref<> instead of PassRefPtr?

Fixed all of these.

> 
> > Source/WebKit2/UIProcess/API/APIOpenPanelParameters.h:41
> > +    ~OpenPanelParameters();
> 
> How about not defining this?

I like putting the destructor out of line for two reasons:

1. It avoids it from getting emitted wherever it can be called (which I think is all Ref/RefPtr<API::OpenPanelParameters> destructors.
2. In this case it will anchor the vtable to the .cpp file to avoid it being emitted all over the place.

[Darin Adler]

(In reply to comment #48)
> I like putting the destructor out of line for two reasons:
> 
> 1. It avoids it from getting emitted wherever it can be called (which I
> think is all Ref/RefPtr<API::OpenPanelParameters> destructors.
> 2. In this case it will anchor the vtable to the .cpp file to avoid it being
> emitted all over the place.

I should keep that in mind for the future. Might even be worth making it a coding style guideline. Especially if in aggregate it actually affects things like build times or object files sizes.

[Anders Carlsson]

Committed r199554: <http://trac.webkit.org/changeset/199554>

[Jiewen Tan]

Reverted r199554 for reason:

The change breaks Yosemite Release/Debug build

Committed r199557: <http://trac.webkit.org/changeset/199557>

[Jiewen Tan]

Here is the build log:
https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%28Build%29/builds/14297

Here is the error:
(view as text)
/Volumes/Data/slave/yosemite-release/build/Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:167:181: error: expected '>'
/Volumes/Data/slave/yosemite-release/build/Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:167:182: error: expected ')'
/Volumes/Data/slave/yosemite-release/build/Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:275:193: error: expected '>'
/Volumes/Data/slave/yosemite-release/build/Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:275:194: error: expected ')'
/Volumes/Data/slave/yosemite-release/build/Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:278:14: error: use of undeclared identifier 'URLs'
/Volumes/Data/slave/yosemite-release/build/Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:284:28: error: use of undeclared identifier 'URLs'

[Anders Carlsson]

Committed r199558: <http://trac.webkit.org/changeset/199558>

[Carlos Alberto Lopez Perez]

(In reply to comment #53)
> Committed r199558: <http://trac.webkit.org/changeset/199558>

- This broke all the CMake ports that were fixed in r199568
- But r199568 was not enough to fix the breakage on the GTK+ port that remains broken: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Build%29/builds/57627/steps/compile-webkit/logs/stdio/text

[Carlos Garcia Campos]

(In reply to comment #54)
> (In reply to comment #53)
> > Committed r199558: <http://trac.webkit.org/changeset/199558>
> 
> - This broke all the CMake ports that were fixed in r199568
> - But r199568 was not enough to fix the breakage on the GTK+ port that
> remains broken:
> https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Build%29/
> builds/57627/steps/compile-webkit/logs/stdio/text

GTK+ build fixed in r199577