Bug 192305 – [WPE] Add API for webview background color configuration

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Philippe Normand]

This should be useful when the WebView is overlaying content on top of another graphics scene.

[Philippe Normand]

Created attachment 356371 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 356371 [details]
Patch

I planned to expose this with the same API as GTK+ webkit_web_view_set_background_color(). Setting a fully transparent color is the same as passing FALSE to set_draws_background(). Have you checked this patch doesn't break GTK+?

[Philippe Normand]

(In reply to Carlos Garcia Campos from comment #2)
> Comment on attachment 356371 [details]
> Patch
> 
> I planned to expose this with the same API as GTK+
> webkit_web_view_set_background_color(). 

Yeah I saw that one but it uses some GDK type :/

> Setting a fully transparent color is
> the same as passing FALSE to set_draws_background(). Have you checked this
> patch doesn't break GTK+?

Not yet, will check now!

[Carlos Garcia Campos]

(In reply to Philippe Normand from comment #3)
> (In reply to Carlos Garcia Campos from comment #2)
> > Comment on attachment 356371 [details]
> > Patch
> > 
> > I planned to expose this with the same API as GTK+
> > webkit_web_view_set_background_color(). 
> 
> Yeah I saw that one but it uses some GDK type :/

Right, in the case of WPE we can either add a color type or simply use double r, g, b, a.

> > Setting a fully transparent color is
> > the same as passing FALSE to set_draws_background(). Have you checked this
> > patch doesn't break GTK+?
> 
> Not yet, will check now!

Please. You can easily test it with MiniBrowser --bg-color. Make sure to force AC mode, since this patch only affects AC mode.

[Philippe Normand]

(In reply to Carlos Garcia Campos from comment #4)
> (In reply to Philippe Normand from comment #3)
> > (In reply to Carlos Garcia Campos from comment #2)
> > > Comment on attachment 356371 [details]
> > > Patch
> > > 
> > > I planned to expose this with the same API as GTK+
> > > webkit_web_view_set_background_color(). 
> > 
> > Yeah I saw that one but it uses some GDK type :/
> 
> Right, in the case of WPE we can either add a color type or simply use
> double r, g, b, a.
> 

Any preference? I'd probably aim for the latter for simplicity reasons.

> > > Setting a fully transparent color is
> > > the same as passing FALSE to set_draws_background(). Have you checked this
> > > patch doesn't break GTK+?
> > 
> > Not yet, will check now!
> 
> Please. You can easily test it with MiniBrowser --bg-color. Make sure to
> force AC mode, since this patch only affects AC mode.

It still works as expected:

WEBKIT_FORCE_COMPOSITING_MODE=1 run-minibrowser --gtk --bg-color="rgba(0,0,0,0)" about:blank 

shows a transparent window

Loading an non-empty url still works as expected as well.

[Carlos Garcia Campos]

(In reply to Philippe Normand from comment #5)
> (In reply to Carlos Garcia Campos from comment #4)
> > (In reply to Philippe Normand from comment #3)
> > > (In reply to Carlos Garcia Campos from comment #2)
> > > > Comment on attachment 356371 [details]
> > > > Patch
> > > > 
> > > > I planned to expose this with the same API as GTK+
> > > > webkit_web_view_set_background_color(). 
> > > 
> > > Yeah I saw that one but it uses some GDK type :/
> > 
> > Right, in the case of WPE we can either add a color type or simply use
> > double r, g, b, a.
> > 
> 
> Any preference? I'd probably aim for the latter for simplicity reasons.

I don't mind. Good thing about adding a type is that we could grow it to support things like parsing color strings similar to GdkRGBA

> > > > Setting a fully transparent color is
> > > > the same as passing FALSE to set_draws_background(). Have you checked this
> > > > patch doesn't break GTK+?
> > > 
> > > Not yet, will check now!
> > 
> > Please. You can easily test it with MiniBrowser --bg-color. Make sure to
> > force AC mode, since this patch only affects AC mode.
> 
> It still works as expected:
> 
> WEBKIT_FORCE_COMPOSITING_MODE=1 run-minibrowser --gtk
> --bg-color="rgba(0,0,0,0)" about:blank 
> 
> shows a transparent window

What about opaque or semi-transparent colors?

> Loading an non-empty url still works as expected as well.

[Philippe Normand]

(In reply to Carlos Garcia Campos from comment #6)
> (In reply to Philippe Normand from comment #5)
> > (In reply to Carlos Garcia Campos from comment #4)
> > > (In reply to Philippe Normand from comment #3)
> > > > (In reply to Carlos Garcia Campos from comment #2)
> > > > > Comment on attachment 356371 [details]
> > > > > Patch
> > > > > 
> > > > > I planned to expose this with the same API as GTK+
> > > > > webkit_web_view_set_background_color(). 
> > > > 
> > > > Yeah I saw that one but it uses some GDK type :/
> > > 
> > > Right, in the case of WPE we can either add a color type or simply use
> > > double r, g, b, a.
> > > 
> > 
> > Any preference? I'd probably aim for the latter for simplicity reasons.
> 
> I don't mind. Good thing about adding a type is that we could grow it to
> support things like parsing color strings similar to GdkRGBA
> 

That's true yeah, but I'm not sure how useful it would be. :)

> > > > > Setting a fully transparent color is
> > > > > the same as passing FALSE to set_draws_background(). Have you checked this
> > > > > patch doesn't break GTK+?
> > > > 
> > > > Not yet, will check now!
> > > 
> > > Please. You can easily test it with MiniBrowser --bg-color. Make sure to
> > > force AC mode, since this patch only affects AC mode.
> > 
> > It still works as expected:
> > 
> > WEBKIT_FORCE_COMPOSITING_MODE=1 run-minibrowser --gtk
> > --bg-color="rgba(0,0,0,0)" about:blank 
> > 
> > shows a transparent window
> 
> What about opaque or semi-transparent colors?
> 
> > Loading an non-empty url still works as expected as well.

Yeah that works too, but only if I first load a normal URI and then manually load about:blank in the existing browser.

[Philippe Normand]

gtk-doc seems to be confused by 2 impls of the same function with different signatures :/

/app/webkit/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4003: warning: Parameter description for webkit_web_view_set_background_color::r is not used from source code comment block.
/app/webkit/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4003: warning: Parameter description for webkit_web_view_set_background_color::g is not used from source code comment block.
/app/webkit/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4003: warning: Parameter description for webkit_web_view_set_background_color::b is not used from source code comment block.
/app/webkit/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4003: warning: Parameter description for webkit_web_view_set_background_color::a is not used from source code comment block.
/app/webkit/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4003: warning: Parameter description for webkit_web_view_set_background_color::rgba is missing in source code comment block.
/app/webkit/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4029: warning: Parameter description for webkit_web_view_get_background_color::r is not used from source code comment block.
/app/webkit/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4029: warning: Parameter description for webkit_web_view_get_background_color::g is not used from source code comment block.
/app/webkit/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4029: warning: Parameter description for webkit_web_view_get_background_color::b is not used from source code comment block.
/app/webkit/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4029: warning: Parameter description for webkit_web_view_get_background_color::a is not used from source code comment block.
/app/webkit/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4029: warning: Parameter description for webkit_web_view_get_background_color::rgba is missing in source code comment block.
./webkit2gtk-4.0-unused.txt:1: warning: 2 unused declarations.They should be added to webkit2gtk-4.0-sections.txt in the appropriate place.

The following API are missing documentation:
	webkit_web_view_get_background_color(r, g, b, a)
	webkit_web_view_set_background_color(r, g, b, a)


Also I tried to properly set the background color in the threaded compositor, without success yet. :(
I'm not sure what's the use-case for a full-blown set_background_color API anyway? Seems to me what's the most important is the opacity anyway.

[Michael Catanzaro]

You could move it to WebKitWebViewWPE.cpp and WebKitWebViewGTK.cpp?

[Philippe Normand]

(In reply to Philippe Normand from comment #5)
> > Please. You can easily test it with MiniBrowser --bg-color. Make sure to
> > force AC mode, since this patch only affects AC mode.
> 
> It still works as expected:
> 
> WEBKIT_FORCE_COMPOSITING_MODE=1 run-minibrowser --gtk
> --bg-color="rgba(0,0,0,0)" about:blank 
> 
> shows a transparent window
> 

Seems like this regressed recently. Does it work for you?

[Philippe Normand]

Created attachment 358458 [details]
Patch

[Philippe Normand]

Created attachment 358459 [details]
WiP patch

[Philippe Normand]

Created attachment 358597 [details]
WiP patch

[Philippe Normand]

Created attachment 358799 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 358799 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [WPE] Add API to allow application to disable background rendering

We should update the bug title now that we are adding api to set a background color.

> Source/WebKit/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:85
> +    if (!drawsBackground && (currentRootLayer->opacity() != opacity))

Parentheses aren't needed in (currentRootLayer->opacity() != opacity). Shouldn't we set the opacity even when !drawsBackground?

> Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp:31
> + * A WebKitColor is a boxed type wrapping a WebCore color.

Don't mention WebCore here, that's an impl detail and user don't need to know what a WebCore color is.

> Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp:36
> +struct _WebKitColor {

This is very simple type that is very unlikely to change, so I would make the struct public so that it can be stack allocated.

> Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp:48
> +    WebCore::Color m_color;

And keep here the channels as members instead of wrapping a WebCore color. Using double values for consistency with GdkRGBA.

> Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp:49
> +    int referenceCount { 1 };

I don't think it's worth making this ref counted.

> Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp:52
> +static WebKitColor* webkitColorRef(WebKitColor* color)

But if refcounted this should be public. I would provide copy/free public functions in any case.

> Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp:70
> +WebKitColor* webkitColorFromColor(const WebCore::Color& color)

webkitColorCreate

> Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp:77
> +const WebCore::Color& webkitColorGetColor(WebKitColor* backgroundColor)

This would be webkitColorToWebCoreColor and returns a new WebCore::Color instead of a ref.

> Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp:83
> + * webkit_color_new_from_string:

I would use webkit_color_parse(), again for consistency with GdkRGBA, returning gboolean and receiving a WebKitColor* so that it can be used with stack allocated colors.

> Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp:84
> + * @colorString: color representation as color nickname or HEX string

color_string

> Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp:110
> +gint webkit_color_get_red_channel(WebKitColor* color)

And then we don't need these public getters

> Source/WebKit/UIProcess/API/wpe/WebKitColor.h:40
> +webkit_color_new_from_string    (const char* colorString);

const char *color_string

> Source/WebKit/UIProcess/API/wpe/WebKitColor.h:43
> +webkit_color_get_red_channel    (WebKitColor* color);

WebKitColor *color

> Source/WebKit/UIProcess/API/wpe/WebKitColor.h:46
> +webkit_color_get_green_channel  (WebKitColor* color);

Ditto.

> Source/WebKit/UIProcess/API/wpe/WebKitColor.h:49
> +webkit_color_get_blue_channel   (WebKitColor* color);

Ditto.

> Source/WebKit/UIProcess/API/wpe/WebKitColor.h:52
> +webkit_color_get_alpha_channel  (WebKitColor* color);

Ditto.

> Source/WebKit/UIProcess/API/wpe/WebKitWebView.h:522
> +webkit_web_view_set_background_color                 (WebKitWebView                *web_view,

I think there's an exytra space between WebKitWebView and *web_view

> Source/WebKit/UIProcess/WebPageProxy.cpp:1549
> +    m_backgroundColor = color;
> +    if (isValid())
> +        m_process->send(Messages::WebPage::SetBackgroundColor(color), m_pageID);

We should also add a creation parameter for the background color now that we are sending it to the web process.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1269
> +    void setBackgroundColor(const WebCore::Color&);

This is specific to WPE and GTK in the ui process, it should be the same here.

> Source/WebKit/WebProcess/WebPage/WebPage.messages.in:29
> +    SetBackgroundColor(WebCore::Color color)

Ditto.

> Tools/MiniBrowser/wpe/main.cpp:201
> +    g_boxed_free(WEBKIT_TYPE_COLOR, backgroundColor);

Boxed types are expected to provide their copy and free functions.

> Tools/MiniBrowser/wpe/main.cpp:211
> +    if (privateMode || automationMode)

This doesn't belong to this patch, but good catch! File a new bug report and r=me

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:1158
> +#if PLATFORM(WPE)

We shouldn't need different tests for WPE and GTK, and if WebKitColor is compatible with GdkRGBA we could even use almost the same code.

[Philippe Normand]

Created attachment 359005 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 359005 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [WPE] Add API to allow application to disable background rendering

We should update the bug title now that we are adding api to set a background color.

> Source/WebCore/page/Frame.cpp:905
> +void Frame::createView(const IntSize& viewportSize, bool transparent, const Color& backgroundColor,

transparent and backgroundColor are kind of redundant now, could we use a single parameter and use Color::transparent when transparent=true? The bg color parameter could then be std::optional.

> Source/WebKit/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:68
> -void CoordinatedGraphicsScene::paintToCurrentGLContext(const TransformationMatrix& matrix, float opacity, const FloatRect& clipRect, const Color& backgroundColor, bool drawsBackground, TextureMapper::PaintFlags PaintFlags)
> +void CoordinatedGraphicsScene::paintToCurrentGLContext(const TransformationMatrix& matrix, const FloatRect& clipRect, const Color& backgroundColor, bool drawsBackground, TextureMapper::PaintFlags PaintFlags)

This is only called by the threaded compositor nowadays, I think we can remove the bg color + opacity from here.

> Source/WebKit/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:82
> +    auto& color = drawsBackground ? backgroundColor : m_viewBackgroundColor;
> +    m_textureMapper->clearColor(color);

Because we are applying the bg color twice, since it's already drawn by the frame.

> Source/WebKit/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:84
> +    float opacity = drawsBackground ? color.alphaAsFloat() : 1;

The layer opacity shouldn't be the same as the bg color opacity.

> Source/WebKit/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.h:75
> +    void paintToCurrentGLContext(const WebCore::TransformationMatrix&, const WebCore::FloatRect&, const WebCore::Color& backgroundColor, bool drawsBackground, WebCore::TextureMapper::PaintFlags = 0);

If we make the bg color parameter optional we don't need the drawsBackground one.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:408
> +    if (!color.isOpaque())
> +        page.setDrawsBackground(false);

I think we should do the same in GTK and WPE, otherwise we don't need to send the bg color to the web process in GTK case.

> Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp:95
> + * Create a new #WebKitColor for the given @color_string

It doesn't create a new WebKitColor, it fills the passed in one.

> Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp:100
> + * Returns: a #gboolean indicating if the @color was correctly filled in or not.

%TRUE if parse succeeded or %FALSE otherwise

> Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp:106
> +    auto webCoreColor = WebCore::Color(colorString);

g_return_val_if_fail(color, nullptr);

> Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp:110
> +    if (color) {

This should always be non-null

> Source/WebKit/UIProcess/API/wpe/WebKitColor.h:38
> + */

Since: 2.24

> Source/WebKit/UIProcess/API/wpe/WebKitColorPrivate.h:22
> +#include "Color.h"

<WebCore/Color.h>

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewWPE.cpp:190
> + * the actual contents are rendered. Note that if the web page loaded in @web_view
> + * specifies a background color, it will take precedence over the background color.

Have we checked this is still true?

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewWPE.cpp:221
> +    auto& webCoreColor = page.backgroundColor();
> +    g_return_if_fail(webCoreColor.isValid());

This should never happen, use an assert instead if you want to be extra sure.

> Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:233
> +

Remove this empty line.

[Carlos Garcia Campos]

Created attachment 359470 [details]
Updated patch

[Carlos Garcia Campos]

Created attachment 359477 [details]
Try to fix win builds

[Carlos Garcia Campos]

We will need a wk2 owner approval for this.

[Carlos Garcia Campos]

WebKit2 owners, please, could any of you take a look at the cross-platform changes in this patch?

[youenn fablet]

Comment on attachment 359477 [details]
Try to fix win builds

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:1539
> +    setBackgroundColor(backgroundColor);

Let's say we call setDrawsBackground to false.
We will change background color from let's say black to transparent.
Let's say we now call setDrawsBackground to true.
I would expect background color to be back to black, but it seems it will be WTF::nullopt.

[Carlos Garcia Campos]

Comment on attachment 359477 [details]
Try to fix win builds

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

>> Source/WebKit/UIProcess/WebPageProxy.cpp:1539
>> +    setBackgroundColor(backgroundColor);
> 
> Let's say we call setDrawsBackground to false.
> We will change background color from let's say black to transparent.
> Let's say we now call setDrawsBackground to true.
> I would expect background color to be back to black, but it seems it will be WTF::nullopt.

There's no change in behavior here. Before this patch false means use transparent color and true means use the default background color (white). It's not actually expected to use both setDrawsBackground and setBackgroundColor, so ports using setDrawsBackground will always use the default bg color on true (as they currently do). And ports using setBackgroundColor don't need to use setDrawsBakground, because they are expected to pass a transparent color instead. Ideally we can simply remove setDrawsBackgroiund (as I've done with the IPC message), but I was afraid of breaking apple ports (there aren't tests for this right?).

[Carlos Garcia Campos]

Any other concern youenn?

[Carlos Garcia Campos]

Created attachment 360849 [details]
Rebased patch

[Carlos Garcia Campos]

Created attachment 361048 [details]
Rebased patch

[Michael Catanzaro]

Comment on attachment 361048 [details]
Rebased patch

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

This looks good to me, but this still needs owner approval. The changes here are extensive.

> Source/WebKit/Shared/WebPageCreationParameters.cpp:354
>  #endif
> +    Optional<Optional<WebCore::Color>> backgroundColor;

There should be a blank line here.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:1142
> +#if PLATFORM(GTK)
> +#define ColorType GdkRGBA
> +#elif PLATFORM(WPE)
> +#define ColorType WebKitColor
> +#endif

using ColorType = GdkRGBA;
using ColorType = WebKitColor;

[Carlos Garcia Campos]

Created attachment 361672 [details]
Patch for landing

Ping owners, please. I've removed WebPageProxy::drawsBackground() and WebPageProxy::setDrawsBackground() to avoid confusion.

[Carlos Garcia Campos]

Created attachment 361676 [details]
Try to fix mac builds

[Carlos Garcia Campos]

Created attachment 361678 [details]
Try to fix mac builds

[Philippe Normand]

Another ping for WK2 owners.. Youenn, are you back? Can you have a look please?

[Timothy Hatcher]

Comment on attachment 361678 [details]
Try to fix mac builds

I'm not a WK2 owner, but this looks fine for Mac.

[youenn fablet]

(In reply to Philippe Normand from comment #31)
> Another ping for WK2 owners.. Youenn, are you back? Can you have a look
> please?

LGTM too.

[Carlos Garcia Campos]

Committed r242082: <https://trac.webkit.org/changeset/242082>

[Philippe Normand]

*** Bug 187646 has been marked as a duplicate of this bug. ***