Bug 140753 – Poor performance on IE's Chalkboard benchmark

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

On the same machine, the benchmark http://ie.microsoft.com/testdrive/Performance/Chalkboard/ is tested with different window sizes:

Window size (2560 x 1260):
Safari 10600.3.14: 70.60 sec
FireFox: 120.40 sec
Chrome: 26.21

Window size (800 x 600):
Safari 10600.3.14: 6.49 sec
FireFox: 98.74 sec
Chrome: 8.14 sec

The reason for having WebKit slower on this benchmark, with larger widow size, is the SVG rendering code does not skip the SVG elements which are outside the clipping rectangle. We draw the all SVG elements even if some of them are completely outside the clipping rectangle. This is very costly with complex primitives like the <path>. The fix is to propagate the correct clipping rectangle in the PaintingInfo when passing it to the SVGShape::paint().

[Said Abou-Hallawa]

The same issue was reported in the past: https://bugs.webkit.org/show_bug.cgi?id=104693. But it looks like it was happening because of SVGImageCache and other stuff and it was resolved because the benchmark completes in 9sec with 1024x1024 screen. A performance test needs to be written for this benchmark to track if it's slowed down in the future.

[Said Abou-Hallawa]

Created attachment 245109 [details]
Patch

[Said Abou-Hallawa]

Safari completes this benchmark on a window size 2560 x 1260 in:

Without the patch: 70.60 sec
With the patch: 10.35 sec

[Build Bot]

Comment on attachment 245109 [details]
Patch

Attachment 245109 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5067252034437120

New failing tests:
fast/images/hidpi-image-position-on-device-pixels.html
svg/custom/anchor-on-use.svg
fast/sub-pixel/zoomed-image-tiles.html
fast/backgrounds/hidpi-background-image-contain-cover-scale-needs-more-precision.html

[Build Bot]

Created attachment 245114 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 245109 [details]
Patch

Attachment 245109 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6224187374960640

New failing tests:
fast/images/hidpi-image-position-on-device-pixels.html
svg/custom/anchor-on-use.svg
fast/sub-pixel/zoomed-image-tiles.html
fast/backgrounds/hidpi-background-image-contain-cover-scale-needs-more-precision.html

[Build Bot]

Created attachment 245115 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Simon Fraser (smfr)]

Comment on attachment 245109 [details]
Patch

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

> Source/WebCore/svg/graphics/SVGImage.cpp:252
> +    view->paint(context, context->clipBounds().isEmpty() ? enclosingIntRect(srcRect) : context->clipBounds());

You shouldn't call clipBounds() twice; it's not that cheap.

[Said Abou-Hallawa]

Created attachment 245204 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #8)
> Comment on attachment 245109 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245109&action=review
> 
> > Source/WebCore/svg/graphics/SVGImage.cpp:252
> > +    view->paint(context, context->clipBounds().isEmpty() ? enclosingIntRect(srcRect) : context->clipBounds());
> 
> You shouldn't call clipBounds() twice; it's not that cheap.

Fixed

[Said Abou-Hallawa]

Created attachment 245239 [details]
Patch

[Said Abou-Hallawa]

Created attachment 245264 [details]
Patch

[Said Abou-Hallawa]

Sometimes when running run-perf-tests from the command line, I get FAILED results after very long time.

Running 1 tests
Running SVG/UnderTheSeeBenchmark.html (1 of 1)
timeout: SVG/UnderTheSeeBenchmark.html
FAILED
Finished: 605.008695 s

When I used "-2" option I got the following results:

Running 1 tests
Running SVG/UnderTheSeeBenchmark.html (1 of 1)
error: SVG/UnderTheSeeBenchmark.html
2015-01-23 16:44:16.142 com.apple.WebKit.Networking.Development[71068:24399801] NO NSURLStorage client.  Cannot set min-size for VM cached resources.

But when opening the test in the browser, it runs really fast and finishes normally. I am not sure if I am doing a mistake in the test or I am passing the wrong options to run-perf-tests.

[Simon Fraser (smfr)]

We may need to fix DRT and WTR to use onscreen windows during perf testing.

[Said Abou-Hallawa]

