Bug 76378 - [EFL] Modify the code to webkit-style and adapt to C++ naming.
Summary: [EFL] Modify the code to webkit-style and adapt to C++ naming.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-16 05:18 PST by JungJik Lee
Modified: 2012-02-06 21:44 PST (History)
7 users (show)

See Also:


Attachments
minor style changes (53.19 KB, patch)
2012-01-16 05:39 PST, JungJik Lee
no flags Details | Formatted Diff | Diff
revised patch (60.67 KB, patch)
2012-01-16 07:39 PST, JungJik Lee
no flags Details | Formatted Diff | Diff
rename callback function (61.66 KB, patch)
2012-01-16 22:13 PST, JungJik Lee
no flags Details | Formatted Diff | Diff
rebased_newly (61.67 KB, patch)
2012-01-19 01:22 PST, JungJik Lee
no flags Details | Formatted Diff | Diff
spelling fixed. (61.67 KB, patch)
2012-01-19 03:34 PST, JungJik Lee
no flags Details | Formatted Diff | Diff
fixed by comments. (59.84 KB, patch)
2012-01-19 04:10 PST, JungJik Lee
no flags Details | Formatted Diff | Diff
new style patch (77.13 KB, patch)
2012-01-29 05:01 PST, JungJik Lee
no flags Details | Formatted Diff | Diff
fix history uri & title name (77.13 KB, patch)
2012-01-29 23:00 PST, JungJik Lee
no flags Details | Formatted Diff | Diff
new style patch (94.24 KB, patch)
2012-01-31 03:48 PST, JungJik Lee
no flags Details | Formatted Diff | Diff
new style patch (94.24 KB, patch)
2012-01-31 18:33 PST, JungJik Lee
no flags Details | Formatted Diff | Diff
fixed style patch (94.22 KB, patch)
2012-02-01 01:05 PST, JungJik Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JungJik Lee 2012-01-16 05:18:38 PST
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.
Comment 1 JungJik Lee 2012-01-16 05:39:10 PST
Created attachment 122620 [details]
minor style changes
Comment 2 KwangHyuk 2012-01-16 06:06:44 PST
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 ?
Comment 3 JungJik Lee 2012-01-16 07:38:05 PST
(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.
Comment 4 JungJik Lee 2012-01-16 07:39:08 PST
Created attachment 122636 [details]
revised patch
Comment 5 Ryuan Choi 2012-01-16 17:21:32 PST
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.
Comment 6 Ryuan Choi 2012-01-16 17:27:23 PST
> 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.
Comment 7 JungJik Lee 2012-01-16 22:11:12 PST
(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!
Comment 8 JungJik Lee 2012-01-16 22:13:26 PST
Created attachment 122714 [details]
rename callback function
Comment 9 Gyuyoung Kim 2012-01-17 02:06:13 PST
Comment on attachment 122714 [details]
rename callback function

Looks good.
Comment 10 JungJik Lee 2012-01-19 01:22:19 PST
Created attachment 123090 [details]
rebased_newly

This patch would be conflicted with recently merged code.
So I rebased it with latest code.
Comment 11 Gyuyoung Kim 2012-01-19 02:01:04 PST
Comment on attachment 123090 [details]
rebased_newly

LGTM.
Comment 12 JungJik Lee 2012-01-19 03:34:26 PST
Created attachment 123096 [details]
spelling fixed.
Comment 13 Ryosuke Niwa 2012-01-19 03:35:52 PST
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?
Comment 14 JungJik Lee 2012-01-19 04:08:01 PST
(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.
Comment 15 JungJik Lee 2012-01-19 04:10:50 PST
Created attachment 123101 [details]
fixed by comments.
Comment 16 Raphael Kubo da Costa (:rakuco) 2012-01-19 05:13:28 PST
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.
Comment 17 JungJik Lee 2012-01-19 06:58:44 PST
(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.
Comment 18 Raphael Kubo da Costa (:rakuco) 2012-01-19 07:06:21 PST
(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?
Comment 19 JungJik Lee 2012-01-19 18:47:59 PST
(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.
Comment 20 Raphael Kubo da Costa (:rakuco) 2012-01-20 04:54:27 PST
(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?
Comment 21 JungJik Lee 2012-01-25 00:14:09 PST
(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.
Comment 22 Gyuyoung Kim 2012-01-25 00:28:35 PST
(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.
Comment 23 Raphael Kubo da Costa (:rakuco) 2012-01-25 06:05:14 PST
(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.
Comment 24 JungJik Lee 2012-01-25 08:33:49 PST
(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.
Comment 25 Raphael Kubo da Costa (:rakuco) 2012-01-25 08:50:57 PST
(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.
Comment 26 Gyuyoung Kim 2012-01-26 21:12:00 PST
(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.
Comment 27 Raphael Kubo da Costa (:rakuco) 2012-01-27 08:39:48 PST
Comment on attachment 123101 [details]
fixed by comments.

I'm clearing the r? and cq? flags while we discuss the patch.
Comment 28 JungJik Lee 2012-01-29 05:01:20 PST
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.
Comment 29 Gyuyoung Kim 2012-01-29 21:02:16 PST
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.
Comment 30 JungJik Lee 2012-01-29 23:00:27 PST
Created attachment 124498 [details]
fix history uri & title name
Comment 31 JungJik Lee 2012-01-31 03:48:25 PST
Created attachment 124703 [details]
new style patch

this is really the last patch about style fix.
Comment 32 WebKit Review Bot 2012-01-31 03:50:44 PST
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.
Comment 33 JungJik Lee 2012-01-31 18:33:37 PST
Created attachment 124866 [details]
new style patch

due to failure on style check, I file the patch again.
Comment 34 Gyuyoung Kim 2012-02-01 00:21:41 PST
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.
Comment 35 JungJik Lee 2012-02-01 01:05:59 PST
Created attachment 124901 [details]
fixed style patch

Use EFL style in ewk_tiled_matrix.h.
Comment 36 JungJik Lee 2012-02-01 04:13:26 PST
(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.
Comment 37 Gyuyoung Kim 2012-02-01 18:31:10 PST
Though patch is huge, almost is good. I hope that there are no coding style nits anymore after this patch. :-)
Comment 38 Andreas Kling 2012-02-06 21:02:44 PST
Comment on attachment 124901 [details]
fixed style patch

rs=me
Comment 39 WebKit Review Bot 2012-02-06 21:44:35 PST
Comment on attachment 124901 [details]
fixed style patch

Clearing flags on attachment: 124901

Committed r106904: <http://trac.webkit.org/changeset/106904>
Comment 40 WebKit Review Bot 2012-02-06 21:44:43 PST
All reviewed patches have been landed.  Closing bug.