[css-grid] Fix percentage height resolution for replaced elements

[css-grid] Fix percentage height resolution for replaced elements

The percentage heights for replaced elements were not working
as expected in grid items.
The patch fixes the different issues to make it work as expected.

If a replaced item is a grid item, it needs to resolve
the percentage height against its grid area.
When the replaced item is a child of a grid item,
and the grid item is stretched,
it needs to use that size to resolve the percentage height too.

BUG=624716
TEST=fast/css-grid-layout/grid-item-with-percent-height-replaced-element.html

Committed: https://crrev.com/7886bec673ccae048c0ef5726ceb21c8fcbd86e0
Cr-Commit-Position: refs/heads/master@{#404896}

[Manuel Rego, 2016-07-05 11:03:44]

rego@igalia.com changed reviewers:
+ cbiesinger@chromium.org, jfernandez@igalia.com, svillar@igalia.com

[svillar, 2016-07-07 11:45:48]

Won't give my OK as I don't really know what this funcion is used for, I'll let
others decide.

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

https://codereview.chromium.org/2121173002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:558: bool
LayoutBoxModelObject::hasAutoHeightOrContainingBlockWithAutoHeight(bool
checkingContainingBlock) const
It's usually considered better to use an enum instead of a boolean, you might
need to add some other values later.

[Manuel Rego, 2016-07-07 14:40:57]

rego@igalia.com changed reviewers:
+ eae@chromium.org

[Manuel Rego, 2016-07-07 14:40:57]

On 2016/07/07 11:45:48, svillar wrote:
> Won't give my OK as I don't really know what this funcion is used for, I'll
let
> others decide.

Adding @eae as reviewer then, as he reviewed similar fixes on Flexbox.

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

https://codereview.chromium.org/2121173002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:558: bool
LayoutBoxModelObject::hasAutoHeightOrContainingBlockWithAutoHeight(bool
checkingContainingBlock) const
On 2016/07/07 11:45:48, svillar wrote:
> It's usually considered better to use an enum instead of a boolean, you might
> need to add some other values later.

Ok, I can change this I'm thinking in something like:
    enum ObjectToCheck { CurrentObject, ContainingBlock };
    bool hasAutoHeightOrContainingBlockWithAutoHeight(ObjectToCheck =
CurrentObject) const;          

But I don't like it. :-/ Do you have a better suggestion? Thanks.

[cbiesinger, 2016-07-07 17:34:27]

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

https://codereview.chromium.org/2121173002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:558: bool
LayoutBoxModelObject::hasAutoHeightOrContainingBlockWithAutoHeight(bool
checkingContainingBlock) const
On 2016/07/07 14:40:57, Manuel Rego wrote:
> On 2016/07/07 11:45:48, svillar wrote:
> > It's usually considered better to use an enum instead of a boolean, you
might
> > need to add some other values later.
> 
> Ok, I can change this I'm thinking in something like:
>     enum ObjectToCheck { CurrentObject, ContainingBlock };
>     bool hasAutoHeightOrContainingBlockWithAutoHeight(ObjectToCheck =
> CurrentObject) const;          
> 
> But I don't like it. :-/ Do you have a better suggestion? Thanks.

To make sure I understand, callers should never pass true here, right? This is
only used for the internal implementation?

I'd be OK with keeping it as a bool in that circumstance, but can you add a
comment in the header saying that this is the case?

[Manuel Rego, 2016-07-08 07:57:04]

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

https://codereview.chromium.org/2121173002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:558: bool
LayoutBoxModelObject::hasAutoHeightOrContainingBlockWithAutoHeight(bool
checkingContainingBlock) const
On 2016/07/07 17:34:27, cbiesinger wrote:
> On 2016/07/07 14:40:57, Manuel Rego wrote:
> > On 2016/07/07 11:45:48, svillar wrote:
> > > It's usually considered better to use an enum instead of a boolean, you
> might
> > > need to add some other values later.
> > 
> > Ok, I can change this I'm thinking in something like:
> >     enum ObjectToCheck { CurrentObject, ContainingBlock };
> >     bool hasAutoHeightOrContainingBlockWithAutoHeight(ObjectToCheck =
> > CurrentObject) const;          
> > 
> > But I don't like it. :-/ Do you have a better suggestion? Thanks.
> 
> To make sure I understand, callers should never pass true here, right? This is
> only used for the internal implementation?
> 
> I'd be OK with keeping it as a bool in that circumstance, but can you add a
> comment in the header saying that this is the case?

Yeah that's the idea.

Probably it'd be better that the public method has not arguments (like before).
And then we've a private method with the bool. E.g.
  public: 
      bool hasAutoHeightOrContainingBlockWithAutoHeight() const {
          return hasAutoHeightOrContainingBlockWithAutoHeight(false);
      }
  private:
      bool hasAutoHeightOrContainingBlockWithAutoHeight(bool
checkingContainingBlock) const {
          ...
      }

WDYT?

[cbiesinger, 2016-07-08 20:14:05]

On 2016/07/08 07:57:04, Manuel Rego wrote:
>
https://codereview.chromium.org/2121173002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):
> 
>
https://codereview.chromium.org/2121173002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:558: bool
> LayoutBoxModelObject::hasAutoHeightOrContainingBlockWithAutoHeight(bool
> checkingContainingBlock) const
> On 2016/07/07 17:34:27, cbiesinger wrote:
> > On 2016/07/07 14:40:57, Manuel Rego wrote:
> > > On 2016/07/07 11:45:48, svillar wrote:
> > > > It's usually considered better to use an enum instead of a boolean, you
> > might
> > > > need to add some other values later.
> > > 
> > > Ok, I can change this I'm thinking in something like:
> > >     enum ObjectToCheck { CurrentObject, ContainingBlock };
> > >     bool hasAutoHeightOrContainingBlockWithAutoHeight(ObjectToCheck =
> > > CurrentObject) const;          
> > > 
> > > But I don't like it. :-/ Do you have a better suggestion? Thanks.
> > 
> > To make sure I understand, callers should never pass true here, right? This
is
> > only used for the internal implementation?
> > 
> > I'd be OK with keeping it as a bool in that circumstance, but can you add a
> > comment in the header saying that this is the case?
> 
> Yeah that's the idea.
> 
> Probably it'd be better that the public method has not arguments (like
before).
> And then we've a private method with the bool. E.g.
>   public: 
>       bool hasAutoHeightOrContainingBlockWithAutoHeight() const {
>           return hasAutoHeightOrContainingBlockWithAutoHeight(false);
>       }
>   private:
>       bool hasAutoHeightOrContainingBlockWithAutoHeight(bool
> checkingContainingBlock) const {
>           ...
>       }
> 
> WDYT?

