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.
Created attachment 235589 [details] Patch
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.
(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 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
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 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.
Created attachment 235660 [details] Patch
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.
Created attachment 235662 [details] Patch
Created attachment 235663 [details] Patch
Comment on attachment 235663 [details] Patch LGTM. However, Greg might wanna take a final look before landing.
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.
Committed r171724: <http://trac.webkit.org/changeset/171724>
(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.