Fix getComputedStyle for non-box-layout elements

Fix getComputedStyle for non-box-layout elements

This fixes a crash introduced by https://codereview.chromium.org/1826423003/
Code that treated a layout object as a Box layout were not properly
guarded by isBox.  This caused a crash when calling getComputedStyle on
positioned non-box elements, e.g. <ruby>.

R=mstensho@opera.com
BUG=610986
Committed: https://crrev.com/86d16a615d6e9abf6d991f430b496c26a4271d10
Cr-Commit-Position: refs/heads/master@{#404336}

[Mr. Kevin, 2016-06-27 22:23:19]

Description was changed from

==========
Fix getComputedStyle for non-box-layout elements

This fixes a crash introduced by https://codereview.chromium.org/1826423003/
Code that treated a layout object as a Box layout were not properly
guarded by isBox.  This caused a crash when calling getComputedStyle on
positioned non-box elements, e.g. <ruby>.

R=mstensho@opera.com
BUG=610986
==========

to

==========
Fix getComputedStyle for non-box-layout elements

This fixes a crash introduced by https://codereview.chromium.org/1826423003/
Code that treated a layout object as a Box layout were not properly
guarded by isBox.  This caused a crash when calling getComputedStyle on
positioned non-box elements, e.g. <ruby>.

R=mstensho@opera.com
BUG=610986
==========

[Mr. Kevin, 2016-06-27 22:23:19]

khart@codeaurora.org changed reviewers:
+ inferno@chromium.org

[Mr. Kevin, 2016-06-27 22:24:12]

The CQ bit was checked by khart@codeaurora.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-27 22:25:09]

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-06-27 23:36:35]

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

[commit-bot: I haz the power, 2016-06-27 23:36:36]

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_...)

[Timothy Loh, 2016-06-28 00:20:29]

https://codereview.chromium.org/2102843002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
(right):

https://codereview.chromium.org/2102843002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:218:
opposite *= -1.f;
This looks like it might hit an ASSERT if the value is something like calc(1px +
2%)?

[Mr. Kevin, 2016-06-28 00:55:52]

https://codereview.chromium.org/2102843002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
(right):

https://codereview.chromium.org/2102843002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:218:
opposite *= -1.f;
On 2016/06/28 00:20:29, Timothy Loh wrote:
> This looks like it might hit an ASSERT if the value is something like calc(1px
+
> 2%)?

Good catch.  I'll see what I can come up with.

[Mr. Kevin, 2016-06-29 02:11:06]

On 2016/06/28 00:55:52, Mr. Kevin wrote:
>
https://codereview.chromium.org/2102843002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
> (right):
> 
>
https://codereview.chromium.org/2102843002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:218:
> opposite *= -1.f;
> On 2016/06/28 00:20:29, Timothy Loh wrote:
> > This looks like it might hit an ASSERT if the value is something like
calc(1px
> +
> > 2%)?
> 
> Good catch.  I'll see what I can come up with.

Fixed the ASSERT, but relative-positioned inline objects are not returning used
values as they should per the spec.  I've tried every variant of container and
width I could think of, but I can't get the width of the container of a non-box
layout object.  (everything returns 0).  So I can't calculate the used value for
a % or calc value for left or right. 
layoutObject->containingBlock()->availableLogicalHeight(ExcludeMarginBorderPadding)
seems to work for the height.  If you could point me in the right direction I'd
love to get everything returning used values

[Mr. Kevin, 2016-06-29 16:54:48]

The CQ bit was checked by khart@codeaurora.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-29 16:55:20]

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-06-29 17:54:58]

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

[commit-bot: I haz the power, 2016-06-29 17:54:59]

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

[Mr. Kevin, 2016-06-30 16:58:01]

khart@codeaurora.org changed reviewers:
+ timloh@chromium.org

[Mr. Kevin, 2016-06-30 17:01:45]

On 2016/06/29 17:54:59, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

It doesn't look like these failures are related to my patch.  Any suggestions?

[Timothy Loh, 2016-06-30 23:47:14]

On 2016/06/30 17:01:45, Mr. Kevin wrote:
> On 2016/06/29 17:54:59, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> >   win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
> 
> It doesn't look like these failures are related to my patch.  Any suggestions?

unexpected_failures:
fast/css/getComputedStyle/getComputedStyle-resolved-values.html

seems related

[Timothy Loh, 2016-06-30 23:48:32]

timloh@chromium.org changed reviewers:
+ eae@chromium.org

[Timothy Loh, 2016-06-30 23:48:33]

+eae, Kevin had some questions about getting the width of non-box containers,
maybe you could help?

[eae, 2016-06-30 23:54:53]

On 2016/06/29 02:11:06, Mr. Kevin wrote:
> On 2016/06/28 00:55:52, Mr. Kevin wrote:
> >
>
https://codereview.chromium.org/2102843002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2102843002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:218:
> > opposite *= -1.f;
> > On 2016/06/28 00:20:29, Timothy Loh wrote:
> > > This looks like it might hit an ASSERT if the value is something like
> calc(1px
> > +
> > > 2%)?
> > 
> > Good catch.  I'll see what I can come up with.
> 
> Fixed the ASSERT, but relative-positioned inline objects are not returning
used
> values as they should per the spec.  I've tried every variant of container and
> width I could think of, but I can't get the width of the container of a
non-box
> layout object.  (everything returns 0).  So I can't calculate the used value
for
> a % or calc value for left or right. 
>
layoutObject->containingBlock()->availableLogicalHeight(ExcludeMarginBorderPadding)
> seems to work for the height.  If you could point me in the right direction
I'd
> love to get everything returning used values

