Bug 35610 – Enable LayoutTests/compositing for Windows when compositing is available

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Chris Marrin]

We need to be running the tests in LayoutTests/compositing when the build has accelerated compositing enabled. Currently we have zero test coverage of the compositing code.

[Chris Marrin]

Created attachment 49854 [details]
Patch

[Darin Adler]

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.

[Simon Fraser (smfr)]

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.

[Chris Marrin]

Created attachment 49863 [details]
Replacement patch

[Chris Marrin]

New patch addresses comments from Simon and Darin

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

> 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.

[Chris Marrin]

Landed in http://trac.webkit.org/changeset/55467

[Chris Marrin]

Reopening to address Simon's concerns

[Chris Marrin]

Created attachment 49937 [details]
Additional Patch

This addresses Simon's latest issues.

[Chris Marrin]

Additional change landed in http://trac.webkit.org/changeset/55481