Bug 54392 - [EFL] Adjust functions of RenderThemeEfl.cpp to WebKit parameter style
Summary: [EFL] Adjust functions of RenderThemeEfl.cpp to WebKit parameter style
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on: 54693
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-14 08:01 PST by Gyuyoung Kim
Modified: 2011-03-03 22:05 PST (History)
8 users (show)

See Also:


Attachments
Patch (4.74 KB, patch)
2011-02-14 08:07 PST, Gyuyoung Kim
tonikitoo: review+
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Patch (4.66 KB, patch)
2011-02-14 14:13 PST, Gyuyoung Kim
mrobinson: review-
Details | Formatted Diff | Diff
Patch (42.26 KB, patch)
2011-03-03 16:21 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2011-02-14 08:01:19 PST
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.
Comment 1 Gyuyoung Kim 2011-02-14 08:07:02 PST
Created attachment 82316 [details]
Patch
Comment 2 Antonio Gomes 2011-02-14 08:36:57 PST
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
Comment 3 Antonio Gomes 2011-02-14 08:37:32 PST
(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 :)
Comment 4 Gyuyoung Kim 2011-02-14 14:13:33 PST
Created attachment 82360 [details]
Patch
Comment 5 Gyuyoung Kim 2011-02-14 15:41:35 PST
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.
Comment 6 Antonio Gomes 2011-02-17 06:09:49 PST
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 ...
Comment 7 Kent Tamura 2011-02-17 06:43:18 PST
I like the original code.  Abbreviations are not good.
Comment 8 Martin Robinson 2011-02-17 15:23:23 PST
Comment on attachment 82360 [details]
Patch

Moving to abbreviations is not the right way to go here. :)
Comment 9 Gyuyoung Kim 2011-03-03 16:21:04 PST
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
Comment 10 Eric Seidel (no email) 2011-03-03 16:24:06 PST
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?
Comment 11 Kent Tamura 2011-03-03 16:30:28 PST
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'.
Comment 12 Gyuyoung Kim 2011-03-03 16:38:39 PST
(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.
Comment 13 Gyuyoung Kim 2011-03-03 16:39:56 PST
(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.
Comment 14 WebKit Commit Bot 2011-03-03 21:51:04 PST
Comment on attachment 84649 [details]
Patch

Clearing flags on attachment: 84649

Committed r80324: <http://trac.webkit.org/changeset/80324>
Comment 15 WebKit Commit Bot 2011-03-03 21:51:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 WebKit Review Bot 2011-03-03 22:05:51 PST
http://trac.webkit.org/changeset/80324 might have broken EFL Linux Release (Build)