[Webkit-unassigned] [Bug 137759] Handle Open Panel Functions Are Unimplemented

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jan 24 13:35:50 PST 2016 

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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #269672|review?                     |review-
              Flags|                            |

--- Comment #42 from Darin Adler <darin at apple.com> ---
Comment on attachment 269672
  --> https://bugs.webkit.org/attachment.cgi?id=269672
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.

-- 
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/20160124/7276583d/attachment-0001.html>

More information about the webkit-unassigned mailing list