Bug 79362 – [EFL] Remove a duplicate allocation of matrix entry.

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

[JungJik Lee]

A matrix is created inside ewk_tile_matrix_new and another matrix is created inside ewk_tile_matrix_zoom_level_set.
So the first matrix is the duplicated one without any use. This patch is for getting rid of a duplicated creation of the matrix.

[JungJik Lee]

Created attachment 129218 [details]
proposal patch

[JungJik Lee]

Created attachment 129455 [details]
patch_1

[JungJik Lee]

Created attachment 130028 [details]
patch_2

[Zoltan Herczeg]

Comment on attachment 130028 [details]
patch_2

Patch looks good, one minor thing:

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

> Source/WebKit/efl/ewk/ewk_tiled_matrix.cpp:371
> +    eina_matrixsparse_size_get(tileMatrix->matrix , rows, columns);

remove space before "rows"

[JungJik Lee]

Created attachment 131304 [details]
patch_3

[Gyuyoung Kim]

Comment on attachment 131304 [details]
patch_3

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

> Source/WebKit/efl/ewk/ewk_tiled_matrix.cpp:238
> + * Find the matrix with the same zoom(@param) and set it as current matrix.

Is @param needed certainly in this comment ?

> Source/WebKit/efl/ewk/ewk_tiled_matrix.cpp:366
> + * Get the current matrix size.

Missing comments related to param, return for this function.

[JungJik Lee]

Comment on attachment 131304 [details]
patch_3

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

>> Source/WebKit/efl/ewk/ewk_tiled_matrix.cpp:238
>> + * Find the matrix with the same zoom(@param) and set it as current matrix.
> 
> Is @param needed certainly in this comment ?

I thought It could make more clear but I could remove it.

>> Source/WebKit/efl/ewk/ewk_tiled_matrix.cpp:366
>> + * Get the current matrix size.
> 
> Missing comments related to param, return for this function.

This API is obvious for purpose. Does it need more description?

[JungJik Lee]

Created attachment 131308 [details]
patch_4

Remove a word on the comment.

[Gyuyoung Kim]

(In reply to comment #7)
 
> This API is obvious for purpose. Does it need more description?

If you want to add function description, in my humble opinion, it is better to adhere doxyzen rule regardless of internal | public.  ewk_view.cpp also added function description as below,

/**
 * @internal                                                              
 *
 * @param ewkView View.                                                   
 * @param message String to show on console.
 * @param lineNumber Line number.                                         
 * @sourceID Source id.
 *
 */ 
void ewk_view_add_console_message(Evas_Object* ewkView, const char* message, unsigned int lineNumber, const char* sourceID)

Grzegorz, how do you think about it ?

[Grzegorz Czajkowski]

(In reply to comment #9)
> (In reply to comment #7)
> > This API is obvious for purpose. Does it need more description?
> If you want to add function description, in my humble opinion, it is better to adhere doxyzen rule regardless of internal | public.  ewk_view.cpp also added function description as below,
> /**
>  * @internal                                                              
>  *
>  * @param ewkView View.                                                   
>  * @param message String to show on console.
>  * @param lineNumber Line number.                                         
>  * @sourceID Source id.
>  *
>  */ 
> void ewk_view_add_console_message(Evas_Object* ewkView, const char* message, unsigned int lineNumber, const char* sourceID)
> Grzegorz, how do you think about it ?

If this function requires documentation I also will use doxygen. Generally backing store doesn't have extended documentation (its purpose is rather for internal use).In my opinion there is no needed to add doxygen documentation for each backing store function.

[JungJik Lee]

Created attachment 131565 [details]
patch_5

Minor fix : remove the comment.

[Gyuyoung Kim]

LGTM.

[Zoltan Herczeg]

Comment on attachment 131565 [details]
patch_5

r=me

[WebKit Review Bot]

Comment on attachment 131565 [details]
patch_5

Clearing flags on attachment: 131565

Committed r110556: <http://trac.webkit.org/changeset/110556>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.