This should be useful when the WebView is overlaying content on top of another graphics scene.
Created attachment 356371 [details] Patch
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+?
(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!
(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.
(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.
(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.
(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.
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.
You could move it to WebKitWebViewWPE.cpp and WebKitWebViewGTK.cpp?
(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?
Created attachment 358458 [details] Patch
Created attachment 358459 [details] WiP patch
Created attachment 358597 [details] WiP patch
Created attachment 358799 [details] Patch
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.
Created attachment 359005 [details] Patch
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.
Created attachment 359470 [details] Updated patch
Created attachment 359477 [details] Try to fix win builds
We will need a wk2 owner approval for this.
WebKit2 owners, please, could any of you take a look at the cross-platform changes in this patch?
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.
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?).
Any other concern youenn?
Created attachment 360849 [details] Rebased patch
Created attachment 361048 [details] Rebased patch
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;
Created attachment 361672 [details] Patch for landing Ping owners, please. I've removed WebPageProxy::drawsBackground() and WebPageProxy::setDrawsBackground() to avoid confusion.
Created attachment 361676 [details] Try to fix mac builds
Created attachment 361678 [details] Try to fix mac builds
Another ping for WK2 owners.. Youenn, are you back? Can you have a look please?
Comment on attachment 361678 [details] Try to fix mac builds I'm not a WK2 owner, but this looks fine for Mac.
(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.
Committed r242082: <https://trac.webkit.org/changeset/242082>
*** Bug 187646 has been marked as a duplicate of this bug. ***