Closed Bug 1447166 Opened 3 years ago Closed 3 years ago

[css-grid] Iterate GridItemInfo array instead of using child frame iterator

Categories

(Core :: Layout, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: mats, Assigned: mats)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

I've been looking into subgrid a bit and I think collecting the subgrid's
children (recursively) into the GridItemInfo array of the "root" grid
might help.  At least for the track sizing phase.  Then we'll process
all those descendant frames as if they are grid items which I think
is essentially what the spec calls for.

So in preparation for that I'm going to change some loops that
are currently using a CSSOrderAwareFrameIterator to iterate
over the GridItemInfo array instead.  That array should already
be in the correct order since we fill it up in iterator order here:
https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/layout/generic/nsGridContainerFrame.cpp#3231-3232,3235
(I think some of the current uses are for algorithms that doesn't
actually care about the order anyway, like track sizing.)
Blocks: 1240834
Attachment #8960390 - Attachment description: unstable-sort → part 3 - [css-grid] No need to use a stable sort here since track sizing doesn't depend on which order we process items (other than span length), (idempotent change)
Attachment #8960390 - Attachment is patch: true
Attachment #8960390 - Flags: review?(dholbert)
Actually, part 3 is probably not strictly idempotent since the unstable sort
might change the order, but none of these methods actually depends on the order
of the items, they should give the same result in any order (except for the
increasing span-length order in track sizing, but I'm not changing that).
I'll remove the "idempotent" bit on part 3...
Attachment #8960388 - Flags: review?(dholbert) → review+
Attachment #8960389 - Flags: review?(dholbert) → review+
Comment on attachment 8960390 [details] [diff] [review]
part 3 - [css-grid] No need to use a stable sort here since track sizing doesn't depend on which order we process items (other than span length), (idempotent change)

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

r=me with nits.

First nit: this patch's commit message is as follows:
***
Bug 1447166 part 3 - [css-grid] No need to use a stable sort here since track sizing doesn't depend on which order we process items (other than span length), (idempotent change).  r=dholbert
***


This sounds more like supporting/background text, rather than summary "describe the change" text.  Perhaps it should say "Switch from stable to unstable sort in grid track ResolveIntrinsicSize function", and what you've got right now should go into a second(/third/etc). line?

::: layout/generic/nsGridContainerFrame.cpp
@@ +4423,5 @@
>      };
>  
> +    // Sort the collected items on span length, shortest first.  There's no need
> +    // for a stable sort here since this method isn't order dependent.
> +    std::sort(step2Items.begin(), step2Items.end(),

The second sentence here is a bit confusing -- if "this method isn't order dependent" as the comment says, then why are we bothering to sort at all? :)

I think you want to add "for a given span length" or something like that onto the end of this comment.
Attachment #8960390 - Flags: review?(dholbert) → review+
Attachment #8960391 - Flags: review?(dholbert) → review+
I updated the comments.  Thanks!
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa53f0a075dc
part 1 - [css-grid] Make FindUsedFlexFraction iterate the GridItemInfo array instead of using a CSSOrderAwareFrameIterator (idempotent change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/2432cfe699b2
part 2 - [css-grid] Make ResolveIntrinsicSize iterate the GridItemInfo array instead of using a CSSOrderAwareFrameIterator (idempotent change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/4123057568d9
part 3 - [css-grid] Switch from stable to unstable sort in grid track ResolveIntrinsicSize function.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d8e461ed3a2
part 4 - [css-grid] Make InitializeItemBaselines iterate the GridItemInfo array instead of using a CSSOrderAwareFrameIterator (idempotent change).  r=dholbert
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.