Bug 85084 – [chromium] Create LinkHighlightLayerChromium class to provide link-highlight preview animations for GraphicsLayerChromium.

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

[W. James MacLean]

[chromium] Create LinkHighlightLayerChromium class to provide link-highlight preview animations for GraphicsLayerChromium.

[W. James MacLean]

Created attachment 139239 [details]
Patch

[Adrienne Walker]

Comment on attachment 139239 [details]
Patch

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

I like this in general.  It seems a little bit wasteful of texture memory to use a content layer for a link highlight, but on the other hand we won't have many of them at once, they're small, and it's so much simpler to just draw the path into a GraphicsContext then to try to draw it behind and not use texture memory.

> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerChromium.cpp:76
> +    OwnPtr<CCActiveAnimation> animation(CCActiveAnimation::create(curve.release(), ++id, 0, CCActiveAnimation::Opacity));

How do the animation ids here work with other ids that are potentially conflicting in the animation system? If somebody removed an animation in css by name that mapped to a conflicting id, it looks like both would get removed by CCLayerAnimationController::removeAnimation.

> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerChromium.cpp:114
> +    if (m_parent)
> +        static_cast<GraphicsLayerChromium*>(m_parent)->didFinishLinkHighlightLayer();

I think you should just use GraphicsLayerChromium as the type here all the way through and not static cast.  This is a LinkHilighterLayerChromium, so it's not like other GraphicsLayer types will work here.

> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerChromium.h:38
> +class LinkHighlightLayerChromium : public RefCounted<LinkHighlightLayerChromium>, public ContentLayerDelegate, public CCLayerAnimationDelegate {

It's really confusing to suffix this with "LayerChromium" when it's really a layer delegate and not actually a layer itself.  Even LinkHighlighter on its own would be clearer to me.

[W. James MacLean]

Comment on attachment 139239 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerChromium.cpp:76
>> +    OwnPtr<CCActiveAnimation> animation(CCActiveAnimation::create(curve.release(), ++id, 0, CCActiveAnimation::Opacity));
> 
> How do the animation ids here work with other ids that are potentially conflicting in the animation system? If somebody removed an animation in css by name that mapped to a conflicting id, it looks like both would get removed by CCLayerAnimationController::removeAnimation.

I will check with Ian and fix this. Perhaps the static animation/group ids in GLC.cpp could be used?

>> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerChromium.cpp:114
>> +        static_cast<GraphicsLayerChromium*>(m_parent)->didFinishLinkHighlightLayer();
> 
> I think you should just use GraphicsLayerChromium as the type here all the way through and not static cast.  This is a LinkHilighterLayerChromium, so it's not like other GraphicsLayer types will work here.

OK, will do!

>> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerChromium.h:38
>> +class LinkHighlightLayerChromium : public RefCounted<LinkHighlightLayerChromium>, public ContentLayerDelegate, public CCLayerAnimationDelegate {
> 
> It's really confusing to suffix this with "LayerChromium" when it's really a layer delegate and not actually a layer itself.  Even LinkHighlighter on its own would be clearer to me.

Oops, that's a legacy thing. I will revise in the new patch. To keep the name shorter, I'll use "LinkHighlightLayerDelegate" without the Chromium.

[W. James MacLean]

Created attachment 139491 [details]
Patch

[W. James MacLean]

(In reply to comment #4)
> Created an attachment (id=139491) [details]
> Patch

I spoke to Ian, and he says it's OK to "reserve" an animation id. To avoid conflicts, I've encapsulated doling out animation/group ids in a function that centralizes any special rules, like "reservations".

I think I've deal with all the other items ... let me know if I misunderstood on any of them.

(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=139491) [details] [details]
> > Patch
> 
> I spoke to Ian, and he says it's OK to "reserve" an animation id. To avoid conflicts, I've encapsulated doling out animation/group ids in a function that centralizes any special rules, like "reservations".
> 
> I think I've deal with all the other items ... let me know if I misunderstood on any of them.

Thank you for wrapping up the code for incrementing group id and animation id - that section looks good to me.

[W. James MacLean]

Created attachment 139524 [details]
Patch

[W. James MacLean]

(In reply to comment #7)
> Created an attachment (id=139524) [details]
> Patch

Rebased so patch will apply.

[Adrienne Walker]

Comment on attachment 139524 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:67
>  static int s_nextGroupId = 1;
> -static int s_nextAnimationId = 1;
> +static int s_nextAnimationId = 2;

Whoa, magic numbers.  Is it possible to put an enum for animations (e.g. { DontUse, LinkHighlight, FirstAvailable }) on an animation class like CCActiveAnimation? That'd make it easier to reserve more numbers in the future if needed.

> Source/WebKit/chromium/tests/LinkHighlightLayerDelegateTest.cpp:80
> +    // Do we need to give it a contents layer?
> +

Can you verify this and then remove this comment if so?

[W. James MacLean]

Created attachment 140530 [details]
Patch

[W. James MacLean]

(In reply to comment #9)
> (From update of attachment 139524 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139524&action=review
> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:67
> >  static int s_nextGroupId = 1;
> > -static int s_nextAnimationId = 1;
> > +static int s_nextAnimationId = 2;
> 
> Whoa, magic numbers.  Is it possible to put an enum for animations (e.g. { DontUse, LinkHighlight, FirstAvailable }) on an animation class like CCActiveAnimation? That'd make it easier to reserve more numbers in the future if needed.

Done. I put it into it's own class on Ian's suggestion, to try and keep CCActiveAnimation free from this sort of implementation detail.

> > Source/WebKit/chromium/tests/LinkHighlightLayerDelegateTest.cpp:80
> > +    // Do we need to give it a contents layer?
> > +
> 
> Can you verify this and then remove this comment if so?

Done (the comment was out of date).

[Adrienne Walker]

Comment on attachment 140530 [details]
Patch

Thanks! R=me.

[WebKit Review Bot]

Comment on attachment 140530 [details]
Patch

Clearing flags on attachment: 140530

Committed r116334: <http://trac.webkit.org/changeset/116334>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 85816

[W. James MacLean]

Created attachment 140596 [details]
Patch for landing

[James Robinson]

Comment on attachment 140596 [details]
Patch for landing

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

Sorry to nit on your already-nitted patch but I'm a bit worried about the LayerAnimationDelegate pointer - it's a weak pointer and nobody is clearing it.

> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerDelegate.cpp:61
> +    m_contentLayer->setLayerAnimationDelegate(this);

who is clearing this? this is cleared in GLC::willBeDestroyed() for other types but nothing here nulls it out that I can see.

> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerDelegate.cpp:91
> +void LinkHighlightLayerDelegate::paintContents(GraphicsContext& gc, const IntRect& clipRect)

if you aren't going to use the parameter the omit the name.  same goes for functions below

I think this will cause compile errors on some systems due to unused parameter warnings

> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerDelegate.h:40
> +class LinkHighlightLayerDelegate : public RefCounted<LinkHighlightLayerDelegate>, public ContentLayerDelegate, public CCLayerAnimationDelegate {

this is a bad name for a concrete class - a delegate is something that concrete classes implement, it isn't itself a concrete class. why isn't this just LinkHighlight?

> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerDelegate.h:45
> +    ContentLayerChromium* getContentLayer();

in WebKit simple getters are named as the member variable, not with a "get" prefix

> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerDelegate.h:48
> +    virtual void paintContents(GraphicsContext&, const IntRect& clipRect);

OVERRIDE, please

> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerDelegate.h:52
> +    virtual void notifyAnimationStarted(double time);
> +    virtual void notifyAnimationFinished(double time);

OVERRIDEs, please

[W. James MacLean]

Comment on attachment 140596 [details]
Patch for landing

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

>> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerDelegate.cpp:61
>> +    m_contentLayer->setLayerAnimationDelegate(this);
> 
> who is clearing this? this is cleared in GLC::willBeDestroyed() for other types but nothing here nulls it out that I can see.

Added to destructor below.

>> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerDelegate.cpp:91
>> +void LinkHighlightLayerDelegate::paintContents(GraphicsContext& gc, const IntRect& clipRect)
> 
> if you aren't going to use the parameter the omit the name.  same goes for functions below
> 
> I think this will cause compile errors on some systems due to unused parameter warnings

Good idea ... done.

>> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerDelegate.h:40
>> +class LinkHighlightLayerDelegate : public RefCounted<LinkHighlightLayerDelegate>, public ContentLayerDelegate, public CCLayerAnimationDelegate {
> 
> this is a bad name for a concrete class - a delegate is something that concrete classes implement, it isn't itself a concrete class. why isn't this just LinkHighlight?

I would have thought that the concrete class was the true delegate, as it is the one capable of doing the work delegated to it. That being said, renamed.

>> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerDelegate.h:45
>> +    ContentLayerChromium* getContentLayer();
> 
> in WebKit simple getters are named as the member variable, not with a "get" prefix

Fixed.

>> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerDelegate.h:48
>> +    virtual void paintContents(GraphicsContext&, const IntRect& clipRect);
> 
> OVERRIDE, please

Done.

>> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerDelegate.h:52
>> +    virtual void notifyAnimationFinished(double time);
> 
> OVERRIDEs, please

Done (and recorded for my checklist on future CLs).

[W. James MacLean]

Created attachment 140716 [details]
Patch

[James Robinson]

Comment on attachment 140716 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LinkHighlight.h:43
> +    virtual  ~LinkHighlight();

extra space between "virtual" and "~LinkHighlight"

> Source/WebKit/chromium/tests/LinkHighlightTest.cpp:44
> +

extra newline

> Source/WebKit/chromium/tests/LinkHighlightTest.cpp:49
> +    virtual void notifyAnimationStarted(const GraphicsLayer*, double time) { }
> +    virtual void notifySyncRequired(const GraphicsLayer*) { }
> +    virtual void paintContents(const GraphicsLayer*, GraphicsContext&, GraphicsLayerPaintingPhase, const IntRect& inClip) { }
> +    virtual bool showDebugBorders(const GraphicsLayer*) const { return false; }
> +    virtual bool showRepaintCounter(const GraphicsLayer*) const { return false; }

OVERRIDE please?

[W. James MacLean]

Created attachment 140748 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 140748 [details]
Patch for landing

Clearing flags on attachment: 140748

Committed r116456: <http://trac.webkit.org/changeset/116456>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Rohde Christiansen]

Comment on attachment 140748 [details]
Patch for landing

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

> Source/WebKit/chromium/tests/LinkHighlightTest.cpp:83
> +    Path highlightPath;
> +    highlightPath.addRect(FloatRect(5, 5, 10, 8));
> +
> +    // Neither of the following operations should crash.
> +    graphicsLayer->addLinkHighlight(highlightPath);

FYI, Qt is having some code in the TapHighlighter class (or similar) which creates nice paths for highlighting. Maybe we can share code :-)

[W. James MacLean]

(In reply to comment #24)
> (From update of attachment 140748 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140748&action=review
> 
> > Source/WebKit/chromium/tests/LinkHighlightTest.cpp:83
> > +    Path highlightPath;
> > +    highlightPath.addRect(FloatRect(5, 5, 10, 8));
> > +
> > +    // Neither of the following operations should crash.
> > +    graphicsLayer->addLinkHighlight(highlightPath);
> 
> FYI, Qt is having some code in the TapHighlighter class (or similar) which creates nice paths for highlighting. Maybe we can share code :-)

Certainly ... where is the code for creating paths located?

[Allan Sandfeld Jensen]

(In reply to comment #25)
> (In reply to comment #24)
> > (From update of attachment 140748 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=140748&action=review
> > 
> > > Source/WebKit/chromium/tests/LinkHighlightTest.cpp:83
> > > +    Path highlightPath;
> > > +    highlightPath.addRect(FloatRect(5, 5, 10, 8));
> > > +
> > > +    // Neither of the following operations should crash.
> > > +    graphicsLayer->addLinkHighlight(highlightPath);
> > 
> > FYI, Qt is having some code in the TapHighlighter class (or similar) which creates nice paths for highlighting. Maybe we can share code :-)
> 
> Certainly ... where is the code for creating paths located?

Source/WebCore/page/GestureTapHighlighter