[webkit-reviews] review granted: [Bug 233676] [WinCairo] Tiling scroll support : [Attachment 448901] Patch

[bugzilla-daemon at webkit.org]

Don Olmstead <don.olmstead at sony.com> has granted Fujii Hironori
<Hironori.Fujii at sony.com>'s request for review:
Bug 233676: [WinCairo] Tiling scroll support
https://bugs.webkit.org/show_bug.cgi?id=233676

Attachment 448901: Patch

https://bugs.webkit.org/attachment.cgi?id=448901&action=review




--- Comment #6 from Don Olmstead <don.olmstead at sony.com> ---
Comment on attachment 448901
  --> https://bugs.webkit.org/attachment.cgi?id=448901
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448901&action=review

LGTM. Just some minor questions and nits.

> Source/WebCore/platform/graphics/texmap/TextureMapperSparseBackingStore.cpp:8
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.

I don't think this should count as a derivative work because the debug drawing
requires the same calls. To me this seems like you've done a lot more than just
copy paste code here.

> Source/WebCore/platform/graphics/texmap/TextureMapperSparseBackingStore.h:8
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.

Ditto

> Source/WebKit/GPUProcess/graphics/wc/WCScene.cpp:55
> +    std::optional<WebCore::TextureMapperSparseBackingStore> backingStore;

Any particular reason you're preferring std::optional here? I'm not saying its
the wrong choice just curious.

> Source/WebKit/WebProcess/WebPage/wc/GraphicsLayerWC.cpp:558
> +	       update.hasBackingStore = false;

Any additional updates need to happen here to remove the store?

> Source/WebKit/WebProcess/WebPage/wc/GraphicsLayerWC.cpp:631
> +    TransformationMatrix currentTransform;
> +    if (customTransform)
> +	   currentTransform = *customTransform;
> +    else if (m_transform)
> +	   currentTransform = *m_transform;
> +
> +    transform.multiply(transformByApplyingAnchorPoint(currentTransform));

Is it worth doing any optimization here for the else case where
currentTransform would be the identity?