Bug 166951 – Add IconLoadingDelegate functionality to WKView

• Component: WebKit API
• Status: RESOLVED FIXED

[Brady Eidson]

Add IconLoadingDelegate functionality to WKView

This is a followup to https://trac.webkit.org/changeset/209640

[Brady Eidson]

Created attachment 298630 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:872:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:872:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:875:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:875:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 2 files


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

[Tim Horton]

Comment on attachment 298630 [details]
Patch

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

> Source/WebKit2/UIProcess/API/mac/WKView.mm:877
> +                    completionHandler([loadCompletionHandler = Block_copy(loadCompletionHandler)](API::Data* data, WebKit::CallbackBase::Error error) {

Plz to BlockPtr.

[Brady Eidson]

Created attachment 298636 [details]
Patch

[Brady Eidson]

Running EWS before landing

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:873:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:873:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:876:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:876:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:878:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 5 in 2 files


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

[Brady Eidson]

Created attachment 298654 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:873:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:873:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:876:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:876:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:878:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 5 in 2 files


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

[Brady Eidson]

Created attachment 298659 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:874:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:874:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:877:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:877:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:879:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 5 in 2 files


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

[Brady Eidson]

https://trac.webkit.org/changeset/210624

[Darin Adler]

Comment on attachment 298659 [details]
Patch

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

> Source/WebKit2/UIProcess/API/mac/WKView.mm:868
> +            return sel_registerName("_shouldLoadIconWithParameters:completionHandler:");

Doesn’t seem good to keep calling sel_registerName every time this function is called. On a related note, why not use @selector instead of an explicit call to set_registerName?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:872
> +        typedef void (^IconLoadCompletionHandler)(NSData*);

In new code, I suggest using instead of typedef. Also, coding style document says we should use a space between NSData and the *.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:875
> +            RetainPtr<_WKLinkIconParameters> parameters = adoptNS([[_WKLinkIconParameters alloc] _initWithLinkIcon:linkIcon]);

This is an example of a place where auto looks much better to me than writing out RetainPtr<xxx>.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:877
> +            [m_wkView performSelector:delegateSelector() withObject:parameters.get() withObject:^void (IconLoadCompletionHandler loadCompletionHandler) {

Is the ".get() needed here?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:890
> +        WKView *m_wkView;

What guarantees this pointer is not used after the WKView has been deallocated?