Bug 69073 – [EFL] Change efl style parameter variables with WebKit coding Style.

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

[Gyuyoung Kim]

As you know, EFL port has used one-letter parameter for public / private functions. But, this has be more difficult for other reviewers to review EFL patches as other efl coding style.

I would like to suggest two ideas for this bug.

1. Use meaningful variable instead of one-letter variable in both .h and .cpp files

2. Only use one-letter variable in public APIs definition. For example,
   
    ewk_view.h
        EAPI Eina_Bool    ewk_view_setting_scripts_can_close_windows_set(Evas_Object *o, Eina_Bool allow);
 
    ewk_view.cpp
        Eina_Bool ewk_view_setting_scripts_can_close_windows_set(Evas_Object* object, Eina_Bool allow)
        {
            EWK_VIEW_SD_GET_OR_RETURN(object, sd, EINA_FALSE);
            EWK_VIEW_PRIV_GET_OR_RETURN(sd, priv, EINA_FALSE);
            ...

I'd like to listen you guys opinions.

[Raphael Kubo da Costa (:rakuco)]

`Evas_Object* object' isn't very meaningful IMO. What I'm doing in DumpRenderTreeSupport is using a name which reflects the use of the given Evas_Object, such as `Evas_Object* ewkView' or `Evas_Object* ewkFrame'.

[Gyuyoung Kim]

Hmm, ewkFrame variable name was already landed in Bug 67114. I think this is not bad. How do you think about this ?

CC'ing ryuan, Łukasz, Grzegorz.

[Ryuan Choi]

(In reply to comment #2)
> Hmm, ewkFrame variable name was already landed in Bug 67114. I think this is not bad. How do you think about this ?
> 
> CC'ing ryuan, Łukasz, Grzegorz.

I agree with kubo's suggestion.

[Lucas De Marchi]

(In reply to comment #1)
> `Evas_Object* object' isn't very meaningful IMO. What I'm doing in DumpRenderTreeSupport is using a name which reflects the use of the given Evas_Object, such as `Evas_Object* ewkView' or `Evas_Object* ewkFrame'.

seems good.

[Gyuyoung Kim]

Created attachment 109731 [details]
Prototype

I make a prototype patch for this bug. Major Changes are as below,

  1. Use ewkFrame instead of o in ewk_frame.cpp
  2. Use ewkView instead of o in ewk_view.cpp
  3. Use menu and item instead of o in ewk_contextmenu.cpp
  4. Use ewkTile instead of o in ewk_tile_xxx.cpp
  5. Use tile parameter name instead of t parameter in ewk_tile_xxx.cpp
  6. Run demarchi's script for ewk_tile_xxx.cpp files.
  7. Remove *void* parameter in ewk_tile_xxx.cpp's internal functions.
  
I think we have to change r, w and h with rect, width and height. But, I don't change them all in this prototype patch yet. If you guys agree with this changes, I will change them as well.

[Lucas De Marchi]

Comment on attachment 109731 [details]
Prototype

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

Too big to be carefully reviewed. But it looks good to me. If you build this and it's still passing the same tests, I'm ok with it.

> Source/WebKit/efl/ewk/ewk_frame.cpp:471
> -Eina_Bool ewk_frame_text_search(const Evas_Object* o, const char* string, Eina_Bool case_sensitive, Eina_Bool forward, Eina_Bool wrap)
> +Eina_Bool ewk_frame_text_search(const Evas_Object* ewkFrame, const char* string, Eina_Bool case_sensitive, Eina_Bool forward, Eina_Bool wrap)

Unrelated, but "string" is a bad name.

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:49
> -    Ewk_Tile *tile;
> +    Ewk_Tile* tile;

Humn... this was not done before because these files were .c

It's fine to do it now, in the same patch. Otherwise this coding-style fixes will never end.

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:816
> -    cols = 1 + (int)ceil((float)w / (float)tw);
> -    rows = 1 + (int)ceil((float)h / (float)th);
> +    cols = 1 + (int)ceil((float)width / (float)tw);
> +    rows = 1 + (int)ceil((float)height / (float)th);

It seems like we should apply the past fixes now that this file is cpp.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #6)
> (From update of attachment 109731 [details])
> > Source/WebKit/efl/ewk/ewk_frame.cpp:471
> > -Eina_Bool ewk_frame_text_search(const Evas_Object* o, const char* string, Eina_Bool case_sensitive, Eina_Bool forward, Eina_Bool wrap)
> > +Eina_Bool ewk_frame_text_search(const Evas_Object* ewkFrame, const char* string, Eina_Bool case_sensitive, Eina_Bool forward, Eina_Bool wrap)
> 
> Unrelated, but "string" is a bad name.

"string" should not be used at all. Sooner or later it will end up conflicting with std::string.

[Gyuyoung Kim]

Created attachment 109900 [details]
Prototype

Update this prototype fixed with you guys pointed out.

[Gyuyoung Kim]

(In reply to comment #8)
> Created an attachment (id=109900) [details]
> Prototype
> 
> Update this prototype fixed with you guys pointed out.

I replace string with text.

[Raphael Kubo da Costa (:rakuco)]

I doubt anyone will ever read this giant patch in its entirety. The only thing I could notice is that the (*cb) -> (* cb) change isn't really correct (if you're really in the mood, you could rename cb to callback, by the way).

Other than that, I can only guess it's OK.

[Gyuyoung Kim]

Created attachment 110118 [details]
Prototype

[Gyuyoung Kim]

(In reply to comment #10)
> I doubt anyone will ever read this giant patch in its entirety. The only thing I could notice is that the (*cb) -> (* cb) change isn't really correct (if you're really in the mood, you could rename cb to callback, by the way).
> 
> Other than that, I can only guess it's OK.

Ok, I modify (* cb) with (*cb).  In addition, I change meaningless parameters of function with meaning's. Because, there are too many meaningless parameters in function parameters. I couldn't ignore them. However, this prototype only focuses on function's parameters. I think local variables and "_"(underbar) parameter variables should be changed by new bugs.

  1. Use ewkFrame instead of o in ewk_frame.cpp
  2. Use ewkView instead of o in ewk_view.cpp
  3. Use menu and item instead of o in ewk_contextmenu.cpp
  4. Use ewkTile instead of o in ewk_tile_xxx.cpp
  5. Use tile parameter name instead of t parameter in ewk_tile_xxx.cpp
  6. Use smartData instead of sd for Ewk_XXXXX_Smart_Data.
  7. Use width, height instead of w, h.
  8. Use xxxEvent instead of ev for event. (e.g. downEvent, upEvent, wheelEvent)
  9. Use scrollX, scrollY, scrollWidth, scrollHeight, centerX, centerY instead of sx, xy, sw, sh, cx, cy,
  10. Use tileUnusedCache instead of tuc in tiled backing store. 
  11. Run demarchi's script for ewk_tile_xxx.cpp files.
  12. Remove *void* parameter in ewk_tile_xxx.cpp's internal functions.
 
I'm not also sure who can review this *giant* patch. But, Antonio recommended that we need to change coding style as soon as possible despite patch is too huge.

CC'ing Antonio, 
Antonio, is this huge patch able to be reviewed ?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #12)
>   1. Use ewkFrame instead of o in ewk_frame.cpp
>   2. Use ewkView instead of o in ewk_view.cpp
>   3. Use menu and item instead of o in ewk_contextmenu.cpp
>   4. Use ewkTile instead of o in ewk_tile_xxx.cpp
>   5. Use tile parameter name instead of t parameter in ewk_tile_xxx.cpp
>   6. Use smartData instead of sd for Ewk_XXXXX_Smart_Data.
>   7. Use width, height instead of w, h.
>   8. Use xxxEvent instead of ev for event. (e.g. downEvent, upEvent, wheelEvent)
>   9. Use scrollX, scrollY, scrollWidth, scrollHeight, centerX, centerY instead of sx, xy, sw, sh, cx, cy,
>   10. Use tileUnusedCache instead of tuc in tiled backing store. 
>   11. Run demarchi's script for ewk_tile_xxx.cpp files.
>   12. Remove *void* parameter in ewk_tile_xxx.cpp's internal functions.
> 
> I'm not also sure who can review this *giant* patch. But, Antonio recommended that we need to change coding style as soon as possible despite patch is too huge.

It might require some effort, but perhaps having a separate patch for each of the issues you listed above might be the only humanly possible way to get these changes reviewed.

[Antonio Gomes]

guys, since it introduces zaro funcionality changes, we can get it in all at once, by rubber stamping it.

no need to spend weeks on it ;)

[Gyuyoung Kim]

Created attachment 110251 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #14)
> guys, since it introduces zaro funcionality changes, we can get it in all at once, by rubber stamping it.
> 
> no need to spend weeks on it ;)

In .cpp files,  I try to change efl style parameters with webkit style. However, I couldn't near to change local variables together with this patch, because of too giant changes. I will file new bug for local variables as soon as this patch is landed.

Majors changes are listed below,
 
  - Use ewkFrame instead of o parameter in ewk_frame.cpp.
  - Use ewkView instead of o parameter in ewk_view.cpp.
  - Use menu and item instead of o parameter in ewk_contextmenu.cpp.
  - Use ewkTile instead of o parameter in ewk_tile_xxx.cpp.
  - Use tile parameter name instead of t parameter in ewk_tile_xxx.cpp.
  - Use smartData instead of sd parameter for Ewk_XXXX_Smart_Data struct.
  - Use width, height instead of w, h parameter.
  - Use xxxEvent instead of ev parameter for event. (e.g. downEvent, upEvent, wheelEvent)
   - Use scrollX, scrollY, scrollWidth, scrollHeight, centerX, centerY, deltaX, deltaY instead of sx, xy, sw, sh, cx, cy, dx, dy.
   - Use tileUnusedCache instead of tuc parameter in tiled backing store.
   - Use red, green, blue and alpha instead of r,g,b,a.
   - Remove "_" from parameter variable.
   - Run demarchi's coding style script for ewk_tile_xxx.cpp files.
   - Remove *void* parameter in ewk_tile_xxx.cpp's internal functions.

[Gyuyoung Kim]

I will notify webkit-efl mailing list of this changes after landing this patch. In addition, I will prepare coding style guideline for WebKit EFL port after finishing this coding style change work. Then, I will write it to EFLWebKit wiki page.

[Raphael Kubo da Costa (:rakuco)]

The patch is getting bigger and bigger ;)

