Bug 192305 - [WPE] Add API for webview background color configuration
Summary: [WPE] Add API for webview background color configuration
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit WPE (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-03 02:36 PST by Philippe Normand
Modified: 2019-01-18 12:18 PST (History)
8 users (show)

See Also:


Attachments
Patch (6.70 KB, patch)
2018-12-03 02:44 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (47.12 KB, patch)
2019-01-06 10:15 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
WiP patch (47.50 KB, patch)
2019-01-06 10:16 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
WiP patch (47.54 KB, patch)
2019-01-08 08:53 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (55.66 KB, patch)
2019-01-10 08:54 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (55.64 KB, patch)
2019-01-13 08:01 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Updated patch (67.73 KB, patch)
2019-01-18 03:36 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix win builds (68.34 KB, patch)
2019-01-18 05:10 PST, Carlos Garcia Campos
cgarcia: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2018-12-03 02:36:22 PST
This should be useful when the WebView is overlaying content on top of another graphics scene.
Comment 1 Philippe Normand 2018-12-03 02:44:36 PST
Created attachment 356371 [details]
Patch
Comment 2 Carlos Garcia Campos 2018-12-03 02:59:21 PST
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+?
Comment 3 Philippe Normand 2018-12-03 03:11:28 PST
(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!
Comment 4 Carlos Garcia Campos 2018-12-03 03:22:57 PST
(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.
Comment 5 Philippe Normand 2018-12-03 03:39:11 PST
(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.
Comment 6 Carlos Garcia Campos 2018-12-03 06:44:55 PST
(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.
Comment 7 Philippe Normand 2018-12-03 07:37:06 PST
(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.
Comment 8 Philippe Normand 2018-12-03 07:50:00 PST
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.
Comment 9 Michael Catanzaro 2018-12-03 08:05:04 PST
You could move it to WebKitWebViewWPE.cpp and WebKitWebViewGTK.cpp?
Comment 10 Philippe Normand 2019-01-06 10:02:34 PST
(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?
Comment 11 Philippe Normand 2019-01-06 10:15:51 PST
Created attachment 358458 [details]
Patch
Comment 12 Philippe Normand 2019-01-06 10:16:58 PST
Created attachment 358459 [details]
WiP patch
Comment 13 Philippe Normand 2019-01-08 08:53:49 PST
Created attachment 358597 [details]
WiP patch
Comment 14 Philippe Normand 2019-01-10 08:54:03 PST
Created attachment 358799 [details]
Patch
Comment 15 Carlos Garcia Campos 2019-01-11 04:14:51 PST
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.
Comment 16 Philippe Normand 2019-01-13 08:01:45 PST
Created attachment 359005 [details]
Patch
Comment 17 Carlos Garcia Campos 2019-01-14 05:31:07 PST
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.
Comment 18 Carlos Garcia Campos 2019-01-18 03:36:16 PST
Created attachment 359470 [details]
Updated patch
Comment 19 Carlos Garcia Campos 2019-01-18 05:10:04 PST
Created attachment 359477 [details]
Try to fix win builds
Comment 20 Carlos Garcia Campos 2019-01-18 05:10:30 PST
We will need a wk2 owner approval for this.