[webkit-reviews] review denied: [Bug 82336] Make it possible to use accelerated compositing for page overlay fading : [Attachment 134074] Patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam.rosenthal at nokia.com> has denied Lars Knudsen
<lars.knudsen at nokia.com>'s request for review:
Bug 82336: Make it possible to use accelerated compositing for page overlay
fading
https://bugs.webkit.org/show_bug.cgi?id=82336

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

------- Additional Comments from Noam Rosenthal <noam.rosenthal at nokia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=134074&action=review


The Qt parts are good, but this patch would modify behavior of WebKit2 on CA,
which I'm not comfortable with.

> Source/WebKit2/WebProcess/WebPage/DrawingArea.cpp:57
> +    , m_pageOverlayDrawOpacity(1.0)

I don't want this variable at all. Use layer opacity when LayerTreeHost returns
true, use regular fade in when it doesn't (for now).

>>> Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:109
>>>	 graphicsContext.setCompositeOperation(CompositeCopy);
>> 
>> Make sure that you test this code, hopefully on a Safari build, from what
I'm seeing it might not blend correctly since you are still using the
CompositeCopy mode.
>> This kind of code is easy to break and I wouldn't push this until we know we
don't break their stuff, both in AC and non-AC mode.
> 
> I need to get a mac then - or someone with a mac to try it out.

Testing would be good, but the code should be written in a way that it's
obvious that behavior on CA is not changed. 
The transparency layer should always be (1), and in Qt we use it to change the
overlayLayer and return true

> Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:102
> +	   context.setFillColor(m_color, ColorSpaceSRGB);

Please use the regular fadeIn if setOverlayOpacity returned false.

> Source/WebKit2/WebProcess/WebPage/ca/LayerTreeHostCA.cpp:190
> +void LayerTreeHostCA::setPageOverlayOpacity(float value)
> +{
> +    ASSERT(m_pageOverlayLayer);
> +    m_pageOverlayLayer->setOpacity(value);
> +    scheduleLayerFlush();
> +}

This should return false and do nothing by default. This patch should not
change the CA implementation.