Bug 225557 – Factor out text indicator bounce layer to be cross-platform

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Factor out find bounce layer

[Megan Gardner]

Created attachment 428078 [details]
Patch

[Wenson Hsieh]

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)

[Wenson Hsieh]

(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)

[Megan Gardner]

Created attachment 428110 [details]
Patch

[Tim Horton]

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

[Tim Horton]

(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)

[Darin Adler]

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

[Megan Gardner]

Created attachment 428287 [details]
Patch for landing

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/77853815>