Replace calls to some JavaScript functions, which Internet Explorer does not support, by equivalent code.
Created attachment 311784 [details] Patch
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?
Created attachment 311796 [details] Patch
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.
Created attachment 311844 [details] Patch
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.
Created attachment 312024 [details] Patch
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.
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.
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.
Comment on attachment 312024 [details] Patch Seems good to make this fix (despite IE being old and unmaintained) but this patch no longer applies.
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!