[webkit-reviews] review granted: [Bug 35610] Enable LayoutTests/compositing for Windows when compositing is available : [Attachment 49863] Replacement patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 35610: Enable LayoutTests/compositing for Windows when compositing is
available
https://bugs.webkit.org/show_bug.cgi?id=35610

Attachment 49863: Replacement patch
https://bugs.webkit.org/attachment.cgi?id=49863&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> 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(&acceleratedCompositingAvail
able);
> +	   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.