Bug 178634 – [Settings] Replace current Settings generation with template file based approach

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

[Settings] Replace current Settings generation with template file based approach

[Sam Weinig]

Created attachment 324504 [details]
Patch

[Sam Weinig]

Created attachment 324531 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 324531 [details]
Patch

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

Almost all my comments are style suggestions that you should feel free to ignore as we don't really have a strict ruby style.

> Source/WebCore/Scripts/GenerateSettings.rb:48
> +  exit(-1)

Style: Probably not normal ruby style to have parenthesis with exit. They could be dropped.

> Source/WebCore/Scripts/GenerateSettings.rb:75
> +    @type = opts["type"] || "bool"
> +    @initial = opts['initial']

Style: Odd mix of double quoted and single quoted strings throughout this file. Would be good to settle on some kind of consistency

> Source/WebCore/Scripts/GenerateSettings.rb:83
> +  def valueType?()
> +    return @type != 'String' && @type != 'URL'
> +  end

Style: The `return` keyword is not necessary here. In fact, I don't think a single `return` in this file is necessary, and just removing them should get the exact same behavior. Each statement returns a value and functions return the value of the last statement. So this could be written without the return. In this case each return is the last (or part of the last statement) being run.

Style: Parenthesis in the method declarations are not necessary. In cases where there are no parameters it is often clearer to leave them off. For instance here you could write this as just `def valueType?`. You already make use of it below without parenthesis. That said, I do think we should keep parenthesis in method declarations and call sites that take parameters (like the initializer declaration and starts_with? uses below).

> Source/WebCore/Scripts/GenerateSettings.rb:167
> +    conditionalsMap = { }

Style: Weird to have the space in the hash initializer.

> Source/WebCore/Scripts/GenerateSettings.rb:173
> +      if not conditionalsMap[setting.conditional]
> +        conditionalsMap[setting.conditional] = []
> +      end
> +
> +      conditionalsMap[setting.conditional] << setting

Style: Weird to have the `not` instead of just using `!` here.

The loop body might read easier in the Perl style, but maybe we should avoid it:

    conditionalsMap[setting.conditional] = [] if !conditionalsMap[setting.conditional]
    conditionalsMap[setting.conditional] << setting

> Source/WebCore/Scripts/SettingsTemplates/InternalSettingsGenerated.h.erb:51
> +    Page* m_page;

Page&? It is assumed to be non-null and valid.

> Source/WebCore/Scripts/SettingsTemplates/Settings.cpp.erb:55
> +<%- for @setting in @unconditionalBoolSetting do -%>

Are bools are packed at the end to save size? Nice!

> Source/WebCore/Scripts/SettingsTemplates/Settings.cpp.erb:56
> +    , m_<%= @setting.name %>(<%= @setting.initial %>)

Nit: We may want to move us to {} initializers. I've been agnostic but I've seen others moving in this direction.
<https://webkit.org/b/176058> [Code Style] Use uniform initializer syntax in member initializer list

> Source/WebCore/Scripts/SettingsTemplates/Settings.cpp.erb:72
> +Settings::~Settings()
> +{
> +}

Nit: Another recent change just changed all of these to use "= default;". I didn't understand why, but it might be good to move in that direction.
<https://webkit.org/b/178528> Use "= default" to denote default constructor or destructor

> Source/WebCore/Scripts/SettingsTemplates/Settings.cpp.erb:75
> +void Settings::<%= @setting.setterFunctionName() %>(<%= @setting.parameterType %> <%= @setting.name %>)

Style: You normally drop the unnecessary ()s on setterFunctionName

> Source/WebCore/Scripts/SettingsTemplates/Settings.cpp.erb:88
> +void Settings::<%= @setting.setterFunctionName() %>(<%= @setting.parameterType %> <%= @setting.name %>)

Style: You normally drop the unnecessary ()s on setterFunctionName

> Source/WebCore/Scripts/SettingsTemplates/Settings.h.erb:64
> +<% end -%>

Style: You normally do "<%-". I don't think the leading '-'s are actually needed anywhere, but I think you are adding them for readability / balance and I like that it is included.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:19572
>  				BC59DEF8169DEDC30016AC34 /* Settings.in */,
> +				7CDE73961F9BD59500390312 /* Settings.yaml */,

Seems Settings.in should be removed from the pbxproj.

> Source/WebCore/page/Settings.yaml:1
> +# FIXME: Add support for global settings.

Style: Other yaml files in WebKit use 2 space indent.

[Sam Weinig]

Created attachment 324536 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 324536 [details]
Patch

Clearing flags on attachment: 324536

Committed r223828: <https://trac.webkit.org/changeset/223828>

[Radar WebKit Bug Importer]

<rdar://problem/35568541>