Fallback to 300x150 instead of 0x0 size for SVG inside content()

Fallback to 300x150 instead of 0x0 size for SVG inside content()

Prior to r379801, the fallback 300x150 in SVGImage::dataChanged() was
used. Post r379801 we instead used the empty rect as fallback. Both
are probably wrong but 300x150 matches what we did previously more
closely and we are less likely to end up with an empty image.

BUG=623528
Committed: https://crrev.com/da6dc5be059c4dd6a6b7f1a4ba9c1f12be3d67ae
Cr-Commit-Position: refs/heads/master@{#404374}

[davve, 2016-07-05 13:42:34]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-05 13:42:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-05 14:32:41]

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

[commit-bot: I haz the power, 2016-07-05 14:32:42]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[davve, 2016-07-05 15:48:16]

davve@opera.com changed reviewers:
+ fs@opera.com

[davve, 2016-07-05 15:48:17]

This is the short-term solution that brings back the old fallback size.

I'm not entirely sure why this fixes the bug. What it seems to affect is that
setNeedsLayoutAndFullPaintInvalidation() is called
LayoutImage::invalidatePaintAndMarkForLayoutIfNeeded() when the "intrinsic" size
has changed. If intrinsic size stays 0x0 (as on trunk) only paint invalidation
is done, and the bug manifests.

Will look more tomorrow but wanted to loop you in in case you have some insight
that I might have missed.

[fs, 2016-07-05 16:20:28]

On 2016/07/05 at 15:48:17, davve wrote:
> This is the short-term solution that brings back the old fallback size.
> 
> I'm not entirely sure why this fixes the bug. What it seems to affect is that
setNeedsLayoutAndFullPaintInvalidation() is called
LayoutImage::invalidatePaintAndMarkForLayoutIfNeeded() when the "intrinsic" size
has changed. If intrinsic size stays 0x0 (as on trunk) only paint invalidation
is done, and the bug manifests.
> 
> Will look more tomorrow but wanted to loop you in in case you have some
insight that I might have missed.

No greater insights I'm afraid. Feels like just another case where a lack of
intrinsic dimensions ends up being problematic (and working by chance only
previously.)

[davve, 2016-07-07 09:02:57]

On 2016/07/05 16:20:28, fs wrote:
> On 2016/07/05 at 15:48:17, davve wrote:
> > This is the short-term solution that brings back the old fallback size.
> > 
> > I'm not entirely sure why this fixes the bug. What it seems to affect is
that
> setNeedsLayoutAndFullPaintInvalidation() is called
> LayoutImage::invalidatePaintAndMarkForLayoutIfNeeded() when the "intrinsic"
size
> has changed. If intrinsic size stays 0x0 (as on trunk) only paint invalidation
> is done, and the bug manifests.
> > 
> > Will look more tomorrow but wanted to loop you in in case you have some
> insight that I might have missed.
> 
> No greater insights I'm afraid. Feels like just another case where a lack of
> intrinsic dimensions ends up being problematic (and working by chance only
> previously.)

I've looked through this again and think it's the right thing to do for now. As
long as the 300x150 default object size is included in intrinsic size in the end
of SVGImage::dataChanged(), we should include it when computing the size for
LayoutImageResourceStyleImage.cpp.

With, if you like, a FIXME that the intrinsic size ideally never should include
the defaultObjectSize (because that turns it into something else, but changing
that is a bigger task.) WDYT?

[fs, 2016-07-07 09:06:39]

