Bug 63377

Summary: [EFL] Remove unused code related with priv->render.process_entire_queue in ewk_tiled_backing_store
Product: WebKit Reporter: KwangHyuk <hyuki.kim>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: antognolli+webkit, gyuyoung.kim, gyuyoung.kim, kenneth, leandro, lucas.de.marchi, rakuco, ryuan.choi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
propose a simple patch
none
Apply latest files
none
Patch
kenneth: review-, kenneth: commit-queue-
Patch rebased. none

Description KwangHyuk 2011-06-24 22:48:33 PDT
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.
Comment 1 KwangHyuk 2011-06-26 23:36:10 PDT
Created attachment 98664 [details]
propose a simple patch
Comment 2 KwangHyuk 2011-06-27 03:01:39 PDT
Created attachment 98694 [details]
Apply latest files
Comment 3 Raphael Kubo da Costa (:rakuco) 2011-06-27 06:18:33 PDT
Makes sense to me. Please mark the first patch as obsolete.
Comment 4 Ryuan Choi 2011-06-27 15:27:48 PDT
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.
Comment 5 KwangHyuk 2011-06-27 17:40:28 PDT
Created attachment 98829 [details]
Patch
Comment 6 Ryuan Choi 2011-06-27 17:42:20 PDT
(In reply to comment #5)
> Created an attachment (id=98829) [details]
> Patch

LGTM.
Comment 7 Gyuyoung Kim 2011-06-27 17:47:25 PDT
Comment on attachment 98829 [details]
Patch

Informal r+ on my side.
Comment 8 Kenneth Rohde Christiansen 2011-06-28 01:29:40 PDT
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?
Comment 9 Gyuyoung Kim 2011-06-28 01:43:33 PDT
(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.
Comment 10 KwangHyuk 2011-06-28 01:58:50 PDT
> 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.
Comment 11 Kenneth Rohde Christiansen 2011-06-28 02:02:16 PDT
(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.
Comment 12 KwangHyuk 2011-06-28 02:12:54 PDT
> 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.
Comment 13 Kenneth Rohde Christiansen 2011-06-28 03:59:44 PDT
(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.
Comment 14 Raphael Kubo da Costa (:rakuco) 2011-06-28 06:10:53 PDT
Looks OK to me.
Comment 15 Antonio Gomes 2011-06-28 09:10:31 PDT
> 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.
Comment 16 KwangHyuk 2011-06-28 18:17:11 PDT
> 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.
Comment 17 KwangHyuk 2011-06-28 18:38:44 PDT
> 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.
Comment 18 KwangHyuk 2011-08-24 19:50:57 PDT
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.
Comment 19 WebKit Review Bot 2011-09-01 18:24:09 PDT
Comment on attachment 105124 [details]
Patch rebased.

Clearing flags on attachment: 105124

Committed r94378: <http://trac.webkit.org/changeset/94378>
Comment 20 WebKit Review Bot 2011-09-01 18:24:14 PDT
All reviewed patches have been landed.  Closing bug.