Expand IntrinsicSizingInfo for SVG

Expand IntrinsicSizingInfo for SVG

Add fields to IntrinsicSizingInfo specifying whether the intrinsic
width and height are specified or not. For SVGs there is a distinction
between missing width/height and setting width/height to 0. There is
code in LayoutReplaced that has specific hooks into the SVG code to
make this distinction. By having separate fields in
IntrinsicSizingInfo this entanglement can be broken.

BUG=585467
Committed: https://crrev.com/a0ec285a1629a9d7a3c62517d4c62106a02fbf70
Cr-Commit-Position: refs/heads/master@{#374909}

[davve, 2016-02-09 17:38:51]

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

[fs, 2016-02-09 18:37:54]

LGTM w/ nit and suggestion

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

https://codereview.chromium.org/1679743006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutReplaced.cpp:559: bool
hasIntrinsicWidth = !intrinsicSizingInfo.emptyWidth;
Maybe consider adding accessors on IntrinsicSizingInfo and then get rid of the
hasIntrinsicWidth local - hasWidth/Height? (I find myself a bit at odds with the
"empty" prefix.)

https://codereview.chromium.org/1679743006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.h (right):

https://codereview.chromium.org/1679743006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.h:118: static FloatSize
calculateIntrinsicSize(SVGSVGElement*);
Nit: Could this be local to LayoutSVGRoot.cpp even? Also, const SVGSVGElement&?

[davve, 2016-02-11 08:52:37]

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

https://codereview.chromium.org/1679743006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutReplaced.cpp:559: bool
hasIntrinsicWidth = !intrinsicSizingInfo.emptyWidth;
On 2016/02/09 18:37:54, fs wrote:
> Maybe consider adding accessors on IntrinsicSizingInfo and then get rid of the
> hasIntrinsicWidth local - hasWidth/Height? (I find myself a bit at odds with
the
> "empty" prefix.)

Turned it around to avoid "empty" prefix. Hopefully I don't need accessors at
this point.

https://codereview.chromium.org/1679743006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.h (right):

https://codereview.chromium.org/1679743006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.h:118: static FloatSize
calculateIntrinsicSize(SVGSVGElement*);
On 2016/02/09 18:37:54, fs wrote:
> Nit: Could this be local to LayoutSVGRoot.cpp even? Also, const
SVGSVGElement&?

Done.

[fs, 2016-02-11 09:22:45]

On 2016/02/11 at 08:52:37, davve wrote:
>
https://codereview.chromium.org/1679743006/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutReplaced.cpp (right):
> 
>
https://codereview.chromium.org/1679743006/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutReplaced.cpp:559: bool
hasIntrinsicWidth = !intrinsicSizingInfo.emptyWidth;
> On 2016/02/09 18:37:54, fs wrote:
> > Maybe consider adding accessors on IntrinsicSizingInfo and then get rid of
the
> > hasIntrinsicWidth local - hasWidth/Height? (I find myself a bit at odds with
the
> > "empty" prefix.)
> 
> Turned it around to avoid "empty" prefix. Hopefully I don't need accessors at
this point.
> 
>
https://codereview.chromium.org/1679743006/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.h (right):
> 
>
https://codereview.chromium.org/1679743006/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.h:118: static
FloatSize calculateIntrinsicSize(SVGSVGElement*);
> On 2016/02/09 18:37:54, fs wrote:
> > Nit: Could this be local to LayoutSVGRoot.cpp even? Also, const
SVGSVGElement&?
> 
> Done.

Still LGTM

[davve, 2016-02-11 15:29:30]

The CQ bit was checked by davve@opera.com

[commit-bot: I haz the power, 2016-02-11 15:29:59]

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

[commit-bot: I haz the power, 2016-02-11 16:45:19]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-16 22:35:53]

Description was changed from

==========
Expand IntrinsicSizingInfo for SVG

Add fields to IntrinsicSizingInfo specifying whether the intrinsic
width and height are specified or not. For SVGs there is a distinction
between missing width/height and setting width/height to 0. There is
code in LayoutReplaced that has specific hooks into the SVG code to
make this distinction. By having separate fields in
IntrinsicSizingInfo this entanglement can be broken.

BUG=585467
==========

to

==========
Expand IntrinsicSizingInfo for SVG

Add fields to IntrinsicSizingInfo specifying whether the intrinsic
width and height are specified or not. For SVGs there is a distinction
between missing width/height and setting width/height to 0. There is
code in LayoutReplaced that has specific hooks into the SVG code to
make this distinction. By having separate fields in
IntrinsicSizingInfo this entanglement can be broken.

BUG=585467

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

[commit-bot: I haz the power, 2016-02-16 22:35:54]

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