Bug 54392 – [EFL] Adjust functions of RenderThemeEfl.cpp to WebKit parameter style

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

[Gyuyoung Kim]

In Bug 53906, I add dummy functions to RenderThemeEFl.cpp. But, the added function's parameter is different from other's.

Other function :
    bool RenderThemeEfl::paintSearchFieldResultsButton(RenderObject* o, const PaintInfo& i, const IntRect& rect) 
    bool RenderThemeEfl::paintSearchFieldResultsDecoration(RenderObject* o, const PaintInfo& i, const IntRect& rect) 
    bool RenderThemeEfl::paintSearchFieldCancelButton(RenderObject* o, const PaintInfo& i, const IntRect& rect)
    ...   

Added functions :
    bool RenderThemeEfl::paintMediaMuteButton(RenderObject* renderObject, const PaintInfo& paintInfo, const IntRect& re ct)
    bool RenderThemeEfl::paintMediaPlayButton(RenderObject* renderObject, const PaintInfo& paintInfo, const IntRect& rect)
    ...

I'd like to change "paintInfo" with "i" in order to sync with other functions.

[Gyuyoung Kim]

Created attachment 82316 [details]
Patch

[Antonio Gomes]

Comment on attachment 82316 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Change pamater name of new functions.

type: pamater

[Antonio Gomes]

(In reply to comment #2)
> (From update of attachment 82316 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82316&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Change pamater name of new functions.
> 
> type: pamater

typo even :)

[Gyuyoung Kim]

Created attachment 82360 [details]
Patch

[Gyuyoung Kim]

Hello Antonio :-)

Thank you for your review. I fix wrong spelling. And, I change parameter name one more thing. Other functions are using "o" for RenderObject* parameter. So, I change renderObject with o as well.

[Antonio Gomes]

Comment on attachment 82360 [details]
Patch

After thinking a bit more about this, we try to avoid abreviations in the source code. Maybe we should renamed it the other way around? Also, most of the parameters are ununsed anyways.

I know other ports also abreviate variable names in their respective RenderThemeXXX methods, but ...

[Kent Tamura]

I like the original code.  Abbreviations are not good.

[Martin Robinson]

Comment on attachment 82360 [details]
Patch

Moving to abbreviations is not the right way to go here. :)

[Gyuyoung Kim]

Created attachment 84649 [details]
Patch

Functions of RenderThemeEfl.cpp have efl coding style. But, WebCore's functions should adhere WebKit coding style.
So, I modify the efl style parameter as below,

- EvasObject* o -> EvasObject* object
- ThemePartCacheEntry* ce -> ThemePartCacheEntry* entry
- PaintInfo& i -> PaintInfo& info
- RenderObject* o -> RenderObject* object

[Eric Seidel (no email)]

Comment on attachment 84649 [details]
Patch

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

Yay!  LGTM.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:1119
> +bool RenderThemeEfl::paintMediaCurrentTime(RenderObject* object, const PaintInfo& info, const IntRect& rect)

Why info instead of paintInfo?

[Kent Tamura]

Comment on attachment 84649 [details]
Patch

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

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:119
> +    struct ThemePartCacheEntry *entry = new struct ThemePartCacheEntry;

nit: We had better use OwnPtr<ThemePartCacheEntry> to avoid 'delete'.

[Gyuyoung Kim]

(In reply to comment #10)
> (From update of attachment 84649 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84649&action=review
> 
> Yay!  LGTM.
> 
> > Source/WebCore/platform/efl/RenderThemeEfl.cpp:1119
> > +bool RenderThemeEfl::paintMediaCurrentTime(RenderObject* object, const PaintInfo& info, const IntRect& rect)
> 
> Why info instead of paintInfo?

I'd like to keep sync with other parameters. Other parameters use last name of data type.

[Gyuyoung Kim]

(In reply to comment #12)
> (In reply to comment #10)
> > (From update of attachment 84649 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=84649&action=review
> > 
> > Yay!  LGTM.
> > 
> > > Source/WebCore/platform/efl/RenderThemeEfl.cpp:1119
> > > +bool RenderThemeEfl::paintMediaCurrentTime(RenderObject* object, const PaintInfo& info, const IntRect& rect)
> > 
> > Why info instead of paintInfo?
> 
> I'd like to keep sync with other parameters. Other parameters use last name of data type.

Thank you for your comment. (In reply to comment #11)
> (From update of attachment 84649 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84649&action=review
> 
> > Source/WebCore/platform/efl/RenderThemeEfl.cpp:119
> > +    struct ThemePartCacheEntry *entry = new struct ThemePartCacheEntry;
> 
> nit: We had better use OwnPtr<ThemePartCacheEntry> to avoid 'delete'.

Thank you for your guidance. This seem to be processed by new bug. I will make a bug for it.

[WebKit Commit Bot]

Comment on attachment 84649 [details]
Patch

Clearing flags on attachment: 84649

Committed r80324: <http://trac.webkit.org/changeset/80324>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/80324 might have broken EFL Linux Release (Build)