Bug 177681 – [Settings] Replace SettingsMacros.h with a generated base class for Settings

• Component: New Bugs
• Status: RESOLVED DUPLICATE

[Sam Weinig]

[Settings] Replace SettingsMacros.h with a generated base class for Settings

[Sam Weinig]

Created attachment 322227 [details]
Patch

[Build Bot]

Attachment 322227 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/TextDirection.h:30:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Scripts/GenerateSettings/GenerateSettingsImplementationFile.py:123:  whitespace before '}'  [pep8/E202] [5]
Total errors found: 2 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 322228 [details]
Patch

[Sam Weinig]

This replaces all the macros used in Settings with a generated base class. Since we are already generating code, it does not really make sense to generate macros.

[Build Bot]

Attachment 322228 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/TextDirection.h:30:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Scripts/GenerateSettings/GenerateSettingsImplementationFile.py:123:  whitespace before '}'  [pep8/E202] [5]
Total errors found: 2 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 322243 [details]
Patch

[Build Bot]

Attachment 322243 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/TextDirection.h:30:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Scripts/GenerateSettings/GenerateSettingsImplementationFile.py:123:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/WebCore/testing/InternalSettings.h:32:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
Total errors found: 3 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 322256 [details]
Patch

[Build Bot]

Attachment 322256 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/TextDirection.h:30:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Scripts/GenerateSettings/GenerateSettingsImplementationFile.py:123:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/WebCore/testing/InternalSettings.h:32:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
Total errors found: 3 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 322259 [details]
Patch

[Build Bot]

Attachment 322259 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/TextDirection.h:30:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Scripts/GenerateSettings/GenerateSettingsImplementationFile.py:123:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/WebCore/testing/InternalSettings.h:32:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
Total errors found: 3 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 322270 [details]
Patch

[Build Bot]

Attachment 322270 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/TextDirection.h:30:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Scripts/GenerateSettings/GenerateSettingsImplementationFile.py:123:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/WebCore/testing/InternalSettings.h:32:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
Total errors found: 3 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

There were two ways I could see removing SettingsMacros.h:
1) Generate a base class for Settings with the contents of SettingsMacros (like we do for InternalSettings with InternalSettingsGenerated)
2) Rename the existing Settings to SettingsBase, and generate Settings it self. This might have the upside of making additional includes easy, as you could always put them in the hand rolled SettingsBase.

I went with option 1 for consistency with InternalSettings, but it may be something to reconsider going forward.

[Sam Weinig]

Created attachment 322271 [details]
Patch

[Build Bot]

Attachment 322271 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/TextDirection.h:30:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Scripts/GenerateSettings/GenerateSettingsImplementationFile.py:123:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/WebCore/testing/InternalSettings.h:32:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
Total errors found: 3 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 322273 [details]
Patch

[Build Bot]

Attachment 322273 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/TextDirection.h:30:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Scripts/GenerateSettings/GenerateSettingsImplementationFile.py:123:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/WebCore/testing/InternalSettings.h:32:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
Total errors found: 3 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 322274 [details]
Patch

[Build Bot]

Attachment 322274 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/TextDirection.h:30:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Scripts/GenerateSettings/GenerateSettingsImplementationFile.py:123:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/WebCore/testing/InternalSettings.h:32:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
Total errors found: 3 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 322278 [details]
Patch

[Build Bot]

Attachment 322278 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/TextDirection.h:30:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Scripts/GenerateSettings/GenerateSettingsImplementationFile.py:123:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/WebCore/testing/InternalSettings.h:32:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
Total errors found: 3 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 322292 [details]
Patch

[Build Bot]

Attachment 322292 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/TextDirection.h:30:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Scripts/GenerateSettings/GenerateSettingsImplementationFile.py:123:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/WebCore/testing/InternalSettings.h:32:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
Total errors found: 3 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 322295 [details]
Patch

[Build Bot]

Attachment 322295 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/TextDirection.h:30:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Scripts/GenerateSettings/GenerateSettingsImplementationFile.py:123:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/WebCore/testing/InternalSettings.h:32:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
Total errors found: 3 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 322301 [details]
Patch

[Build Bot]

Attachment 322301 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/TextDirection.h:30:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Scripts/GenerateSettings/GenerateSettingsImplementationFile.py:123:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/WebCore/testing/InternalSettings.h:32:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
Total errors found: 3 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 322301 [details]
Patch

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

I am not sure about the build failure on Windows.

> Source/WebCore/ChangeLog:16
> +          exists for the IDL generator, and expect and enum to be in a header of the

typo: "and" -> "an"

> Source/WebCore/Scripts/GenerateSettings/GenerateSettingsHeaderFile.py:106
> +    if setting.type == 'IntSize':
> +        return "\"IntSize.h\""
> +
> +    # Default to using the type name for the include
> +    return "\"" + setting.type + ".h\""

