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

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P2)

Description

[Mats Palmgren (inactive)]

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

Comment 1

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=655c4d4cb984f856ee7cc14b1962e220ffe5f69e

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid] Make FindUsedFlexFraction iterate the GridItemInfo array instead of using a CSSOrderAwareFrameIterator (idempotent change)

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Make ResolveIntrinsicSize iterate the GridItemInfo array instead of using a CSSOrderAwareFrameIterator (idempotent change)

Comment 4

[Mats Palmgren (inactive)]

Attachment: 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)

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-grid] Make InitializeItemBaselines iterate the GridItemInfo array instead of using a CSSOrderAwareFrameIterator (idempotent change)

Comment 6

[Mats Palmgren (inactive)]

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

Comment 7

[Daniel Holbert [:dholbert]]

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.

Comment 8

[Mats Palmgren (inactive)]

I updated the comments.  Thanks!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/fa53f0a075dc
https://hg.mozilla.org/mozilla-central/rev/2432cfe699b2
https://hg.mozilla.org/mozilla-central/rev/4123057568d9
https://hg.mozilla.org/mozilla-central/rev/6d8e461ed3a2