Summary: | [EFL] Modify type of both col and row parameters for backing store's internal api. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | KwangHyuk <hyuki.kim> | ||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | antognolli+webkit, gyuyoung.kim, leandro, lucas.de.marchi, rakuco, ryuan.choi, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Other | ||||||||
OS: | Linux | ||||||||
Attachments: |
|
Description
KwangHyuk
2011-07-27 21:48:30 PDT
Created attachment 102227 [details]
Patch
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. Created attachment 102242 [details]
Patch
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. Shall i get your opinion for the 2nd patch, :-) Comment on attachment 102242 [details]
Patch
Informal r+.
Comment on attachment 102242 [details]
Patch
Hopefully callers never pass negative values to these functions.
Comment on attachment 102242 [details] Patch Clearing flags on attachment: 102242 Committed r93399: <http://trac.webkit.org/changeset/93399> All reviewed patches have been landed. Closing bug. |