Ewk_Tiled_Matrix contains a lot of C'ism and does not always follow coding style. We should clean it up.
Created attachment 163848 [details] Patch
Could someone please take a look at this one?
Comment on attachment 163848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163848&action=review Good to me. I found some small things which you may like. > Source/WebKit/efl/ewk/ewk_tiled_matrix.cpp:252 > if (iterator->zoom != zoom) > continue; > - entry = iterator; > - tileMatrix->matrices = eina_inlist_promote(tileMatrix->matrices, EINA_INLIST_GET(entry)); > - tileMatrix->matrix = entry->matrix; > + > + tileMatrix->matrices = eina_inlist_promote(tileMatrix->matrices, EINA_INLIST_GET(iterator)); > + tileMatrix->matrix = iterator->matrix; > return true; if (iterator->zoom == zoom) { ... return true; } Although it may not be related to C'sm, I think that you like this more. > Source/WebKit/efl/ewk/ewk_tiled_matrix.cpp:579 > + unsigned long col = info->col; > + unsigned long row = info->row; > > Ewk_Tile* tile = static_cast<Ewk_Tile*>(eina_matrixsparse_data_idx_get(tileMatrix->matrix, row, col)); What do you think about passing them instead of making local variable? > Source/WebKit/efl/ewk/ewk_tiled_matrix.cpp:628 > + bool last_empty = false; lastEmpty looks correct for webkit style.
(In reply to comment #3) > (From update of attachment 163848 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163848&action=review > > Good to me. > > I found some small things which you may like. > > > Source/WebKit/efl/ewk/ewk_tiled_matrix.cpp:252 > > if (iterator->zoom != zoom) > > continue; > > - entry = iterator; > > - tileMatrix->matrices = eina_inlist_promote(tileMatrix->matrices, EINA_INLIST_GET(entry)); > > - tileMatrix->matrix = entry->matrix; > > + > > + tileMatrix->matrices = eina_inlist_promote(tileMatrix->matrices, EINA_INLIST_GET(iterator)); > > + tileMatrix->matrix = iterator->matrix; > > return true; > > if (iterator->zoom == zoom) { > ... > return true; > } > > Although it may not be related to C'sm, I think that you like this more. > > > Source/WebKit/efl/ewk/ewk_tiled_matrix.cpp:579 > > + unsigned long col = info->col; > > + unsigned long row = info->row; > > > > Ewk_Tile* tile = static_cast<Ewk_Tile*>(eina_matrixsparse_data_idx_get(tileMatrix->matrix, row, col)); > > What do you think about passing them instead of making local variable? > > > Source/WebKit/efl/ewk/ewk_tiled_matrix.cpp:628 > > + bool last_empty = false; > > lastEmpty looks correct for webkit style. Thanks for taking a look Ryuan. I'll improve the patch based on your comments.
Created attachment 164051 [details] Patch Take Ryuan's feedback into consideration.
(In reply to comment #5) > Created an attachment (id=164051) [details] > Patch > > Take Ryuan's feedback into consideration. Looks good to me. Thank you.
Comment on attachment 164051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164051&action=review Looks fine thanks. > Source/WebKit/efl/ewk/ewk_tiled_matrix.cpp:399 > +#ifndef NDEBUG Don't you need to check this routine in normal mode ?
(In reply to comment #7) > (From update of attachment 164051 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164051&action=review > > Looks fine thanks. > > > Source/WebKit/efl/ewk/ewk_tiled_matrix.cpp:399 > > +#ifndef NDEBUG > > Don't you need to check this routine in normal mode ? If you look at the code, it does nothing besides doing extra checks and printing a warning. This is debug code IMO. In release mode, it would only slow us down.
(In reply to comment #8) > If you look at the code, it does nothing besides doing extra checks and printing a warning. This is debug code IMO. In release mode, it would only slow us down. I just wanna verify it. Thanks. :)
Comment on attachment 164051 [details] Patch Clearing flags on attachment: 164051 Committed r128560: <http://trac.webkit.org/changeset/128560>
All reviewed patches have been landed. Closing bug.