[svg] in img element doesn't render correctly when height is changed dynamically.

[svg] in img element doesn't render correctly when height is changed dynamically.

BUG=354822
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170437

[pdr., 2014-03-24 16:03:58]

LGTM with nits. Feel free to commit as you see fit.

https://codereview.chromium.org/210083003/diff/1/LayoutTests/svg/as-image/set...
File LayoutTests/svg/as-image/set-img-height-expected.html (right):

https://codereview.chromium.org/210083003/diff/1/LayoutTests/svg/as-image/set...
LayoutTests/svg/as-image/set-img-height-expected.html:10: <img id="scaleMe"
src="resources/default-profile.svg"/>
<img> is one of those weird not-self-closing tags. Can you change this to just
<img> instead of <img/>?

https://codereview.chromium.org/210083003/diff/1/LayoutTests/svg/as-image/set...
File LayoutTests/svg/as-image/set-img-height.html (right):

https://codereview.chromium.org/210083003/diff/1/LayoutTests/svg/as-image/set...
LayoutTests/svg/as-image/set-img-height.html:19: testRunner.notifyDone();
Indentation

https://codereview.chromium.org/210083003/diff/1/LayoutTests/svg/as-image/set...
LayoutTests/svg/as-image/set-img-height.html:20: }, 100);
I know I sound like a broken record on these setTimeouts :) This settimeout
seems too high; can it be replaced with raf (which guarantees a layout), forcing
a layout (body.offsetTop), or a shorter interval?

[f(malita), 2014-03-24 16:23:12]

lgtm + nit

https://codereview.chromium.org/210083003/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGRoot.h (right):

https://codereview.chromium.org/210083003/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGRoot.h:59: setNeedsLayout();
This can be hit during paint(), which would normally be a big no-no.

But SVGImage::draw() performs a view layout before painting, so as long as
setContainerSize() happens before that and doesn't bubble outside the view scope
(which I don't think it does) it should be fine. Maybe you can add a comment
explaining why this is ok.

Come to think of it, we should probably ASSERT(!view->needsLayout()) in
SVGImage::draw(), after view->paint(...).

[Erik Dahlström (inactive), 2014-03-26 08:12:24]

On 2014/03/24 16:23:12, Florin Malita wrote:
> lgtm + nit
> 
>
https://codereview.chromium.org/210083003/diff/1/Source/core/rendering/svg/Re...
> File Source/core/rendering/svg/RenderSVGRoot.h (right):
> 
>
https://codereview.chromium.org/210083003/diff/1/Source/core/rendering/svg/Re...
> Source/core/rendering/svg/RenderSVGRoot.h:59: setNeedsLayout();
> This can be hit during paint(), which would normally be a big no-no.

Would the assert you mention below be sufficient to catch this, or is it
possible to add another assert here to guard against that?

> But SVGImage::draw() performs a view layout before painting, so as long as
> setContainerSize() happens before that and doesn't bubble outside the view
scope
> (which I don't think it does) it should be fine. Maybe you can add a comment
> explaining why this is ok.

TBH my understanding of this code is still somewhat limited. I do think that it
would be wrong to paint the svg without first having done a layout for the case
where the size has changed.

> Come to think of it, we should probably ASSERT(!view->needsLayout()) in
> SVGImage::draw(), after view->paint(...).

Added.

[Erik Dahlström (inactive), 2014-03-26 08:21:25]

On 2014/03/24 16:03:58, pdr wrote:
> LGTM with nits. Feel free to commit as you see fit.
> 
>
https://codereview.chromium.org/210083003/diff/1/LayoutTests/svg/as-image/set...
> File LayoutTests/svg/as-image/set-img-height-expected.html (right):
> 
>
https://codereview.chromium.org/210083003/diff/1/LayoutTests/svg/as-image/set...
> LayoutTests/svg/as-image/set-img-height-expected.html:10: <img id="scaleMe"
> src="resources/default-profile.svg"/>
> <img> is one of those weird not-self-closing tags. Can you change this to just
> <img> instead of <img/>?
> 
>
https://codereview.chromium.org/210083003/diff/1/LayoutTests/svg/as-image/set...
> File LayoutTests/svg/as-image/set-img-height.html (right):
> 
>
https://codereview.chromium.org/210083003/diff/1/LayoutTests/svg/as-image/set...
> LayoutTests/svg/as-image/set-img-height.html:19: testRunner.notifyDone();
> Indentation
> 
>
https://codereview.chromium.org/210083003/diff/1/LayoutTests/svg/as-image/set...
> LayoutTests/svg/as-image/set-img-height.html:20: }, 100);
> I know I sound like a broken record on these setTimeouts :) This settimeout
> seems too high; can it be replaced with raf (which guarantees a layout),
forcing
> a layout (body.offsetTop), or a shorter interval?

I have converted it to a repaint test instead, because that's really what it is.

I have a major concern however, and that is that repaint tests don't seem to
work. When loading the testcase outside of run-webkit-tests the test fails and
passes as expected with and without this patch. However, when using
run-webkit-tests the snapshot seems to be taken at the wrong time, so it
incorrectly detects trunk as passing even though it should fail.

Given that repaint tests don't seem to work properly I'm worried that this will
just mean more and more repaint bugs will slip into Blink unnoticed. If you have
any ideas for how to fix this please let me know. If not, I'll bring this up in
the recent blink-dev thread about repaint tests to see if anyone has any ideas
there.

[f(malita), 2014-03-26 13:50:30]

On 2014/03/26 08:12:24, Erik Dahlström wrote:
>
https://codereview.chromium.org/210083003/diff/1/Source/core/rendering/svg/Re...
> > Source/core/rendering/svg/RenderSVGRoot.h:59: setNeedsLayout();
> > This can be hit during paint(), which would normally be a big no-no.
> 
> Would the assert you mention below be sufficient to catch this, or is it
> possible to add another assert here to guard against that?

I think it should be sufficient.

 
> > Come to think of it, we should probably ASSERT(!view->needsLayout()) in
> > SVGImage::draw(), after view->paint(...).
> 
> Added.

Thanks. LGTM++

[Erik Dahlström (inactive), 2014-03-27 14:46:15]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-03-27 14:46:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ed@opera.com/210083003/20001

[commit-bot: I haz the power, 2014-03-27 14:57:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 14:57:09]

Try jobs failed on following builders:
  tryserver.blink on blink_presubmit

[Erik Dahlström (inactive), 2014-03-27 16:35:03]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-03-27 16:35:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ed@opera.com/210083003/80001

[commit-bot: I haz the power, 2014-03-27 16:59:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 16:59:27]

Try jobs failed on following builders:
  tryserver.blink on blink_presubmit

[Erik Dahlström (inactive), 2014-03-28 08:08:09]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-03-28 08:08:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ed@opera.com/210083003/80001

[commit-bot: I haz the power, 2014-03-28 08:35:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 08:35:48]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[Erik Dahlström (inactive), 2014-03-28 08:46:32]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-03-28 08:46:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ed@opera.com/210083003/100001

[commit-bot: I haz the power, 2014-03-28 09:17:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 09:18:00]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[Erik Dahlström (inactive), 2014-03-31 09:11:55]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-03-31 09:12:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ed@opera.com/210083003/120001

[commit-bot: I haz the power, 2014-03-31 10:16:13]

Change committed as 170437