Some variables still have EFL-style names (w, cw, dx, etc) and some changes might not be needed (foo_bar -> fooBar). But I'm fine if this is committed as-is too, it's just impossible to properly review such a giant patch :)

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 110251 [details]
Patch

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

I've pointed out some issues I found while randomly looking at the patch.

> Source/WebKit/efl/ewk/ewk_js.cpp:700
> +Eina_Hash* ewk_js_object_properties_get(const Ewk_JS_Object* object)

Parameters which are not used should just have their names removed.

> Source/WebKit/efl/ewk/ewk_js.cpp:705
> +const char* ewk_js_object_name_get(const Ewk_JS_Object* object)

Ditto.

> Source/WebKit/efl/ewk/ewk_js.cpp:710
> +void ewk_js_variant_free(Ewk_JS_Variant* variant)

Ditto.

> Source/WebKit/efl/ewk/ewk_js.cpp:714
> +void ewk_js_variant_array_free(Ewk_JS_Variant* variant, int count)

Ditto.

> Source/WebKit/efl/ewk/ewk_js.cpp:718
> +Ewk_JS_Object* ewk_js_object_new(const Ewk_JS_Class_Meta* metaClass)

Ditto.

> Source/WebKit/efl/ewk/ewk_js.cpp:723
> +void ewk_js_object_free(Ewk_JS_Object* object)

