Bug 217962 – Clean up DumpRenderTree in preparation for supporting arbitrary test header commands

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

[Sam Weinig]

Cleanup DumpRenderTree in preparation for supporting arbitrary test header commands

[Sam Weinig]

Created attachment 411872 [details]
Patch

[Sam Weinig]

Created attachment 411873 [details]
Patch

[Sam Weinig]

Created attachment 411893 [details]
Patch

[Sam Weinig]

Created attachment 411895 [details]
Patch

[Sam Weinig]

Created attachment 411902 [details]
Patch

[Sam Weinig]

Created attachment 411906 [details]
Patch

[Sam Weinig]

Created attachment 411916 [details]
Patch

[Sam Weinig]

Interesting, the only difference between the last two was that in previous one I allocated my a fresh WebPreferences for each test:

WebPreferences *preferences = [[WebPreferences alloc] init];
[webView setPreferences:preferences];
[preferences release];

but that really seems to break things on the bots.

[Sam Weinig]

Created attachment 411937 [details]
Patch

[Sam Weinig]

(In reply to Sam Weinig from comment #8)
> Interesting, the only difference between the last two was that in previous
> one I allocated my a fresh WebPreferences for each test:
> 
> WebPreferences *preferences = [[WebPreferences alloc] init];
> [webView setPreferences:preferences];
> [preferences release];
> 
> but that really seems to break things on the bots.

I will investigate why using a fresh WebPreferences breaks things so bad in the next round of changes, for now, this amount of cleanup should be enough.

[Darin Adler]

Comment on attachment 411937 [details]
Patch

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

> Tools/DumpRenderTree/TestOptions.cpp:37
> +    if (features.boolWebPreferenceFeatures.empty()) {
> +        features.boolWebPreferenceFeatures = {

Could we use an initializer instead of an if statement?

> Tools/DumpRenderTree/TestOptions.cpp:110
> +    if (m_features.experimentalFeatures != options.m_features.experimentalFeatures)
> +        return false;
> +    if (m_features.internalDebugFeatures != options.m_features.internalDebugFeatures)
> +        return false;
> +    if (m_features.boolWebPreferenceFeatures != options.m_features.boolWebPreferenceFeatures)
> +        return false;
> +    if (m_features.doubleWebPreferenceFeatures != options.m_features.doubleWebPreferenceFeatures)
> +        return false;
> +    if (m_features.uint32WebPreferenceFeatures != options.m_features.uint32WebPreferenceFeatures)
> +        return false;
> +    if (m_features.stringWebPreferenceFeatures != options.m_features.stringWebPreferenceFeatures)
> +        return false;
> +    if (m_features.boolTestRunnerFeatures != options.m_features.boolTestRunnerFeatures)
> +        return false;
> +    if (m_features.doubleTestRunnerFeatures != options.m_features.doubleTestRunnerFeatures)
> +        return false;
> +    if (m_features.stringTestRunnerFeatures != options.m_features.stringTestRunnerFeatures)
> +        return false;
> +    if (m_features.stringVectorTestRunnerFeatures != options.m_features.stringVectorTestRunnerFeatures)
> +        return false;
> +    return true;

If we had a function that tied these into a tuple we could write this in one line.

> Tools/DumpRenderTree/TestOptions.h:46
> +    // Test Runner Specific Features

Can’t help thinking we need a hyphen or two to make this read correctly, like "Test-Runner-Specific Features" maybe?

> Tools/DumpRenderTree/TestOptions.h:57
> +    const std::unordered_map<std::string, bool>& boolWebPreferenceFeatures() const { return m_features.boolWebPreferenceFeatures; }
> +    const std::unordered_map<std::string, double>& doubleWebPreferenceFeatures() const { return m_features.doubleWebPreferenceFeatures; }
> +    const std::unordered_map<std::string, uint32_t>& uint32WebPreferenceFeatures() const { return m_features.uint32WebPreferenceFeatures; }
> +    const std::unordered_map<std::string, std::string>& stringWebPreferenceFeatures() const { return m_features.stringWebPreferenceFeatures; }

Isn’t there a way to use auto for the function return value that could make these simpler?

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1005
> +    preferences.attachmentElementEnabled = boolWebPreferenceFeatureValue("AttachmentElementEnabled", false, options);
> +    preferences.acceleratedDrawingEnabled = boolWebPreferenceFeatureValue("AcceleratedDrawingEnabled", false, options);
> +    preferences.menuItemElementEnabled = boolWebPreferenceFeatureValue("MenuItemElementEnabled", false, options);
> +    preferences.keygenElementEnabled = boolWebPreferenceFeatureValue("KeygenElementEnabled", false, options);
> +    preferences.modernMediaControlsEnabled = boolWebPreferenceFeatureValue("ModernMediaControlsEnabled", true, options);
> +    preferences.inspectorAdditionsEnabled = boolWebPreferenceFeatureValue("InspectorAdditionsEnabled", false, options);
> +    preferences.allowCrossOriginSubresourcesToAskForCredentials = boolWebPreferenceFeatureValue("AllowCrossOriginSubresourcesToAskForCredentials", false, options);
> +    preferences.colorFilterEnabled = boolWebPreferenceFeatureValue("ColorFilterEnabled", false, options);
> +    preferences.selectionAcrossShadowBoundariesEnabled = boolWebPreferenceFeatureValue("SelectionAcrossShadowBoundariesEnabled", true, options);
> +    preferences.webGPUEnabled = boolWebPreferenceFeatureValue("WebGPUEnabled", false, options);
> +    preferences.CSSLogicalEnabled = boolWebPreferenceFeatureValue("CSSLogicalEnabled", false, options);
> +    preferences.lineHeightUnitsEnabled = boolWebPreferenceFeatureValue("LineHeightUnitsEnabled", false, options);
> +    preferences.adClickAttributionEnabled = boolWebPreferenceFeatureValue("AdClickAttributionEnabled", false, options);
> +    preferences.resizeObserverEnabled = boolWebPreferenceFeatureValue("ResizeObserverEnabled", false, options);
> +    preferences.CSSOMViewSmoothScrollingEnabled = boolWebPreferenceFeatureValue("CSSOMViewSmoothScrollingEnabled", false, options);
> +    preferences.coreMathMLEnabled = boolWebPreferenceFeatureValue("CoreMathMLEnabled", false, options);
> +    preferences.requestIdleCallbackEnabled = boolWebPreferenceFeatureValue("RequestIdleCallbackEnabled", false, options);
> +    preferences.asyncClipboardAPIEnabled = boolWebPreferenceFeatureValue("AsyncClipboardAPIEnabled", false, options);
> +    preferences.usesPageCache = boolWebPreferenceFeatureValue("UsesBackForwardCache", false, options);
> +    preferences.layoutFormattingContextIntegrationEnabled = boolWebPreferenceFeatureValue("LayoutFormattingContextIntegrationEnabled", true, options);
> +    preferences.aspectRatioOfImgFromWidthAndHeightEnabled = boolWebPreferenceFeatureValue("AspectRatioOfImgFromWidthAndHeightEnabled", false, options);
> +    preferences.allowTopNavigationToDataURLs = boolWebPreferenceFeatureValue("AllowTopNavigationToDataURLs", true, options);
> +    preferences.contactPickerAPIEnabled = boolWebPreferenceFeatureValue("ContactPickerAPIEnabled", false, options);

Macros or generated code or something? Repetitive.

[Sam Weinig]

(In reply to Darin Adler from comment #11)
> Comment on attachment 411937 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411937&action=review
> 
> > Tools/DumpRenderTree/TestOptions.cpp:37
> > +    if (features.boolWebPreferenceFeatures.empty()) {
> > +        features.boolWebPreferenceFeatures = {
> 
> Could we use an initializer instead of an if statement?

I really wanted to use designated initializer syntax here (https://en.cppreference.com/w/cpp/language/aggregate_initialization#Designated_initializers), but that is a c++20 thing. Trying to use a normal initializer looked quite confusing due to all the empty unorderered_maps that needed to be explicitly initialized.

> 
> > Tools/DumpRenderTree/TestOptions.cpp:110
> > +    if (m_features.experimentalFeatures != options.m_features.experimentalFeatures)
> > +        return false;
> > +    if (m_features.internalDebugFeatures != options.m_features.internalDebugFeatures)
> > +        return false;
> > +    if (m_features.boolWebPreferenceFeatures != options.m_features.boolWebPreferenceFeatures)
> > +        return false;
> > +    if (m_features.doubleWebPreferenceFeatures != options.m_features.doubleWebPreferenceFeatures)
> > +        return false;
> > +    if (m_features.uint32WebPreferenceFeatures != options.m_features.uint32WebPreferenceFeatures)
> > +        return false;
> > +    if (m_features.stringWebPreferenceFeatures != options.m_features.stringWebPreferenceFeatures)
> > +        return false;
> > +    if (m_features.boolTestRunnerFeatures != options.m_features.boolTestRunnerFeatures)
> > +        return false;
> > +    if (m_features.doubleTestRunnerFeatures != options.m_features.doubleTestRunnerFeatures)
> > +        return false;
> > +    if (m_features.stringTestRunnerFeatures != options.m_features.stringTestRunnerFeatures)
> > +        return false;
> > +    if (m_features.stringVectorTestRunnerFeatures != options.m_features.stringVectorTestRunnerFeatures)
> > +        return false;
> > +    return true;
> 
> If we had a function that tied these into a tuple we could write this in one
> line.
> 
> > Tools/DumpRenderTree/TestOptions.h:46
> > +    // Test Runner Specific Features
> 
> Can’t help thinking we need a hyphen or two to make this read correctly,
> like "Test-Runner-Specific Features" maybe?

Done.

> 
> > Tools/DumpRenderTree/TestOptions.h:57
> > +    const std::unordered_map<std::string, bool>& boolWebPreferenceFeatures() const { return m_features.boolWebPreferenceFeatures; }
> > +    const std::unordered_map<std::string, double>& doubleWebPreferenceFeatures() const { return m_features.doubleWebPreferenceFeatures; }
> > +    const std::unordered_map<std::string, uint32_t>& uint32WebPreferenceFeatures() const { return m_features.uint32WebPreferenceFeatures; }
> > +    const std::unordered_map<std::string, std::string>& stringWebPreferenceFeatures() const { return m_features.stringWebPreferenceFeatures; }
> 
> Isn’t there a way to use auto for the function return value that could make
> these simpler?

Done.

> 
> > Tools/DumpRenderTree/mac/DumpRenderTree.mm:1005
> > +    preferences.attachmentElementEnabled = boolWebPreferenceFeatureValue("AttachmentElementEnabled", false, options);
> > +    preferences.acceleratedDrawingEnabled = boolWebPreferenceFeatureValue("AcceleratedDrawingEnabled", false, options);
> > +    preferences.menuItemElementEnabled = boolWebPreferenceFeatureValue("MenuItemElementEnabled", false, options);
> > +    preferences.keygenElementEnabled = boolWebPreferenceFeatureValue("KeygenElementEnabled", false, options);
> > +    preferences.modernMediaControlsEnabled = boolWebPreferenceFeatureValue("ModernMediaControlsEnabled", true, options);
> > +    preferences.inspectorAdditionsEnabled = boolWebPreferenceFeatureValue("InspectorAdditionsEnabled", false, options);
> > +    preferences.allowCrossOriginSubresourcesToAskForCredentials = boolWebPreferenceFeatureValue("AllowCrossOriginSubresourcesToAskForCredentials", false, options);
> > +    preferences.colorFilterEnabled = boolWebPreferenceFeatureValue("ColorFilterEnabled", false, options);
> > +    preferences.selectionAcrossShadowBoundariesEnabled = boolWebPreferenceFeatureValue("SelectionAcrossShadowBoundariesEnabled", true, options);
> > +    preferences.webGPUEnabled = boolWebPreferenceFeatureValue("WebGPUEnabled", false, options);
> > +    preferences.CSSLogicalEnabled = boolWebPreferenceFeatureValue("CSSLogicalEnabled", false, options);
> > +    preferences.lineHeightUnitsEnabled = boolWebPreferenceFeatureValue("LineHeightUnitsEnabled", false, options);
> > +    preferences.adClickAttributionEnabled = boolWebPreferenceFeatureValue("AdClickAttributionEnabled", false, options);
> > +    preferences.resizeObserverEnabled = boolWebPreferenceFeatureValue("ResizeObserverEnabled", false, options);
> > +    preferences.CSSOMViewSmoothScrollingEnabled = boolWebPreferenceFeatureValue("CSSOMViewSmoothScrollingEnabled", false, options);
> > +    preferences.coreMathMLEnabled = boolWebPreferenceFeatureValue("CoreMathMLEnabled", false, options);
> > +    preferences.requestIdleCallbackEnabled = boolWebPreferenceFeatureValue("RequestIdleCallbackEnabled", false, options);
> > +    preferences.asyncClipboardAPIEnabled = boolWebPreferenceFeatureValue("AsyncClipboardAPIEnabled", false, options);
> > +    preferences.usesPageCache = boolWebPreferenceFeatureValue("UsesBackForwardCache", false, options);
> > +    preferences.layoutFormattingContextIntegrationEnabled = boolWebPreferenceFeatureValue("LayoutFormattingContextIntegrationEnabled", true, options);
> > +    preferences.aspectRatioOfImgFromWidthAndHeightEnabled = boolWebPreferenceFeatureValue("AspectRatioOfImgFromWidthAndHeightEnabled", false, options);
> > +    preferences.allowTopNavigationToDataURLs = boolWebPreferenceFeatureValue("AllowTopNavigationToDataURLs", true, options);
> > +    preferences.contactPickerAPIEnabled = boolWebPreferenceFeatureValue("ContactPickerAPIEnabled", false, options);
> 
> Macros or generated code or something? Repetitive.

This will all be removed in a subsequent changes, so I didn't bother doing anything fancy.

Thanks for the review.

[Sam Weinig]

Committed r268795: <https://trac.webkit.org/changeset/268795>

[Radar WebKit Bug Importer]

<rdar://problem/70529125>