Bug 178634 - [Settings] Replace current Settings generation with template file based approach
Summary: [Settings] Replace current Settings generation with template file based approach
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks: 178637
  Show dependency treegraph
 
Reported: 2017-10-21 13:59 PDT by Sam Weinig
Modified: 2017-11-15 12:59 PST (History)
6 users (show)

See Also:


Attachments
Patch (104.63 KB, patch)
2017-10-21 14:15 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (106.82 KB, patch)
2017-10-22 12:56 PDT, Sam Weinig
joepeck: review+
sam: commit-queue?
Details | Formatted Diff | Diff
Patch (107.44 KB, patch)
2017-10-22 18:47 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2017-10-21 13:59:36 PDT
[Settings] Replace current Settings generation with template file based approach
Comment 1 Sam Weinig 2017-10-21 14:15:09 PDT
Created attachment 324504 [details]
Patch
Comment 2 Sam Weinig 2017-10-22 12:56:00 PDT
Created attachment 324531 [details]
Patch
Comment 3 Joseph Pecoraro 2017-10-22 18:29:13 PDT
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.
Comment 4 Sam Weinig 2017-10-22 18:47:21 PDT
Created attachment 324536 [details]
Patch
Comment 5 WebKit Commit Bot 2017-10-22 20:13:37 PDT
Comment on attachment 324536 [details]
Patch

Clearing flags on attachment: 324536

Committed r223828: <https://trac.webkit.org/changeset/223828>
Comment 6 Radar WebKit Bug Importer 2017-11-15 12:59:04 PST
<rdar://problem/35568541>