Bug 233676 – [WinCairo] Tiling scroll support

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Fujii Hironori]

[WinCairo] Tiling scroll support

[Fujii Hironori]

Created attachment 445503 [details]
WIP patch

[Fujii Hironori]

Created attachment 447091 [details]
WIP patch

[Fujii Hironori]

Created attachment 448832 [details]
WIP patch

[Fujii Hironori]

This WIP patch doesn't render video frames in AC mode.
Filed: Bug 235088 – [MediaFoundation] Invalidate only the videa area using MediaPlayer::repaint(), not the whole FrameView

[Fujii Hironori]

Created attachment 448901 [details]
Patch

[Don Olmstead]

Comment on attachment 448901 [details]
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?

[Fujii Hironori]

Comment on attachment 448901 [details]
Patch

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

Thank you for the review.

>> Source/WebCore/platform/graphics/texmap/TextureMapperSparseBackingStore.cpp:8
>> + * 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.

Will fix.

>> 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.

TextureMapperTiledBackingStore is a RefCounted class, but TextureMapperSparseBackingStore isn't because it doesn't need to be ref-counted.

>> Source/WebKit/WebProcess/WebPage/wc/GraphicsLayerWC.cpp:558
>> +            update.hasBackingStore = false;
> 
> Any additional updates need to happen here to remove the store?

I don't think so.

>> Source/WebKit/WebProcess/WebPage/wc/GraphicsLayerWC.cpp:631
>> +    transform.multiply(transformByApplyingAnchorPoint(currentTransform));
> 
> Is it worth doing any optimization here for the else case where currentTransform would be the identity?

I just copied the code from GraphicsLayerCA::layerTransform.
I haven't benchmarked it yet. I don't know how hot this part is.
BTW, TransformState::applyTransform has isIntegerTranslation fast path.

[Fujii Hironori]

Created attachment 449017 [details]
Patch for landing

[Fujii Hironori]

Comment on attachment 449017 [details]
Patch for landing

Clearing flags on attachment: 449017

Committed r287967 (245996@trunk): <https://commits.webkit.org/245996@trunk>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/87524879>