[Webkit-unassigned] [Bug 137759] [Mac] Add API for open panel handling to WKWebView

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=137759

--- Comment #44 from Brian Michel <brian.michel at gmail.com> ---
(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
> > + at class WKWebView;
> > + at 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
> > + at 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 (brian.michel at gmail.com). 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!

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160125/3cc02b43/attachment-0001.html>