Bug 65302 - [EFL] Modify type of both col and row parameters for backing store's internal api.
Summary: [EFL] Modify type of both col and row parameters for backing store's internal...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-27 21:48 PDT by KwangHyuk
Modified: 2011-08-19 06:23 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.91 KB, patch)
2011-07-27 21:51 PDT, KwangHyuk
no flags Details | Formatted Diff | Diff
Patch (3.76 KB, patch)
2011-07-28 05:16 PDT, KwangHyuk
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description KwangHyuk 2011-07-27 21:48:30 PDT
As both col and row parameter's type for both _ewk_tiled_backing_store_item_fill and  ewk_tiled_backing_store_item_add are different from each other, I modify type of them.
In addition, in order to increase readability of _ewk_tiled_backing_store_item_fill, some related lines of those parameters are modified.
Comment 1 KwangHyuk 2011-07-27 21:51:48 PDT
Created attachment 102227 [details]
Patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2011-07-28 05:00:42 PDT
IMO the patch needs further investigation.

The data types were changed from _ewk_tiled_backing_store_item_add upwards, but you also need to check which data is passed to it as well (by quickly skimming through the file I can see some long to unsigned int conversions done before item_add is called, for example), so that it is verified that no useless casts are done or whether other data types should be changed. _ewk_tiled_backing_store_view_cols_end_add (which calls item_add) and _ewk_tiled_backing_store_recalc_renderers (which calls end_add) are good starting points.

> Source/WebKit/efl/ChangeLog:7
> +        and  ewk_tiled_backing_store_item_add are different from each other, I modify type of them.

Extra space after "and".

> Source/WebKit/efl/ChangeLog:9
> +        some related lines of those parameters are modified.

From what I see, the changes made were fairly obvious, so this paragraph should not be needed. If you did some changes which were not obvious, it'd be better to mention them below in each function in the ChangeLog instead of here.
Comment 3 KwangHyuk 2011-07-28 05:16:24 PDT
Created attachment 102242 [details]
Patch
Comment 4 KwangHyuk 2011-07-28 05:27:08 PDT
First of all, thank you for your review,


> The data types were changed from _ewk_tiled_backing_store_item_add upwards, but you also need to check which data is passed to it as well (by quickly skimming through the file I can see some long to unsigned int conversions done before item_add is called, for example), so that it is verified that no useless casts are done or whether other data types should be changed. _ewk_tiled_backing_store_view_cols_end_add (which calls item_add) and _ewk_tiled_backing_store_recalc_renderers (which calls end_add) are good starting points.

I agree with you. In order to reduce the type conversions, tiled_matrix and callers data type are better to be changed.
So, I will generate new patch for it soon.

In addition, as you know, eina_matrixsparse is now using unsigned long type for both col and row parameter, so, I am thinking that ewk is also better to use same parameter type for col and row.


> Extra space after "and".
Fixed.

> From what I see, the changes made were fairly obvious, so this paragraph should not be needed.
Fixed.
Comment 5 KwangHyuk 2011-08-09 05:39:25 PDT
Shall i get your opinion for the 2nd patch, :-)
Comment 6 Leandro Pereira 2011-08-09 07:34:12 PDT
Comment on attachment 102242 [details]
Patch

Informal r+.
Comment 7 Adam Roben (:aroben) 2011-08-19 05:25:20 PDT
Comment on attachment 102242 [details]
Patch

Hopefully callers never pass negative values to these functions.
Comment 8 WebKit Review Bot 2011-08-19 06:23:51 PDT
Comment on attachment 102242 [details]
Patch

Clearing flags on attachment: 102242

Committed r93399: <http://trac.webkit.org/changeset/93399>
Comment 9 WebKit Review Bot 2011-08-19 06:23:56 PDT
All reviewed patches have been landed.  Closing bug.