Bug 96638 – [EFL] Remove a lot of C'ism from Ewk_Tiled_Matrix

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

[Chris Dumez]

Ewk_Tiled_Matrix contains a lot of C'ism and does not always follow coding style. We should clean it up.

[Chris Dumez]

Created attachment 163848 [details]
Patch

[Chris Dumez]

Could someone please take a look at this one?

[Ryuan Choi]

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.

[Chris Dumez]

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

[Chris Dumez]

Created attachment 164051 [details]
Patch

Take Ryuan's feedback into consideration.

[Ryuan Choi]

(In reply to comment #5)
> Created an attachment (id=164051) [details]
> Patch
> 
> Take Ryuan's feedback into consideration.

Looks good to me.
Thank you.

[Gyuyoung Kim]

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 ?

[Chris Dumez]

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

[Gyuyoung Kim]

(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. :)

[WebKit Review Bot]

Comment on attachment 164051 [details]
Patch

Clearing flags on attachment: 164051

Committed r128560: <http://trac.webkit.org/changeset/128560>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.