Cleanups to viewport-handling and image invalidation in LayoutSVGImage

Cleanups to viewport-handling and image invalidation in LayoutSVGImage

Split updateImageViewport into its two (now independent except for the
bbox, so ordering matters still) parts, naming them updateBoundingBox
and updateImageContainerSize.
Stop updating the bbox in imageChanged since updateImageContainerSize
will update the container size of the referenced image regardless.
Also drop the (explicit) SVGResource::removeClientFromCache call from
imageChanged, because markForLayoutAndParentResourceInvalidation is
called just after it, and will do what's required already.

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

[fs, 2015-07-01 15:48:36]

fs@opera.com changed reviewers:
+ fmalita@chromium.org, pdr@chromium.org, schenney@chromium.org

[fs, 2015-07-01 15:48:37]

Tangential to the referenced bug, but found while looking at it...

[f(malita), 2015-07-01 16:39:35]

https://codereview.chromium.org/1219493005/diff/1/Source/core/layout/svg/Layo...
File Source/core/layout/svg/LayoutSVGImage.cpp (right):

https://codereview.chromium.org/1219493005/diff/1/Source/core/layout/svg/Layo...
Source/core/layout/svg/LayoutSVGImage.cpp:188: updateImageContainerSize();
If we're no longer updating the bbox here, is it possible to leave the
LayoutSVGImage in an inconsistent state (what happens at paint time)?

(based on that comment, it sounds like we're already in dangerous territory here
- wondering if this makes it worse)

[fs, 2015-07-01 16:58:13]

https://codereview.chromium.org/1219493005/diff/1/Source/core/layout/svg/Layo...
File Source/core/layout/svg/LayoutSVGImage.cpp (right):

https://codereview.chromium.org/1219493005/diff/1/Source/core/layout/svg/Layo...
Source/core/layout/svg/LayoutSVGImage.cpp:188: updateImageContainerSize();
On 2015/07/01 16:39:34, f(malita) wrote:
> If we're no longer updating the bbox here, is it possible to leave the
> LayoutSVGImage in an inconsistent state (what happens at paint time)?
> 
> (based on that comment, it sounds like we're already in dangerous territory
here
> - wondering if this makes it worse)

The image (dimensions) does not affect the viewport (== the bbox) in which we
paint it. When the above comment was added, the artificial bounds change was
required to trigger the additional logic to update the container size - this is
no longer the case.
If the bounds change we'll get a layout - which will update the bbox and the
container size - so if the bounds are dirty at the point when this is called,
the container size will be updated again during layout. If the bounds are not
dirty this is a no-op except for the case mentioned in the comment (loading
finished after layout) - which we do have a test for.

[f(malita), 2015-07-01 17:07:32]

On 2015/07/01 at 16:58:13, fs wrote:
>
https://codereview.chromium.org/1219493005/diff/1/Source/core/layout/svg/Layo...
> File Source/core/layout/svg/LayoutSVGImage.cpp (right):
> 
>
https://codereview.chromium.org/1219493005/diff/1/Source/core/layout/svg/Layo...
> Source/core/layout/svg/LayoutSVGImage.cpp:188: updateImageContainerSize();
> On 2015/07/01 16:39:34, f(malita) wrote:
> > If we're no longer updating the bbox here, is it possible to leave the
> > LayoutSVGImage in an inconsistent state (what happens at paint time)?
> > 
> > (based on that comment, it sounds like we're already in dangerous territory
here
> > - wondering if this makes it worse)
> 
> The image (dimensions) does not affect the viewport (== the bbox) in which we
paint it. When the above comment was added, the artificial bounds change was
required to trigger the additional logic to update the container size - this is
no longer the case.
> If the bounds change we'll get a layout - which will update the bbox and the
container size - so if the bounds are dirty at the point when this is called,
the container size will be updated again during layout. If the bounds are not
dirty this is a no-op except for the case mentioned in the comment (loading
finished after layout) - which we do have a test for.

Thanks for the explanation.  LGTM, but pdr or schenney should also take a look.

[fs, 2015-07-02 08:47:57]

On 2015/07/01 17:07:32, f(malita) wrote:
> On 2015/07/01 at 16:58:13, fs wrote:
> >
>
https://codereview.chromium.org/1219493005/diff/1/Source/core/layout/svg/Layo...
> > File Source/core/layout/svg/LayoutSVGImage.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1219493005/diff/1/Source/core/layout/svg/Layo...
> > Source/core/layout/svg/LayoutSVGImage.cpp:188: updateImageContainerSize();
> > On 2015/07/01 16:39:34, f(malita) wrote:
> > > If we're no longer updating the bbox here, is it possible to leave the
> > > LayoutSVGImage in an inconsistent state (what happens at paint time)?
> > > 
> > > (based on that comment, it sounds like we're already in dangerous
territory
> here
> > > - wondering if this makes it worse)
> > 
> > The image (dimensions) does not affect the viewport (== the bbox) in which
we
> paint it. When the above comment was added, the artificial bounds change was
> required to trigger the additional logic to update the container size - this
is
> no longer the case.
> > If the bounds change we'll get a layout - which will update the bbox and the
> container size - so if the bounds are dirty at the point when this is called,
> the container size will be updated again during layout. If the bounds are not
> dirty this is a no-op except for the case mentioned in the comment (loading
> finished after layout) - which we do have a test for.
> 
> Thanks for the explanation.  LGTM, but pdr or schenney should also take a
look.

Thanks. Holding for the President(s).

[pdr., 2015-07-02 19:13:31]

On 2015/07/02 at 08:47:57, fs wrote:
> On 2015/07/01 17:07:32, f(malita) wrote:
> > On 2015/07/01 at 16:58:13, fs wrote:
> > >
> >
https://codereview.chromium.org/1219493005/diff/1/Source/core/layout/svg/Layo...
> > > File Source/core/layout/svg/LayoutSVGImage.cpp (right):
> > > 
> > >
> >
https://codereview.chromium.org/1219493005/diff/1/Source/core/layout/svg/Layo...
> > > Source/core/layout/svg/LayoutSVGImage.cpp:188: updateImageContainerSize();
> > > On 2015/07/01 16:39:34, f(malita) wrote:
> > > > If we're no longer updating the bbox here, is it possible to leave the
> > > > LayoutSVGImage in an inconsistent state (what happens at paint time)?
> > > > 
> > > > (based on that comment, it sounds like we're already in dangerous
territory
> > here
> > > > - wondering if this makes it worse)
> > > 
> > > The image (dimensions) does not affect the viewport (== the bbox) in which
we
> > paint it. When the above comment was added, the artificial bounds change was
> > required to trigger the additional logic to update the container size - this
is
> > no longer the case.
> > > If the bounds change we'll get a layout - which will update the bbox and
the
> > container size - so if the bounds are dirty at the point when this is
called,
> > the container size will be updated again during layout. If the bounds are
not
> > dirty this is a no-op except for the case mentioned in the comment (loading
> > finished after layout) - which we do have a test for.
> > 
> > Thanks for the explanation.  LGTM, but pdr or schenney should also take a
look.
> 
> Thanks. Holding for the President(s).

LGTM, sorry about the review times lately. Slimming paint..


I'm just a vice president though

and an ice cold beer is my vice.

[pdr., 2015-07-02 19:13:41]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2015-07-02 19:13:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1219493005/1

[commit-bot: I haz the power, 2015-07-02 20:24:28]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198241