Ditto.

> Source/WebKit/efl/ewk/ewk_js.cpp:727
> +Eina_Bool ewk_js_object_invoke(Ewk_JS_Object* object, Ewk_JS_Variant* args, int argCount, Ewk_JS_Variant* result)

Ditto.

> Source/WebKit/efl/ewk/ewk_js.cpp:732
> +Ewk_JS_Object_Type ewk_js_object_type_get(Ewk_JS_Object* object)

Ditto.

> Source/WebKit/efl/ewk/ewk_js.cpp:737
> +void ewk_js_object_type_set(Ewk_JS_Object* object, Ewk_JS_Object_Type type)

Ditto.

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:98
> -        Eina_Bool suspend:1;
> +        Eina_Bool suspend : 1;

Hmm, I think this is not needed.

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:844
> -        Eina_Inlist **start, **end;
> +        Eina_Inlist** start, ** end;
>          start = priv->view.items + old_rows;
>          end = priv->view.items + rows;

Funny pointer placement, the variables can be declared and instantiated at the same time.

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:849
> -                (priv, start, 0, cols);
> +                    (priv, start, 0, cols);

Unrelated?

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:861
> -        Eina_Inlist **start, **end;
> +        Eina_Inlist** start, **end;

Ditto about funny pointer placement.

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:869
> -                    (priv, start, old_cols, todo);
> +                        (priv, start, old_cols, todo);

