[css-grid-2] Add a few frame bits to keep track of subgrid state

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks 1 bug)

Trunk
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

No description provided.
Blocks: 1465296
(FYI, the bits in part 3 will be set in bug 1465296, which comes next...)
Comment on attachment 8981718 [details] [diff] [review]
part 1 - [css-grid-2] Add frame bits to nsGridContainerFrame that is set if it's a subgrid in an axis

Review of attachment 8981718 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: layout/generic/nsGridContainerFrame.cpp
@@ +6346,5 @@
> +    }
> +    if (parent && parent->IsGridContainerFrame()) {
> +      const auto* pos = StylePosition();
> +      if (pos->GridTemplateColumns().mIsSubgrid) {
> +        bits = NS_STATE_GRID_IS_COL_SUBGRID;

Nit: consider using "bits |=" (not "=") here, to be more consistent with the |= in the other case here, and to be more future-proof against possible further tweaks or reorderings of these statements.

(note that you've initialized bits to 0, so |= is equivalent to = here, since this is the first assignment after that initialization.)

::: layout/generic/nsGridContainerFrame.h
@@ +218,5 @@
>    ExplicitNamedAreas* GetExplicitNamedAreas() const {
>      return GetProperty(ExplicitNamedAreasProperty());
>    }
>  
> +  /** Return true if this frame subgridded in its aAxis. */

Maybe s/subgridded/is subgridded/?  (or "is a subgrid" to be clearer still?  I'm not sure that "subgridded" is defined anywhere...)

@@ +226,5 @@
> +                                          : NS_STATE_GRID_IS_COL_SUBGRID);
> +  }
> +  bool IsColSubgrid() const { return IsSubgrid(mozilla::eLogicalAxisInline); }
> +  bool IsRowSubgrid() const { return IsSubgrid(mozilla::eLogicalAxisBlock); }
> +  /** Return true if this frame subgridded in any axis. */

(as above, the phrase "if this frame subgridded" feels odd to me, but not a big deal)
Attachment #8981718 - Flags: review?(dholbert) → review+
Comment on attachment 8981720 [details] [diff] [review]
part 2 - [css-grid-2] Add bits to the GridItemInfo state that is set if the item is a subgrid in the container's axis

Review of attachment 8981720 [details] [diff] [review]:
-----------------------------------------------------------------

Commit message nit:
> Add bits ... that is set

s/that is/that are/

::: layout/generic/nsGridContainerFrame.cpp
@@ +562,5 @@
> +      auto parentWM = aFrame->GetParent()->GetWritingMode();
> +      bool isOrthogonal = parentWM.IsOrthogonalTo(f->GetWritingMode());
> +      if (f->IsColSubgrid()) {
> +        mState[isOrthogonal ? eLogicalAxisBlock : eLogicalAxisInline] =
> +          StateBits::eIsSubgrid;

It'd probably be best to use "|=" rather than "=" here, to just selectively set this bit without touching the others.

(It happens to be the case that mState is 0 at this point, so stomping on it doesn't matter; but that's kind of fragile to depend on, because it's possible we might set other bits in between in the future.)

This applies twice (for the IsColSubgrid and the IsRowSubgrid case)
Attachment #8981720 - Flags: review?(dholbert) → review+
Comment on attachment 8981722 [details] [diff] [review]
part 3 - [css-grid-2] Add frame bits to nsGridContainerFrame that is set if it has an item that is a subgrid in that axis

Review of attachment 8981722 [details] [diff] [review]:
-----------------------------------------------------------------

Similar to part 2, commit message nit:
> Add frame bits...that is set
s/that is/that are/

::: layout/generic/nsFrameStateBits.h
@@ +354,5 @@
>  
> +// True if the container contains items subgridded in its inline axis
> +FRAME_STATE_BIT(GridContainer, 26, NS_STATE_GRID_HAS_COL_SUBGRID_ITEMS)
> +
> +// True if the container contains items subgridded in its block axis

3 nits:
 (1) add period at the end of each comment here.

 (2) In the bit's name as well as in the comment, I think you might mean "ITEM" / "item" -- there's no guarantee we have plural "items", right?  (Maybe there are plural, but in general, not necessarily.)  Conveniently, that makes the bit name 1 character shorter, too. :))

 (3) It's not clear why we're using different terminology ("is a subgrid" vs. "subgridded") between the first two bits [part 1] vs. these latter 2 bits [part 3].  Maybe make that terminology consistent? I think "is a subgrid" is well defined, vs. "subgridded" isn't really, so I'd lean towards the former phrasing...

::: layout/generic/nsGridContainerFrame.cpp
@@ +6400,5 @@
>    } else {
>      bits = aPrevInFlow->GetStateBits() & (NS_STATE_GRID_IS_COL_SUBGRID |
> +                                          NS_STATE_GRID_IS_ROW_SUBGRID |
> +                                          NS_STATE_GRID_HAS_COL_SUBGRID_ITEMS |
> +                                          NS_STATE_GRID_HAS_ROW_SUBGRID_ITEMS);

Nit: this cloning doesn't make 100% sense -- because technically, it'd be possible for our prev-in-flow to have a subgrid without us having a subgrid, right?  (if the subgrid is entirely inside one of the parent grid's fragments, then subsequent fragments don't technically have a subgrid)

Maybe that doesn't matter for our purposes, though; if these state-bits are just to prevent us doing expensive stuff in the common no-subgrids-at-all case, then this seems fine.

::: layout/generic/nsGridContainerFrame.h
@@ +232,5 @@
>      return HasAnyStateBits(NS_STATE_GRID_IS_ROW_SUBGRID |
>                             NS_STATE_GRID_IS_COL_SUBGRID);
>    }
>  
> +  /** Return true if this frame have subgrid items subgridded in our aAxis. */

2 nits:
 (1)  s/this frame have/this frame has/

 (2) "subgrid items subgridded" is confusing (and also incorrectly plural when we don't necessarily know it's plural).  Maybe "Return true if this frame has at least one item that is a subgrid in our aAxis"?

@@ +238,5 @@
> +    return HasAnyStateBits(
> +      aAxis == mozilla::eLogicalAxisBlock ? NS_STATE_GRID_HAS_ROW_SUBGRID_ITEMS
> +                                          : NS_STATE_GRID_HAS_COL_SUBGRID_ITEMS);
> +  }
> +  /** Return true if this frame have any subgrid items. */

