Bug 135333 – [EFL] Alpha value of ewk_view_bg_color_set is not working

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

Although we passed the alpha of ewk_view_bg_color_set as 0,
we can't get the transparent webview because image object is opaque.

[Ryuan Choi]

Created attachment 235589 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 235589 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        [EFL] Alpha value of ewk_view_bg_color_set is not working

Though ewk_view_bg_color_set doesn't work, the API test has been passed. I think we need to update the test case as well.

[Ryuan Choi]

(In reply to comment #2)
> (From update of attachment 235589 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235589&action=review
> 
> > Source/WebKit2/ChangeLog:3
> > +        [EFL] Alpha value of ewk_view_bg_color_set is not working
> 
> Though ewk_view_bg_color_set doesn't work, the API test has been passed. I think we need to update the test case as well.

I agree with you but it's not simple task because we need pixel tests for unit test.

Can I make new bug to improve test case of bg_color_set and our unit test frame work?

[Build Bot]

Comment on attachment 235589 [details]
Patch

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

New failing tests:
media/media-fragments/TC0001.html

[Build Bot]

Created attachment 235591 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

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

[Grzegorz Czajkowski]

Comment on attachment 235589 [details]
Patch

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

Nice fix! Added some comments before landing.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1438
> +Color EwkView::backgroundColor()
> +{
> +    int red, green, blue, alpha;
> +    WKViewGetBackgroundColor(wkView(), &red, &green, &blue, &alpha);
> +    return Color(red, green, blue, alpha);
> +}

This getter unnecessarily converts numerical colors to WebCore::Color object and from WebCore::Color to numerical values in ewk_view_bg_color_get.
In my opinion, calling WKViewGetBackgroundColor(...) directly in ewk_view_bg_color_get() was fine as they takes the same parameters.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:736
> +    impl->setBackgroundColor(WebCore::Color(red, green, blue, alpha));

Ditto. Additionally, it brings additional WebCore dependency to WK2/ewk view. IMHO, implementing this logic here (setting alpha value to ewk_view object) is fine as it's related to ewk_view layer itself.

[Ryuan Choi]

Created attachment 235660 [details]
Patch

[Ryuan Choi]

Comment on attachment 235589 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1438
>> +}
> 
> This getter unnecessarily converts numerical colors to WebCore::Color object and from WebCore::Color to numerical values in ewk_view_bg_color_get.
> In my opinion, calling WKViewGetBackgroundColor(...) directly in ewk_view_bg_color_get() was fine as they takes the same parameters.

OK, I changed not to use Color

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:736
>> +    impl->setBackgroundColor(WebCore::Color(red, green, blue, alpha));
> 
> Ditto. Additionally, it brings additional WebCore dependency to WK2/ewk view. IMHO, implementing this logic here (setting alpha value to ewk_view object) is fine as it's related to ewk_view layer itself.

There are two colors, the color of evas_object and background.
Because we control the color of evas_object in EwkView, I decided to move this logic to EwkView.
Especially, both logic should call same API, evas_object_image_alpha_set, by checking the alpha of each other.
So, I think that it looks better to have both logic in EwkView.

ps> In fact, the relation between EwkView and ewk_view.cpp is quite confusing.

[Ryuan Choi]

Created attachment 235662 [details]
Patch

[Ryuan Choi]

Created attachment 235663 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 235663 [details]
Patch

LGTM. However, Greg might wanna take a final look before landing.

[Grzegorz Czajkowski]

Comment on attachment 235663 [details]
Patch

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

Thanks for the changes Ryuan! In addition, please consider removing EwkView::backgroundColor(...) Added detailed explanation inline. However, If you prefer to exposed it, feel free to land the patch set.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1438
> +void EwkView::backgroundColor(int* red, int* green, int* blue, int* alpha)
> +{
> +    WKViewGetBackgroundColor(wkView(), red, green, blue, alpha);
> +}

Do we need this getter? I guess your goal was to have pair (set, get) for background but I don't think it's mandatory. We already expose ewk_view_bg_color_get(...) and WKViewGetBackgroundColor() so if someone wanted to get background color he would call them. Adding additional method which does the same and takes the same parameters might be overkill.

Additionally, EwkView::backgroundColor(int* red, int* green, int* blue, int* alpha) brings C style to EwkView which as far as I remember is not recommended.

[Ryuan Choi]

Committed r171724: <http://trac.webkit.org/changeset/171724>

[Ryuan Choi]

(In reply to comment #12)
> (From update of attachment 235663 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235663&action=review
> 
> Thanks for the changes Ryuan! In addition, please consider removing EwkView::backgroundColor(...) Added detailed explanation inline. However, If you prefer to exposed it, feel free to land the patch set.
> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1438
> > +void EwkView::backgroundColor(int* red, int* green, int* blue, int* alpha)
> > +{
> > +    WKViewGetBackgroundColor(wkView(), red, green, blue, alpha);
> > +}
> 
> Do we need this getter? I guess your goal was to have pair (set, get) for background but I don't think it's mandatory. We already expose ewk_view_bg_color_get(...) and WKViewGetBackgroundColor() so if someone wanted to get background color he would call them. Adding additional method which does the same and takes the same parameters might be overkill.
> 
> Additionally, EwkView::backgroundColor(int* red, int* green, int* blue, int* alpha) brings C style to EwkView which as far as I remember is not recommended.

I landed after followed your suggestion.
I also think that we have too many interfaces.