On 2016/07/07 at 09:02:57, davve wrote:
> On 2016/07/05 16:20:28, fs wrote:
> > On 2016/07/05 at 15:48:17, davve wrote:
> > > This is the short-term solution that brings back the old fallback size.
> > > 
> > > I'm not entirely sure why this fixes the bug. What it seems to affect is
that
> > setNeedsLayoutAndFullPaintInvalidation() is called
> > LayoutImage::invalidatePaintAndMarkForLayoutIfNeeded() when the "intrinsic"
size
> > has changed. If intrinsic size stays 0x0 (as on trunk) only paint
invalidation
> > is done, and the bug manifests.
> > > 
> > > Will look more tomorrow but wanted to loop you in in case you have some
> > insight that I might have missed.
> > 
> > No greater insights I'm afraid. Feels like just another case where a lack of
> > intrinsic dimensions ends up being problematic (and working by chance only
> > previously.)
> 
> I've looked through this again and think it's the right thing to do for now.
As long as the 300x150 default object size is included in intrinsic size in the
end of SVGImage::dataChanged(), we should include it when computing the size for
LayoutImageResourceStyleImage.cpp.
> 
> With, if you like, a FIXME that the intrinsic size ideally never should
include the defaultObjectSize (because that turns it into something else, but
changing that is a bigger task.) WDYT?

Yes, that sounds reasonable to me. BTW, did you check what other browser do in
this particular case? LGTM with or without extended FIXME/TODO.

[davve, 2016-07-07 11:59:43]

On 2016/07/07 09:06:39, fs wrote:
> On 2016/07/07 at 09:02:57, davve wrote:
> 
> Yes, that sounds reasonable to me. BTW, did you check what other browser do in
> this particular case? LGTM with or without extended FIXME/TODO.

For :after, it looks to me like we create a LayoutBlockFlow
corresponding to the :after with a LayoutImage child. LayoutImage has
default (0x0) intrinsic ratio until the image has loaded.  When the
newly loaded SVG image has no intrinsic size, we get into trouble
becaue we can't distinguish between:

 1. (default intrinsic size 0x0) -> (loaded intrinsic size 0x0) -
    should cause re-layout of LayoutImage because a non-intrinsic
    sized SVG is still able to draw.

 2 (loaded intrinsic size 0x0) -> (loaded intrinsic size 0x0) - not
    meant to cause re-layout.

The actual computed intrinsicSize in
LayoutImage::invalidatePaintAndMarkForLayoutIfNeeded() is actually
computed to doesn't seem to matter much. It's overwritten during the
following layout. The intrinsic size stuff there seems to mostly be an
optmization to not layout when a repaint will do, and this
optimization gets into the way.

So as long as we get the layout, as we do when (1) is:

  (not loaded intrinsic size 0x0) -> (loaded intrinsic size !0x!0)

