Bug 217962

Summary: Clean up DumpRenderTree in preparation for supporting arbitrary test header commands
Product: WebKit Reporter: Sam Weinig <sam>
Component: New BugsAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch darin: review+

Description Sam Weinig 2020-10-20 09:13:01 PDT
Cleanup DumpRenderTree in preparation for supporting arbitrary test header commands
Comment 1 Sam Weinig 2020-10-20 09:19:22 PDT Comment hidden (obsolete)
Comment 2 Sam Weinig 2020-10-20 09:29:06 PDT Comment hidden (obsolete)
Comment 3 Sam Weinig 2020-10-20 11:42:06 PDT Comment hidden (obsolete)
Comment 4 Sam Weinig 2020-10-20 12:04:44 PDT Comment hidden (obsolete)
Comment 5 Sam Weinig 2020-10-20 13:04:19 PDT Comment hidden (obsolete)
Comment 6 Sam Weinig 2020-10-20 13:14:05 PDT Comment hidden (obsolete)
Comment 7 Sam Weinig 2020-10-20 14:37:53 PDT
Created attachment 411916 [details]
Patch
Comment 8 Sam Weinig 2020-10-20 16:01:45 PDT
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.
Comment 9 Sam Weinig 2020-10-20 16:24:05 PDT
Created attachment 411937 [details]
Patch
Comment 10 Sam Weinig 2020-10-20 16:27:01 PDT
(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.
Comment 11 Darin Adler 2020-10-20 18:51:32 PDT
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.
Comment 12 Sam Weinig 2020-10-21 08:02:15 PDT
(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.
Comment 13 Sam Weinig 2020-10-21 08:04:22 PDT
Committed r268795: <https://trac.webkit.org/changeset/268795>
Comment 14 Radar WebKit Bug Importer 2020-10-21 08:05:19 PDT
<rdar://problem/70529125>