Bug 109875 – Convert 3 settings to use Settings.in

• Component: New Bugs
• Status: RESOLVED FIXED

[Tony Chang]

Convert 3 settings to use Settings.in

[Tony Chang]

Created attachment 188449 [details]
Patch

[Build Bot]

Comment on attachment 188449 [details]
Patch

Attachment 188449 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16570665

[Build Bot]

Comment on attachment 188449 [details]
Patch

Attachment 188449 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16585141

[Tony Chang]

Created attachment 188592 [details]
Patch

[Tony Chang]

Created attachment 189133 [details]
Patch

[Tony Chang]

(In reply to comment #5)
> Created an attachment (id=189133) [details]
> Patch

Retrying on mac and win.

[Tony Chang]

Maybe abarth or rniwa would like to review this?

[Ryosuke Niwa]

Comment on attachment 189133 [details]
Patch

rs=me.

[WebKit Review Bot]

Comment on attachment 189133 [details]
Patch

Clearing flags on attachment: 189133

Committed r143398: <http://trac.webkit.org/changeset/143398>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Comment on attachment 189133 [details]
Patch

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

> Source/WebCore/dom/make_names.pl:1021
> -    if (!MediaPlayer::isAvailable() || (settings && !settings->isMediaEnabled()))
> +    if (!MediaPlayer::isAvailable() || (settings && !settings->mediaEnabled()))

Shouldn't this be isMediaEnabled to match WebKit style?  Are we not generating "is" prefixes for bools?

[Eric Seidel (no email)]

I'm very glad to see this go in, but it seems the settings.in generator may need a style review. :)

[Tony Chang]

(In reply to comment #11)
> (From update of attachment 189133 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189133&action=review
> 
> > Source/WebCore/dom/make_names.pl:1021
> > -    if (!MediaPlayer::isAvailable() || (settings && !settings->isMediaEnabled()))
> > +    if (!MediaPlayer::isAvailable() || (settings && !settings->mediaEnabled()))
> 
> Shouldn't this be isMediaEnabled to match WebKit style?  Are we not generating "is" prefixes for bools?

I'm happy to change the style, but the style guide says getters should match the name of the variable being gotten:
http://www.webkit.org/coding/coding-style.html#names-setter-getter

I could change the variable to isMediaEnabled/setIsMediaEnabled if that sounds better.

[Eric Seidel (no email)]

(In reply to comment #13)
> (In reply to comment #11)
> > (From update of attachment 189133 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=189133&action=review
> > 
> > > Source/WebCore/dom/make_names.pl:1021
> > > -    if (!MediaPlayer::isAvailable() || (settings && !settings->isMediaEnabled()))
> > > +    if (!MediaPlayer::isAvailable() || (settings && !settings->mediaEnabled()))
> > 
> > Shouldn't this be isMediaEnabled to match WebKit style?  Are we not generating "is" prefixes for bools?
> 
> I'm happy to change the style, but the style guide says getters should match the name of the variable being gotten:
> http://www.webkit.org/coding/coding-style.html#names-setter-getter
> 
> I could change the variable to isMediaEnabled/setIsMediaEnabled if that sounds better.

I guess I'm thinking of:
https://www.webkit.org/coding/coding-style.html#names-bool

Which would have these named:
bool m_isMediaEnabled;
void setMediaEnabled(bool);
bool isMediaEnabled()

At least that's my understanding of the style.