Bug 127539 – [EFL][WK2] Extract the control of page background out of color_set

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

EFL have a way to change the color using evas_object_color_set and we used it to change background.
But, it's buggy, confusing and not enough.

In EFL, alpha of evas_object_color_set will control the opacity of the contents internally(not only background of webview but also contents of the webview).
But, we used this to decide whether to draw background transparently.
So, we can't make transparent webview which draws only contents without default background color.

Other color components also does not related to the page background color.
They just used to decide whether to draw background or not.

Indeed, when we disabled to draw page background, we may see the broken rendering because we don't clear the buffer.

So, I propose to change  the policy and code.
- RGB of color_set will be the clearColor. It will be used when we disabled to draw page background(ewk_view_draws_background_enabled_set(ewkview, EINA_FALSE).
- Th alpha of color_set will be only used for transpancy of whole contents.
- ewk_view_draws_transparent_background_enabled_set will enable/disable the transparent background color.
- ewk_view_draws_background_enabled_set will enable/disable whether to draw page background (current name is ewk_view_draws_page_background_set)

[Ryuan Choi]

After discussed more with eunmi and hyowon,
I changed my mind.

Instead of keeping two APIs with color_set,
ewk_view_bg_color_set looks enough for the application developers.

They will only want to change the default background color.
All other are internal issues.

[Ryuan Choi]

Created attachment 223066 [details]
suggestion

[Ryuan Choi]

Comment on attachment 223066 [details]
suggestion

Although we landed this, we can't have webview drawn other color.

It looks a bug of coordinated graphics or texmap when disabled DrawsBackground with transparant background.

I will investigate it more.

[Ryuan Choi]

Created attachment 223094 [details]
suggestion

[Build Bot]

Comment on attachment 223094 [details]
suggestion

Attachment 223094 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4968362132111360

New failing tests:
http/tests/security/contentSecurityPolicy/source-list-parsing-06.html

[Build Bot]

Created attachment 223105 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Ryuan Choi]

Created attachment 223193 [details]
suggestion

[Ryuan Choi]

ping, noam, gyuyong.

Could you take a look at this ?

[Jinwoo Song]

Comment on attachment 223193 [details]
suggestion

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:734
> +        WKViewSetDrawsTransparentBackground(impl->wkView(), false);

s/false/true ?

[Gyuyoung Kim]

Comment on attachment 223193 [details]
suggestion

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:900
> +EAPI void ewk_view_bg_color_set(Evas_Object *o, int r, int g, int b, int a);

Any API test for this new API ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:911
> +EAPI void ewk_view_bg_color_get(Evas_Object *o, int *r, int *g, int *b, int *a);

Missing *const* in front of Evas_Object.

[Gyuyoung Kim]

Comment on attachment 223193 [details]
suggestion

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:900
> +EAPI void ewk_view_bg_color_set(Evas_Object *o, int r, int g, int b, int a);

Any API test for this new API ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:911
> +EAPI void ewk_view_bg_color_get(Evas_Object *o, int *r, int *g, int *b, int *a);

Missing *const* in front of Evas_Object.

[Ryuan Choi]

Created attachment 226492 [details]
Patch

[Ryuan Choi]

Created attachment 226496 [details]
Patch

[Ryuan Choi]

(In reply to comment #9)
> (From update of attachment 223193 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223193&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:734
> > +        WKViewSetDrawsTransparentBackground(impl->wkView(), false);
> 
> s/false/true ?

It's not important if we disable DrawsBackground.
Anyway, I removed it.

(In reply to comment #11)
> (From update of attachment 223193 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223193&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:900
> > +EAPI void ewk_view_bg_color_set(Evas_Object *o, int r, int g, int b, int a);
> 
> Any API test for this new API ?

I added simple one.
In order to test it better, we should have capture API.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:911
> > +EAPI void ewk_view_bg_color_get(Evas_Object *o, int *r, int *g, int *b, int *a);
> 
> Missing *const* in front of Evas_Object.

Right, I fixed.
Thanks.

[Jinwoo Song]

Looks good to me.

[Gyuyoung Kim]

Basically I don't object to add APIs to change background color. However, I wonder whether other ports want to use this functionality. Do you know what port already support this feature or will support it ?

[Ryuan Choi]

(In reply to comment #16)
> Basically I don't object to add APIs to change background color. However, I wonder whether other ports want to use this functionality. Do you know what port already support this feature or will support it ?

Well, I don't know which port want this feature because basic browser does not require transparent webview.
we need this to support our web application framework.

Anyway, because Root layer always set ContentsOpaque to true, other ports except IOS can't render transparent background with accelerated compositing.
(see http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderLayerBacking.cpp#L308)

FYI, WebCore itself can draw contents with transparent background by turning DrawsBackground off.
This patch just make it working with Coordinated graphics on the EFL port.

[Gyuyoung Kim]

(In reply to comment #17)
> (In reply to comment #16)
> > Basically I don't object to add APIs to change background color. However, I wonder whether other ports want to use this functionality. Do you know what port already support this feature or will support it ?
> 
> Well, I don't know which port want this feature because basic browser does not require transparent webview.
> we need this to support our web application framework.
> 
> Anyway, because Root layer always set ContentsOpaque to true, other ports except IOS can't render transparent background with accelerated compositing.
> (see http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderLayerBacking.cpp#L308)
> 
> FYI, WebCore itself can draw contents with transparent background by turning DrawsBackground off.
> This patch just make it working with Coordinated graphics on the EFL port.

I see. I don't mind to land this patch from EFL port side. I think this patch needs to get review from WK2 owner and Coordinated Graphics owner. Noam or Benjamin may review this patch.

[Gyuyoung Kim]

Comment on attachment 226496 [details]
Patch

This patch only modifies the coordinated graphics and just enable a code block of rendering for EFL port. So, I believe port maintainer can land this. LGTM.

[Ryuan Choi]

Committed r166566: <http://trac.webkit.org/changeset/166566>

[Ryuan Choi]

Comment on attachment 226496 [details]
Patch

clearing flags.
Landed patch after fixed merge issue.