Bug 85084

Created attachment 139239 [details] Patch

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.

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.

Created attachment 139491 [details] Patch

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

Created attachment 139524 [details] Patch

(In reply to comment #7) > Created an attachment (id=139524) [details] > Patch Rebased so patch will apply.

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?

Created attachment 140530 [details] Patch

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

Comment on attachment 140530 [details] Patch Thanks! R=me.

Comment on attachment 140530 [details] Patch Clearing flags on attachment: 140530 Committed r116334: <http://trac.webkit.org/changeset/116334>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 85816

Created attachment 140596 [details] Patch for landing

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

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

Created attachment 140716 [details] Patch

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?

Created attachment 140748 [details] Patch for landing

Comment on attachment 140748 [details] Patch for landing Clearing flags on attachment: 140748 Committed r116456: <http://trac.webkit.org/changeset/116456>

All reviewed patches have been landed. Closing bug.

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

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

(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