we will get the correct rendering later (compared to FF in the cases
I've tested).

[fs, 2016-07-07 12:17:28]

On 2016/07/07 at 11:59:43, davve wrote:
> On 2016/07/07 09:06:39, fs wrote:
> > On 2016/07/07 at 09:02:57, davve wrote:
> > 
> > Yes, that sounds reasonable to me. BTW, did you check what other browser do
in
> > this particular case? LGTM with or without extended FIXME/TODO.
> 
> For :after, it looks to me like we create a LayoutBlockFlow
> corresponding to the :after with a LayoutImage child. LayoutImage has
> default (0x0) intrinsic ratio until the image has loaded.  When the
> newly loaded SVG image has no intrinsic size, we get into trouble
> becaue we can't distinguish between:
> 
>  1. (default intrinsic size 0x0) -> (loaded intrinsic size 0x0) -
>     should cause re-layout of LayoutImage because a non-intrinsic
>     sized SVG is still able to draw.
> 
>  2 (loaded intrinsic size 0x0) -> (loaded intrinsic size 0x0) - not
>     meant to cause re-layout.
> 
> The actual computed intrinsicSize in
> LayoutImage::invalidatePaintAndMarkForLayoutIfNeeded() is actually
> computed to doesn't seem to matter much. It's overwritten during the
> following layout. The intrinsic size stuff there seems to mostly be an
> optmization to not layout when a repaint will do, and this
> optimization gets into the way.
> 
> So as long as we get the layout, as we do when (1) is:
> 
>   (not loaded intrinsic size 0x0) -> (loaded intrinsic size !0x!0)
> 
> we will get the correct rendering later (compared to FF in the cases
> I've tested).

Ok. I think the "not loaded" -> "loaded" transition can be detected now
(couldn't previously); imageNotifyFinished vs. imageChanged (I think
imageNotifyFinished implies imageChanged in some way too.) Is that something we
could use to trigger a layout?

[davve, 2016-07-07 13:11:32]

On 2016/07/07 12:17:28, fs wrote:
> On 2016/07/07 at 11:59:43, davve wrote:
> > On 2016/07/07 09:06:39, fs wrote:
> > > On 2016/07/07 at 09:02:57, davve wrote:
> > > 
> > > Yes, that sounds reasonable to me. BTW, did you check what other browser
do
> in
> > > this particular case? LGTM with or without extended FIXME/TODO.
> > 
> > For :after, it looks to me like we create a LayoutBlockFlow
> > corresponding to the :after with a LayoutImage child. LayoutImage has
> > default (0x0) intrinsic ratio until the image has loaded.  When the
> > newly loaded SVG image has no intrinsic size, we get into trouble
> > becaue we can't distinguish between:
> > 
> >  1. (default intrinsic size 0x0) -> (loaded intrinsic size 0x0) -
> >     should cause re-layout of LayoutImage because a non-intrinsic
> >     sized SVG is still able to draw.
> > 
> >  2 (loaded intrinsic size 0x0) -> (loaded intrinsic size 0x0) - not
> >     meant to cause re-layout.
> > 
> > The actual computed intrinsicSize in
> > LayoutImage::invalidatePaintAndMarkForLayoutIfNeeded() is actually
> > computed to doesn't seem to matter much. It's overwritten during the
> > following layout. The intrinsic size stuff there seems to mostly be an
> > optmization to not layout when a repaint will do, and this
> > optimization gets into the way.
> > 
> > So as long as we get the layout, as we do when (1) is:
> > 
> >   (not loaded intrinsic size 0x0) -> (loaded intrinsic size !0x!0)
> > 
> > we will get the correct rendering later (compared to FF in the cases
> > I've tested).
> 
> Ok. I think the "not loaded" -> "loaded" transition can be detected now
> (couldn't previously); imageNotifyFinished vs. imageChanged (I think
> imageNotifyFinished implies imageChanged in some way too.) Is that something
we
> could use to trigger a layout?

Maybe. I'm not sure that would catch cases where e.g. the viewBox changes and
the :after (or what it is) hasn't got a specified size. In that case we still
have to rely on imageChanged, I think.

[davve, 2016-07-08 08:03:47]

The CQ bit was checked by davve@opera.com

[commit-bot: I haz the power, 2016-07-08 08:04:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 14:25:22]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-08 14:27:48]

Description was changed from

==========
Fallback to 300x150 instead of 0x0 size for SVG inside content()

Prior to r379801, the fallback 300x150 in SVGImage::dataChanged() was
used. Post r379801 we instead used the empty rect as fallback. Both
are probably wrong but 300x150 matches what we did previously more
closely and we are less likely to end up with an empty image.

BUG=623528
==========

to

==========
Fallback to 300x150 instead of 0x0 size for SVG inside content()

Prior to r379801, the fallback 300x150 in SVGImage::dataChanged() was
used. Post r379801 we instead used the empty rect as fallback. Both
are probably wrong but 300x150 matches what we did previously more
closely and we are less likely to end up with an empty image.

BUG=623528

Committed: https://crrev.com/da6dc5be059c4dd6a6b7f1a4ba9c1f12be3d67ae
Cr-Commit-Position: refs/heads/master@{#404374}
==========

[commit-bot: I haz the power, 2016-07-08 14:27:49]

Patchset 2 (id:??) landed as
https://crrev.com/da6dc5be059c4dd6a6b7f1a4ba9c1f12be3d67ae
Cr-Commit-Position: refs/heads/master@{#404374}