Bug 233676

Summary: [WinCairo] Tiling scroll support
Product: WebKit Reporter: Fujii Hironori <Hironori.Fujii>
Component: Tools / TestsAssignee: Fujii Hironori <Hironori.Fujii>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, cmarcelo, don.olmstead, ews-watchlist, gyuyoung.kim, kondapallykalyan, luiz, ryuan.choi, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 234290, 235088    
Bug Blocks:    
Attachments:
Description Flags
WIP patch
none
WIP patch
none
WIP patch
none
Patch
none
Patch for landing none

Description Fujii Hironori 2021-11-30 19:30:49 PST
[WinCairo] Tiling scroll support
Comment 1 Fujii Hironori 2021-11-30 19:31:15 PST
Created attachment 445503 [details]
WIP patch
Comment 2 Fujii Hironori 2021-12-13 17:54:42 PST
Created attachment 447091 [details]
WIP patch
Comment 3 Fujii Hironori 2022-01-10 23:27:22 PST
Created attachment 448832 [details]
WIP patch
Comment 4 Fujii Hironori 2022-01-11 16:15:00 PST
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
Comment 5 Fujii Hironori 2022-01-11 20:25:50 PST
Created attachment 448901 [details]
Patch
Comment 6 Don Olmstead 2022-01-12 17:27:48 PST
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?
Comment 7 Fujii Hironori 2022-01-12 17:54:12 PST
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.
Comment 8 Fujii Hironori 2022-01-12 18:07:14 PST
Created attachment 449017 [details]
Patch for landing
Comment 9 Fujii Hironori 2022-01-12 18:17:27 PST
Comment on attachment 449017 [details]
Patch for landing

Clearing flags on attachment: 449017

Committed r287967 (245996@trunk): <https://commits.webkit.org/245996@trunk>
Comment 10 Fujii Hironori 2022-01-12 18:17:32 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2022-01-12 18:18:17 PST
<rdar://problem/87524879>