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

[bugzilla-daemon at webkit.org]

Darin Adler <darin 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 49854: Patch
https://bugs.webkit.org/attachment.cgi?id=49854&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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(&acceleratedCompositingAvail
able);
> +	   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.intermed
iate/$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.