[webkit-reviews] review granted: [Bug 178634] [Settings] Replace current Settings generation with template file based approach : [Attachment 324531] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Oct 22 18:29:13 PDT 2017


Joseph Pecoraro <joepeck at webkit.org> has granted Sam Weinig <sam at webkit.org>'s
request for review:
Bug 178634: [Settings] Replace current Settings generation with template file
based approach
https://bugs.webkit.org/show_bug.cgi?id=178634

Attachment 324531: Patch

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




--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 324531
  --> https://bugs.webkit.org/attachment.cgi?id=324531
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.


More information about the webkit-reviews mailing list