Bug 135333 - [EFL] Alpha value of ewk_view_bg_color_set is not working
Summary: [EFL] Alpha value of ewk_view_bg_color_set is not working
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-07-27 21:32 PDT by Ryuan Choi
Modified: 2014-07-29 00:54 PDT (History)
8 users (show)

See Also:


Attachments
Patch (4.79 KB, patch)
2014-07-27 21:45 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (482.79 KB, application/zip)
2014-07-27 23:47 PDT, Build Bot
no flags Details
Patch (4.71 KB, patch)
2014-07-28 20:56 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (4.77 KB, patch)
2014-07-28 21:04 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (4.73 KB, patch)
2014-07-28 21:27 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-07-27 21:32:33 PDT
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.
Comment 1 Ryuan Choi 2014-07-27 21:45:53 PDT
Created attachment 235589 [details]
Patch
Comment 2 Gyuyoung Kim 2014-07-27 22:43:02 PDT
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.
Comment 3 Ryuan Choi 2014-07-27 23:05:34 PDT
(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?
Comment 4 Build Bot 2014-07-27 23:47:31 PDT
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
Comment 5 Build Bot 2014-07-27 23:47:36 PDT
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
Comment 6 Grzegorz Czajkowski 2014-07-28 01:06:18 PDT
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.
Comment 7 Ryuan Choi 2014-07-28 20:56:48 PDT
Created attachment 235660 [details]
Patch
Comment 8 Ryuan Choi 2014-07-28 21:02:21 PDT
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.
Comment 9 Ryuan Choi 2014-07-28 21:04:20 PDT
Created attachment 235662 [details]
Patch
Comment 10 Ryuan Choi 2014-07-28 21:27:54 PDT
Created attachment 235663 [details]
Patch
Comment 11 Gyuyoung Kim 2014-07-28 21:55:38 PDT
Comment on attachment 235663 [details]
Patch

LGTM. However, Greg might wanna take a final look before landing.
Comment 12 Grzegorz Czajkowski 2014-07-29 00:06:12 PDT
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.
Comment 13 Ryuan Choi 2014-07-29 00:52:53 PDT
Committed r171724: <http://trac.webkit.org/changeset/171724>
Comment 14 Ryuan Choi 2014-07-29 00:54:00 PDT
(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.