<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Handle Open Panel Functions Are Unimplemented"
href="https://bugs.webkit.org/show_bug.cgi?id=137759">bug 137759</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #269672 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Handle Open Panel Functions Are Unimplemented"
href="https://bugs.webkit.org/show_bug.cgi?id=137759#c42">Comment # 42</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Handle Open Panel Functions Are Unimplemented"
href="https://bugs.webkit.org/show_bug.cgi?id=137759">bug 137759</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=269672&action=diff" name="attach_269672" title="Patch">attachment 269672</a> <a href="attachment.cgi?id=269672&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=269672&action=review">https://bugs.webkit.org/attachment.cgi?id=269672&action=review</a>
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.
<span class="quote">> Source/WebKit2/ChangeLog:3
> + Handle Open Panel Functions Are Unimplemented</span >
We don’t capitalize every word of bug titles; even though they are “titles”, “title case” is not called for.
<span class="quote">> 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:</span >
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.
<span class="quote">> 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</span >
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.
<span class="quote">> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:39
> +@class WKWebView;
> +@class WKUIOpenPanelParameters;</span >
We normally keep these alphabetically, so please sort these in instead of adding them at the bottom.
<span class="quote">> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:43
> +/*! A class conforming to WKOpenPanelResultListener provides methods
> +for interacting with files.
> +*/</span >
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.
<span class="quote">> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:46
> +/*! @abstract passes an array of file urls to the web view.</span >
Should be capitalized: “Passes”.
“file URLs”, not “file urls”.
<span class="quote">> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:47
> + @param fileURLs The array of file urls to pass to the web view.</span >
Ditto.
<span class="quote">> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:48
> + @discussion passing nil, or an empty array will have the effect of calling the cancel selector.</span >
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.
<span class="quote">> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:50
> +- (void)chooseFiles:(NSArray *)fileURLs;</span >
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.
<span class="quote">> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:139
> + @param listener The listener object to be called when selection has completed
> + or cancelled.</span >
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.
<span class="quote">> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegate.h:140
> + @param parameters Parameters to configure the open panel with as specificed by the webpage.</span >
typo: "specificed"
<span class="quote">> 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.</span >
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.
<span class="quote">> Source/WebKit2/UIProcess/API/Cocoa/WKUIOpenPanelParameters.h:37
> +/*! @abstract Whether or not multiple files are allowed.
> + */</span >
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.
*/
<span class="quote">> Source/WebKit2/UIProcess/API/Cocoa/WKUIOpenPanelParameters.m:33
> +- (void)_setAllowsMultipleFiles:(BOOL)allowsMultipleFiles {</span >
WebKit coding style puts the brace on a separate line.
<span class="quote">> Source/WebKit2/UIProcess/API/Cocoa/WKUIOpenPanelParametersPrivate.h:30
> +@interface WKUIOpenPanelParameters (WKPrivate)</span >
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.
<span class="quote">> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:104
> +#if PLATFORM(MAC)
> + m_delegateMethods.webViewRunOpenPanelWithResultListenerParameters = [delegate respondsToSelector:@selector(webView:runOpenPanelWithResultListener:parameters:)];
> +#endif</span >
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.
<span class="quote">> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:241
> +bool UIDelegate::UIClient::runOpenPanel(WebKit::WebPageProxy *page, WebKit::WebFrameProxy *frame, WebKit::WebOpenPanelParameters *parameters, WebKit::WebOpenPanelResultListenerProxy *listener)</span >
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.
<span class="quote">> 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;
> + }</span >
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.
<span class="quote">> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:254
> + NSWindow *window = [m_uiDelegate.m_webView window];
> +
> + if (!window) {
> + listener->invalidate();
> + return false;
> + }</span >
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.
<span class="quote">> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:261
> + [delegate webView:m_uiDelegate.m_webView runOpenPanelWithResultListener:(id <WKOpenPanelResultListener>)adoptNS([[WKConcreteOpenPanelResultListener alloc] initWithListenerProxy:listener]) parameters:(WKUIOpenPanelParameters *)delegateParameters];</span >
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".
<span class="quote">> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.h:2
> + * Copyright (C) 2015 Brian Michel (<a href="mailto:brian.michel@gmail.com">brian.michel@gmail.com</a>). All rights reserved.</span >
2016
<span class="quote">> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.h:37
> +- (instancetype)initWithListenerProxy:(WebKit::WebOpenPanelResultListenerProxy *)proxy;</span >
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.
<span class="quote">> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:41
> + WebOpenPanelResultListenerProxy *m_listener;</span >
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.
<span class="quote">> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:44
> +- (instancetype)initWithListenerProxy:(WebOpenPanelResultListenerProxy *)proxy {</span >
WebKit coding style puts the open brace on a separate line.
<span class="quote">> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:53
> +#pragma mark - WKOpenPanelResultListener</span >
Not sure we really need this mark. This is a small simple class.
<span class="quote">> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:55
> +- (void)chooseFiles:(NSArray *)fileURLs {</span >
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.
<span class="quote">> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:57
> + NSUInteger count = [fileURLs count];
> + if (!count)</span >
No reason to put this into a local variable since we only use it once. I’d write this:
if (!fileURLs.count)
<span class="quote">> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:60
> + Vector<RefPtr<API::Object>> urls;</span >
I personally would dodge the ugly "urls" name by naming the vector something else, like "files".
<span class="quote">> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:64
> + for (NSURL *fileURL in fileURLs)
> + urls.uncheckedAppend(adoptRef(toImpl(WKURLCreateWithCFURL((CFURLRef)fileURL))));</span >
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.
<span class="quote">> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:67
> + RefPtr<API::Array> fileURLsRef = API::Array::create(std::move(urls));
> + m_listener->chooseFiles(fileURLsRef.get());</span >
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());
<span class="quote">> Source/WebKit2/UIProcess/Cocoa/WKConcreteOpenPanelResultListener.m:71
> +- (void)cancel {</span >
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.
<span class="quote">> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:1082
> + 72C06CFF1C531D1A001291DE /* WKUIOpenPanelParametersPrivate.h in Headers */ = {isa = PBXBuildFile; fileRef = 72C06CFE1C531D1A001291DE /* WKUIOpenPanelParametersPrivate.h */; settings = {ATTRIBUTES = (Private, ); }; };</span >
We don’t want this visible outside the framework, so it should be Internal, not private. The ATTRIBUTES = (Private, ) here is wrong.
<span class="quote">> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:1084
> + 72FCB6B41C307C2B00E8A012 /* WKConcreteOpenPanelResultListener.h in Headers */ = {isa = PBXBuildFile; fileRef = 72FCB6B11C307C2B00E8A012 /* WKConcreteOpenPanelResultListener.h */; settings = {ATTRIBUTES = (Private, ); }; };</span >
We don’t want this visible outside the framework, so it should be Internal, not private. The ATTRIBUTES = (Private, ) here is wrong.
<span class="quote">> 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):</span >
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.
<span class="quote">> 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</span >
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.
<span class="quote">> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:452
> + [openPanel setAllowsMultipleSelection:parameters.allowsMultipleFiles];</span >
I would have written:
openPanel.allowsMultipleSelection = parameters.allowsMultipleFiles;
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2ObjC/WKUIDelegateOpenPanelTest.mm:26
> +#include "config.h"</span >
Should be #import.
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2ObjC/WKUIDelegateOpenPanelTest.mm:31
> +#include <wtf/RetainPtr.h></span >
Should be #import. Should be in the same paragraph with the other includes just above.
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2ObjC/WKUIDelegateOpenPanelTest.mm:33
> +#if WK_API_ENABLED && !PLATFORM(IOS)</span >
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).
<span class="quote">> Tools/TestWebKitAPI/Tests/WebKit2ObjC/WKUIDelegateOpenPanelTest.mm:36
> +static bool doneOpening;
> +static bool doneLoading;</span >
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.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>