[webkit-reviews] review denied: [Bug 68595] [EFL] Add matrix list to reuse tile matrix for each different zoom level. : [Attachment 108321] Patch

[bugzilla-daemon at webkit.org]

Raphael Kubo da Costa <kubo at profusion.mobi> has denied KwangHyuk
<hyuki.kim at samsung.com>'s request for review:
Bug 68595: [EFL] Add matrix list to reuse tile matrix for each different zoom
level.
https://bugs.webkit.org/show_bug.cgi?id=68595

Attachment 108321: Patch
https://bugs.webkit.org/attachment.cgi?id=108321&action=review

------- Additional Comments from Raphael Kubo da Costa <kubo at profusion.mobi>
View in context: https://bugs.webkit.org/attachment.cgi?id=108321&action=review


The idea sounds good, but the implementation needs some love. As we're trying
to adequate ourselves to WebKit's coding style, it would be really good if
these one or two-letter variables had proper names. There are many separate
places which allocate and remove entries from the list, this code should all be
put in single functions.

> Source/WebKit/efl/ChangeLog:7
> +	   And then it will resue the tile matrix previously created by getting
it from this matrix list.

resue -> reuse

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.c:119
> +	   Eina_Bool layout:1;

Isn't it possible to use change.model here?

> Source/WebKit/efl/ewk/ewk_tiled_matrix.c:72
> +Ewk_Tile_Matrix_Entry *ewk_tile_matrix_matrix_from_zoom_get(Ewk_Tile_Matrix
*tm, float zoom);

The implementation can be moved here and the function can be made static.
"from_zoom" doesn't sound good in the name. Maybe just
_ewk_tile_matrix_matrix_get, or _ewk_tile_matrix_get?

> Source/WebKit/efl/ewk/ewk_tiled_matrix.h:33
> +void ewk_tile_matrix_change_matrix(Ewk_Tile_Matrix* tm, float zoom);

This name doesn't tell what the function is supposed to do. How about
ewk_tile_matrix_zoom_level_set() or something along these lines?