Bug 63377 – [EFL] Remove unused code related with priv->render.process_entire_queue in ewk_tiled_backing_store

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

[KwangHyuk]

As priv->render.process_entire_queue affects no logical flow of ewk_tiled_backing_store but setter api, Removing some code related with this field is required.

[KwangHyuk]

Created attachment 98664 [details]
propose a simple patch

[KwangHyuk]

Created attachment 98694 [details]
Apply latest files

[Raphael Kubo da Costa (:rakuco)]

Makes sense to me. Please mark the first patch as obsolete.

[Ryuan Choi]

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

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.c:-1941
> -void ewk_tiled_backing_store_process_entire_queue_set(Evas_Object *o, Eina_Bool value)

I think that you should remove this function in ewk_view_tiled_backing_store.h.

[KwangHyuk]

Created attachment 98829 [details]
Patch

[Ryuan Choi]

(In reply to comment #5)
> Created an attachment (id=98829) [details]
> Patch

LGTM.

[Gyuyoung Kim]

Comment on attachment 98829 [details]
Patch

Informal r+ on my side.

[Kenneth Rohde Christiansen]

Comment on attachment 98829 [details]
Patch

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

Why is the cq set to cq+? That is wrong - the patch is not even reviewed yet.

> Source/WebKit/efl/ChangeLog:8
> +        As priv->render.process_entire_queue affects no logical flow of ewk_tiled_backing_store but setter api,

I don't understand what you are saying here, please rephrase. And please tell why it is fine to remove that setter API?

[Gyuyoung Kim]

(In reply to comment #8)
> (From update of attachment 98829 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98829&action=review
> 
> Why is the cq set to cq+? That is wrong - the patch is not even reviewed yet.

Kenneth, sorry for my cq+. Some bugs was set cq+ before getting r+. Of course, the bug got r+ later. I will never give cq+ first in future.

[KwangHyuk]

> Why is the cq set to cq+? That is wrong - the patch is not even reviewed yet.

I am wondering about it too. Sorry for the confusion, there might be my mistake.

> I don't understand what you are saying here, please rephrase. And please tell  why it is fine to remove that setter API?

As this field might be intended to use for updating dirty tiles in tile matrix at once, but, there was no related code with this field which can be used for any condition check or other logical flow,but, two apis (ewk_view_tiled_process_entire_queue_set, ewk_tiled_backing_store_process_entire_queue_set) that I am trying to remove are just provided as setter api for setting priv->render.process_entire_queue to TRUE or FALSE.
So, It can be set , but it's not effective because no module is using it.

[Kenneth Rohde Christiansen]

(In reply to comment #10)
> > Why is the cq set to cq+? That is wrong - the patch is not even reviewed yet.
> 
> I am wondering about it too. Sorry for the confusion, there might be my mistake.
> 
> > I don't understand what you are saying here, please rephrase. And please tell  why it is fine to remove that setter API?
> 
> As this field might be intended to use for updating dirty tiles in tile matrix at once, but, there was no related code with this field which can be used for any condition check or other logical flow,but, two apis (ewk_view_tiled_process_entire_queue_set, ewk_tiled_backing_store_process_entire_queue_set) that I am trying to remove are just provided as setter api for setting priv->render.process_entire_queue to TRUE or FALSE.
> So, It can be set , but it's not effective because no module is using it.

As an outsider there is no way I can know that, nor that it is OK with all EFL developers. Why was API committed anyway if it is not being used... that means that it is basically dead code.

[KwangHyuk]

> As an outsider there is no way I can know that, nor that it is OK with all EFL developers. Why was API committed anyway if it is not being used... that means that it is basically dead code.

As far as I remember, this field might have any meaning and it might be effective, but, open source is alive and sometimes some logical flow is changed by any developer, as a result, any api seems to be in somewhere else as like dirty pool who nobody won’t visit. :-)

Anyway, we better remove all dead code or apis which was written for future usage.

[Kenneth Rohde Christiansen]

(In reply to comment #12)
> > As an outsider there is no way I can know that, nor that it is OK with all EFL developers. Why was API committed anyway if it is not being used... that means that it is basically dead code.
> 
> As far as I remember, this field might have any meaning and it might be effective, but, open source is alive and sometimes some logical flow is changed by any developer, as a result, any api seems to be in somewhere else as like dirty pool who nobody won’t visit. :-)
> 
> Anyway, we better remove all dead code or apis which was written for future usage.

If you all (everyone working on EFL) agree on that, then i can review such patches. But please be more careful not adding dead code in the future.

[Raphael Kubo da Costa (:rakuco)]

Looks OK to me.

[Antonio Gomes]

> As far as I remember, this field might have any meaning and it might be effective, but, open source is alive and sometimes some logical flow is changed by any developer, as a result, any api seems to be in somewhere else as like dirty pool who nobody won’t visit. :-)
> 
> Anyway, we better remove all dead code or apis which was written for future usage.

You then need to better evaluate your public APIs before making them public. Adding and removing public APIs is costy to both developer, users of the API and reviewers.

[KwangHyuk]

> If you all (everyone working on EFL) agree on that, then i can review such patches. But please be more careful not adding dead code in the future.

You are right, We had better watch out when we add new code If it will be opened for the public.

[KwangHyuk]

> You then need to better evaluate your public APIs before making them public. Adding and removing public APIs is costy to both developer, users of the API and reviewers.

Hi tonikitoo, In fact, from our side, one developer will start categorizing APIs in order to separate APIs which must be opened or not, after this work, there would be small change in regard of public APIs.

[KwangHyuk]

Created attachment 105124 [details]
Patch rebased.

I have announced the removal of one api related with this patch via webkit-efl mailing list.
And there was no objection.

[WebKit Review Bot]

Comment on attachment 105124 [details]
Patch rebased.

Clearing flags on attachment: 105124

Committed r94378: <http://trac.webkit.org/changeset/94378>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.