Created attachment 245269 [details]
Patch

[Said Abou-Hallawa]

I changed the tests to take less time be having the number of target rectangles be "1". On my machine WorldcupBenchmark.html takes 14 seconds and UnderTheSeeBenchmark.html takes 80 seconds. I think the patch is now ready for review.

[Said Abou-Hallawa]

Created attachment 245272 [details]
Patch

[Said Abou-Hallawa]

I removed the browser targets so the test moves the image to a single target.

[Ryosuke Niwa]

Comment on attachment 245269 [details]
Patch

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

> PerformanceTests/SVG/resources/RenderAnimate.js:56
> +var globals = {

Why don't we create a class instead?  Having all properties in one object and accessing it from various functions seems like a code smell.

> PerformanceTests/SVG/resources/RenderAnimate.js:66
> +    image    : document.getElementById("Image"),
> +    timer    : document.getElementById("Timer"),
> +    targets  : window.testRunner ? performanceTargets : browserTargets,
> +    index    : 0,
> +    delay    : window.testRunner ? 0 : 1000,
> +    duration : { start : null, end : null },
> +    window   : { w: 0, h: 0 },
> +    source   : { w: 0, h: 0 },
> +    target   : { x: 0, y: 0, w: 0, h: 0 },
> +    current  : { x: 0, y: 0, w: 0, h: 0 },

Please don't align : like this per our style guideline.  Also, spell out width and height.

> PerformanceTests/SVG/resources/RenderAnimate.js:69
> +// Performance test controller

This comment repeats what the code says. Please remove.

> PerformanceTests/SVG/resources/RenderAnimate.js:74
> +        // We have to remove all the elements in the page when the test ends. Otherwise the
> +        // run-perf-test will say it failed

Ditto.

> PerformanceTests/SVG/resources/RenderAnimate.js:82
> +// The entry point of the test

Ditto.

> PerformanceTests/SVG/resources/RenderAnimate.js:87
> +    // Break the loop if the test is teared down
> +    if (globals.image == null)
> +        return;

We shouldn't need this code. measureValueAsync returns false when we're done.

> PerformanceTests/SVG/resources/RenderAnimate.js:91
> +    // setupTest() only once
> +    if (globals.window.w == 0)
> +        setupTest();

Why don't we invert the relationship between two functions so that setupTest will call startTest at the end?

> PerformanceTests/SVG/resources/RenderAnimate.js:93
> +    setupRun();

Why does this need to be a separate function?
It seems like it's not called anywhere else and "setupRun" doesn't convey any extra information "startTest" or startTest doesn't.

> PerformanceTests/SVG/resources/RenderAnimate.js:105
> +    // Store the original image zize

This comment repeats the code. Please remove it.

> PerformanceTests/SVG/resources/RenderAnimate.js:126
> +    globals.target.x  = globals.current.x;
> +    globals.target.y  = globals.current.y;
> +    globals.target.w  = globals.current.w;
> +    globals.target.h  = globals.current.h;
> +
> +    globals.index          = 0;
> +    globals.duration.start = null;
> +    globals.duration.end   = null;

We shouldn't align = signs like this per our style guide.

> PerformanceTests/SVG/resources/RenderAnimate.js:130
> +    globals.image.style.visibility = "visible";
> +    globals.image.style.opacity    = 1;
> +    globals.timer.style.opacity    = 1;

Ditto.

> PerformanceTests/SVG/resources/RenderAnimate.js:144
> +    if (++globals.index < globals.targets.length) {

We prefer early return so it's better to write this as:
globals.index++;
if (globals.index >= globals.targets.length) {
  setTimeout(nextRun, globals.delay);
  return;
}
... (no else).

> PerformanceTests/SVG/resources/RenderAnimate.js:175
> +            && Math.abs(globals.current.y - globals.target.y) < 0.01 
> +            && Math.abs(globals.current.w - globals.target.w) < 0.01) {

Wrong indentation.  && should appear exactly 4 spaces to the right of "if (".

> PerformanceTests/SVG/resources/RenderAnimate.js:176
> +        // If we we are vert close to the target, move to the next target

Instead of saying this in comment, allocate a descriptive local variable such as "approximatelyAtTarget".

> PerformanceTests/SVG/resources/RenderAnimate.js:179
> +        // Keep moving to the target rectangle gradually

This comment repeats the code. Remove.

> PerformanceTests/SVG/resources/RenderAnimate.js:190
> +    globals.image.style.width = globals.current.w + "px";
> +    globals.image.style.left  = globals.current.x + "px";
> +    globals.image.style.top   = globals.current.y + "px";

Ditto about aligning = signs.

> PerformanceTests/SVG/resources/RenderAnimate.js:193
> +function nextRun()

I would call this function recordResultAndScheduleNewRun or something since it does a lot more than just scheduling the next run.

[Said Abou-Hallawa]

Created attachment 245464 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #19)
> Comment on attachment 245269 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245269&action=review
> 
> > PerformanceTests/SVG/resources/RenderAnimate.js:56
> > +var globals = {
> 
> Why don't we create a class instead?  Having all properties in one object
> and accessing it from various functions seems like a code smell.
> 

Done. Thanks for the advice.

> > PerformanceTests/SVG/resources/RenderAnimate.js:66
> > +    image    : document.getElementById("Image"),
> > +    timer    : document.getElementById("Timer"),
> > +    targets  : window.testRunner ? performanceTargets : browserTargets,
> > +    index    : 0,
> > +    delay    : window.testRunner ? 0 : 1000,
> > +    duration : { start : null, end : null },
> > +    window   : { w: 0, h: 0 },
> > +    source   : { w: 0, h: 0 },
> > +    target   : { x: 0, y: 0, w: 0, h: 0 },
> > +    current  : { x: 0, y: 0, w: 0, h: 0 },
> 
> Please don't align : like this per our style guideline.  Also, spell out
> width and height.
> 

The script is almost rewritten but the new one does not have any alignment for colons.

> > PerformanceTests/SVG/resources/RenderAnimate.js:69
> > +// Performance test controller
> 
> This comment repeats what the code says. Please remove.
> 

Comment was removed.

> > PerformanceTests/SVG/resources/RenderAnimate.js:74
> > +        // We have to remove all the elements in the page when the test ends. Otherwise the
> > +        // run-perf-test will say it failed
> 
> Ditto.
> 

Comment was removed.

> > PerformanceTests/SVG/resources/RenderAnimate.js:82
> > +// The entry point of the test
> 
> Ditto.
> 

Comment was removed.

> > PerformanceTests/SVG/resources/RenderAnimate.js:87
> > +    // Break the loop if the test is teared down
> > +    if (globals.image == null)
> > +        return;
> 
> We shouldn't need this code. measureValueAsync returns false when we're done.
> 

Done. The return value of measureValueAsync() controls when we should exit the test.

> > PerformanceTests/SVG/resources/RenderAnimate.js:91
> > +    // setupTest() only once
> > +    if (globals.window.w == 0)
> > +        setupTest();
> 
> Why don't we invert the relationship between two functions so that setupTest
> will call startTest at the end?
> 

The structure of the test is now different and I think the new one is clearer.

> > PerformanceTests/SVG/resources/RenderAnimate.js:93
> > +    setupRun();
> 
> Why does this need to be a separate function?
> It seems like it's not called anywhere else and "setupRun" doesn't convey
> any extra information "startTest" or startTest doesn't.
> 

The structure of the test is now different and I think the new one is clearer.

> > PerformanceTests/SVG/resources/RenderAnimate.js:105
> > +    // Store the original image zize
> 
> This comment repeats the code. Please remove it.
> 

Comment was removed.

> > PerformanceTests/SVG/resources/RenderAnimate.js:126
> > +    globals.target.x  = globals.current.x;
> > +    globals.target.y  = globals.current.y;
> > +    globals.target.w  = globals.current.w;
> > +    globals.target.h  = globals.current.h;
> > +
> > +    globals.index          = 0;
> > +    globals.duration.start = null;
> > +    globals.duration.end   = null;
> 
> We shouldn't align = signs like this per our style guide.
> 

Alignment is removed.

> > PerformanceTests/SVG/resources/RenderAnimate.js:130
> > +    globals.image.style.visibility = "visible";
> > +    globals.image.style.opacity    = 1;
> > +    globals.timer.style.opacity    = 1;
> 
> Ditto.
> 

Alignment was removed.

> > PerformanceTests/SVG/resources/RenderAnimate.js:144
> > +    if (++globals.index < globals.targets.length) {
> 
> We prefer early return so it's better to write this as:
> globals.index++;
> if (globals.index >= globals.targets.length) {
>   setTimeout(nextRun, globals.delay);
>   return;
> }
> ... (no else).
> 

Early return was done.

> > PerformanceTests/SVG/resources/RenderAnimate.js:175
> > +            && Math.abs(globals.current.y - globals.target.y) < 0.01 
> > +            && Math.abs(globals.current.w - globals.target.w) < 0.01) {
> 
> Wrong indentation.  && should appear exactly 4 spaces to the right of "if (".
> 

This code was removed and rewritten in a different way but there is no need for alignment in the new code.

> > PerformanceTests/SVG/resources/RenderAnimate.js:176
> > +        // If we we are vert close to the target, move to the next target
> 
> Instead of saying this in comment, allocate a descriptive local variable
> such as "approximatelyAtTarget".
> 

Done. A new object is defined 'Rectangle' and this object has a method called isAlmostEqual().

> > PerformanceTests/SVG/resources/RenderAnimate.js:179
> > +        // Keep moving to the target rectangle gradually
> 
> This comment repeats the code. Remove.
> 

Comment was removed.

> > PerformanceTests/SVG/resources/RenderAnimate.js:190
> > +    globals.image.style.width = globals.current.w + "px";
> > +    globals.image.style.left  = globals.current.x + "px";
> > +    globals.image.style.top   = globals.current.y + "px";
> 
> Ditto about aligning = signs.
> 

Alignment was removed.

> > PerformanceTests/SVG/resources/RenderAnimate.js:193
> > +function nextRun()
> 
> I would call this function recordResultAndScheduleNewRun or something since
> it does a lot more than just scheduling the next run.

The structure of the test is now different and I think the new one is clearer.

[Ryosuke Niwa]

Comment on attachment 245464 [details]
Patch

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

> PerformanceTests/SVG/resources/RenderAnimator.js:3
> +//
> +// This code is used to run an SVG benchmark through the WebKit performance test infrastructure.
> +//

This seems like an obvious point that doesn't deserve a comment.

> PerformanceTests/SVG/resources/RenderAnimator.js:5
> +//   1. Include it at the end of the body tag in the HTML

Is this really a requirement?  Can't we just wait for DOMContentLoaded for example?

> PerformanceTests/SVG/resources/RenderAnimator.js:6
> +//   2. The HTML has only two elements whose IDs are "Image" and "Timer".

Can we just insert "Timer" element right after the image element automatically instead?

> PerformanceTests/SVG/resources/RenderAnimator.js:17
> +function bind(f, obj) {
> +    return function() {
> +        return f.apply(obj, arguments);
> +    };
> +}

JavaScript has builtin "bind".  e.g. f.bind(obj).apply(arguments).

> PerformanceTests/SVG/resources/RenderAnimator.js:69
> +function ElapsedTimer() {

I wouldn't call this a timer since it wouldn't schedule anything.
How about ElapsedTime?

> PerformanceTests/SVG/resources/RenderAnimator.js:131
> +    this.initialize = function() {
> +        _animateMoveIndex = 0;
> +        this.nextTargetRect();
> +        _animateRect = _targetRect;
> +    };

Why can't we do this in the constructor itself and create a new animator in nextRun?
It seems weird to keep calling initialize on the same object.

> PerformanceTests/SVG/resources/RenderAnimator.js:181
> +        setTimeout(bind(this.startRun, this), this.timeoutDelay);

Why don't we just do: this.startRun.bind(this) instead?

> PerformanceTests/SVG/resources/RenderAnimator.js:184
> +    startRun : function() {

Since this function is only called inside nextRun, I would make this a closure inside nextRun instead.
We have too many functions that start or move to next frame, rect, etc... that it's getting confusing.

> PerformanceTests/SVG/resources/RenderAnimator.js:189
> +    nextTarget : function() {

nextTarget seems like a function that returns the next target rect.
We should verb'fy this function; e.g. moveToNextTargetRectOrFinish.

> PerformanceTests/SVG/resources/RenderAnimator.js:198
> +    nextMove : function() {

I would call this moveToNextAnimateRect.

> PerformanceTests/SVG/resources/RenderAnimator.js:222
> +    maximizeWindow : function() {
> +        window.moveTo(0, 0);
> +        window.resizeTo(screen.width, screen.height);
> +    },

I don't think we want to do this because it would mean that the benchmark result will depend on the screen size.
r- because of this screen-size dependency.

> PerformanceTests/SVG/resources/RenderAnimator.js:240
> +    renderAnimator.initialize();
> +    renderAnimator.nextRun();

We don't we make initialize call nextRun?  Maybe we can rename initialize to "run" if we did that.

[Radar WebKit Bug Importer]

<rdar://problem/19621168>

[Said Abou-Hallawa]

Created attachment 245513 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #22)
> Comment on attachment 245464 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245464&action=review
> 
> > PerformanceTests/SVG/resources/RenderAnimator.js:3
> > +//
> > +// This code is used to run an SVG benchmark through the WebKit performance test infrastructure.
> > +//
> 
> This seems like an obvious point that doesn't deserve a comment.
> 

The comment was removed

> > PerformanceTests/SVG/resources/RenderAnimator.js:5
> > +//   1. Include it at the end of the body tag in the HTML
> 
> Is this really a requirement?  Can't we just wait for DOMContentLoaded for
> example?
> 

Fixed. Also the script was moved in the <head> section of the HTML.

> > PerformanceTests/SVG/resources/RenderAnimator.js:6
> > +//   2. The HTML has only two elements whose IDs are "Image" and "Timer".
> 
> Can we just insert "Timer" element right after the image element
> automatically instead?
> 

Done. And the script can be used for any element and not necessarily to be an image. The only requirement now is to have an HTML element with className = "Benchmark"

> > PerformanceTests/SVG/resources/RenderAnimator.js:17
> > +function bind(f, obj) {
> > +    return function() {
> > +        return f.apply(obj, arguments);
> > +    };
> > +}
> 
> JavaScript has builtin "bind".  e.g. f.bind(obj).apply(arguments).
> 

Fixed.

> > PerformanceTests/SVG/resources/RenderAnimator.js:69
> > +function ElapsedTimer() {
> 
> I wouldn't call this a timer since it wouldn't schedule anything.
> How about ElapsedTime?
> 

Name was changed.

> > PerformanceTests/SVG/resources/RenderAnimator.js:131
> > +    this.initialize = function() {
> > +        _animateMoveIndex = 0;
> > +        this.nextTargetRect();
> > +        _animateRect = _targetRect;
> > +    };
> 
> Why can't we do this in the constructor itself and create a new animator in
> nextRun?
> It seems weird to keep calling initialize on the same object.
> 

Done.

> > PerformanceTests/SVG/resources/RenderAnimator.js:181
> > +        setTimeout(bind(this.startRun, this), this.timeoutDelay);
> 
> Why don't we just do: this.startRun.bind(this) instead?
> 

Fixed.

> > PerformanceTests/SVG/resources/RenderAnimator.js:184
> > +    startRun : function() {
> 
> Since this function is only called inside nextRun, I would make this a
> closure inside nextRun instead.
> We have too many functions that start or move to next frame, rect, etc...
> that it's getting confusing.
> 

Done.

> > PerformanceTests/SVG/resources/RenderAnimator.js:189
> > +    nextTarget : function() {
> 
> nextTarget seems like a function that returns the next target rect.
> We should verb'fy this function; e.g. moveToNextTargetRectOrFinish.
> 

Name was changed to moveToNextTargetRect() since the meaning is "move to the next target rect if possible". Otherwise I have to name nextMove() to moveToNextAnimateRectOrNextAnimateRect(). I do not think we need to name a function with all its possible outcomes or side effects.

> > PerformanceTests/SVG/resources/RenderAnimator.js:198
> > +    nextMove : function() {
> 
> I would call this moveToNextAnimateRect.
> 

Name was changed as suggested.

> > PerformanceTests/SVG/resources/RenderAnimator.js:222
> > +    maximizeWindow : function() {
> > +        window.moveTo(0, 0);
> > +        window.resizeTo(screen.width, screen.height);
> > +    },
> 
> I don't think we want to do this because it would mean that the benchmark
> result will depend on the screen size.
> r- because of this screen-size dependency.
> 

Done. Now the windowSize is set to (3000, 1500) since I noticed the tiling causes the SVG rendering to slow down significantly when the width is slightly bigger than 2000. If the width is less than 2000, the SVG rendering is very fast even without this patch.

With the windowSize hardcoded and without this patch, the rendering is slow regardless of the browser window size. With this patch, the performance is always faster than without it.

When resizing the window is removed, run-perf-tests is taking few seconds with one target rect. So I added few more target rects to ensure the results are more accurate. On my machine, each test is taking around 40 seconds.

> > PerformanceTests/SVG/resources/RenderAnimator.js:240
> > +    renderAnimator.initialize();
> > +    renderAnimator.nextRun();
> 
> We don't we make initialize call nextRun?  Maybe we can rename initialize to
> "run" if we did that.

renderAnimator.initialize() was removed. It functionality was moved to the object constructor.

[Simon Fraser (smfr)]

Comment on attachment 245513 [details]
Patch

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

> Source/WebCore/rendering/RenderImage.cpp:477
> -        bool clip = !contentRect.contains(paintRect);
> -        GraphicsContextStateSaver stateSaver(*context, clip);
> -        if (clip)
> -            context->clip(contentRect);
> +        GraphicsContextStateSaver stateSaver(*context, true);
> +        context->clip(snapRectToDevicePixels(contentRect, deviceScaleFactor));

We think this is wrong.

[Said Abou-Hallawa]

Created attachment 245589 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #26)
> Comment on attachment 245513 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245513&action=review
> 
> > Source/WebCore/rendering/RenderImage.cpp:477
> > -        bool clip = !contentRect.contains(paintRect);
> > -        GraphicsContextStateSaver stateSaver(*context, clip);
> > -        if (clip)
> > -            context->clip(contentRect);
> > +        GraphicsContextStateSaver stateSaver(*context, true);
> > +        context->clip(snapRectToDevicePixels(contentRect, deviceScaleFactor));
> 
> We think this is wrong.

Yes you are right. I misunderstood the purpose of this code and I thought it is clipping to the actual dirtyRect. The code has changed accordingly.

[Ryosuke Niwa]

Comment on attachment 245589 [details]
Patch

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

r=me on the perf. test you're adding. smfr should review the actual code change.

> PerformanceTests/SVG/resources/RenderAnimator.js:4
> +// equal to "Benchmark".  The function runTest() has to be called after the DOMContentLoaded
> +// event is fired.

Instead of commenting this, why don't we just automatically call runTest at DOMContentLoaded?
window.addEventListener('DOMContentLoaded', runTest);

[Said Abou-Hallawa]

Created attachment 245597 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #29)
> Comment on attachment 245589 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245589&action=review
> 
> r=me on the perf. test you're adding. smfr should review the actual code
> change.
> 
> > PerformanceTests/SVG/resources/RenderAnimator.js:4
> > +// equal to "Benchmark".  The function runTest() has to be called after the DOMContentLoaded
> > +// event is fired.
> 
> Instead of commenting this, why don't we just automatically call runTest at
> DOMContentLoaded?
> window.addEventListener('DOMContentLoaded', runTest);

I could not use 'DOMContentLoaded' event because it gets fired when the DOM is completely loaded, but without waiting for resources like images to finish loading. In these test, the SVG is not loaded when 'DOMContentLoaded' is fired so the test was getting the wrong image size. So I had to add a listener for 'load' event instead.

[WebKit Commit Bot]

Comment on attachment 245597 [details]
Patch

Clearing flags on attachment: 245597

Committed r179335: <http://trac.webkit.org/changeset/179335>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.