Bug 222014 – Change App Highlight delegate to pass more information about the highlight for display.

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Change App Highlight delegate to pass more information about the highlight for display.

[Megan Gardner]

Created attachment 420570 [details]
Patch

[Tim Horton]

Comment on attachment 420570 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKAppHighlight.h:37
> +- (instancetype) init NS_UNAVAILABLE;

)init

> Source/WebKit/UIProcess/API/Cocoa/_WKAppHighlight.h:39
> +@property (nonatomic, readonly, copy) NSData *data;

I think just "data" is a bit mysterious. This is the thing that you pass to "restore", right? I think we can come up with a name.

> Source/WebKit/UIProcess/API/Cocoa/_WKAppHighlight.h:49
> +@property (nonatomic, readonly) BOOL newGroup;

This one doesn't make sense here and should be the second parameter on the method, I think

> Source/WebKit/UIProcess/API/Cocoa/_WKAppHighlight.mm:2
> + * Copyright (C) 20201 Apple Inc. All rights reserved.

Wow that's a distant future you've got there

> Source/WebKit/UIProcess/API/Cocoa/_WKAppHighlightDelegate.h:36
> +- (void)_webView:(WKWebView *)webView storeAppHighlight:(_WKAppHighlight *)highlight WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

I propose: _webView:didCreateHighlight:creatingNewGroup:

[Tim Horton]

> _webView:didCreateHighlight:creatingNewGroup:

or ... _webView:didCreateHighlight:inNewGroup:?

[Megan Gardner]

Comment on attachment 420570 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/_WKAppHighlight.h:39
>> +@property (nonatomic, readonly, copy) NSData *data;
> 
> I think just "data" is a bit mysterious. This is the thing that you pass to "restore", right? I think we can come up with a name.

I actually call it 'highlight' in much of the previous patch, but if we come up with an even better name, I'll change it everywhere.

[Wenson Hsieh]

Comment on attachment 420570 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1432
> +    _WKAppHighlight *wkHighlight = [[_WKAppHighlight alloc] initWithData:highlight.highlight->createNSData().get() text:text image:nil createNewGroup:highlight.isNewGroup == WebCore::CreateNewGroupForHighlight::Yes ? YES : NO];

It looks like this will leak.

[Megan Gardner]

Created attachment 420578 [details]
Patch

[Megan Gardner]

Created attachment 420586 [details]
Patch

[Tim Horton]

Comment on attachment 420586 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKAppHighlight.mm:39
> +    _text = text;

Where'd the image go

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtension.mm:29
> +

Too many newlines

[Megan Gardner]

Created attachment 420611 [details]
Patch

[Megan Gardner]

Created attachment 420614 [details]
Patch

[Tim Horton]

Comment on attachment 420614 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1434
> +    if ([delegate respondsToSelector:@selector(_webView:storeAppHighlight:inNewGroup:)])

You should probably just bail at the top, none of this work is worth anything if you're not going to call the delegate.

> Source/WebKit/UIProcess/API/Cocoa/_WKAppHighlight.h:41
> +@property (nonatomic, readonly, copy) NSString * text;

* is a-floating

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtension.mm:29
> +

This extraneous newline is still here

[Tim Horton]

Comment on attachment 420614 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKAppHighlight.mm:39
> +    _text = text;

Where'd the image go? I think you missed my last set of comments

[Megan Gardner]

Created attachment 420619 [details]
Patch

[Megan Gardner]

Created attachment 420655 [details]
Patch for landing

[EWS]

Committed r273013: <https://commits.webkit.org/r273013>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420655 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74440009>