Sounds good

[Manuel Rego, 2016-07-11 09:15:06]

Ok, so I've just uploaded a new version making
hasAutoHeightOrContainingBlockWithAutoHeight(bool) private.

PTAL, thanks!

[cbiesinger, 2016-07-11 15:55:49]

lgtm

[Manuel Rego, 2016-07-12 05:13:50]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2016-07-12 05:14: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-12 06:41:22]

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

[commit-bot: I haz the power, 2016-07-12 06:41:23]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Manuel Rego, 2016-07-12 07:29:04]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2016-07-12 07:29:56]

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-12 08:40:12]

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

[commit-bot: I haz the power, 2016-07-12 08:40:14]

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

[Manuel Rego, 2016-07-12 08:48:00]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2016-07-12 08:48:18]

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-12 09:04:47]

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

[commit-bot: I haz the power, 2016-07-12 09:04:48]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Manuel Rego, 2016-07-12 09:17:30]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2016-07-12 09:17:46]

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-12 10:14:25]

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

[commit-bot: I haz the power, 2016-07-12 10:14:26]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Manuel Rego, 2016-07-12 11:19:56]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2016-07-12 11:20:19]

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-12 13:43:43]

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

[commit-bot: I haz the power, 2016-07-12 13:43:45]

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

[Manuel Rego, 2016-07-12 13:48:31]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2016-07-12 13:48:57]

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-12 15:30:03]

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

[commit-bot: I haz the power, 2016-07-12 15:30:04]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[eae, 2016-07-12 16:08:43]

LGTM

[eae, 2016-07-12 16:09:17]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2016-07-12 16:10:24]

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-12 20:19:55]

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

[commit-bot: I haz the power, 2016-07-12 20:19:57]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Manuel Rego, 2016-07-12 20:38:58]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2016-07-12 20:41:14]

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-13 02:14:36]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-13 02:17:12]

Description was changed from

==========
[css-grid] Fix percentage height resolution for replaced elements

The percentage heights for replaced elements were not working
as expected in grid items.
The patch fixes the different issues to make it work as expected.

If a replaced item is a grid item, it needs to resolve
the percentage height against its grid area.
When the replaced item is a child of a grid item,
and the grid item is stretched,
it needs to use that size to resolve the percentage height too.

BUG=624716
TEST=fast/css-grid-layout/grid-item-with-percent-height-replaced-element.html
==========

to

==========
[css-grid] Fix percentage height resolution for replaced elements

The percentage heights for replaced elements were not working
as expected in grid items.
The patch fixes the different issues to make it work as expected.

If a replaced item is a grid item, it needs to resolve
the percentage height against its grid area.
When the replaced item is a child of a grid item,
and the grid item is stretched,
it needs to use that size to resolve the percentage height too.

BUG=624716
TEST=fast/css-grid-layout/grid-item-with-percent-height-replaced-element.html

Committed: https://crrev.com/7886bec673ccae048c0ef5726ceb21c8fcbd86e0
Cr-Commit-Position: refs/heads/master@{#404896}
==========

[commit-bot: I haz the power, 2016-07-13 02:17:14]

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