s/have/has/
Attachment #8981722 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #6)
> > +        mState[isOrthogonal ? eLogicalAxisBlock : eLogicalAxisInline] =
> > +          StateBits::eIsSubgrid;
> 
> It'd probably be best to use "|=" rather than "=" here, to just selectively
> set this bit without touching the others.

That doesn't compile though, so I left it as is.
"error: assigning to 'nsGridContainerFrame::GridItemInfo::StateBits' from incompatible type 'int'"
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fbed517647c
part 1 - [css-grid-2] Add frame bits to nsGridContainerFrame that is set if it's a subgrid in an axis.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/427ecb724099
part 2 - [css-grid-2] Add bits to the GridItemInfo state that are set if the item is a subgrid in the container's axis.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4e28bd3162
part 3 - [css-grid-2] Add frame bits to nsGridContainerFrame that are set if it has an item that is a subgrid in that axis.  r=dholbert
Depends on: 1467239
Sorry, wrong bug... sigh
Attachment #8985258 - Attachment is obsolete: true
Attachment #8985258 - Flags: review?(dholbert)
Attachment #8985259 - Attachment is obsolete: true
Attachment #8985259 - Flags: review?(dholbert)
Attachment #8985261 - Attachment is obsolete: true
Attachment #8985261 - Flags: review?(dholbert)
You need to log in before you can comment on or make changes to this bug.