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

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied 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 158077: Patch
https://bugs.webkit.org/attachment.cgi?id=158077&action=review

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


Thanks for adding the tests, although it seems like a number of them do not
pass.  I'm confused by the static-ness of m_linkHighlightGlobal.

I'm also not quite sure by what you mean by "triggering compositor layer
changes in layout tests via JavaScript" - do you mean that when the final
display is made the layer tree doesn't reflect what you expect it to?  You can
try doing a document.body.offsetTop before calling
layoutTestController.display() if you are forcing manual painting to flush
pending layout.  We should do a layout at the end of the test currently.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:83
> +LinkHighlightClient* GraphicsLayerChromium::m_linkHighlightGlobal = 0;

statics are named s_foo, since they aren't member variables.  This is quite
gross - are you sure we need it?  I understand that the link highlight is
global per view, but is it also global across multiple WebViews?

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:177
> +    static LinkHighlightClient* m_linkHighlightGlobal;
> +    bool m_ownsLinkHighlight;

Could you explain why there's this setup instead of having a member variable
m_linkHighlight that is null when this GLC does not have ownership of the link
highlight?

> Source/WebKit/chromium/src/WebViewImpl.cpp:531
> +    // The following block is development code only! DO NOT COMMIT!!
> +    if (event.button == WebMouseEvent::ButtonMiddle) {
> +	   enableTouchHighlight(IntPoint(event.x, event.y));
> +	   return;
> +    }

Remove this

> Source/WebKit/chromium/src/WebViewImpl.cpp:742
> +#if OS(LINUX)

OS(LINUX) is almost certainly wrong here - are there even any other platforms
that send GestureTapDown right now?

> Source/WebKit/chromium/src/WebViewImpl.cpp:749
> +	   // Preempt any unstarted highlight animations, then fall through to
regular handler.

This also looks wrong - if we start a scroll on the compositor thread then the
main thread will never receive a GestureScrollBegin and the highlight will stay
active, won't it?

> LayoutTests/platform/chromium/TestExpectations:3484
> +BUGWKxxxx1 LINUX : gesture-tapHighlight-1-overflow-div-scroll-clip.html =
IMAGE

why are you marking these failing on linux? where are your baselines from?

you need to specify the full path if you want to actually suppress image diffs
on them