Bug 85084 - [chromium] Create LinkHighlightLayerChromium class to provide link-highlight preview animations for GraphicsLayerChromium.
: [chromium] Create LinkHighlightLayerChromium class to provide link-highlight ...
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 85816
: 84487
  Show dependency treegraph
 
Reported: 2012-04-27 11:15 PST by
Modified: 2012-05-11 07:12 PST (History)


Attachments
Patch (19.38 KB, patch)
2012-04-27 11:21 PST, W. James MacLean
no flags Review Patch | Details | Formatted Diff | Diff
Patch (21.41 KB, patch)
2012-04-30 11:35 PST, W. James MacLean
no flags Review Patch | Details | Formatted Diff | Diff
Patch (21.46 KB, patch)
2012-04-30 14:50 PST, W. James MacLean
no flags Review Patch | Details | Formatted Diff | Diff
Patch (26.81 KB, patch)
2012-05-07 08:23 PST, W. James MacLean
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (26.63 KB, patch)
2012-05-07 14:49 PST, W. James MacLean
no flags Review Patch | Details | Formatted Diff | Diff
Patch (26.09 KB, patch)
2012-05-08 07:26 PST, W. James MacLean
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (26.14 KB, patch)
2012-05-08 11:26 PST, W. James MacLean
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-27 11:15:37 PST
[chromium] Create LinkHighlightLayerChromium class to provide link-highlight preview animations for GraphicsLayerChromium.
------- Comment #1 From 2012-04-27 11:21:03 PST -------
Created an attachment (id=139239) [details]
Patch
------- Comment #2 From 2012-04-30 09:59:51 PST -------
(From update of attachment 139239 [details])
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.
------- Comment #3 From 2012-04-30 10:19:49 PST -------
(From update of attachment 139239 [details])
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.
------- Comment #4 From 2012-04-30 11:35:14 PST -------
Created an attachment (id=139491) [details]
Patch
------- Comment #5 From 2012-04-30 11:37:24 PST -------
(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.
------- Comment #6 From 2012-04-30 11:40:50 PST -------
(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=139491) [details] [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.
------- Comment #7 From 2012-04-30 14:50:37 PST -------
Created an attachment (id=139524) [details]
Patch
------- Comment #8 From 2012-04-30 15:03:58 PST -------
(In reply to comment #7)
> Created an attachment (id=139524) [details] [details]
> Patch

Rebased so patch will apply.
------- Comment #9 From 2012-05-04 12:54:19 PST -------
(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.

> 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?
------- Comment #10 From 2012-05-07 08:23:56 PST -------
Created an attachment (id=140530) [details]
Patch
------- Comment #11 From 2012-05-07 08:25:28 PST -------
(In reply to comment #9)
> (From update of attachment 139524 [details] [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).
------- Comment #12 From 2012-05-07 10:57:06 PST -------
(From update of attachment 140530 [details])
Thanks! R=me.
------- Comment #13 From 2012-05-07 11:57:03 PST -------
(From update of attachment 140530 [details])
Clearing flags on attachment: 140530

Committed r116334: <http://trac.webkit.org/changeset/116334>
------- Comment #14 From 2012-05-07 11:57:09 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #15 From 2012-05-07 12:40:19 PST -------
Re-opened since this is blocked by 85816
------- Comment #16 From 2012-05-07 14:49:14 PST -------
Created an attachment (id=140596) [details]
Patch for landing
------- Comment #17 From 2012-05-07 15:01:50 PST -------
(From update of attachment 140596 [details])
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
------- Comment #18 From 2012-05-08 06:49:21 PST -------
(From update of attachment 140596 [details])
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).
------- Comment #19 From 2012-05-08 07:26:59 PST -------
Created an attachment (id=140716) [details]
Patch
------- Comment #20 From 2012-05-08 10:27:23 PST -------
(From update of attachment 140716 [details])
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?
------- Comment #21 From 2012-05-08 11:26:26 PST -------
Created an attachment (id=140748) [details]
Patch for landing
------- Comment #22 From 2012-05-08 14:56:39 PST -------
(From update of attachment 140748 [details])
Clearing flags on attachment: 140748

Committed r116456: <http://trac.webkit.org/changeset/116456>
------- Comment #23 From 2012-05-08 14:56:46 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #24 From 2012-05-11 02:21:10 PST -------
(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 :-)
------- Comment #25 From 2012-05-11 06:29:52 PST -------
(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?
------- Comment #26 From 2012-05-11 07:12:12 PST -------
(In reply to comment #25)
> (In reply to comment #24)
> > (From update of attachment 140748 [details] [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