Summary: | Data interaction support for WK1 | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Megan Gardner <megan_gardner> | ||||||||||||
Component: | New Bugs | Assignee: | Megan Gardner <megan_gardner> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bdakin, thorton, wenson_hsieh | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Megan Gardner
2017-03-01 15:49:52 PST
Created attachment 303138 [details]
Patch
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. Created attachment 303152 [details]
Patch
Created attachment 303154 [details]
Patch
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 Created attachment 303155 [details]
Patch
Created attachment 303161 [details]
Patch
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. |