Bug 73015 – [EFL] Add alpha channel API to single and tiled backing store.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[JungJik Lee]

Created attachment 116340 [details]
proposal_patch

Rename bg_color_set API to alpha_set because it only uses alpha channel in parameters
And new alpha_set API is added into tiled backing store.
Alpha_set API is for turning the alpha channel on/off on the tiled or single backing store objects.

[WebKit Review Bot]

Attachment 116340 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/efl/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JungJik Lee]

Created attachment 116341 [details]
minor_edit.patch

[JungJik Lee]

Created attachment 116347 [details]
rebased patch

attach new rebased patch.

[Gyuyoung Kim]

Comment on attachment 116347 [details]
rebased patch

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

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.h:103
> +void ewk_tiled_backing_store_alpha_set(Evas_Object* o, Eina_Bool has_alpha);

In EFL style, it looks they don't use prefix for Eina_Bool type as below,

ewk_view_setting_auto_load_images_set(Evas_Object *o, Eina_Bool automatic);
ewk_view_zoom_weak_smooth_scale_set(Evas_Object *o, Eina_Bool smooth_scale);
ewk_view_text_matches_highlight_set(Evas_Object *o, Eina_Bool highlight);

So, it seems it is better to use "Eina_Bool alpha" instead of "Eina_Bool has_alpha"

[KwangHyuk]

(In reply to comment #4)
> (From update of attachment 116347 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116347&action=review
> 
> > Source/WebKit/efl/ewk/ewk_tiled_backing_store.h:103
> > +void ewk_tiled_backing_store_alpha_set(Evas_Object* o, Eina_Bool has_alpha);
> 
> In EFL style, it looks they don't use prefix for Eina_Bool type as below,
> 
> ewk_view_setting_auto_load_images_set(Evas_Object *o, Eina_Bool automatic);
> ewk_view_zoom_weak_smooth_scale_set(Evas_Object *o, Eina_Bool smooth_scale);
> ewk_view_text_matches_highlight_set(Evas_Object *o, Eina_Bool highlight);
> 
> So, it seems it is better to use "Eina_Bool alpha" instead of "Eina_Bool has_alpha"

IMO, Jungjik might refer the evas_object api.
evas_object_image_alpha_set (Evas_Object *obj, Eina_Bool has_alpha);

As a result, I agree with api name Jungjik proposed. :)

[KwangHyuk]

LGTM because it will works.

But, may be next time, I suggest you to consider the ewk_tiled_backing_store_alpha_set API's current implementation.
I can't make sure whether current implementation is good enough for the case when this API is called because it just set the flag and no instant action for the tiles inside of viewport area.

[Gyuyoung Kim]

Comment on attachment 116347 [details]
rebased patch

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

>>> Source/WebKit/efl/ewk/ewk_tiled_backing_store.h:103
>>> +void ewk_tiled_backing_store_alpha_set(Evas_Object* o, Eina_Bool has_alpha);
>> 
>> In EFL style, it looks they don't use prefix for Eina_Bool type as below,
>> 
>> ewk_view_setting_auto_load_images_set(Evas_Object *o, Eina_Bool automatic);
>> ewk_view_zoom_weak_smooth_scale_set(Evas_Object *o, Eina_Bool smooth_scale);
>> ewk_view_text_matches_highlight_set(Evas_Object *o, Eina_Bool highlight);
>> 
>> So, it seems it is better to use "Eina_Bool alpha" instead of "Eina_Bool has_alpha"
> 
> IMO, Jungjik might refer the evas_object api.
> evas_object_image_alpha_set (Evas_Object *obj, Eina_Bool has_alpha);
> 
> As a result, I agree with api name Jungjik proposed. :)

Ok, Looks good.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 116347 [details]
rebased patch

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

At first glance, this looks like a change for change's sake. I agree that currently the other parameters of bg_color_set are not being used, however we don't know what child implementations might want to do, so I think it is better to err on the safe side and provide more parameters, not less, here. If you provide a function to set alpha, it doesn't really hurt to pass r, g and b with it.

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:211
> +    evas_object_image_alpha_set(item->tile->image, priv->view.tile.hasAlpha);

Was there any problem you noticed with this not being set before?

[JungJik Lee]

Widget does not have exact rectangle shape, some of them has round shape. so it needs alpha set API not to show background image.
If you do not want to rename the API, I will change them. However IMO, I like to have the exact working code and naming. when it need to be changed, it would be better to change them at that moment and refactor more frequently if it is possible.

[Raphael Kubo da Costa (:rakuco)]

Please use some context in your replies so it's easier to see what part of previous comments you're referring to.

(In reply to comment #9)
> Widget does not have exact rectangle shape, some of them has round shape. so it needs alpha set API not to show background image.

OK.

> If you do not want to rename the API, I will change them. However IMO, I like to have the exact working code and naming. when it need to be changed, it would be better to change them at that moment and refactor more frequently if it is possible.

That makes sense if we consider our own needs, I'm trying to think from a third party developer's perspective, who may need to subclass one of the backing store smart objects and will then be unable to reimplement some part of the API in case he needs to.

[JungJik Lee]

(In reply to comment #10)
> Please use some context in your replies so it's easier to see what part of previous comments you're referring to.
Next time I will reply the answer more carefully.
> 
> (In reply to comment #9)
> > Widget does not have exact rectangle shape, some of them has round shape. so it needs alpha set API not to show background image.
> 
> OK.
> 
> > If you do not want to rename the API, I will change them. However IMO, I like to have the exact working code and naming. when it need to be changed, it would be better to change them at that moment and refactor more frequently if it is possible.
> 
> That makes sense if we consider our own needs, I'm trying to think from a third party developer's perspective, who may need to subclass one of the backing store smart objects and will then be unable to reimplement some part of the API in case he needs to.
I understand your thought and if you think the API is subclassing API, I think the function prototype should be preserved. I misunderstood it. I will recover the API name.

[JungJik Lee]

Created attachment 116765 [details]
restore the API name

Add bg_color_set API into tiled backing store and restore the API name from the previous patch.

[Raphael Kubo da Costa (:rakuco)]

LGTM.

[Chang Shu]

Comment on attachment 116765 [details]
restore the API name

LGTM

[WebKit Review Bot]

Comment on attachment 116765 [details]
restore the API name

Clearing flags on attachment: 116765

Committed r101585: <http://trac.webkit.org/changeset/101585>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.