[WinCairo] Enable gpu_process_canvas_rendering and gpu_process_webgl by default

• Component: WebKit2
• Status: RESOLVED FIXED

[Fujii Hironori]

[WinCairo] Enable gpu_process_canvas_rendering and gpu_process_webgl by default

[Fujii Hironori]

Created attachment 442959 [details]
Patch

[Don Olmstead]

Comment on attachment 442959 [details]
Patch

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

Think there's a better way to address this.

As an aside it would be nice to be able to set these registry values through the MiniBrowser.

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:164
> -#if ENABLE(GPU_PROCESS_BY_DEFAULT)
> +#if ENABLE(GPU_PROCESS_BY_DEFAULT) || PLATFORM(WIN)

Can't we just add `ENABLE_GPU_PROCESS_BY_DEFAULT` in the CMake code?

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:196
> -    return isFeatureFlagEnabled("gpu_process_webgl", false);
> +#if PLATFORM(WIN)
> +    bool defaultValue = true;
> +#else
> +    bool defaultValue = false;
> +#endif
> +    return isFeatureFlagEnabled("gpu_process_webgl", defaultValue);

Couldn't this be written in terms of `ENABLE(GPU_PROCESS_BY_DEFAULT)`?

Maybe need to ask the Apple folks if this is ok.

> Source/WebKit/Shared/win/WebPreferencesDefaultValuesWin.cpp:46
> -        return false;
> +        return defaultValue;
>      if (keyType != REG_DWORD)
> -        return false;
> +        return defaultValue;
>      if (dataSize != sizeof data)
> -        return false;
> +        return defaultValue;

Whoops! This is a good catch here.

[Simon Fraser (smfr)]

Comment on attachment 442959 [details]
Patch

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

>> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:196
>> +    return isFeatureFlagEnabled("gpu_process_webgl", defaultValue);
> 
> Couldn't this be written in terms of `ENABLE(GPU_PROCESS_BY_DEFAULT)`?
> 
> Maybe need to ask the Apple folks if this is ok.

I don't think we want the default to be true if ENABLE(GPU_PROCESS_BY_DEFAULT) is defined. We switch WebGL independently from the other GPU Process things.

[Don Olmstead]

Comment on attachment 442959 [details]
Patch

After getting more info on what Apple is up to with that ENABLE_GPU_PROCESS_BY_DEFAULT I agree that wee should just use PLATFORM(WIN) here. Changing my review

[EWS]

Committed r285127 (243766@main): <https://commits.webkit.org/243766@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/84896922>

[Fujii Hironori]

Thank you for the review.

(In reply to Don Olmstead from comment #2)
> As an aside it would be nice to be able to set these registry values through
> the MiniBrowser.

I'd like to remove WinCairo WK2 non-GPU process mode support.
WinCairo supports WK1 that supports only non-GPU process mode.

[Don Olmstead]

(In reply to Fujii Hironori from comment #7)
> Thank you for the review.
> 
> (In reply to Don Olmstead from comment #2)
> > As an aside it would be nice to be able to set these registry values through
> > the MiniBrowser.
> 
> I'd like to remove WinCairo WK2 non-GPU process mode support.
> WinCairo supports WK1 that supports only non-GPU process mode.

Agreed to removing non-GPU Process mode support.

The comment was not just about GPU Process feature flags it was about not having a good way to set any feature flags in MiniBrowser.