| Summary: | Enable LayoutTests/compositing for Windows when compositing is available | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Marrin <cmarrin> | ||||||||
| Component: | Layout and Rendering | Assignee: | Chris Marrin <cmarrin> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | simon.fraser | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | OS X 10.5 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Chris Marrin
2010-03-02 14:41:29 PST
Created attachment 49854 [details]
Patch
Comment on attachment 49854 [details] Patch Seems like a great thing to do! > + virtual HRESULT STDMETHODCALLTYPE setAcceleratedCompositingEnabled( > + /* [in] */ BOOL acceleratedCompositingEnabled); > + > + virtual HRESULT STDMETHODCALLTYPE acceleratedCompositingEnabled( > + /* [retval][out] */ BOOL* acceleratedCompositingEnabled); > + > virtual HRESULT STDMETHODCALLTYPE fontSmoothingContrast( > /* [retval][out] */ float* contrast); > > @@ -396,8 +402,6 @@ public: > /* [in] */ BSTR key, > /* [in] */ BSTR value); > > - virtual HRESULT STDMETHODCALLTYPE setAcceleratedCompositingEnabled(BOOL); > - virtual HRESULT STDMETHODCALLTYPE acceleratedCompositingEnabled(BOOL*); This doesn't seem to be a helpful change. Should work without changing anything in this file. > + if (!stricmp(argv[i], "--print-supported-features")) { > + printSupportedFeatures = true; > + continue; > + } > + > + Extra blank line here. > + if (printSupportedFeatures) { > + BOOL acceleratedCompositingAvailable; > + standardPreferences->acceleratedCompositingEnabled(&acceleratedCompositingAvailable); > + fprintf(stderr, "SupportedFeatures:%s\n", acceleratedCompositingAvailable ? "AcceleratedCompositing" : ""); > + return 0; > + } Why stderr instead of stdout? I think a plain old printf would be good for this, since we specifically asked for this to be printed. > -if (!checkWebCoreFeatureSupport("Accelerated Compositing", 0)) { > +if (isCygwin()) { > + my $cmd = $dumpTool . " --print-supported-features 2>&1"; > + my $result = `$cmd 2>&1`; Can this logic go inside the checkWebCoreFeatureSupport function instead of out here at the top level? > + if (!($result =~ m/AcceleratedCompositing/)) { You can write this with !~ instead and cut down on the parentheses. Also no need for the "m" before the slash. if ($result !~ /AcceleratedCompositing/) > Index: WebKitTools/Scripts/webkitdirs.pm > =================================================================== > --- WebKitTools/Scripts/webkitdirs.pm (revision 55032) > +++ WebKitTools/Scripts/webkitdirs.pm (working copy) > @@ -562,7 +562,7 @@ sub builtDylibPathForName > return "$configurationProductDir/$libraryName.framework/Versions/A/$libraryName"; > } > if (isAppleWinWebKit()) { > - if ($libraryName eq "JavaScriptCore") { > + if ($libraryName eq "JavaScriptCore" or $libraryName eq "WebCore") { > return "$baseProductDir/lib/$libraryName.lib"; > } else { > return "$baseProductDir/$libraryName.intermediate/$configuration/$libraryName.intermediate/$libraryName.lib"; This change does not match the change log entry. And I don't see why you're changing this. Can this land separately with a comment explaining the change. I'm going to say review+ despite a few of the concerns above; the main thing here is to start using these tests. Please consider fixing the things I mentioned though. Comment on attachment 49854 [details] Patch > Index: WebKit/win/WebPreferences.h > =================================================================== > --- WebKit/win/WebPreferences.h (revision 55032) > +++ WebKit/win/WebPreferences.h (working copy) > @@ -344,6 +344,12 @@ public: > virtual HRESULT STDMETHODCALLTYPE zoomsTextOnly( > /* [retval][out] */ BOOL *zoomsTextOnly); > > + virtual HRESULT STDMETHODCALLTYPE setAcceleratedCompositingEnabled( > + /* [in] */ BOOL acceleratedCompositingEnabled); > + > + virtual HRESULT STDMETHODCALLTYPE acceleratedCompositingEnabled( > + /* [retval][out] */ BOOL* acceleratedCompositingEnabled); > + I think you have to add new methods at the end, to avoid changes in vtable layout causing backwards compatibility issues. > Index: WebKitTools/ChangeLog > =================================================================== > + * Scripts/webkitdirs.pm: Runs DRT to see if HW comp is available on Windows Really? The change is not obviously doing this. > Index: WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp > =================================================================== > + if (!stricmp(argv[i], "--print-supported-features")) { > + printSupportedFeatures = true; > + continue; > + } > + > + Extra line here. > Index: WebKitTools/Scripts/run-webkit-tests > =================================================================== > --- WebKitTools/Scripts/run-webkit-tests (revision 55032) > +++ WebKitTools/Scripts/run-webkit-tests (working copy) > @@ -444,13 +444,21 @@ if (!$testMedia) { > $ignoredDirectories{'http/tests/media'} = 1; > } > > -if (!checkWebCoreFeatureSupport("Accelerated Compositing", 0)) { > +if (isCygwin()) { > + my $cmd = $dumpTool . " --print-supported-features 2>&1"; > + my $result = `$cmd 2>&1`; > + if (!($result =~ m/AcceleratedCompositing/)) { > + $ignoredDirectories{'compositing'} = 1; > + $ignoredDirectories{'animations/3d'} = 1; > + $ignoredDirectories{'transforms/3d'} = 1; > + } > +} elsif (!checkWebCoreFeatureSupport("AcceleratedCompositing", 0)) { > $ignoredDirectories{'compositing'} = 1; > -} > > -if (!checkWebCoreFeatureSupport("3D Rendering", 0)) { > - $ignoredDirectories{'animations/3d'} = 1; > - $ignoredDirectories{'transforms/3d'} = 1; > + if (!checkWebCoreFeatureSupport("3D Rendering", 0)) { > + $ignoredDirectories{'animations/3d'} = 1; > + $ignoredDirectories{'transforms/3d'} = 1; > + } > } It looks like you remove the separate checks for accelerated compositing and 3d rendering. If this was deliberate, your changelog entry should justify it. > Index: WebKitTools/Scripts/webkitdirs.pm > =================================================================== > --- WebKitTools/Scripts/webkitdirs.pm (revision 55032) > +++ WebKitTools/Scripts/webkitdirs.pm (working copy) > @@ -562,7 +562,7 @@ sub builtDylibPathForName > return "$configurationProductDir/$libraryName.framework/Versions/A/$libraryName"; > } > if (isAppleWinWebKit()) { > - if ($libraryName eq "JavaScriptCore") { > + if ($libraryName eq "JavaScriptCore" or $libraryName eq "WebCore") { > return "$baseProductDir/lib/$libraryName.lib"; > } else { > return "$baseProductDir/$libraryName.intermediate/$configuration/$libraryName.intermediate/$libraryName.lib"; I'm not clear on why this change is needed. r- for the vtable issue. Created attachment 49863 [details]
Replacement patch
New patch addresses comments from Simon and Darin Comment on attachment 49863 [details] Replacement patch > Index: WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp > =================================================================== > @@ -1246,6 +1252,13 @@ int main(int argc, char* argv[]) > standardPreferences->setJavaScriptEnabled(TRUE); > standardPreferences->setDefaultFontSize(16); > > + if (printSupportedFeatures) { > + BOOL acceleratedCompositingAvailable; > + standardPreferences->acceleratedCompositingEnabled(&acceleratedCompositingAvailable); > + printf("SupportedFeatures:%s\n", acceleratedCompositingAvailable ? "AcceleratedCompositing" : ""); > + return 0; I kinda feel like this should print out "AcceleratedCompositing, 3DTransforms" so that run-webkit-tests can check for both separately. > Index: WebKitTools/Scripts/run-webkit-tests > =================================================================== > -if (!checkWebCoreFeatureSupport("Accelerated Compositing", 0)) { > +if (isCygwin()) { > + my $cmd = $dumpTool . " --print-supported-features 2>&1"; > + my $result = `$cmd 2>&1`; > + if ($result !~ /AcceleratedCompositing/) { > + $ignoredDirectories{'compositing'} = 1; > + $ignoredDirectories{'animations/3d'} = 1; > + $ignoredDirectories{'transforms/3d'} = 1; > + } > +} elsif (!checkWebCoreFeatureSupport("Accelerated Compositing", 0)) { > $ignoredDirectories{'compositing'} = 1; > -} > > -if (!checkWebCoreFeatureSupport("3D Rendering", 0)) { > - $ignoredDirectories{'animations/3d'} = 1; > - $ignoredDirectories{'transforms/3d'} = 1; > + if (!checkWebCoreFeatureSupport("3D Rendering", 0)) { > + $ignoredDirectories{'animations/3d'} = 1; > + $ignoredDirectories{'transforms/3d'} = 1; > + } > } I still think this is wrong. You're only testing for NOT 3d support inside the NOT accelerated compositing block. I'd prefer to see: my($hasAcceleratedCompositing) = false; my($has3DRendering) = false; if (isCygwin()) { $hasAcceleratedCompositing = ... $has3DRendering = ... } elsif { $hasAcceleratedCompositing = ... $has3DRendering = ... } if (!$hasAcceleratedCompositing) { ... } if (!$has3DRendering) { ... } > > Index: LayoutTests/platform/win/Skipped > =================================================================== > --- LayoutTests/platform/win/Skipped (revision 55032) > +++ LayoutTests/platform/win/Skipped (working copy) > @@ -689,9 +689,19 @@ fast/multicol/single-line.html > # Need to add functionality to DumpRenderTree to handle error pages > fast/history/back-forward-reset-after-error-handling.html > > -# Tests requiring 3D_RENDERING and ACCELERATED_COMPOSITING support > -transforms/3d > -compositing > +# ACCELERATED_COMPOSITING tests that crash > +compositing/layers-inside-overflow-scroll.html > +compositing/self-painting-layers.html > +compositing/geometry/clipped-video-controller.html > +compositing/geometry/video-fixed-scrolling.html > +compositing/geometry/video-opacity-overlay.html > +compositing/overflow/scroll-ancestor-update.html > +compositing/reflections/load-video-in-reflection.html > +compositing/video/video-background-color.html Please file one or more bugs to cover the crashing tests. r=me, as long as you fix the logic in run-webkit-tests. > I kinda feel like this should print out "AcceleratedCompositing, 3DTransforms"
> so that run-webkit-tests can check for both separately.
And of course the "3DTransforms" string would only be appended if ENABLE(3D_RENDERING) is true.
Landed in http://trac.webkit.org/changeset/55467 Reopening to address Simon's concerns Created attachment 49937 [details]
Additional Patch
This addresses Simon's latest issues.
Additional change landed in http://trac.webkit.org/changeset/55481 |