Order bounds update correctly for LayoutSVGShape and LayoutSVGImage

Order bounds update correctly for LayoutSVGShape and LayoutSVGImage

Since SVGResourcesCache::clientLayoutChanged can set the
m_needsBoundariesUpdate flag, make sure to check it, and act, after
that call. This ensures that state is consistent. when layout() return.
Also add a comment about the delicate situation in
LayoutSVGShape::layout wrt shape vs. bounds update. Additionally rename
the local variable used to notify the parent that its bounds needs to be
updated to |updateParentBoundaries|.

BUG=603956
Committed: https://crrev.com/fe8421b9573ca696e596248811edf0a5d23fc191
Cr-Commit-Position: refs/heads/master@{#389410}

[fs, 2016-04-22 18:31:11]

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

[fs, 2016-04-22 18:31:11]

I noticed this while vacuuming the layout code. I've yet to come up with a case
where this causes any ill effects (although it's easy to trigger inconsistent
state - i.e the bounds-update flag remain set when layout() completes - just
have a filter applied and then trigger a layout of the object.)

[pdr., 2016-04-22 22:33:33]

Nice catch, I bet there are some subtle bugs fixed by this. Just a couple of
small comments.

https://codereview.chromium.org/1907333002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right):

https://codereview.chromium.org/1907333002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:91: // Invalidate
all resources of this client if our layout changed.
It's a little confusing to have this in different places in different svg layout
functions. Can we just move it to the very top in all of them?

https://codereview.chromium.org/1907333002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:106:
clearNeedsLayout();
Possibly assert here (and in the other functions) that all the m_needs* are
false?

[fs, 2016-04-22 22:53:07]

https://codereview.chromium.org/1907333002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right):

https://codereview.chromium.org/1907333002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:91: // Invalidate
all resources of this client if our layout changed.
On 2016/04/22 at 22:33:33, pdr wrote:
> It's a little confusing to have this in different places in different svg
layout functions. Can we just move it to the very top in all of them?

I hope we can... (This is pretty side-effecty - as was found out here - but
reasonably shouldn't touch/rely on state from the client.)

https://codereview.chromium.org/1907333002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:106:
clearNeedsLayout();
On 2016/04/22 at 22:33:33, pdr wrote:
> Possibly assert here (and in the other functions) that all the m_needs* are
false?

Yeah, that's probably a good idea.

[fs, 2016-04-23 16:53:16]

On 2016/04/22 at 22:53:07, fs wrote:
>
https://codereview.chromium.org/1907333002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right):
> 
>
https://codereview.chromium.org/1907333002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:91: // Invalidate
all resources of this client if our layout changed.
> On 2016/04/22 at 22:33:33, pdr wrote:
> > It's a little confusing to have this in different places in different svg
layout functions. Can we just move it to the very top in all of them?
> 
> I hope we can... (This is pretty side-effecty - as was found out here - but
reasonably shouldn't touch/rely on state from the client.)
> 
>
https://codereview.chromium.org/1907333002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:106:
clearNeedsLayout();
> On 2016/04/22 at 22:33:33, pdr wrote:
> > Possibly assert here (and in the other functions) that all the m_needs* are
false?
> 
> Yeah, that's probably a good idea.

Shuffled and done.

[pdr., 2016-04-24 22:38:45]

LGTM

[pdr., 2016-04-24 22:38:48]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2016-04-24 22:38:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1907333002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1907333002/20001

[commit-bot: I haz the power, 2016-04-25 01:37:31]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-25 01:39:36]

Description was changed from

==========
Order bounds update correctly for LayoutSVGShape and LayoutSVGImage

Since SVGResourcesCache::clientLayoutChanged can set the
m_needsBoundariesUpdate flag, make sure to check it, and act, after
that call. This ensures that state is consistent. when layout() return.
Also add a comment about the delicate situation in
LayoutSVGShape::layout wrt shape vs. bounds update. Additionally rename
the local variable used to notify the parent that its bounds needs to be
updated to |updateParentBoundaries|.

BUG=603956
==========

to

==========
Order bounds update correctly for LayoutSVGShape and LayoutSVGImage

Since SVGResourcesCache::clientLayoutChanged can set the
m_needsBoundariesUpdate flag, make sure to check it, and act, after
that call. This ensures that state is consistent. when layout() return.
Also add a comment about the delicate situation in
LayoutSVGShape::layout wrt shape vs. bounds update. Additionally rename
the local variable used to notify the parent that its bounds needs to be
updated to |updateParentBoundaries|.

BUG=603956

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

[commit-bot: I haz the power, 2016-04-25 01:39:37]

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