[webkit-reviews] review granted: [Bug 84487] [chromium] Add touch link highlight animation layers. : [Attachment 159477] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 21 15:06:15 PDT 2012


James Robinson <jamesr at chromium.org> has granted W. James MacLean
<wjmaclean at chromium.org>'s request for review:
Bug 84487: [chromium] Add touch link highlight animation layers.
https://bugs.webkit.org/show_bug.cgi?id=84487

Attachment 159477: Patch
https://bugs.webkit.org/attachment.cgi?id=159477&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=159477&action=review


R=me

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:126
> +	   // If we have an active link highlight, it may need to transfer to a
new GraphicsLayer,
> +	   // so we need to give it a chance to update its layer affiliation.
> +	   m_linkHighlight->invalidate();

this comment is misleading - the linkHighlight doesn't update its layer
affiliation in the invalidate() call.

can you just make clearCurrent...() set m_geometryNeedsUpdate on the
LinkHighlight? if the GraphicsLayerChromium the highlight is attached to goes
away then it definitely needs to find a new owner

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:800
> +	   // This is the only place where the global copy of the
LinkHighlightClient pointer
> +	   // is dereferenced. GraphicsLayerChromium::updateLayerIsDrawable()
may be called before
> +	   // this layer is added into the tree, so we need a way to tell the
link highlight to
> +	   // re-check its layer affiliation.

this comment is still out of date. we don't have a global copy of
m_linkHighlight, we have one per WebViewImpl.

I think putting this code in updateLayerIsDrawable() is super sketchy - you're
relying on this being called whenever we enter the tree which is true
currently, but not necessarily always going to be the case. I think with the
other fixes you can get rid of this now.

> Source/WebKit/chromium/tests/data/test_touch_link_highlight.html:1
> +<html>

<!DOCTYPE html> first, please. all our tests should be in standards mode unless
they are testing quirks behavior

>
LayoutTests/platform/chromium-linux/compositing/gestures/gesture-tapHighlight-2
-iframe-scrolled-inner.html:7
> +<iframe id="nestingFrame" src="resources/1-nested-frame-noncomposited.html"
style="position : relative; left : 10px; top : 10px; width : 450px; height :
150px;"></iframe>

nit: no space between the property name and the :. i.e. "position: relative"
not "position : relative"

>
LayoutTests/platform/chromium-linux/compositing/gestures/resources/1-frame-nonc
omposited.html:1
> +<html>

<!DOCTYPE html> on these too

> LayoutTests/platform/chromium/TestExpectations:3510
> +BUGWK94357 LINUX :
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-overflow-di
v-scroll-clip.html = IMAGE

why are these marked as expected failures? you've added baselines that look
approximately correct to me. we won't be able to tell if they regress if they
are marked as expected failures.

if they aren't right, why are you checking in baselines at all?


More information about the webkit-reviews mailing list