[webkit-reviews] review granted: [Bug 217962] Clean up DumpRenderTree in preparation for supporting arbitrary test header commands : [Attachment 411937] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 217962: Clean up DumpRenderTree in preparation for supporting arbitrary
test header commands
https://bugs.webkit.org/show_bug.cgi?id=217962

Attachment 411937: Patch

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




--- Comment #11 from Darin Adler <darin at apple.com> ---
Comment on attachment 411937
  --> https://bugs.webkit.org/attachment.cgi?id=411937
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.