Bug 68595 - [EFL] Add matrix list to reuse tile matrix for each different zoom level.
Summary: [EFL] Add matrix list to reuse tile matrix for each different zoom level.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-21 23:53 PDT by KwangHyuk
Modified: 2011-11-17 21:52 PST (History)
10 users (show)

See Also:


Attachments
Patch. (11.75 KB, patch)
2011-09-21 23:54 PDT, KwangHyuk
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (11.75 KB, patch)
2011-09-22 06:10 PDT, KwangHyuk
rakuco: review-
Details | Formatted Diff | Diff
Patch. (11.84 KB, patch)
2011-10-14 19:55 PDT, KwangHyuk
no flags Details | Formatted Diff | Diff
Patch. (11.84 KB, application/octet-stream)
2011-10-14 20:21 PDT, KwangHyuk
no flags Details
Patch. (11.84 KB, patch)
2011-10-15 07:57 PDT, KwangHyuk
no flags Details | Formatted Diff | Diff
Patch. (11.84 KB, patch)
2011-10-16 19:43 PDT, KwangHyuk
rakuco: commit-queue-
Details | Formatted Diff | Diff
Patch. (11.82 KB, patch)
2011-10-17 17:47 PDT, KwangHyuk
no flags Details | Formatted Diff | Diff
patch rebased. (12.18 KB, patch)
2011-11-10 20:49 PST, KwangHyuk
no flags Details | Formatted Diff | Diff
Patch rebased. (12.14 KB, patch)
2011-11-16 23:52 PST, KwangHyuk
kenneth: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch rebased. (12.15 KB, patch)
2011-11-17 16:44 PST, 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-09-21 23:53:09 PDT
Matrix list keeps each tiled matrix for different zoom level when zoom level is changed.
And then it will resue the tile matrix previously created by getting it from this matrix list.
Comment 1 KwangHyuk 2011-09-21 23:54:06 PDT
Created attachment 108284 [details]
Patch.
Comment 2 WebKit Review Bot 2011-09-22 01:20:37 PDT
Comment on attachment 108284 [details]
Patch.

Attachment 108284 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9797123

New failing tests:
svg/custom/svg-fonts-word-spacing.html
Comment 3 KwangHyuk 2011-09-22 06:10:53 PDT
Created attachment 108321 [details]
Patch
Comment 4 Raphael Kubo da Costa (:rakuco) 2011-09-22 09:45:50 PDT
Comment on attachment 108321 [details]
Patch

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?
Comment 5 KwangHyuk 2011-09-23 17:15:19 PDT
> 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.

Ok, let me check. :-)

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

Ok, thank you for your correction.

>> Source/WebKit/efl/ewk/ewk_tiled_backing_store.c:119
>> +        Eina_Bool layout:1;
>
> Isn't it possible to use change.model here?

Good point.

>> 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?

Make sense.

>> 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?

Ok, I will try it. :-)
Comment 6 KwangHyuk 2011-10-14 19:55:50 PDT
Created attachment 111121 [details]
Patch.

> 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.

The most of your opinions are applied.

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

Applied.

>> Source/WebKit/efl/ewk/ewk_tiled_backing_store.c:119
>> +        Eina_Bool layout:1;
>
> Isn't it possible to use change.model here?

Applied.

>> 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?

Applied.

>> 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?

Applied.
Comment 7 KwangHyuk 2011-10-14 20:21:52 PDT
Created attachment 111122 [details]
Patch.
Comment 8 KwangHyuk 2011-10-15 07:57:32 PDT
Created attachment 111135 [details]
Patch.
Comment 9 Gyuyoung Kim 2011-10-16 19:33:04 PDT
Comment on attachment 111135 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=111135&action=review

> Source/WebKit/efl/ewk/ewk_tiled_matrix.cpp:283
> +

Unneeded Line.

> Source/WebKit/efl/ewk/ewk_tiled_matrix.cpp:284
> +

ditto.
Comment 10 KwangHyuk 2011-10-16 19:43:17 PDT
Created attachment 111200 [details]
Patch.
Comment 11 Raphael Kubo da Costa (:rakuco) 2011-10-17 08:54:01 PDT
Comment on attachment 111200 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=111200&action=review

It looks much better now. This part of the code is not my area of expertise, but if it works, then I'm OK with it after you fix these small nitpicks below.

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:1493
> +

Extra empty line.

> Source/WebKit/efl/ewk/ewk_tiled_matrix.cpp:186
> + * @param currentZoom current zoom level for the matrix.

"zoomLevel" makes more sense here.
Comment 12 KwangHyuk 2011-10-17 17:47:56 PDT
Created attachment 111359 [details]
Patch.
Comment 13 Raphael Kubo da Costa (:rakuco) 2011-10-17 18:08:45 PDT
LGTM.
Comment 14 Gyuyoung Kim 2011-10-17 20:21:54 PDT
Comment on attachment 111359 [details]
Patch.

LGTM too.
Comment 15 KwangHyuk 2011-11-10 20:49:50 PST
Created attachment 114618 [details]
patch rebased.
Comment 16 KwangHyuk 2011-11-16 23:52:30 PST
Created attachment 115535 [details]
Patch rebased.
Comment 17 Kenneth Rohde Christiansen 2011-11-17 03:01:07 PST
Comment on attachment 115535 [details]
Patch rebased.

rs=me given the informal reviews and as this is very EFL only
Comment 18 WebKit Review Bot 2011-11-17 04:09:07 PST
Comment on attachment 115535 [details]
Patch rebased.

Rejecting attachment 115535 [details] from commit-queue.

New failing tests:
platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html
Full output: http://queues.webkit.org/results/10461130
Comment 19 KwangHyuk 2011-11-17 16:44:47 PST
Created attachment 115709 [details]
Patch rebased.
Comment 20 Gyuyoung Kim 2011-11-17 21:25:29 PST
Comment on attachment 115709 [details]
Patch rebased.

LGTM too.
Comment 21 WebKit Review Bot 2011-11-17 21:52:49 PST
Comment on attachment 115709 [details]
Patch rebased.

Clearing flags on attachment: 115709

Committed r100724: <http://trac.webkit.org/changeset/100724>
Comment 22 WebKit Review Bot 2011-11-17 21:52:56 PST
All reviewed patches have been landed.  Closing bug.