Bug 76378 – [EFL] Modify the code to webkit-style and adapt to C++ naming.

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

[JungJik Lee]

I still could find some wrong webkit-style code and C style naming code.
The logic of the code is not touched. I just have changed the style.

[JungJik Lee]

Created attachment 122620 [details]
minor style changes

[KwangHyuk]

Looks fine, but small fix would be required.

> Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:1974
> +        const unsigned long column = info->col;

Why don't you replace info with more meaningful term ?

> Source/WebKit/efl/ewk/ewk_tiled_model.cpp:350
> +        render_start = (double)timev.tv_sec +

Will you let render_start allow ?

> Source/WebKit/efl/ewk/ewk_view.cpp:259
> +    Ewk_View_Private_Data* ptr = smartData->_priv

Why don't you replace prt ?

[JungJik Lee]

(In reply to comment #2)
> Looks fine, but small fix would be required.
> 
> > Source/WebKit/efl/ewk/ewk_tiled_backing_store.cpp:1974
> > +        const unsigned long column = info->col;
Done!
> 
> Why don't you replace info with more meaningful term ?
> 
> > Source/WebKit/efl/ewk/ewk_tiled_model.cpp:350
> > +        render_start = (double)timev.tv_sec +
> 
> Will you let render_start allow ?
Done!
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:259
> > +    Ewk_View_Private_Data* ptr = smartData->_priv
> 
> Why don't you replace prt ?
Done!
But we still have many abbreviations and underscores. I am going to revise them occasionally and continually.

[JungJik Lee]

Created attachment 122636 [details]
revised patch

[Ryuan Choi]

Comment on attachment 122636 [details]
revised patch

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

In my quick scanning, almost is good.

> Source/WebKit/efl/ewk/ewk_tiled_model.cpp:711
> + * @param tileFreeCb function used to free tiles.

In previous point, we used Callback instead of Cb.
So, how about using same style?

> Source/WebKit/efl/ewk/ewk_view.cpp:242
> +#define EWK_VIEW_TYPE_CHECK(ewkView, ...)                                   \
> +    do {                                                                    \
> +        const char* _tmp_otype = evas_object_type_get(ewkView);             \
> +        const Evas_Smart* _tmp_s = evas_object_smart_smart_get(ewkView);    \
> +        if (EINA_UNLIKELY(!_tmp_s)) {                                       \
> +            EINA_LOG_CRIT                                                   \
> +                ("%p (%s) is not a smart object!", ewkView,                 \
> +                _tmp_otype ? _tmp_otype : "(null)");                        \
> +            return __VA_ARGS__;                                             \
> +        }                                                                   \
> +        const Evas_Smart_Class* _tmp_sc = evas_smart_class_get(_tmp_s);     \
> +        if (EINA_UNLIKELY(!_tmp_sc)) {                                      \
> +            EINA_LOG_CRIT                                                   \
> +                ("%p (%s) is not a smart object!", ewkView,                 \
> +                _tmp_otype ? _tmp_otype : "(null)");                        \
> +            return __VA_ARGS__;                                             \
> +        }                                                                   \
> +        if (EINA_UNLIKELY(_tmp_sc->data != EWK_VIEW_TYPE_STR)) {            \
> +            EINA_LOG_CRIT                                                   \
> +                ("%p (%s) is not of an ewk_view (need %p, got %p)!",        \
> +                ewkView, _tmp_otype ? _tmp_otype : "(null)",                \
> +                EWK_VIEW_TYPE_STR, _tmp_sc->data);                          \
> +            return __VA_ARGS__;                                             \
> +        }                                                                   \

IMO, your changelog is not enough because this is not naming change.

BTW, I want to know any reasons that these macro functions can't be changed to static inline functions.
and if not, I prefer only one space before \ 
I'll share this opinion to webkit-efl-dev.

[Ryuan Choi]

> BTW, I want to know any reasons that these macro functions can't be changed to static inline functions.

please ignore this. this is my wrong opinion, sorry for the confusion.

[JungJik Lee]

(In reply to comment #5)
> (From update of attachment 122636 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122636&action=review
> 
> In my quick scanning, almost is good.
> 
> > Source/WebKit/efl/ewk/ewk_tiled_model.cpp:711
> > + * @param tileFreeCb function used to free tiles.
> 
I've checked it out. Done!

[JungJik Lee]

Created attachment 122714 [details]
rename callback function

[Gyuyoung Kim]

Comment on attachment 122714 [details]
rename callback function

Looks good.

[JungJik Lee]

Created attachment 123090 [details]
rebased_newly

This patch would be conflicted with recently merged code.
So I rebased it with latest code.

[Gyuyoung Kim]

Comment on attachment 123090 [details]
rebased_newly

LGTM.

[JungJik Lee]

Created attachment 123096 [details]
spelling fixed.

[Ryosuke Niwa]

Comment on attachment 123090 [details]
rebased_newly

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

Seems reasonable. r=me provided the change log entry is updated.

> Source/WebKit/efl/ChangeLog:3
> +        [EFL] Adapt C++ style and Modify to webkit-style.

Typo: s/Adapt/Adopt/

> Source/WebKit/efl/ChangeLog:9
> +        Modify the code to webkit-style and adapt to C++ style naming.
> +        The logic of the code is not changed.

You should mention that this patch is about renaming variables to use camelCase and spelling out variable names instead of using abbreviations. In fact, you might want to rename the bug summary accordingly.

> Source/WebKit/efl/ewk/ewk_view.cpp:220
> +#define EWK_VIEW_TYPE_CHECK(ewkView, ...)                                   \
> +    do {                                                                    \

We don't normally align \ like this; we place \ after ) followed by exactly one space.

> Source/WebKit/efl/ewk/ewk_view.cpp:2961
> + * @param window_features Features of the new window being created. If it's @c
> + * 0, it will be created a window with default features.

I don't understand this comment. Is it saying that if @c is 0, it will create a window with default features?

[JungJik Lee]

(In reply to comment #13)
> (From update of attachment 123090 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123090&action=review
> 
> Seems reasonable. r=me provided the change log entry is updated.
> 
> > Source/WebKit/efl/ChangeLog:3
> > +        [EFL] Adapt C++ style and Modify to webkit-style.
> 
> Typo: s/Adapt/Adopt/
> 
it's done.

> > Source/WebKit/efl/ChangeLog:9
> > +        Modify the code to webkit-style and adapt to C++ style naming.
> > +        The logic of the code is not changed.
> 
> You should mention that this patch is about renaming variables to use camelCase and spelling out variable names instead of using abbreviations. In fact, you might want to rename the bug summary accordingly.
> 
Done!

> > Source/WebKit/efl/ewk/ewk_view.cpp:220
> > +#define EWK_VIEW_TYPE_CHECK(ewkView, ...)                                   \
> > +    do {                                                                    \
> 
> We don't normally align \ like this; we place \ after ) followed by exactly one space.
> 
Done!

> > Source/WebKit/efl/ewk/ewk_view.cpp:2961
> > + * @param window_features Features of the new window being created. If it's @c
> > + * 0, it will be created a window with default features.
> 
> I don't understand this comment. Is it saying that if @c is 0, it will create a window with default features?

As far as I know, @c is doxygen keyword to decorate the document. EFL port is documented by doxygen. so it mean if parameter is 0, it will create a window with default feature.

[JungJik Lee]

Created attachment 123101 [details]
fixed by comments.

[Raphael Kubo da Costa (:rakuco)]

Can we just make sure this is the last patch of its kind for the time being? The amount of commits with coding style fixes in the last months is overwhelming, and they have made history navigation and annotation _really_ annoying.

[JungJik Lee]

(In reply to comment #16)
> Can we just make sure this is the last patch of its kind for the time being? The amount of commits with coding style fixes in the last months is overwhelming, and they have made history navigation and annotation _really_ annoying.

I think one or two times left to complete this nuisance job. And I also think it makes difficulties in navigation and annotation. However it doesn't look good when coexisting two different styles. if it's not completed, like a broken window, someone will try to go back in previous style.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #17)
> I think one or two times left to complete this nuisance job.

Isn't it possible to fix everything in this patch?

[JungJik Lee]

(In reply to comment #18)
> (In reply to comment #17)
> > I think one or two times left to complete this nuisance job.
> 
> Isn't it possible to fix everything in this patch?
I looked it over quickly, it is almost over 2000 lines. I think it can not be reviewed.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #19)
> I looked it over quickly, it is almost over 2000 lines. I think it can not be reviewed.

Something smells weird then. We've already gone through no less than 8 bug reports changing the coding style (besides the previous ones which got reverted, contributing to make the history mess even worse).

Were the changes you are proposing (and the ones not included in this patch) supposed to be part of any of the bugs which blocked bug 68209?

[JungJik Lee]

(In reply to comment #20)
> (In reply to comment #19)
> > I looked it over quickly, it is almost over 2000 lines. I think it can not be reviewed.
> 
> Something smells weird then. We've already gone through no less than 8 bug reports changing the coding style (besides the previous ones which got reverted, contributing to make the history mess even worse).
> 
> Were the changes you are proposing (and the ones not included in this patch) supposed to be part of any of the bugs which blocked bug 68209?

I read the blocked patches and there are no duplicated code with mine. So I think this patch is still valid. If you are worrying about messing up the history, I will try to join everything together to one patch by adding more.

[Gyuyoung Kim]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > I looked it over quickly, it is almost over 2000 lines. I think it can not be reviewed.
> > 
> > Something smells weird then. We've already gone through no less than 8 bug reports changing the coding style (besides the previous ones which got reverted, contributing to make the history mess even worse).
> > 
> > Were the changes you are proposing (and the ones not included in this patch) supposed to be part of any of the bugs which blocked bug 68209?
> 
> I read the blocked patches and there are no duplicated code with mine. So I think this patch is still valid. If you are worrying about messing up the history, I will try to join everything together to one patch by adding more.

Though I tried to change coding style at once, it looks there are still unchanged source. I'm really sorry.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > I looked it over quickly, it is almost over 2000 lines. I think it can not be reviewed.
> > 
> > Something smells weird then. We've already gone through no less than 8 bug reports changing the coding style (besides the previous ones which got reverted, contributing to make the history mess even worse).
> > 
> > Were the changes you are proposing (and the ones not included in this patch) supposed to be part of any of the bugs which blocked bug 68209?
> 
> I read the blocked patches and there are no duplicated code with mine. So I think this patch is still valid. If you are worrying about messing up the history, I will try to join everything together to one patch by adding more.

If you have an overall idea of what kind of changes are still needed (eg. '*' location, variable naming etc), please tell us so we can discuss whether it makes more sense to commit everything at once or not.

[JungJik Lee]

(In reply to comment #23)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > (In reply to comment #19)
> > > > I looked it over quickly, it is almost over 2000 lines. I think it can not be reviewed.
> > > 
> > > Something smells weird then. We've already gone through no less than 8 bug reports changing the coding style (besides the previous ones which got reverted, contributing to make the history mess even worse).
> > > 
> > > Were the changes you are proposing (and the ones not included in this patch) supposed to be part of any of the bugs which blocked bug 68209?
> > 
> > I read the blocked patches and there are no duplicated code with mine. So I think this patch is still valid. If you are worrying about messing up the history, I will try to join everything together to one patch by adding more.
> 
> If you have an overall idea of what kind of changes are still needed (eg. '*' location, variable naming etc), please tell us so we can discuss whether it makes more sense to commit everything at once or not.

Sure, I will inform you about this patch or similar implementation in future via mailing-list, IRC or email. Before I compete this patch, I will ask your opinion.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #24)
> Sure, I will inform you about this patch or similar implementation in future via mailing-list, IRC or email. Before I compete this patch, I will ask your opinion.

If you refer to sharing what changes are still needed, it would also be good to list them here in this report so the discussion is complete for future reference.

[Gyuyoung Kim]

(In reply to comment #24)

> Sure, I will inform you about this patch or similar implementation in future via mailing-list, IRC or email. Before I compete this patch, I will ask your opinion.

Though ewk_tiled_xxx header files are not opened as public, they are still using mixed coding style. When you send an email to webkit-efl mailing list, please consider it as well.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 123101 [details]
fixed by comments.

I'm clearing the r? and cq? flags while we discuss the patch.

[JungJik Lee]

Created attachment 124463 [details]
new style patch

I've changed the code by following the rules below.
Change the code to C++ style by using camelCase. 
Give full name to too short variable names.
Correct misspelled words. ex) tilieUnusedCache > tileUnusedCache.
Correct words which mismatches between the comments and parameters.

[Gyuyoung Kim]

Comment on attachment 124463 [details]
new style patch

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

> Source/WebKit/efl/ewk/ewk_history.cpp:258
> +    WTF::String uriString = WTF::String::fromUTF8(uri);

Is *String* unneeded in uriString variable ?

> Source/WebKit/efl/ewk/ewk_history.cpp:259
> +    WTF::String titleString = WTF::String::fromUTF8(title);

ditto.

[JungJik Lee]

Created attachment 124498 [details]
fix history uri & title name

[JungJik Lee]

Created attachment 124703 [details]
new style patch

this is really the last patch about style fix.

[WebKit Review Bot]

Attachment 124703 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   67a004e..148477e  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 106351 = 67a004e6a82b832ecb29c8ac1912a1019be73b2a
r106352 = 148477e5d717c315b77d4ba9b95d975c9bae1c82
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[JungJik Lee]

Created attachment 124866 [details]
new style patch

due to failure on style check, I file the patch again.

[Gyuyoung Kim]

Comment on attachment 124866 [details]
new style patch

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

> Source/WebKit/efl/ewk/ewk_tiled_matrix.h:29
> +Ewk_Tile_Matrix *ewk_tile_matrix_new(Ewk_Tile_Unused_Cache *tileUnusedCache, unsigned long columns, unsigned long rows, float zoomLevel, Evas_Colorspace colorSpace, void (*render_callback)(void *data, Ewk_Tile *tile, const Eina_Rectangle *update), const void *renderData);

Is it possibility to open ewk_tiled_xxx header files for application ? If they never be opened to outer, we don't need to use abbreviation in ewk_tiled_xxx files. But, if there is a few possibility, I think we need to keep this style.

> Source/WebKit/efl/ewk/ewk_tiled_matrix.h:34
> +void ewk_tile_matrix_invalidate(Ewk_Tile_Matrix *tileMatrix);

Hmm, above functions use full name.

[JungJik Lee]

Created attachment 124901 [details]
fixed style patch

Use EFL style in ewk_tiled_matrix.h.

[JungJik Lee]

(In reply to comment #34)
> (From update of attachment 124866 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124866&action=review
> 
> > Source/WebKit/efl/ewk/ewk_tiled_matrix.h:29
> > +Ewk_Tile_Matrix *ewk_tile_matrix_new(Ewk_Tile_Unused_Cache *tileUnusedCache, unsigned long columns, unsigned long rows, float zoomLevel, Evas_Colorspace colorSpace, void (*render_callback)(void *data, Ewk_Tile *tile, const Eina_Rectangle *update), const void *renderData);
> 
> Is it possibility to open ewk_tiled_xxx header files for application ? If they never be opened to outer, we don't need to use abbreviation in ewk_tiled_xxx files. But, if there is a few possibility, I think we need to keep this style.
> 
> > Source/WebKit/efl/ewk/ewk_tiled_matrix.h:34
> > +void ewk_tile_matrix_invalidate(Ewk_Tile_Matrix *tileMatrix);
> 
> Hmm, above functions use full name.

Application is possible to get the cache memory size so that ewk_tiled_XXX can be opened.

[Gyuyoung Kim]

Though patch is huge, almost is good. I hope that there are no coding style nits anymore after this patch. :-)

[Andreas Kling]

Comment on attachment 124901 [details]
fixed style patch

rs=me

[WebKit Review Bot]

Comment on attachment 124901 [details]
fixed style patch

Clearing flags on attachment: 124901

Committed r106904: <http://trac.webkit.org/changeset/106904>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.