Factor out find bounce layer
Created attachment 428078 [details] Patch
Comment on attachment 428078 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428078&action=review > Source/WebCore/page/cocoa/WebTextIndicatorLayer.h:35 > + RefPtr<WebCore::TextIndicator> _textIndicator; Nit - it looks like this could be a Ref<WebCore::TextIndicator> instead? > Source/WebCore/page/cocoa/WebTextIndicatorLayer.mm:167 > + RetainPtr<CGColorRef> rimShadowColor = CGColorCreateGenericGray(0, 0.35); > + RetainPtr<CGColorRef> dropShadowColor = CGColorCreateGenericGray(0, 0.2); > + RetainPtr<CGColorRef> borderColor = CGColorCreateSRGB(0.96, 0.9, 0, 1); adoptCF() here to fix the leaks. > Source/WebCore/page/cocoa/WebTextIndicatorLayer.mm:171 > + highlightColor = CGColorCreateSRGB(.99, .89, 0.22, 1.0); (This leaks too)
(In reply to Wenson Hsieh from comment #2) > Comment on attachment 428078 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428078&action=review > > > Source/WebCore/page/cocoa/WebTextIndicatorLayer.h:35 > > + RefPtr<WebCore::TextIndicator> _textIndicator; > > Nit - it looks like this could be a Ref<WebCore::TextIndicator> instead? (Never mind — as you discovered, we can't use Ref because ivars in ObjC++ must be default constructible :P) > > > Source/WebCore/page/cocoa/WebTextIndicatorLayer.mm:167 > > + RetainPtr<CGColorRef> rimShadowColor = CGColorCreateGenericGray(0, 0.35); > > + RetainPtr<CGColorRef> dropShadowColor = CGColorCreateGenericGray(0, 0.2); > > + RetainPtr<CGColorRef> borderColor = CGColorCreateSRGB(0.96, 0.9, 0, 1); > > adoptCF() here to fix the leaks. > > > Source/WebCore/page/cocoa/WebTextIndicatorLayer.mm:171 > > + highlightColor = CGColorCreateSRGB(.99, .89, 0.22, 1.0); > > (This leaks too)
Created attachment 428110 [details] Patch
Comment on attachment 428110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428110&action=review > Source/WebCore/ChangeLog:3 > + Factor out find bounce layer This really undersells the change. It's not just "factor it out" but really "reimplement it in terms of CALayer to make it cross-platform" > Source/WebCore/ChangeLog:9 > + compatable with iOS. sp: Compatible > Source/WebCore/ChangeLog:11 > + No behavior change. Please make sure you test find in both modern WebKit and WebKitLegacy, since there are precisely 0 automated tests for TextIndicatorWindow+friends. > Source/WebCore/page/cocoa/WebTextIndicatorLayer.mm:163 > + Weird extraneous newline > Source/WebCore/page/cocoa/WebTextIndicatorLayer.mm:165 > + RetainPtr<CGColorRef> rimShadowColor =adoptCF(CGColorCreateGenericGray(0, 0.35)); Missing a space after the = > Source/WebCore/page/mac/TextIndicatorWindow.mm:119 > + [m_textIndicatorView setWantsLayer:YES]; > + [m_textIndicatorView setLayer:m_textIndicatorLayer.get()]; IIRC the order of these is important. The documentation says that you want them in the opposite order to make a layer-hosting view, which I think is the mode you want? Take a gander at the discussion section in https://developer.apple.com/documentation/appkit/nsview/1483695-wantslayer?language=objc
(In reply to Tim Horton from comment #5) > Comment on attachment 428110 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428110&action=review > > > Source/WebCore/ChangeLog:3 > > + Factor out find bounce layer > > This really undersells the change. It's not just "factor it out" but really > "reimplement it in terms of CALayer to make it cross-platform" > > > Source/WebCore/ChangeLog:9 > > + compatable with iOS. > > sp: Compatible > > > Source/WebCore/ChangeLog:11 > > + No behavior change. > > Please make sure you test find in both modern WebKit and WebKitLegacy, since > there are precisely 0 automated tests for TextIndicatorWindow+friends. Not just find: test Lookup! (with force-touch trackpad, or three-finger tap, or right-click on a selection; they all use different animations)
Comment on attachment 428110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428110&action=review Some style tweaks you could consider: not required > Source/WebCore/page/cocoa/WebTextIndicatorLayer.mm:57 > +const CFTimeInterval bounceAnimationDuration = 0.12; > +const CFTimeInterval bounceWithCrossfadeAnimationDuration = 0.3; > +const CFTimeInterval fadeInAnimationDuration = 0.15; > +const CFTimeInterval timeBeforeFadeStarts = bounceAnimationDuration + 0.2; > +const CFTimeInterval fadeOutAnimationDuration = 0.3; > + > +const CGFloat midBounceScale = 1.25; > +const CGFloat borderWidth = 0; > +const CGFloat cornerRadius = 0; > +const CGFloat dropShadowOffsetX = 0; > +const CGFloat dropShadowOffsetY = 1; > +const CGFloat dropShadowBlurRadius = 2; > +const CGFloat rimShadowBlurRadius = 1; > + > +NSString *textLayerKey = @"TextLayer"; > +NSString *dropShadowLayerKey = @"DropShadowLayer"; > +NSString *rimShadowLayerKey = @"RimShadowLayer"; We should use constexpr for all of these. > Source/WebCore/page/cocoa/WebTextIndicatorLayer.mm:59 > +using namespace WebCore; We should avoid this, and instead write the WebCore:: prefix where needed. > Source/WebCore/page/cocoa/WebTextIndicatorLayer.mm:65 > +#if PLATFORM(MAC) > +OBJC_CLASS NSColor; > +using CocoaColor = NSColor; > +#endif Should get this from the CocoaColor.h header. Since we need this in WebCore, should move that header from WebKit to WebCore. > Source/WebCore/page/cocoa/WebTextIndicatorLayer.mm:161 > + RetainPtr<NSMutableArray> bounceLayers = adoptNS([[NSMutableArray alloc] init]); We should use auto here. > Source/WebCore/page/cocoa/WebTextIndicatorLayer.mm:167 > + RetainPtr<CGColorRef> rimShadowColor =adoptCF(CGColorCreateGenericGray(0, 0.35)); > + RetainPtr<CGColorRef> dropShadowColor = adoptCF(CGColorCreateGenericGray(0, 0.2)); > + RetainPtr<CGColorRef> borderColor = adoptCF(CGColorCreateSRGB(0.96, 0.9, 0, 1)); We should use auto for these. Also there is a missing space after the "=" in the rimShadowColor line. > Source/WebCore/page/cocoa/WebTextIndicatorLayer.mm:174 > + Vector<FloatRect> textRectsInBoundingRectCoordinates = _textIndicator->textRectsInBoundingRectCoordinates(); auto > Source/WebCore/page/cocoa/WebTextIndicatorLayer.mm:176 > + Vector<Path> paths = PathUtilities::pathsWithShrinkWrappedRects(textRectsInBoundingRectCoordinates, cornerRadius); auto
Created attachment 428287 [details] Patch for landing
Committed r277330 (237589@main): <https://commits.webkit.org/237589@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 428287 [details].
<rdar://problem/77853815>