Bug 172841 – Fix MotionMark to work in Internet Explorer

• Component: Tools / Tests
• Status: NEW

[Said Abou-Hallawa]

Replace calls to some JavaScript functions, which Internet Explorer does not support, by equivalent code.

[Said Abou-Hallawa]

Created attachment 311784 [details]
Patch

[Jon Lee]

Comment on attachment 311784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311784&action=review

> PerformanceTests/MotionMark/tests/master/resources/image-data.js:40
> +        var nameExtension = Utilities.isInternetExplorer() ? "100.png" : ".svg";

Comment please.

> PerformanceTests/MotionMark/tests/master/resources/leaves.js:37
> +            if (this._position.x < 0)

I think we can simplify this logic. |size.width| and |stage.size.width| is always positive. So I think we can get rid of this second if:

if (this._position.x < -this.size.width)
    this._position.x += ...
else if ( this._position.x > this.stage.size.width)
    this._position.x -= ....

> PerformanceTests/MotionMark/tests/master/resources/leaves.js:38
> +                this._position.x += (this.size.width + this.stage.size.width);

Please remove parens.

> PerformanceTests/MotionMark/tests/master/resources/leaves.js:40
> +                this._position.x -= (this.size.width + this.stage.size.width);

Ditto.

> PerformanceTests/MotionMark/tests/master/resources/leaves.js:-118
> -        particle.element.remove();

Can we polyfill this instead?

[Said Abou-Hallawa]

Created attachment 311796 [details]
Patch

[Jon Lee]

Comment on attachment 311796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311796&action=review

> PerformanceTests/MotionMark/resources/extensions.js:237
> +    this.parentNode.removeChild(this);

This is wrong. We should only set this for UA’s that do not have the method defined.

[Said Abou-Hallawa]

Created attachment 311844 [details]
Patch

[Sam Weinig]

Comment on attachment 311844 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311844&action=review

> PerformanceTests/MotionMark/tests/master/resources/image-data.js:42
> +        // IE taints the canvas context if an SVG image is drawn on it. Any
> +        // following calls to getImageData will throw CORS security exception.
> +        var nameExtension = Utilities.isInternetExplorer() ? "100.png" : ".svg";

It seems like it would be better to have the test be consistent across browsers, so why not have everyone one use 100.png? 

Also, modern convention says you should do detection, not sniffing, so if you want to keep the svg in one, png in another, you should replace the Utilities.isInternetExplorer() check, with a Utilities.canDrawSVGToCanvasWithoutTainting() check, which does that, and checks for an exception. That way, if IE changes, it will get the desired behavior.

That said, I think having everyone do the same thing is better.

[Said Abou-Hallawa]

Created attachment 312024 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 311844 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311844&action=review

>> PerformanceTests/MotionMark/tests/master/resources/image-data.js:42
>> +        var nameExtension = Utilities.isInternetExplorer() ? "100.png" : ".svg";
> 
> It seems like it would be better to have the test be consistent across browsers, so why not have everyone one use 100.png? 
> 
> Also, modern convention says you should do detection, not sniffing, so if you want to keep the svg in one, png in another, you should replace the Utilities.isInternetExplorer() check, with a Utilities.canDrawSVGToCanvasWithoutTainting() check, which does that, and checks for an exception. That way, if IE changes, it will get the desired behavior.
> 
> That said, I think having everyone do the same thing is better.

I implemented Utilities.canDrawSVGToCanvasWithoutTainting() which detects whether the browser throws an exception when calling getImageData() after drawing an SVG on the canvas. Beside the ones we are working around here, IE has other limitations. For example the Focus test does not work correctly because it does not render the blur filter correctly. Another limitation is data uri image has to be in url format.

I think we need to draw the SVG images in this test since this test is the only one that loads and renders SVG images in MotionMark master suite. If we make all the browser uses the png images, no test in MotionMark will be exercising the SVG code path. I think also we should not limit ourselves to IE bugs/limitations especially Safari, Chrome and FireFox behave the same but IE does not.

[Said Abou-Hallawa]

Comment on attachment 311844 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311844&action=review

>>> PerformanceTests/MotionMark/tests/master/resources/image-data.js:42
>>> +        var nameExtension = Utilities.isInternetExplorer() ? "100.png" : ".svg";
>> 
>> It seems like it would be better to have the test be consistent across browsers, so why not have everyone one use 100.png? 
>> 
>> Also, modern convention says you should do detection, not sniffing, so if you want to keep the svg in one, png in another, you should replace the Utilities.isInternetExplorer() check, with a Utilities.canDrawSVGToCanvasWithoutTainting() check, which does that, and checks for an exception. That way, if IE changes, it will get the desired behavior.
>> 
>> That said, I think having everyone do the same thing is better.
> 
> I implemented Utilities.canDrawSVGToCanvasWithoutTainting() which detects whether the browser throws an exception when calling getImageData() after drawing an SVG on the canvas. Beside the ones we are working around here, IE has other limitations. For example the Focus test does not work correctly because it does not render the blur filter correctly. Another limitation is data uri image has to be in url format.
> 
> I think we need to draw the SVG images in this test since this test is the only one that loads and renders SVG images in MotionMark master suite. If we make all the browser uses the png images, no test in MotionMark will be exercising the SVG code path. I think also we should not limit ourselves to IE bugs/limitations especially Safari, Chrome and FireFox behave the same but IE does not.

I was wrong when I said "this test is the only one that loads and renders SVG images in MotionMark master suite." Actually Suits test does render SVG images also.

[Sam Weinig]

In that case, why not just say MotionMark does not run in IE? It seems worse to me to have a benchmark that does different things in different browsers, as it is no longer comparing apples to apples.

[Maciej Stachowiak]

Comment on attachment 312024 [details]
Patch

Seems good to make this fix (despite IE being old and unmaintained) but this patch no longer applies.

[Ahmad Saleem]

Internet Explorer is gone, so I don't think we don't need to keep it open. Any input?

Comment 11 do mention that it is good fix despite IE exist or not? Appreciate if someone can share input. Thanks!