<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [Mac] Add API for open panel handling to WKWebView"
href="https://bugs.webkit.org/show_bug.cgi?id=137759#c44">Comment # 44</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [Mac] Add API for open panel handling to WKWebView"
href="https://bugs.webkit.org/show_bug.cgi?id=137759">bug 137759</a>
from <span class="vcard"><a class="email" href="mailto:brian.michel@gmail.com" title="Brian Michel <brian.michel@gmail.com>"> <span class="fn">Brian Michel</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=137759#c42">comment #42</a>)
<span class="quote">> 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.
>
> > 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 (<a href="mailto:brian.michel@gmail.com">brian.michel@gmail.com</a>). 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.</span >
Darin, thank you so much for the feedback, I'll work on these changes and get them submitted by the end of the week!</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>