Bug 127539 - [EFL][WK2] Extract the control of page background out of color_set
Summary: [EFL][WK2] Extract the control of page background out of color_set
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryuan Choi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-23 20:52 PST by Ryuan Choi
Modified: 2014-03-31 23:11 PDT (History)
20 users (show)

See Also:


Attachments
suggestion (17.10 KB, patch)
2014-02-03 20:21 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
suggestion (19.47 KB, patch)
2014-02-04 03:39 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (522.84 KB, application/zip)
2014-02-04 06:06 PST, Build Bot
no flags Details
suggestion (19.62 KB, patch)
2014-02-04 18:12 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (19.48 KB, patch)
2014-03-12 03:52 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (20.58 KB, patch)
2014-03-12 04:54 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2014-01-23 20:52:25 PST
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)
Comment 1 Ryuan Choi 2014-01-26 23:36:24 PST
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.
Comment 2 Ryuan Choi 2014-02-03 20:21:20 PST
Created attachment 223066 [details]
suggestion
Comment 3 Ryuan Choi 2014-02-03 20:25:50 PST
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.
Comment 4 Ryuan Choi 2014-02-04 03:39:48 PST
Created attachment 223094 [details]
suggestion
Comment 5 Build Bot 2014-02-04 06:06:15 PST
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
Comment 6 Build Bot 2014-02-04 06:06:19 PST
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
Comment 7 Ryuan Choi 2014-02-04 18:12:36 PST
Created attachment 223193 [details]
suggestion
Comment 8 Ryuan Choi 2014-03-07 18:51:21 PST
ping, noam, gyuyong.

Could you take a look at this ?
Comment 9 Jinwoo Song 2014-03-11 22:14:05 PDT
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 ?
Comment 10 Gyuyoung Kim 2014-03-12 03:50:30 PDT
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.
Comment 11 Gyuyoung Kim 2014-03-12 03:51:22 PDT
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.
Comment 12 Ryuan Choi 2014-03-12 03:52:39 PDT
Created attachment 226492 [details]
Patch
Comment 13 Ryuan Choi 2014-03-12 04:54:22 PDT
Created attachment 226496 [details]
Patch
Comment 14 Ryuan Choi 2014-03-12 05:00:12 PDT
(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.
Comment 15 Jinwoo Song 2014-03-17 02:37:54 PDT
Looks good to me.
Comment 16 Gyuyoung Kim 2014-03-17 03:13:41 PDT
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 ?
Comment 17 Ryuan Choi 2014-03-17 03:33:55 PDT
(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.
Comment 18 Gyuyoung Kim 2014-03-17 04:13:21 PDT
(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.
Comment 19 Gyuyoung Kim 2014-03-31 22:01:12 PDT
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.
Comment 20 Ryuan Choi 2014-03-31 23:10:34 PDT
Committed r166566: <http://trac.webkit.org/changeset/166566>
Comment 21 Ryuan Choi 2014-03-31 23:11:45 PDT
Comment on attachment 226496 [details]
Patch

clearing flags.
Landed patch after fixed merge issue.