Looks like the IntSize special case is not needed.

> Source/WebCore/Scripts/GenerateSettings/GenerateSettingsHeaderFile.py:148
> +    # FIXME: When webcoreExport is "", line has extra space.

I don’t understand. That does not seem to be true.

[Sam Weinig]

(In reply to Darin Adler from comment #29)
> Comment on attachment 322301 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322301&action=review
> 
> I am not sure about the build failure on Windows.

Yeah, me either. I'll let it run again, and check the bots. A few of the older patches worked on windows, and I was mostly just fixing Mac / iOS specific issues since then, so I have feeling it is not me, but I will figure it out.

> 
> > Source/WebCore/ChangeLog:16
> > +          exists for the IDL generator, and expect and enum to be in a header of the
> 
> typo: "and" -> "an"

Will fix.

> 
> > Source/WebCore/Scripts/GenerateSettings/GenerateSettingsHeaderFile.py:106
> > +    if setting.type == 'IntSize':
> > +        return "\"IntSize.h\""
> > +
> > +    # Default to using the type name for the include
> > +    return "\"" + setting.type + ".h\""
> 
> Looks like the IntSize special case is not needed.

Oh, indeed!

> 
> > Source/WebCore/Scripts/GenerateSettings/GenerateSettingsHeaderFile.py:148
> > +    # FIXME: When webcoreExport is "", line has extra space.
> 
> I don’t understand. That does not seem to be true.

Ha. That's a bug from the old SettingsMacro code that had that comment and I must have copied it over. I was so happy to fix this bug.

Thanks for the review.

[Sam Weinig]

Created attachment 322304 [details]
Patch

[Build Bot]

Attachment 322304 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/TextDirection.h:30:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Scripts/GenerateSettings/GenerateSettingsImplementationFile.py:123:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/WebCore/testing/InternalSettings.h:32:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
Total errors found: 3 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 322304 [details]
Patch

Clearing flags on attachment: 322304

Committed r222686: <http://trac.webkit.org/changeset/222686>

[Ryan Haddad]

This change seems to have introduced a crash, seen here with http/tests/security/frameNavigation/xss-DENIED-targeted-link-navigation.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x0000000108e6959a WebCore::TimerBase::setNextFireTime(WTF::MonotonicTime) + 106 (Timer.cpp:379)
1   com.apple.WebCore             	0x0000000108e602f7 WebCore::ThreadTimers::sharedTimerFiredInternal() + 167 (ThreadTimers.cpp:118)
2   com.apple.WebCore             	0x000000010886629f WebCore::timerFired(__CFRunLoopTimer*, void*) + 31 (MainThreadSharedTimerCF.cpp:75)
3   com.apple.CoreFoundation      	0x00007fffa4d34e04 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
4   com.apple.CoreFoundation      	0x00007fffa4d34a93 __CFRunLoopDoTimer + 1075
5   com.apple.CoreFoundation      	0x00007fffa4d345ea __CFRunLoopDoTimers + 298
6   com.apple.CoreFoundation      	0x00007fffa4d2bfc1 __CFRunLoopRun + 2081
7   com.apple.CoreFoundation      	0x00007fffa4d2b544 CFRunLoopRunSpecific + 420
8   DumpRenderTree                	0x000000010506fc09 runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 2557 (DumpRenderTree.mm:2026)
9   DumpRenderTree                	0x000000010506efcc dumpRenderTree(int, char const**) + 3219 (DumpRenderTree.mm:1290)
10  DumpRenderTree                	0x000000010507071c DumpRenderTreeMain(int, char const**) + 1528 (DumpRenderTree.mm:1406)
11  libdyld.dylib                 	0x00007fffba8d9235 start + 1


https://build.webkit.org/results/Apple%20Sierra%20Release%20WK1%20(Tests)/r222711%20(4978)/http/tests/security/frameNavigation/xss-DENIED-targeted-link-navigation-crash-log.txt

[Ryan Haddad]

(In reply to Ryan Haddad from comment #34)
> This change seems to have introduced a crash, seen here with
> http/tests/security/frameNavigation/xss-DENIED-targeted-link-navigation.html

Testing a build with r222686, I can reproduce the crash with this command:
run-webkit-tests --leaks -1 http/tests/security/frameNavigation

I cannot reproduce with a build of r222685.

[Ryan Haddad]

Rolled out this change (and dependent changes) in https://trac.webkit.org/changeset/222806/webkit because of the impact to EWS.

[Sam Sneddon [:gsnedders]]

Relanded at r223607.

*** This bug has been marked as a duplicate of bug 178419 ***