Bug 96631

Summary: [EFL] Remove a lot of C'ism from Ewk_Tiled_Backing_Store
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit EFLAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, kenneth, lucas.de.marchi, rakuco, ryuan.choi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Chris Dumez 2012-09-13 04:32:15 PDT
Ewk_Tiled_Backing_Store contains a lot of C'ism and does not always follow coding style. It needs to be cleaned up.
Comment 1 Chris Dumez 2012-09-13 04:34:16 PDT
Created attachment 163838 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-09-13 05:19:39 PDT
Comment on attachment 163838 [details]
Patch

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

LGTM but I guess someone else should have a look, lots of changes, easy to miss something

> Source/WebKit/efl/ChangeLog:9
> +        Remove a lot of C'ism from Ewk_Tiled_Backing_Store and
> +        fix coding style.

For WebKit2 we should really just use the tiled backing store Qt uses. It integrates very well with accelerated compositing etc and is 100% implemented in WebCore

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:202
>          CRITICAL("item->tile=%p, but it should be 0!", item->tile);

Why not using the EINA_ERROR_CRIT ?

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:234
> +    for (int i = 0; i < priv->view.rows; ++i) {

unsigned?

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:239
>          EINA_INLIST_FOREACH(list, item)
>              if (item->tile)
>                  _ewk_tiled_backing_store_tile_dissociate(priv, item, last_used);

This is wrong coding style, should use braces ebcause the content spans multiple actual lines

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:1014
> +    Eina_Inlist** iterator = priv->view.items;
> +    Eina_Inlist** iteratorEnd = iterator + priv->view.rows;

it
end ?

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:1777
>      }
>  
> -    if (!eina_tile_grid_slicer_setup(&renderSlicer,
> -        renderRect->x, renderRect->y, renderRect->w, renderRect->h, tileWidth, tileHeight)) {
> +        Eina_Tile_Grid_Slicer renderSlicer;
> +    if (!eina_tile_grid_slicer_setup(&renderSlicer, renderRect->x, renderRect->y, renderRect->w, renderRect->h, tileWidth, tileHeight)) {

isnt that indented wrongly?
Comment 3 Chris Dumez 2012-09-13 05:57:32 PDT
Comment on attachment 163838 [details]
Patch

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

>> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:202
>>          CRITICAL("item->tile=%p, but it should be 0!", item->tile);
> 
> Why not using the EINA_ERROR_CRIT ?

Well, it's not my code but it seems we use those macros in WK1 EFL. They are defined in ewk_private.h. Basically this macro is a wrapper for EINA_LOG_DOM_CRIT().

>> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:234
>> +    for (int i = 0; i < priv->view.rows; ++i) {
> 
> unsigned?

Hmm. It is defined as a long in the struct :(
I'll switch to "unsigned long" then.
Comment 4 Chris Dumez 2012-09-13 06:59:15 PDT
Created attachment 163863 [details]
Patch

Take Kenneth's feedback into consideration.
Comment 5 WebKit Review Bot 2012-09-13 08:08:07 PDT
Comment on attachment 163863 [details]
Patch

Rejecting attachment 163863 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
  return self.open(self.click(*args, **kwds))
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 203, in open
    return self._mech_open(url, data, timeout=timeout)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 255, in _mech_open
    raise response
webkitpy.thirdparty.autoinstalled.mechanize._response.httperror_seek_wrapper: HTTP Error 500: Internal Server Error

Full output: http://queues.webkit.org/results/13839529
Comment 6 WebKit Review Bot 2012-09-13 08:09:08 PDT
Comment on attachment 163863 [details]
Patch

Clearing flags on attachment: 163863

Committed r128464: <http://trac.webkit.org/changeset/128464>
Comment 7 WebKit Review Bot 2012-09-13 08:09:11 PDT
All reviewed patches have been landed.  Closing bug.