Bug 218114 – Remove Accelerated2dCanvasEnabled WebPreferences entry

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

[Zan Dobersek]

Remove Accelerated2dCanvasEnabled WebPreferences entry

[Zan Dobersek]

Created attachment 412155 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Zan Dobersek]

Created attachment 412156 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 412155 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3183
> -    return settings->priv->preferences->accelerated2dCanvasEnabled();
> +    return false;

We should add a g_warning here sayig the feature has been removes and this always return false.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:-3196
> -
> -    WebKitSettingsPrivate* priv = settings->priv;
> -    if (priv->preferences->accelerated2dCanvasEnabled() == enabled)
> -        return;
> -
> -    priv->preferences->setAccelerated2dCanvasEnabled(enabled);
> -    g_object_notify(G_OBJECT(settings), "enable-accelerated-2d-canvas");

And here too, saying this does nothing now.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp:369
> +ALLOW_DEPRECATED_DECLARATIONS_BEGIN
> +    // Accelerated 2D canvas setting is deprecated and no-op.
> +    g_assert_false(webkit_settings_get_enable_accelerated_2d_canvas(settings));
> +    webkit_settings_set_enable_accelerated_2d_canvas(settings, TRUE);
> +    g_assert_false(webkit_settings_get_enable_accelerated_2d_canvas(settings));
> +ALLOW_DEPRECATED_DECLARATIONS_END

I think we can just remove the tests

[Zan Dobersek]

Created attachment 412158 [details]
Patch

[Zan Dobersek]

Created attachment 412163 [details]
Patch

[Adrian Perez]

Comment on attachment 412163 [details]
Patch

LGTM. Please wait until the Mac/iOS EWS bots pass—I think the changes
look fine but it won't hurt to wait and be sure. I suppose touching the
code there in this case is non-controversial and we might not need
someone from Apple to rubber stamp the changes because the code which
implemented the feature is gone 🤔️

[Zan Dobersek]

Comment on attachment 412163 [details]
Patch

Clearing flags on attachment: 412163

Committed r268965: <https://trac.webkit.org/changeset/268965>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/70673860>

[Michael Catanzaro]

Comment on attachment 412163 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3183
> +    return false;

Nit: return FALSE 

Though it doesn't make any difference in this case, our code style is to use it everywhere with gboolean to be safe, because sometimes it does matter, e.g.:

// Good
g_object_set(settings, "enable-accelerated-2d-canvas", FALSE, NULL);

// Illegal access
g_object_set(settings, "enable-accelerated-2d-canvas", false, NULL);

[Michael Catanzaro]

Eh, my example is wrong. :D I should have used g_object_get (there it really does matter).

[Zan Dobersek]

(In reply to Michael Catanzaro from comment #11)
> Comment on attachment 412163 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412163&action=review
> 
> > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3183
> > +    return false;
> 
> Nit: return FALSE 
> 

Tweaked in r269723.
https://trac.webkit.org/changeset/269723