Summary: | Clean up DumpRenderTree in preparation for supporting arbitrary test header commands | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||||||||||||
Component: | New Bugs | Assignee: | 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
Sam Weinig
2020-10-20 09:13:01 PDT
Created attachment 411872 [details]
Patch
Created attachment 411873 [details]
Patch
Created attachment 411893 [details]
Patch
Created attachment 411895 [details]
Patch
Created attachment 411902 [details]
Patch
Created attachment 411906 [details]
Patch
Created attachment 411916 [details]
Patch
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. Created attachment 411937 [details]
Patch
(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 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. (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. Committed r268795: <https://trac.webkit.org/changeset/268795> |