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.
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.
Created attachment 116341 [details] minor_edit.patch
Created attachment 116347 [details] rebased patch attach new rebased patch.
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"
(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. :)
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.
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.
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?
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.
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.
(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.
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.
LGTM.
Comment on attachment 116765 [details] restore the API name LGTM
Comment on attachment 116765 [details] restore the API name Clearing flags on attachment: 116765 Committed r101585: <http://trac.webkit.org/changeset/101585>
All reviewed patches have been landed. Closing bug.