Unrelated?

> Source/WebKit/efl/ewk/ewk_tiled_model.cpp:142
> +static inline void _ewk_tile_account_allocated(const Ewk_Tile* tile) { }
> +static inline void _ewk_tile_account_freed(const Ewk_Tile* tile) { }

It makes sense to just remove the parameter name here.

> Source/WebKit/efl/ewk/ewk_tiled_model.cpp:153
> +    uint32_t* dst32, * dst32_end, c1;
> +    uint64_t* dst64, * dst64_end, c2;

The pointer declarations look funny now. How about declaring and initializing the values at the same time?

> Source/WebKit/efl/ewk/ewk_view.cpp:598
> -    priv->page_settings->setDefaultFixedFontSize(12);
> -    priv->page_settings->setDefaultFontSize(16);
> +    priv->page_settings->setDefaultFixedFontSize(10);
> +    priv->page_settings->setDefaultFontSize(13);

Unrelated?

[Gyuyoung Kim]

Created attachment 110624 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #19)
> (From update of attachment 110251 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110251&action=review
> 
> I've pointed out some issues I found while randomly looking at the patch.
> 
> > Source/WebKit/efl/ewk/ewk_js.cpp:700
> > +Eina_Hash* ewk_js_object_properties_get(const Ewk_JS_Object* object)
> 
> Parameters which are not used should just have their names removed.
> 

I modify object variable with jsObject. This is more clear and similar to ewk_view and ewk_frame's variable change.

> > Source/WebKit/efl/ewk/ewk_js.cpp:710
> > +void ewk_js_variant_free(Ewk_JS_Variant* variant)
> 
> Ditto.

I modify variant with jsVariant.

> > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:98
> > -        Eina_Bool suspend:1;
> > +        Eina_Bool suspend : 1;
> 
> Hmm, I think this is not needed.

This change comes from Demarchi script. The script was already adjusted to other .cpp files. I think ewk_tiled_backing_store also needs to have this style. 

> > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:844
> > -        Eina_Inlist **start, **end;
> > +        Eina_Inlist** start, ** end;
> >          start = priv->view.items + old_rows;
> >          end = priv->view.items + rows;
> 
> Funny pointer placement, the variables can be declared and instantiated at the same time.


As mentioned in Comment #16, this patch only focuses on variable parameter. But, ewk_tiled_xxx files are more changed by Demarchi coding style script additionally. Although above changes come from demarchi script, I modify them in this patch together.

[Gyuyoung Kim]

Created attachment 110629 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

I don't have much to add to my previous comments. I wonder if it doesn't make sense to run uncrustify in a separate patch, so this one only changes the function parameter names.

[WebKit Review Bot]

Comment on attachment 110629 [details]
Patch

Clearing flags on attachment: 110629

Committed r97329: <http://trac.webkit.org/changeset/97329>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.