Bug 246604 - [WinCairo] Use RenderThemeAdwaita instead of RenderThemeWin
Summary: [WinCairo] Use RenderThemeAdwaita instead of RenderThemeWin
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-10-17 00:05 PDT by Fujii Hironori
Modified: 2022-10-18 22:44 PDT (History)
24 users (show)

See Also:

Attachments
WIP patch (4.82 KB, patch)
2022-10-17 00:05 PDT, Fujii Hironori 		no flags Details | Formatted Diff | Diff
Patch (13.53 KB, patch)
2022-10-17 22:18 PDT, Fujii Hironori 		no flags Details | Formatted Diff | Diff
Patch (13.53 KB, patch)
2022-10-17 22:23 PDT, Fujii Hironori 		no flags Details | Formatted Diff | Diff
Patch (13.89 KB, patch)
2022-10-18 14:13 PDT, Fujii Hironori 		no flags Details | Formatted Diff | Diff
Patch (14.52 KB, patch)
2022-10-18 14:39 PDT, Fujii Hironori 		no flags Details | Formatted Diff | Diff
Patch (14.50 KB, patch)
2022-10-18 18:39 PDT, Fujii Hironori 		no flags Details | Formatted Diff | Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2022-10-17 00:05:55 PDT 
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.
Comment 1 Fujii Hironori 2022-10-17 00:21:29 PDT 
See also:
  Bug 246264 – [GPU Process] Enable drawing form controls for GPU Process on macOS
Comment 2 Fujii Hironori 2022-10-17 22:18:18 PDT 
Created attachment 463051 [details]
Patch
Comment 3 Fujii Hironori 2022-10-17 22:23:47 PDT 
Created attachment 463052 [details]
Patch
Comment 4 Fujii Hironori 2022-10-18 14:13:07 PDT 
Created attachment 463064 [details]
Patch
Comment 5 Fujii Hironori 2022-10-18 14:39:04 PDT 
Created attachment 463066 [details]
Patch
Comment 6 Darin Adler 2022-10-18 15:15:22 PDT 
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.
Comment 7 Don Olmstead 2022-10-18 15:20:04 PDT 
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.
Comment 8 Fujii Hironori 2022-10-18 17:52:55 PDT 
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.
Comment 9 Fujii Hironori 2022-10-18 18:39:52 PDT 
Created attachment 463071 [details]
Patch
Comment 10 EWS 2022-10-18 22:43:51 PDT 
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].
Comment 11 Radar WebKit Bug Importer 2022-10-18 22:44:19 PDT 
<rdar://problem/101324228>