InlineBox::width should do the trick, does it not work here?

[Mr. Kevin, 2016-07-01 21:30:55]

On 2016/06/30 23:54:53, eae wrote:
> On 2016/06/29 02:11:06, Mr. Kevin wrote:
> > On 2016/06/28 00:55:52, Mr. Kevin wrote:
> > >
> >
>
https://codereview.chromium.org/2102843002/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2102843002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:218:
> > > opposite *= -1.f;
> > > On 2016/06/28 00:20:29, Timothy Loh wrote:
> > > > This looks like it might hit an ASSERT if the value is something like
> > calc(1px
> > > +
> > > > 2%)?
> > > 
> > > Good catch.  I'll see what I can come up with.
> > 
> > Fixed the ASSERT, but relative-positioned inline objects are not returning
> used
> > values as they should per the spec.  I've tried every variant of container
and
> > width I could think of, but I can't get the width of the container of a
> non-box
> > layout object.  (everything returns 0).  So I can't calculate the used value
> for
> > a % or calc value for left or right. 
> >
>
layoutObject->containingBlock()->availableLogicalHeight(ExcludeMarginBorderPadding)
> > seems to work for the height.  If you could point me in the right direction
> I'd
> > love to get everything returning used values
> 
> InlineBox::width should do the trick, does it not work here?

Maybe?
Given const LayoutInline* layoutInline=toLayoutInline(layoutObject), then
layoutInline->linesBoundingBox() has width() and height() equal to 0.0,
and layoutInline->lineBoxes()->firstLineBox() is nullptr.
Is there a different way to get an InlineBox that I'm not seeing?

BTW, The render tree for the LayoutInline of interest looks like this:

layer at (23,23) size 61x19
  LayoutInline (relative positioned) {EM} at (0,0) size 61x19 [bgcolor=#008000]
    LayoutText {#text} at (35,0) size 61x19
      text run at (35,0) width 61: "should be"

[Mr. Kevin, 2016-07-05 17:10:22]

The CQ bit was checked by khart@codeaurora.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-05 17:10:31]

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 19:00:36]

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

[commit-bot: I haz the power, 2016-07-05 19:00:37]

Dry run: This issue passed the CQ dry run.

[eae, 2016-07-05 23:25:32]

ok, LGTM

[Mr. Kevin, 2016-07-07 00:33:58]

On 2016/07/05 23:25:32, eae wrote:
> ok, LGTM

Is anything else needed for this to go forward?

[Timothy Loh, 2016-07-07 01:40:03]

On 2016/07/07 00:33:58, Mr. Kevin wrote:
> On 2016/07/05 23:25:32, eae wrote:
> > ok, LGTM
> 
> Is anything else needed for this to go forward?

I don't think so

[Mr. Kevin, 2016-07-08 00:28:03]

The CQ bit was checked by khart@codeaurora.org

[commit-bot: I haz the power, 2016-07-08 00:29:26]

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 09:44:29]

Description was changed from

==========
Fix getComputedStyle for non-box-layout elements

This fixes a crash introduced by https://codereview.chromium.org/1826423003/
Code that treated a layout object as a Box layout were not properly
guarded by isBox.  This caused a crash when calling getComputedStyle on
positioned non-box elements, e.g. <ruby>.

R=mstensho@opera.com
BUG=610986
==========

to

==========
Fix getComputedStyle for non-box-layout elements

This fixes a crash introduced by https://codereview.chromium.org/1826423003/
Code that treated a layout object as a Box layout were not properly
guarded by isBox.  This caused a crash when calling getComputedStyle on
positioned non-box elements, e.g. <ruby>.

R=mstensho@opera.com
BUG=610986
==========

[commit-bot: I haz the power, 2016-07-08 09:44:30]

Committed patchset #3 (id:40001)

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

Description was changed from

==========
Fix getComputedStyle for non-box-layout elements

This fixes a crash introduced by https://codereview.chromium.org/1826423003/
Code that treated a layout object as a Box layout were not properly
guarded by isBox.  This caused a crash when calling getComputedStyle on
positioned non-box elements, e.g. <ruby>.

R=mstensho@opera.com
BUG=610986
==========

to

==========
Fix getComputedStyle for non-box-layout elements

This fixes a crash introduced by https://codereview.chromium.org/1826423003/
Code that treated a layout object as a Box layout were not properly
guarded by isBox.  This caused a crash when calling getComputedStyle on
positioned non-box elements, e.g. <ruby>.

R=mstensho@opera.com
BUG=610986

Committed: https://crrev.com/86d16a615d6e9abf6d991f430b496c26a4271d10
Cr-Commit-Position: refs/heads/master@{#404336}
==========

[commit-bot: I haz the power, 2016-07-08 09:49:17]

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