Bug 65302 – [EFL] Modify type of both col and row parameters for backing store's internal api.

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

[KwangHyuk]

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.

[KwangHyuk]

Created attachment 102227 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

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.

[KwangHyuk]

Created attachment 102242 [details]
Patch

[KwangHyuk]

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.

[KwangHyuk]

Shall i get your opinion for the 2nd patch, :-)

[Leandro Pereira]

Comment on attachment 102242 [details]
Patch

Informal r+.

[Adam Roben (:aroben)]

Comment on attachment 102242 [details]
Patch

Hopefully callers never pass negative values to these functions.

[WebKit Review Bot]

Comment on attachment 102242 [details]
Patch

Clearing flags on attachment: 102242

Committed r93399: <http://trac.webkit.org/changeset/93399>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.