Bug 169062 – Data interaction support for WK1

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Data interaction support for WK1

[Megan Gardner]

Created attachment 303138 [details]
Patch

[Wenson Hsieh]

Comment on attachment 303138 [details]
Patch

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

> Source/WebKit/mac/WebCoreSupport/WebDragClient.mm:64
> +

Nit - extra newline here.

> Source/WebKit/mac/WebView/WebView.mm:623
> +    CGRect _selectionRectInRootViewCoordinates;

Can we @synthesize these properties instead of making these all ivars?

> Source/WebKit/mac/WebView/WebView.mm:642
> +    for (FloatRect rect : indicatorData.textRectsInBoundingRectCoordinates)

Nit - auto rect here.

> Source/WebKit/mac/WebView/WebViewData.h:289
> +    

Nit - extra newline here.

> Source/WebKit/mac/WebView/WebViewInternal.h:259
> +- (void)_setDataInteractionData:(CGImageRef)image textIndicator:(WebCore::TextIndicatorData&)textIndicator atClientPosition:(CGPoint)clientPosition anchorPoint:(CGPoint)anchorPoint action:(uint64_t)action;

I think this should be an std::optional, or a RefPtr, or just a raw pointer if the first two are not available. We plumb an optional text indicator here, if I understand correctly.

[Megan Gardner]

Created attachment 303152 [details]
Patch

[Megan Gardner]

Created attachment 303154 [details]
Patch

[Wenson Hsieh]

Comment on attachment 303154 [details]
Patch

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

r=me, with some minor nits. You may need a WebKit2 reviewer (Tim, Beth, Simon, Dan...) to rubber stamp the tiny WK2 change though.

> Source/WebKit/mac/WebView/WebView.mm:1789
> +        _private->textIndicatorData = [[WebUITextIndicatorData alloc] initWithImage:image TextIndicatorData:indicatorData.value() scale:_private->page->deviceScaleFactor()];

Nit - According to https://webkit.org/code-style-guidelines, "each piece of the selector should start with a lowercase letter and use intercaps." -- IMO we should make this textIndicatorData, and also just pass nil to avoid having two different initializers.

> Source/WebKit/mac/WebView/WebView.mm:1810
> +#endif // ENABLE(DATA_INTERACTION)

Nit - should be // ENABLE(DATA_INTERACTION) && defined(__cplusplus)

> Source/WebKit/mac/WebView/WebViewData.h:288
> +    WebUITextIndicatorData *textIndicatorData;

This should be a RetainPtr, and we should use adoptNS() when we init it.

> Source/WebKit/mac/WebView/WebViewPrivate.h:193
> +

Nit - extra newline

[Megan Gardner]

Created attachment 303155 [details]
Patch

[Megan Gardner]

Created attachment 303161 [details]
Patch

[Wenson Hsieh]

Comment on attachment 303161 [details]
Patch

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

> Source/WebKit2/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=169062

It would be great to include the rdar link here.

[Megan Gardner]

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