Bug 54733

Summary: [EFL] Add coding style exceptions for EFL port
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: WebKit EFLAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, antognolli+webkit, commit-queue, eric, gyuyoung.kim, k.blank, kenneth, leandro, levin, lucas.de.marchi, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 55292    
Attachments:
Description Flags
Patch
none
Patch
none
Proposed Patch
none
Proposed Patch none

Description Gyuyoung Kim 2011-02-18 01:34:50 PST
I fix style errors in below ewk files.

* ewk/ewk_contextmenu.h
* ewk/ewk_cookies.h
* ewk/ewk_history.h
* ewk/ewk_util.h


I'm not sure if this approach is correct. Hello profusion guys, could you let me know your opinions ?
Comment 1 Gyuyoung Kim 2011-02-18 01:36:36 PST
Created attachment 82935 [details]
Patch
Comment 2 Antonio Gomes 2011-02-18 05:23:27 PST
Comment on attachment 82935 [details]
Patch

Is it up for review? LGTM
Comment 3 Gyuyoung Kim 2011-02-20 17:50:06 PST
Comment on attachment 82935 [details]
Patch

I'd like to listen Profusion's guys before requesting review. If you are ok, I'd like to get review for this patch.
Comment 4 Adam Barth 2011-02-21 12:30:49 PST
Comment on attachment 82935 [details]
Patch

Are these files supposed to be in WebKit style or in EFL style?  Generally, public APIs in WebKit tend to be in the style of the embedding system.
Comment 5 Lucas De Marchi 2011-02-21 13:17:47 PST
Comment on attachment 82935 [details]
Patch

I really dislike this. This public header follows the EFL coding style and is already being ignored by the bot.
Comment 6 Gyuyoung Kim 2011-02-21 16:07:11 PST
(In reply to comment #5)
> (From update of attachment 82935 [details])
> I really dislike this. This public header follows the EFL coding style and is already being ignored by the bot.

Hello Lucas,

There are many WebKit style errors in ewk header files. Should we ignore the WebKit style errors ?
Comment 7 Gyuyoung Kim 2011-02-24 03:59:38 PST
(In reply to comment #4)
> (From update of attachment 82935 [details])
> Are these files supposed to be in WebKit style or in EFL style?  Generally, public APIs in WebKit tend to be in the style of the embedding system.

EFL port was implemented by EFL style. But, it seems there are conflicts between efl style and webkit style. If efl port keeps current style, can the webkit style checking rule be changed ?
Comment 8 Adam Barth 2011-02-24 14:06:04 PST
> EFL port was implemented by EFL style. But, it seems there are conflicts between efl style and webkit style. If efl port keeps current style, can the webkit style checking rule be changed ?

Yes.  If you look at how check-webkit-style works, you'll see similar exceptions for GTK, for example.
Comment 9 Gyuyoung Kim 2011-02-24 16:21:38 PST
(In reply to comment #8)
> > EFL port was implemented by EFL style. But, it seems there are conflicts between efl style and webkit style. If efl port keeps current style, can the webkit style checking rule be changed ?
> 
> Yes.  If you look at how check-webkit-style works, you'll see similar exceptions for GTK, for example.

Ok, I will check the exceptions for GTK, I think EFL needs to have exceptions rules similar to GTK. I will report this issue in here after finishing my urgent works. :-) Thank you in advance.
Comment 10 Lucas De Marchi 2011-02-25 04:10:33 PST
(In reply to comment #9)
> (In reply to comment #8)
> > > EFL port was implemented by EFL style. But, it seems there are conflicts between efl style and webkit style. If efl port keeps current style, can the webkit style checking rule be changed ?
> > 
> > Yes.  If you look at how check-webkit-style works, you'll see similar exceptions for GTK, for example.
> 
> Ok, I will check the exceptions for GTK, I think EFL needs to have exceptions rules similar to GTK. I will report this issue in here after finishing my urgent works. :-) Thank you in advance.

Leandro already had added such exceptions last time I checked.


Leandro, what happened?
Comment 11 Leandro Pereira 2011-02-25 06:58:23 PST
(In reply to comment #10)
> 
> Leandro already had added such exceptions last time I checked.
> 
> 
> Leandro, what happened?
>

By a quick inspection (without grep or anything fancier -- I don't have a recent WebKit checkout on this machine), it looks like the CPP checkers were rewritten and the EFL exceptions got lost.
Comment 12 Gyuyoung Kim 2011-02-25 20:00:57 PST
Created attachment 83920 [details]
Patch

I find the exception in checker.py. So, I add additional exception to checker.py. (parameter_name)
I need to revert my previous patches.  Bug 54842, Bug 54718, Bug 54624, Bug 54693. I will revert those after landing this patch.
Comment 13 Gyuyoung Kim 2011-02-25 21:11:24 PST
Created attachment 83921 [details]
Proposed Patch

I add additional efl directories to the exception.
Comment 14 WebKit Review Bot 2011-02-25 21:13:10 PST
Attachment 83921 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1

Tools/Scripts/webkitpy/style/checker.py:166:  trailing whitespace  [pep8/W291] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Gyuyoung Kim 2011-02-25 21:24:33 PST
Created attachment 83925 [details]
Proposed Patch

Fix style error.
Comment 16 WebKit Commit Bot 2011-02-26 07:38:29 PST
Comment on attachment 83925 [details]
Proposed Patch

Clearing flags on attachment: 83925

Committed r79789: <http://trac.webkit.org/changeset/79789>
Comment 17 WebKit Commit Bot 2011-02-26 07:38:36 PST
All reviewed patches have been landed.  Closing bug.