Bug 246604 – [WinCairo] Use RenderThemeAdwaita instead of RenderThemeWin

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Fujii Hironori]

Created attachment 463020 [details]
WIP patch

[WinCairo] Use RenderThemeAdwaita instead of RenderThemeWin

RenderThemeWin doesn't work with UseGPUProcessForDOMRenderingEnabled.
Let's use RenderThemeAdwaita. Piggy backed on GTK efforts.

[Fujii Hironori]

See also:
  Bug 246264 – [GPU Process] Enable drawing form controls for GPU Process on macOS

[Fujii Hironori]

Created attachment 463051 [details]
Patch

[Fujii Hironori]

Created attachment 463052 [details]
Patch

[Fujii Hironori]

Created attachment 463064 [details]
Patch

[Fujii Hironori]

Created attachment 463066 [details]
Patch

[Darin Adler]

Comment on attachment 463066 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeAdwaita.cpp:241
> +#elif PLATFORM(WIN)

The code below doesn’t look platform-specific. Can we instead just use "#else" here? Or is this actively specific to PLATFORM(WIN)?

> Source/WebCore/rendering/RenderThemeAdwaita.cpp:243
> +    if (!path)

Typically it’s best to check isNull() instead of using ! for WTF::String. But also, maybe FileSystem::readEntireFile already harmlessly fails if passed a null string, so maybe we don’t need this check.

[Don Olmstead]

Comment on attachment 463066 [details]
Patch

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

Also LGTM

> Source/WebCore/platform/adwaita/ScrollbarThemeAdwaita.cpp:330
> +            gboolean warpSlider = FALSE;
> +            g_object_get(gtk_settings_get_default(),
> +                "gtk-primary-button-warps-slider",
> +                &warpSlider, nullptr);
> +            return warpSlider;

I'm not a fan of the variable shadowing here.

>> Source/WebCore/rendering/RenderThemeAdwaita.cpp:241
>> +#elif PLATFORM(WIN)
> 
> The code below doesn’t look platform-specific. Can we instead just use "#else" here? Or is this actively specific to PLATFORM(WIN)?

The webkitBundlePath code is specific to PLATFORM(WIN) at this time.

[Fujii Hironori]

Comment on attachment 463066 [details]
Patch

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

>> Source/WebCore/platform/adwaita/ScrollbarThemeAdwaita.cpp:330
>> +            return warpSlider;
> 
> I'm not a fan of the variable shadowing here.

Will fix.

>> Source/WebCore/rendering/RenderThemeAdwaita.cpp:243
>> +    if (!path)
> 
> Typically it’s best to check isNull() instead of using ! for WTF::String. But also, maybe FileSystem::readEntireFile already harmlessly fails if passed a null string, so maybe we don’t need this check.

Right. Will fix.

[Fujii Hironori]

Created attachment 463071 [details]
Patch

[EWS]

Committed 255715@main (284e48c374b0): <https://commits.webkit.org/255715@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 463071 [details].

[Radar WebKit Bug Importer]

<rdar://problem/101324228>