[webkit-reviews] review denied: [Bug 35610] Enable LayoutTests/compositing for Windows when compositing is available : [Attachment 49854] Patch

[bugzilla-daemon at webkit.org]

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

Attachment 49854: Patch
https://bugs.webkit.org/attachment.cgi?id=49854&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> 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.intermed
iate/$libraryName.lib";

I'm not clear on why this change is needed.

r- for the vtable issue.