[webkit-reviews] review denied: [Bug 68535] [chromium] Revise zoom animator backend to use full transform instead of just scale. : [Attachment 108173] 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 68535: [chromium] Revise zoom animator backend to use full transform
instead of just scale.
https://bugs.webkit.org/show_bug.cgi?id=68535

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

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


> Source/WebCore/page/Settings.h:612
> +	   double m_zoomAnimatorPosX;
> +	   double m_zoomAnimatorPosY;

do you really need double precision? could this be an IntPoint (or FloatPoint
if you need floating point values)

it's also unfortunate that this is growing into a bag of properties. Is there
any reasonable way to consolidate these?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:201
> +	       const GC3Dint filter = (m_tiler->hasBorderTexels() ||
!layerTreeHost()->isZoomTransformIdentity()) ? GraphicsContext3D::LINEAR :
GraphicsContext3D::NEAREST;

This seems very wrong.	If we are going to filter and apply a scale to tiled
content then we have to have border texels or we'll generate rendering
artifacts. It looks like this logic is attempting to apply filtering without
uploading border texels which will not work.

It seems like in order for this to work you need to decide ahead of time
whether you want to apply this animation and provide border texels for the
root, or perhaps just always apply border texels on the root layer tiles and
change the other logic that keys off of "do I have border texels?" as a proxy
for "am I the root layer?" to be more explicit.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2580
> +	   zoomMatrix.translate3d(m_page->settings()->zoomAnimatorPosX(),
m_page->settings()->zoomAnimatorPosY(), 0);

if you are doing a 2d translation then use translate(), don't use
translate3d(..., ..., 0). the latter is less clear a the callsight and will do
extra unnecessary math

> Source/WebKit/chromium/src/WebViewImpl.cpp:2581
> +	   zoomMatrix.scale3d(m_page->settings()->zoomAnimatorScale(),
m_page->settings()->zoomAnimatorScale(), 1);

you want .scale() here.