Mats Palmgren (inactive) Assignee
	Description • 9 years ago

http://dev.w3.org/csswg/css-grid/#grid-baselines

	Mats Palmgren (inactive) Assignee
	Comment 1 • 8 years ago

The spec requires that we find "the first/last such grid item (in grid order)"
https://drafts.csswg.org/css-grid/#grid-baselines
so we need to be able iterate the items in reverse.
This part adds support for reverse iteration in "order-modified document order".

	Mats Palmgren (inactive) Assignee
	Comment 2 • 8 years ago

Regarding "in this fragment" in the commit message - the spec doesn't
really say anything about fragmentation.  I think it would be weird
to let items on a different page affect baselines.  (I'll file a spec
issue about this.)

	Mats Palmgren (inactive) Assignee
	Comment 4 • 8 years ago

The explicit handling of items that are grids is a bit experimental ATM,
pending the outcome of our London discussion on baselines.  My idea
here is that Get[I|B]Baseline, or something like it, could be a general
method on nsIFrame and replace the current baseline methods eventually.

For now, I'm also making only the fragment that contains the first row
contribute it's first-baseline group the container's first-baseline
(bullet 1 in the spec); (ditto for the fragment with the last row
and last-baseline).  I think it would be possible to do it for the first/
last row *in each fragment* but I feel like this patch is probably closer
to what the spec currently says.  (We can change this once the spec is
clearer about fragmentation.)

	Mats Palmgren (inactive) Assignee
	Comment 5 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=452e7a8335ce

	Mats Palmgren (inactive) Assignee
	Comment 6 • 8 years ago

I'll update part 4 to use the first baseline instead, since the CSSWG have
made this decision:

https://lists.w3.org/Archives/Public/www-style/2016Jun/0158.html

  - RESOLVED: vertical-align: baseline means first baseline except
              for inline-blocks (due to CSS2.1 legacy)

	Mats Palmgren (inactive) Assignee
	Comment 7 • 8 years ago

Minor tweak to use the first-baseline instead in GetLogicalBaseline, per comment 6.

Interdiff:
 +    nscoord b;
-+    GetBBaseline(BaselineSharingGroup::eLast, &b);
-+    return BSize(aWM) - b;
++    GetBBaseline(BaselineSharingGroup::eFirst, &b);
++    return b;

	Mats Palmgren (inactive) Assignee
	Comment 8 • 8 years ago

While fixing up the reftest to account for the above change, I discovered
a minor bug in the first-baseline calculation at the end of 
nsGridContainerFrame::SynthesizeBaseline.  Fixed like so:

-  return aGroup == BaselineSharingGroup::eFirst ?
-    start + baseline + aCBBorderPaddingStart :
+  return aGroup == BaselineSharingGroup::eFirst ? start + baseline :

"start" is the result of "child->GetLogicalNormalPosition" in the relevant
axis here so it already has "aCBBorderPaddingStart" baked in since it's
frame offset.

	Mats Palmgren (inactive) Assignee
	Comment 9 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=92fd90f504b2

	Daniel Holbert [:dholbert]
	Comment 10 • 8 years ago

Comment on attachment 8761231 [details] [diff] [review]
part 1 - [css-grid] Make GridItemCSSOrderIterator use nsFrameList iterators...

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

Sorry for the delay here -- been meaning to get to this, but I've been letting other reviews & intern/new-hire onboarding+mentoring take priority while you've been on vacation, and I've let this slip more than I should've.

I'm about to go on a few days of vacation myself, but I want to make a (small) dent in this bug before I do -- and then I'm hoping to finish this off when I'm back next Tues/Weds (after which I've got a few more days out).

So, here are my review comments so far:

::: layout/generic/nsGridContainerFrame.cpp
@@ +297,5 @@
> +  static bool CSSOrderComparator(nsIFrame* const& a, nsIFrame* const& b);
> +  bool IsForward() const { return mDirection == IteratorDirection::eForward; }
> +  Iterator begin(const nsFrameList& aList);
> +  Iterator end(const nsFrameList& aList);
> +  IteratorDirection mDirection;

Maybe add a blank line before mDirection here, so that it's easier to visually separate the member-data from the methods?

Also, it looks like mDirection is set once (in the constructor init list) and then never modified and only accessed via the IsForward() API.

So, perhaps it should be declared as "const" and private, to make that modification/reading pattern more clear/explicit up-front?

@@ +302,5 @@
> +};
> +
> +template<typename Iterator>
> +class nsGridContainerFrame::GridItemCSSOrderIteratorT :
> +  public GridItemCSSOrderBase<Iterator>

The layers of abstraction are a bit hard to keep track of here, and (for example) it's not immediately clear to me why we have the distinction between GridItemCSSOrderBase vs. GridItemCSSOrderIteratorT.

Could you add a brief comment above each of these classes to explain their purpose & to give a hint at the relationship / separation-of-concerns?

@@ +343,4 @@
>      } else {
>        count *= 2; // XXX somewhat arbitrary estimate for now...
>        mArray.emplace(count);
> +      for (Iterator i(Base::begin(mChildren)), end(Base::end(mChildren));

"end" might be a slightly problematic name here, since it shadows the (inherited by default) Base::end method, right?  (Probably only matters if we were invoking "end" by name inside of this scope, but seems worth avoiding the naming collision for its own sake.)

Maybe s/end/iEnd/ (where i = iterator, iEnd = iteratorEnd) for this local variable?

@@ +349,3 @@
>        }
> +      // Clang 3.6.2 crashes if this assertion is uncommented:
> +      // MOZ_ASSERT(mArray->Length() > 1,

Does this compiler crash still happen? If so, could you file a bug about it and mention the bug number here (and mention it in the one other place where we this patch adds this comment+disabled-assertion)

(At some point we'll presumably be on newer clang versions that don't crash, and we'll want to uncomment.)

@@ +359,5 @@
>        SkipPlaceholders();
>      }
>    }
>  
> +  bool IsForward() const { return Base::IsForward(); }

Do we actually need this?  (Seems like we should just inherit Base::IsForward for free and get this exact behavior without needing this line of code...)

@@ +385,5 @@
>  
> +  void SetGridItemCount(size_t aGridItemCount)
> +  {
> +    mGridItemCount = aGridItemCount;
> +    mGridItemIndex = IsForward() ? 0 : mGridItemCount - 1;

Should this method assert that mGridItemIndex is already at the extreme beginning/end (based on IsForward()) when it's called?  It looks like it's expecting that (and adjusting the index to the "new updated extreme")

Also, if we have 0 grid items (and call SetGridItemCount(0)), it looks like "mGridItemCount - 1" will underflow to +infinity here (i.e. we'll end up with mGridItemIndex=+infinity).  Is that bad?  Perhaps we should assert that aGridItemCount is nonzero, if that's really something we expect to never happen. (Or if it can happen, this code needs to handle it somehow).

@@ +402,5 @@
>          }
>        }
>      } else {
> +      if (IsForward()) {
> +        for (; mArrayIndex < int64_t(mArray->Length()); ++mArrayIndex) {

Do we really need to walk the array in a different direction based on IsForward()?  Couldn't we just populate mArray in the correct directionality up-front (with a forward/reverse traversal of our children), and do away with many of the array-indexing IsForward() branches?

I could see needing this switching if IsForward() changed dynamically, but it doesn't immediately look to me like it does...

(Maybe I'm missing how these classes fit together & when we actually lock in the directionality of this smart iterator class...)

@@ +487,3 @@
>    // Used if child list is *not* in ascending 'order'.
>    Maybe<nsTArray<nsIFrame*>> mArray;
> +  int64_t mArrayIndex;

Why are we changing to int64_t here? In particular:
 * Why signed? (are we expecting negative indices?)
 * Why not just use the standard index-type for nsTArray (which is size_t these days)?
 * Why is this different from the index/count declared below it?

If there is indeed a good reason we need our array-index to be int64_t-typed here, it's probably worth calling out with a comment; otherwise this looks a bit out of place.

	Daniel Holbert [:dholbert]
	Comment 11 • 8 years ago

(In reply to Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) from comment #10)
> Does this compiler crash still happen? If so, could you file a bug about it
> and mention the bug number here

(To clarify: I mean a bugzilla.mozilla.org bug, so we can keep track of it, & have a place to land the uncommenting-patch, & perhaps notice if we hit a similar issue elsewhere. Though a clang bug might be merited too.)

	Mats Palmgren (inactive) Assignee
	Comment 12 • 8 years ago

It appears that there have been major spec changes to baseline alignment
while I was on vacation...

One change is that the border-box should be used for synthesizing
grid container baselines, IIUC, per:
https://lists.w3.org/Archives/Public/www-style/2016Sep/0000.html
"  - The proposals of what to standardize around were:
      A. Make boxes without a natural baseline and boxes
          establishing an orthogonal flow synthesize a baseline.
          A.1 Base this synthesized baseline on the margin box edges
          A.2 Base this synthesized baseline on some other box edge
...
  - RESOLVED: Accept option A2 with the modification that we're
              talking about the border box.
"

The Grid spec now says:
https://drafts.csswg.org/css-grid/#grid-baselines
"3.  Otherwise, the grid container has no first (last) baseline set,
     and one is synthesized if needed according to the rules of its
     alignment context."
where "alignment context" links to css-align:
https://drafts.csswg.org/css-align-3/#shared-alignment-context
but I can't find any useful text there about which box to use.
I'll assume it's the border-box though, per the CSSWG minutes.

Furthermore, it appears that block-axis baseline alignment was removed
everywhere, which I strongly disagree with, fwiw.  I'm trying to
figure out why this change was made.

	Mats Palmgren (inactive) Assignee
	Comment 13 • 8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #10)
> The layers of abstraction are a bit hard to keep track of here, and (for
> example) it's not immediately clear to me why we have the distinction
> between GridItemCSSOrderBase vs. GridItemCSSOrderIteratorT.

Yeah, I over-generalized that a bit.  FYI, the base class there is
a trick to specialize part of the ctor for the sub-class without
having to repeat the common part in both.

I realized I actually only needed this to initialize mDirection
[differently based on the template param type] but it's only used
by IsForward() so it's simpler to just specialize IsForward()
(to return true/false) instead.  So I removed the base class
and simplified everything else.

I also kept the assertions that crashed clang 3.6.2 and #ifdef'ed
them out for that version.  I think that version is still
supported so we'll need to keep this for a while.

	Mats Palmgren (inactive) Assignee
	Comment 14 • 8 years ago

I changed to synthesizing a baseline from the content box to the border-box,
per the spec change.

There are probably some more tweaks needed to remove/add block-axis
baseline alignment, but I think we can land this for now until the spec
settles on that.  I'll do that in a follow-up bug.

	Mats Palmgren (inactive) Assignee
	Comment 15 • 8 years ago

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

	Daniel Holbert [:dholbert]
	Comment 16 • 8 years ago

Comment on attachment 8788868 [details] [diff] [review]
part 1 - [css-grid] Make GridItemCSSOrderIterator use nsFrameList iterators...

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +394,5 @@
>    }
>  
> +  void SetGridItemCount(size_t aGridItemCount)
> +  {
> +    MOZ_ASSERT(mIter.isSome() || mArray->Length() == aGridItemCount,

Why do we need to expose this SetGridItemCount() setter at all?  It seems like a bit of a footgun to expose this outside of this class (and rely on it being called as a secondary initialization step, outside of the constructor).  Can't we just set up mGridItemCount/mGridItemIndex at the end of the constructor? (since that's where we actually populate mArray and/or walk across everything in mIter already)  Do the callers really have better information than this object does, about how many entries it contains?

If we really do need this method, it should probably have a code-comment to explain why it exists & under what circumstances it's expected to be called...

@@ +397,5 @@
> +  {
> +    MOZ_ASSERT(mIter.isSome() || mArray->Length() == aGridItemCount,
> +               "grid item count mismatch");
> +    mGridItemCount = aGridItemCount;
> +    mGridItemIndex = IsForward() ? 0 : mGridItemCount - 1;

Here (and in 2 other places), we subtract from mGridItemIndex, which could underflow to +infinity (though that only happens if mGridItemCount is 0, in two of the three places) Could that underflow-to-infinity cause trouble?  If not & if this is normal expected behavior which we handle cleanly, perhaps add some documentation alongside the mGridItemIndex explaining that this underflow/infinity-value is a possibility (and specifically, it'll happen when it goes beyond the last grid item, if IsForward() is false).

@@ +442,5 @@
>                 "the list of child frames must not change while iterating!");
>  #endif
>      if (mSkipPlaceholders ||
>          (**this)->GetType() != nsGkAtoms::placeholderFrame) {
> +      IsForward() ? ++mGridItemIndex : --mGridItemIndex;

(here's the second spot where we could underflow mGridItemIndex)

@@ +465,4 @@
>      } else {
>        mArrayIndex = 0;
>      }
> +    mGridItemIndex = IsForward() ? 0 : mGridItemCount - 1;

(here's the third spot where we could underflow mGridItemIndex)

@@ +510,5 @@
> +
> +template<>
> +bool GridItemCSSOrderIterator::
> +CSSOrderComparator(nsIFrame* const& a, nsIFrame* const& b)
> +{ return a->StylePosition()->mOrder < b->StylePosition()->mOrder; }

Wrapping is off here. This should probably be...

bool
GridItemCSSOrderIterator::CSSOrderComparator(nsIFrame* const& a,
                                             nsIFrame* const& b)

...so that the method name (GridItemCSSOrderIterator::CSSOrderComparator) is all on one line.

Same goes for all of the new functions directly below this -- they should consistently wrap between name of the return-type and the function's scoped name.

(Technically the "{" and "}" should get their own lines at this point in the file, too; that would make this a bit more verbose, but IIRC we're trying to err on the side of just following the coding style for bracing/wrapping, so that eventually we could use an automated tool to enforce the coding-style rules and not have to coach it for a bunch of special cases in old code. I'll leave the braces up to your discretion, though.)

	Daniel Holbert [:dholbert]
	Comment 17 • 8 years ago

Comment on attachment 8788868 [details] [diff] [review]
part 1 - [css-grid] Make GridItemCSSOrderIterator use nsFrameList iterators...

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

One more nit on part 1 (following up on the underflow thing):

::: layout/generic/nsGridContainerFrame.cpp
@@ +388,5 @@
>    size_t GridItemIndex() const
>    {
>      MOZ_ASSERT(!AtEnd());
>      MOZ_ASSERT((**this)->GetType() != nsGkAtoms::placeholderFrame,
>                 "MUST not call this when at a placeholder");

Now that we maintain "mGridItemCount", we can & should assert that this getter is returning an in-range value. Let's add something like this here (in GridItemIndex()):

  MOZ_ASSERT(mGridItemIndex < mGridItemCount,
             "Returning an out-of-range mGridItemIndex...");

This should be safe to assert, since we already assert !AtEnd() before this, which should mean that we never call this function when the index is out of bounds. (That also makes this new assertion semi-redundant, except that (unlike AtEnd()) it'll actually be directly sanity-checking the index that we're about to return.)

This assertion would relax me about the underflow that I noted in my previous comment here.

	Daniel Holbert [:dholbert]
	Comment 18 • 8 years ago

Comment on attachment 8761234 [details] [diff] [review]
part 2 - [css-grid] Add methods for finding the first/last grid item in Grid Order in this fragment

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +5919,5 @@
> +  for (; !aIter.AtEnd(); aIter.Next()) {
> +    const GridItemInfo& item = aGridItems[aIter.GridItemIndex()];
> +    uint32_t major = (item.mArea.*aMajor).mStart;
> +    if ((item.mArea.*aMajor).mEnd <= aFragmentStartTrack) {
> +      continue; // item doesn't span any track in this fragment

This "major" variable isn't used until *after* this "if" clause, so it should be declared further down (probably right alongside the "minor" variable-decl.)

@@ +5948,5 @@
> +  aIter.Reset();
> +  int32_t lastMajorTrack = int32_t(aFirstExcludedTrack) - 1;
> +  for (; !aIter.AtEnd(); aIter.Next()) {
> +    const GridItemInfo& item = aGridItems[aIter.GridItemIndex()];
> +    int32_t major = (item.mArea.*aMajor).mEnd - 1;

This "- 1" subtraction is a bit mysterious. (Similar for "minor" below this.)

I suspect (but am not at all sure) it's because |major| is meant to be a track index, whereas |mEnd| is a line index, and so you subtract 1 from mEnd to get the preceding-adjacent track number?

If that's the case, please add comment and/or rename the variable -- "major" is pretty vague about what it represents.  Maybe call it "itemFinalMajorTrack" or something like that, if that's what it represents? (and similar for "minor"; and possibly similar for their analogues in the FindFirst function)

@@ +5950,5 @@
> +  for (; !aIter.AtEnd(); aIter.Next()) {
> +    const GridItemInfo& item = aGridItems[aIter.GridItemIndex()];
> +    int32_t major = (item.mArea.*aMajor).mEnd - 1;
> +    MOZ_ASSERT((item.mArea.*aMajor).mStart < aFirstExcludedTrack,
> +               "this item should have been pushed by now");

What does this assertion mean? This function doesn't do any pushing.  (I'm guessing this has to do with fragmentation that we expect to have happened earlier, and aFirstExcludedTrack has something to do with the fragmentation break point?)

Whatever this means, it looks like it strongly restricts the range of valid |aFirstExcludedTrack| values that we'll allow this function to be called with -- so please document that restriction. (e.g. if a caller were to pass aFirstExcludedTrack=0 and we had any grid items at all, it would trivially make us fail this fatal assertion, I think)

@@ +5952,5 @@
> +    int32_t major = (item.mArea.*aMajor).mEnd - 1;
> +    MOZ_ASSERT((item.mArea.*aMajor).mStart < aFirstExcludedTrack,
> +               "this item should have been pushed by now");
> +    if (major < int32_t(aFragmentStartTrack)) {
> +      continue; // item doesn't span any track in this fragment

It looks like this is assuming the fragmentation axis is the same as our "major" axis.

But what if it's not?  Does this still work?  (i.e. if we're computing the column-axis baseline [but we fragment between rows], do we need to somehow prevent cells in other fragments' rows from influencing the column baseline of *this* fragment? I don't see how these functions would prevent that from happening right now -- we have no way of addressing these meddling rows as being "out of bounds", since aFragmentStartTrack is a column-index in this hypothetical scenario.)

::: layout/generic/nsGridContainerFrame.h
@@ +185,5 @@
>    // Helper to move child frames into the kExcessOverflowContainersList:.
>    void MergeSortedExcessOverflowContainers(nsFrameList& aList);
>  
> +  /**
> +   * Find the first item in Grid Order.

This isn't just finding the first item. It finds the first item in a given grid fragment (starting with track |aFragmentStartTrack|), or something like that, right?

Please expand this comment a bit, to clarify.

@@ +195,5 @@
> +                           LineRange GridArea::* aMajor,
> +                           LineRange GridArea::* aMinor,
> +                           uint32_t aFragmentStartTrack);
> +  /**
> +   * Find the last item in Grid Order.

As above, please expand this to clarify what aFragmentStartTrack and aFirstExcludedTrack args mean.  (Please also clarify somewhere why we take aFirstExcluded track in this FindLast function but not in the FindFirst one).

	Daniel Holbert [:dholbert]
	Comment 19 • 8 years ago

Comment on attachment 8761236 [details] [diff] [review]
part 3 - [css-grid] Add a couple of members to record fragmentation state.

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +5152,5 @@
>      }
>    }
>  
>    // Record a break before |aEndRow|.
> +  aState.mNextFragmentStartRow = aEndRow;

Shouldn't we push this new line down a bit, so it's *inside* the "if (aEndRow < aState.mRows.mSizes.Length())" clause?  Seems like this should be grouped with the BreakBeforeRow() call inside that clause.

(Also, perhaps we should assert that aState.mInFragmentainer is true, either here or somewhere above this?)

@@ +5295,5 @@
>  
>    nscoord bSize = aContentArea.BSize(wm);
>    Maybe<Fragmentainer> fragmentainer = GetNearestFragmentainer(aState);
>    if (MOZ_UNLIKELY(fragmentainer.isSome())) {
> +    aState.mInFragmentainer = true;

It seems a bit bad that the mInFragmentainer member-variable is just wrong until we get to this particular line in ReflowChildren.  I worry that some future code that we add before this point might accidentally check aState.mInFragmentainer, and incorrectly think that it can trust its value.

Perhaps we should either:
 (1) set mInFragmentainer in the GridReflowInput constructor.

OR, maybe-better:
 (2) Move our Maybe<Fragmentainer> local variable to simply be a GridReflowInput member-variable, and initialize *that* in the constructor  (and perhaps expose an IsInFragmentainer() convenience accessor on the GridReflowInput object)

What do you think?

	Daniel Holbert [:dholbert]
	Comment 20 • 8 years ago

Comment on attachment 8788873 [details] [diff] [review]
part 4 - [css-grid] Implement Grid Container Baselines.

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +5864,5 @@
> +    if (baselines != BaselineSet::eNone) {
> +      // We need to create a new iterator and GridItemInfo array because we
> +      // might have pushed some children at this point.
> +      // Even if the gridReflowInput iterator is invalid we can reuse its
> +      // state about order to optimize initialization of the new iterator. 

Nit: end-of-line whitespace

::: layout/generic/nsGridContainerFrame.h
@@ +274,5 @@
> +  }
> +  bool GetIBaseline(BaselineSharingGroup aBaselineGroup, nscoord* aResult) const
> +  {
> +    *aResult = mBaseline[mozilla::eLogicalAxisInline][aBaselineGroup];
> +    return true;

Why are we bothering with having infallible "return true" in these getters? Why not just make them return void?

Also, perhaps we should be non-fatally asserting (either here or in callers) that *aResult isn't the (default) sentinel-value NS_INTRINSIC_WIDTH_UNKNOWN? It looks like callers do arithmetic with *aResult without checking it, right now (e.g. adding m.IStart or m.IEnd, subtracting it from frameSize).  An assertion would be nice to ensure that arithmetic is actually operating on a length instead of a sentinel.

	Mats Palmgren (inactive) Assignee
	Comment 21 • 8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #16)
> > +    MOZ_ASSERT(mIter.isSome() || mArray->Length() == aGridItemCount,
> 
> Why do we need to expose this SetGridItemCount() setter at all?

It's an optimization to avoid iterating the child frame list
in the ctor to count how many non-placeholder frames there are
(i.e. how many grid items we have).  It's only needed for
reverse-iterators, which we only create at a point where
we known how many grid items we have.

I removed the SetGridItemCount() call on the forward-iterator
to further underline it's not used for anything there.

> > +    mGridItemIndex = IsForward() ? 0 : mGridItemCount - 1;
> 
> Here (and in 2 other places), we subtract from mGridItemIndex, which could
> underflow to +infinity (though that only happens if mGridItemCount is 0, in
> two of the three places) Could that underflow-to-infinity cause trouble?

mGridItemCount is only used for setting mGridItemIndex to the start
position of a reverse-iterator.
mGridItemIndex is only used in methods that have the precondition:
    MOZ_ASSERT(!AtEnd());
which will assert if mGridItemCount is zero.

Also note that the return value from GridItemIndex() is always
used to index mGridItems (nsTArray), so that will also assert.

I've added a note saying that it's OK to underflow since the result
is not used.


(In reply to Daniel Holbert [:dholbert] from comment #17)
> Now that we maintain "mGridItemCount", we can & should assert that this
> getter is returning an in-range value. Let's add something like this here
> (in GridItemIndex()):
> 
>   MOZ_ASSERT(mGridItemIndex < mGridItemCount,
>              "Returning an out-of-range mGridItemIndex...");

It's a bit redundant since the sole purpose of GridItemIndex()
is to index mGridItems which will assert too, but sure...

	Mats Palmgren (inactive) Assignee
	Comment 22 • 8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #18)
> This "major" variable isn't used until *after* this "if" clause, so it
> should be declared further down (probably right alongside the "minor"
> variable-decl.)

Fixed.

> > +    int32_t major = (item.mArea.*aMajor).mEnd - 1;
> 
> This "- 1" subtraction is a bit mysterious. (Similar for "minor" below this.)

I added a comment:
    // Subtract 1 from the end line to get the item's last track index.

> If that's the case, please add comment and/or rename the variable -- "major"
> is pretty vague about what it represents.  Maybe call it
> "itemFinalMajorTrack" or something like that, if that's what it represents?

With that comment I think it's reasonably clear.
Sometimes, using more verbose names actually makes the code less readable.

> > +    MOZ_ASSERT((item.mArea.*aMajor).mStart < aFirstExcludedTrack,
> > +               "this item should have been pushed by now");
> 
> What does this assertion mean?

Yes, it's quite unrelated to the method per se but it's a good invariant
to check while we're here.  aFirstExcludedTrack is the first track in
the next container fragment (or one beyond the last track in the last
fragment), so we shouldn't find and any items in *this* fragment with
a start row in some later fragment (which is why I said it should have
been pushed at this point).

I re-phrased the message as:
"found an item that belongs to some later fragment"
Is that clearer?

> Whatever this means, it looks like it strongly restricts the range of valid
> |aFirstExcludedTrack| values that we'll allow this function to be called
> with -- so please document that restriction. (e.g. if a caller were to pass
> aFirstExcludedTrack=0 and we had any grid items at all, it would trivially
> make us fail this fatal assertion, I think)

Yes, the invariant only holds in the way this method is called
currently, not for all possible (valid) aFirstExcludedTrack one
could imagine.  I've added a comment about it.

> > +    if (major < int32_t(aFragmentStartTrack)) {
> > +      continue; // item doesn't span any track in this fragment
> 
> It looks like this is assuming the fragmentation axis is the same as our
> "major" axis.

Yep, there are likely numerous places where we assume fragmentation
only occurs in the block-axis.

> But what if it's not?  Does this still work?

We simply don't fragment in the inline-axis at all.  That's bug 1266265.
(That spec change occured after we had implemented fragmentation.)

> ::: layout/generic/nsGridContainerFrame.h
> > +   * Find the first item in Grid Order.
> 
> This isn't just finding the first item. It finds the first item in a given
> grid fragment (starting with track |aFragmentStartTrack|), or something like
> that, right?

Right, I appended "in this fragment".

> > +   * Find the last item in Grid Order.
> 
> As above, please expand this to clarify what aFragmentStartTrack and
> aFirstExcludedTrack args mean.

I think aFragmentStartTrack is obvious from its name alone.
I've documented aFirstExcludedTrack.

	Mats Palmgren (inactive) Assignee
	Comment 23 • 8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #19)
> > +  aState.mNextFragmentStartRow = aEndRow;
> 
> Shouldn't we push this new line down a bit, so it's *inside* the "if
> (aEndRow < aState.mRows.mSizes.Length())" clause?

No, we also want to set it if EndRow == aState.mRows.mSizes.Length(),
i.e. for the last fragment.

> Perhaps we should either:
>  (1) set mInFragmentainer in the GridReflowInput constructor.
> 
> OR, maybe-better:
>  (2) Move our Maybe<Fragmentainer> local variable to simply be a
> GridReflowInput member-variable, and initialize *that* in the constructor 
> (and perhaps expose an IsInFragmentainer() convenience accessor on the
> GridReflowInput object)
> 
> What do you think?

Sounds like fodder for a separate (non-blocking) refactoring bug.

	Mats Palmgren (inactive) Assignee
	Comment 24 • 8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #20)
> > +  bool GetIBaseline(BaselineSharingGroup aBaselineGroup, nscoord* aResult) const
> > +  {
> > +    *aResult = mBaseline[mozilla::eLogicalAxisInline][aBaselineGroup];
> > +    return true;
> 
> Why are we bothering with having infallible "return true" in these getters?
> Why not just make them return void?

A grid container has baselines in both axis.  I was designing this as
a method that we could eventually make virtual on nsIFrame at some point
and then other frame types could return false.

(BTW, I'll revisit block-axis baselines in a follow-up bug before
release, but I think this is good enough to land for now.  Hopefully
I use this to make a demo that will convince @fantasai that block-axis
baselines are desireable.)

> Also, perhaps we should be non-fatally asserting (either here or in callers)
> that *aResult isn't the (default) sentinel-value NS_INTRINSIC_WIDTH_UNKNOWN?

OK, I added a couple.  Jesse is bound to find false positives though... :-(

	Mats Palmgren (inactive) Assignee
	Comment 25 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ad791068cd5

	Daniel Holbert [:dholbert]
	Comment 26 • 8 years ago

(In reply to Mats Palmgren (:mats) from comment #22)
> I re-phrased the message as:
> "found an item that belongs to some later fragment"
> Is that clearer?

Yes -- thanks.

> > > +    if (major < int32_t(aFragmentStartTrack)) {
> > > +      continue; // item doesn't span any track in this fragment
> > 
> > It looks like this is assuming the fragmentation axis is the same as our
> > "major" axis.
> 
> Yep, there are likely numerous places where we assume fragmentation
> only occurs in the block-axis.
[...]
> We simply don't fragment in the inline-axis at all.

Thanks -- I understand that, but that's not quite answering the question that I meant to ask.

My point here was, the code here *isn't necessarily operating on the block axis*. It's operating on the *major axis*. (which could be block or inline, depending on the caller).  And we're treating "major axis" as the axis where fragmentation is (potentially) occurring, but we don't actually know if fragmentation is possible in that axis.

This would be clearer if you documented that |aFragmentStartTrack| is a track-index in the |aMajor| axis, AND that if there's no fragmentation in that axis, it should simply be 0.  (I think that's true?)

Otherwise, if we're e.g. some fragment that starts at Row 25, it kinda sounds like aFragmentStartTrack would always be 25 here (and that this function would just internally figure out that this is irrelevant, perhaps.)

> I think aFragmentStartTrack is obvious from its name alone.

I don't think it's obvious, because
(1) the function deals with both types of tracks, and it's not clear whether this is a major index vs. a fragmentation-axis-index
(2) It's not obvious what aFragmentStartTrack should be, if we're fragmented but aMajor happens to be the non-fragmented axis.

Could you add the documentation suggested above (saying that it's in the |aMajor| axis and that it should be 0 if there's no fragmentation in that axis)?

	Daniel Holbert [:dholbert]
	Comment 27 • 8 years ago

Comment on attachment 8794613 [details] [diff] [review]
part 1 - [css-grid] Make GridItemCSSOrderIterator use nsFrameList iterators...

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

r=me on part 1, with the following addressed however you see fit (phrased as a response to comment 21):

(In reply to Mats Palmgren (:mats) from comment #21)
> > Why do we need to expose this SetGridItemCount() setter at all?
> 
> It's an optimization to avoid iterating the child frame list
> in the ctor to count how many non-placeholder frames there are
> (i.e. how many grid items we have).

Ah, ok.  I was confused because the constructor *does already* iterate the child frame list (and could count the children) -- but now I see that this only happens if the passed-in orderState is  eUnknownOrder.  And at the one SetGridItemCount() callsite, we invoked the Iterator constructor with a different OrderState, so we do indeed skip the iteration.

So: this makes more sense to me now (thanks).

> I removed the SetGridItemCount() call on the forward-iterator
> to further underline it's not used for anything there.

Hmm...  So, it kind of scares me that we *only sometimes* initialize a generically-named "item-count" variable that we use for iteration/array-indexing.  And when we do initialize it, we have to remember to explicitly do so, in an extra call outside of the constructor.

It also seems odd that we could initialize it twice without noticing, by accident (in part because we reuse grid iterators thorughout grid code).

Perhaps could make its usage/initialization a bit stricter by making it "Maybe<size_t>" instead of just "size_t", and making all of its callsites just use *mGridItemCount instead of just mGridItemCount?  And then in SetGridItemCount, we could assert that it hasn't yet been initialized, and then emplace() it with the passed-in value. (This verifies that we don't double-initialize it by accident.)  And in the ~GridItemCSSOrderIteratorT destructor, we could assert that mGridItemCount exists IFF IsForward() is false.  All together, that would enforce that we call SetGridItemCount under the correct conditions, the correct number of times.

I think we should either do that, or restore the (technically-unnecessary-but-perhaps-confusionmaking-if-it's-missing) SetGridItemCount call that you removed for the forward iterator... or perhaps some combination of the two? :)

	Daniel Holbert [:dholbert]
	Comment 28 • 8 years ago

Comment on attachment 8794614 [details] [diff] [review]
part 2 - [css-grid] Add methods for finding the first/last grid item in Grid Order in this fragment

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

r=me, with the following:

::: layout/generic/nsGridContainerFrame.h
@@ +254,5 @@
>  
> +  /**
> +   * Find the first item in Grid Order in this fragment.
> +   * https://drafts.csswg.org/css-grid/#grid-order
> +   */

Please add documentation for aFragmentStartTrack, per second half of comment 26.

@@ +264,5 @@
> +                           uint32_t aFragmentStartTrack);
> +  /**
> +   * Find the last item in Grid Order in this fragment.
> +   * @param aFirstExcludedTrack should be the first track in the next fragment
> +   * or one beyond the last track in the last fragment.

Thanks for adding this aFirstExcludedTrack documentation. Two nits:

 (1) s/first track/first track (in aMajor's axis)/
 (Otherwise it sounds [incorrectly] like aFirstExcludedTrack might be strictly a row index, even if aMajor were columns -- because rows are the only tracks that get split across fragments [for now at least].  So in the absence of clarification on the axis, "first track in the next fragment" sounds logically like it might be implying "first *row* in the next fragment".)

 (2) s/last/final/ to be less ambiguous.
  ("last" can mean "previous", so "last fragment" sounds like "previous fragment" here. Same for "last track", to a lesser extent.)

	Daniel Holbert [:dholbert]
	Comment 29 • 8 years ago

Comment on attachment 8761236 [details] [diff] [review]
part 3 - [css-grid] Add a couple of members to record fragmentation state.

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

(In reply to Mats Palmgren (:mats) from comment #23)
> (In reply to Daniel Holbert [:dholbert] from comment #19)
 > Perhaps we should either:
> >  (1) set mInFragmentainer in the GridReflowInput constructor.
> > 
> > OR, maybe-better:
> >  (2) Move our Maybe<Fragmentainer> local variable to simply be [...]
> > What do you think?
> 
> Sounds like fodder for a separate (non-blocking) refactoring bug.

Perhaps.  Though, I brought it up as an issue with this patch, it's a problem that's being introduced in this patch.  (The problem being, we now have this new piece of state called "mInFragmentainer" which we can't really depend on being accurate without careful analysis of the control flow.)

I suppose I'm OK with this being punted to a followup, though in that case I'd like mInFragmentainer to have a comment next to it saying e.g. "// Initialized lazily, when we find the fragmentainer", as a hint that it can't be directly trusted.  (Maybe that's sufficient and we don't need a followup; I'll leave that up to you.)

r=me with that comment added

	Mats Palmgren (inactive) Assignee
	Comment 30 • 8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #26)
> This would be clearer if you documented that |aFragmentStartTrack| is a
> track-index in the |aMajor| axis, AND that if there's no fragmentation in
> that axis, it should simply be 0.  (I think that's true?)

Yes, exactly.  I added:
@param aFragmentStartTrack is the first track in this fragment in the same
axis as aMajor.  Pass zero if that's not the axis we're fragmenting in.

Does that make it clearer?

	Daniel Holbert [:dholbert]
	Comment 31 • 8 years ago

Yes, thanks!

	Mats Palmgren (inactive) Assignee
	Comment 32 • 8 years ago

A small bug fix: rather than using tracks.mSizes.Length() in both axis,
we should use mNextFragmentStartRow in the axis we fragment.
(for the aFirstExcludedTrack param)

	Mats Palmgren (inactive) Assignee
	Comment 33 • 8 years ago

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

	Daniel Holbert [:dholbert]
	Comment 34 • 8 years ago

Comment on attachment 8795703 [details] [diff] [review]
part 4 - [css-grid] Implement Grid Container Baselines.

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

Part 4 review feedback, wave 1 of 2 (haven't made it quite to the end):

::: layout/generic/nsGridContainerFrame.cpp
@@ +128,5 @@
> +  if (aGroup == BaselineSharingGroup::eFirst) {
> +    return aWM.IsAlphabeticalBaseline() ? aBorderBoxSize : aBorderBoxSize / 2;
> +  }
> +  MOZ_ASSERT(aGroup == BaselineSharingGroup::eLast);
> +  return aWM.IsAlphabeticalBaseline() ? 0 : aBorderBoxSize / 2;

Nit: if aBorderBoxSize is an odd number, and we're using the central baseline, this function will produce a different position for the "First" vs. "Last" baseline (even though they should be the same central position).  E.g. if aBorderBoxSize is 11, then we'd say the first-baseline is 5 from the top, and the last-baseline is 5 from the bottom (6 from the top).

It's probably uncommon that this would cause problems -- but if that 1-nscoord unit happens to be on a pixel-rounding boundary, I can imagine it triggering subtle "jumps" for changes that should really not trigger a jump at all.

Could you fix this to consistently return the same central baseline?  I think this should do (for the lower division):
  // Round up for central baseline offset, to be consistent w/ eFirst position:
  return aWM.IsAlphabeticalBaseline() ? 0 :
    (aBorderBoxSize / 2) + (aBorderBoxSize % 2);

(Or you could use NSCoordDivRem, but that's probably overkill.)

@@ +4097,5 @@
>        if (state & ItemState::eFirstBaseline) {
> +        if (grid) {
> +          isOrthogonal == isInlineAxis ?
> +            grid->GetBBaseline(BaselineSharingGroup::eFirst, &baseline) :
> +            grid->GetIBaseline(BaselineSharingGroup::eFirst, &baseline);

It's a bit deceiving to have "isOrthogonal = [choice between two functions which happen to always return true]".  In particular:
 - it incorrectly gives the impression that isOrthogonal might end up false -- i.e. that this code is actually determining whether or not we're orthogonal.
 - it incorrectly gives the impression that we're capturing & handling Get{B,I}Baseline() failure somehow. (They happen to never fail, but this is capturing their return value & using it, which makes it look like we're testing whether they failed.)

Could you just directly set "isOrthogonal = true" here, and then (separately) call one or the other baseline function? 
(This applies to two places in the patch.)

@@ +4102,5 @@
> +        }
> +        if (grid ||
> +            nsLayoutUtils::GetFirstLineBaseline(wm, child, &baseline)) {
> +          NS_ASSERTION(baseline != NS_INTRINSIC_WIDTH_UNKNOWN,
> +                       "unknown baseline");

Maybe broaden the message to "about to use an unknown baseline", to be clearer about what's bad here?
(This applies to two places in the patch.)

@@ +4127,4 @@
>            auto frameSize = isInlineAxis ? child->ISize(wm) : child->BSize(wm);
>            auto m = child->GetLogicalUsedMargin(wm);
> +          if (!grid) {
> +            baseline = frameSize - baseline;

Add a comment to explain this subtraction -- e.g.:
   // Convert to distance from border-box end.

(This helps hint at an explanation for the "if (!grid)" check, which is otherwise a bit mysterious.  [the explanation being: the child grid will have already returned a baseline in this distance-from-end representation.])

@@ +5523,5 @@
>        MergeSortedOverflow(pushedList);
>        AddStateBits(NS_STATE_GRID_DID_PUSH_ITEMS);
> +      // NOTE since we messed with our child list here, we intentionally
> +      // make aState.mIter invalid to avoid any use of it after this point.
> +      aState.mIter.Invalidate();

This chunk seems like a nice belt-and-suspenders protection, but does it actually belong in this patch?  It isn't obviously related to (or used by) anything else in this patch, as far as I can tell.

I wonder if it'd be better to group this change into an earlier patch (in part 1 perhaps -- that's where we create this Invalidate() method, after all).

@@ +5928,5 @@
> +    }
> +    uint32_t len = gridReflowInput.mRows.mSizes.Length();
> +    if (gridReflowInput.mStartRow != len &&
> +        gridReflowInput.mNextFragmentStartRow == len) {
> +      baselines = BaselineSet(baselines | BaselineSet::eLast);

Could you add a comment above these checks, to clarify what they're aiming to do with all these mStartRow / len / etc. comparisons?

After staring at this a bit, I think the intent is something like:
  // Only compute first-baseline if this is the first (nonempty) fragment, and
  // only compute last-baseline if this is the last (nonempty) fragment.

@@ +5950,5 @@
> +      for (; !iter->AtEnd(); iter->Next()) {
> +        auto child = **iter;
> +        for (const auto& info : gridReflowInput.mGridItems) {
> +          if (info.mFrame == child) {
> +            gridItems->AppendElement(info);

Could you explain this loop? It looks like it's doing a double nested iteration over our grid items  (For each item, we walk across gridReflowInput.mGridItems)... Why?  Isn't this nested loop going to be O(n^2) in the number of items, and (if so) can we avoid that?

@@ +5959,5 @@
> +    auto sz = frameRect.Size();
> +    CalculateBaselines(baselines, iter.ptrOr(nullptr), gridItems.ptrOr(nullptr),
> +                       gridReflowInput.mCols, 0,
> +                       gridReflowInput.mCols.mSizes.Length(), wm,
> +                       sz, bp.IStart(wm), bp.IEnd(wm), desiredSize.ISize(wm));

nit: wrap "sz" up to the previous line, so that the wrapping is consistent between this CalculateBaselines call and the one immediately after it.

@@ +6381,5 @@
> +  WritingMode          aCBWM)
> +{
> +  if (MOZ_UNLIKELY(!aGridOrderItem.mItem)) {
> +    // No item in this fragment - synthesize a baseline from our content box.
> +    return ::SynthesizeBaselineFromBorderBox(aGroup, aCBWM, aCBSize);

Nit: the comment and the function-call (slightly) disagree.  The comment says "from our content box", the function call says "FromBorderBox".

	Daniel Holbert [:dholbert]
	Comment 35 • 8 years ago

Comment on attachment 8795703 [details] [diff] [review]
part 4 - [css-grid] Implement Grid Container Baselines.

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

Part 4 review feedback, wave 2 of 2:
(r- because there's enough to address/clarify here that it'd be worth taking one more review pass):

::: layout/generic/nsGridContainerFrame.cpp
@@ +6399,5 @@
> +                     grid->GetBBaseline(aGroup, &baseline);
> +    } else if (!isOrthogonal && aGridOrderItem.mIsInEdgeTrack &&
> +               nsLayoutUtils::GetLastLineBaseline(childWM, child, &baseline)) {
> +      if (aGroup == BaselineSharingGroup::eLast) {
> +        baseline = size - baseline;

As above, it'd be helpful to explain this baseline-conversion-subtraction here. (in part, to hint at why we only bother to do it in this restricted non-grid-child case)

e.g. maybe add a comment on the end of this line, like:
        baseline = size - baseline; ; // Convert to distance from border-box end.

(This applies further down, too, in "else" clause of the outer branch here.

@@ +6408,5 @@
> +  } else {
> +    start = child->GetLogicalNormalPosition(aCBWM, aCBPhysicalSize).I(aCBWM);
> +    size = child->ISize(aCBWM);
> +    if (grid) {
> +      isOrthogonal ? grid->GetBBaseline(aGroup, &baseline) :

I think the "if (grid)" check here is missing a "&& aGridOrderItem.mIsInEdgeTrack" -- at least, that condition is present in the analogous code in the other main branch here (for eLogicalAxisBlock).

@@ +6441,5 @@
> +  const auto axis = aTracks.mAxis;
> +  auto firstBaseline = aTracks.mBaseline[BaselineSharingGroup::eFirst];
> +  if (!(aBaselineSet & BaselineSet::eFirst)) {
> +    mBaseline[axis][BaselineSharingGroup::eFirst] =
> +      ::SynthesizeBaselineFromBorderBox(BaselineSharingGroup::eFirst, aWM,

This logic seems backwards. It looks like it's saying:

  if (the caller did *not* request eFirst baseline) { 
    Synthesize something for our eFirst baseline.
  }

Maybe I'm misunderstanding what aBaselineSet represents here, or something else about the logic?   Or maybe the thing we're synthesizing here is just a dummy value (though it seems kind-of meaningful, for a dummy value)...

A code-comment would be helpful, in any case.... (This goes for the eLast version of this clause further down, too.)

@@ +6471,5 @@
> +  if (!(aBaselineSet & BaselineSet::eLast)) {
> +    mBaseline[axis][BaselineSharingGroup::eLast] =
> +      ::SynthesizeBaselineFromBorderBox(BaselineSharingGroup::eLast, aWM,
> +                                        aCBSize);
> +    return;

I tend to think we should get rid of this stray "return", and put everything after this into an "else if".  The early-return isn't saving us on indentation, and it's making the logic unnecessarily different between the top half & bottom half of this function.   (It'd be nice for that logic to be structured as similarly as possible, for easier high-level comparison, so we can be sure we're being consistent & easily see the differences that are actually relevant.)

To that end:
 * we should move the "auto lastBaseline" declaration up a bit higher (right now it's a few lines below this point)
 * we should convert the current "return;} if (...)" to "} else if (...) {"

So I think this should end up looking like:
  auto lastBaseline = aTracks.mBaseline[BaselineSharingGroup::eLast];
  if (!(aBaselineSet & BaselineSet::eLast)) {
    mBaseline[axis][BaselineSharingGroup::eLast] =
      ::SynthesizeBaselineFromBorderBox(BaselineSharingGroup::eLast, aWM,
                                        aCBSize);
  } else if (lastBaseline == NS_INTRINSIC_WIDTH_UNKNOWN) {

@@ +6481,5 @@
> +    auto orderState = aIter->ItemsAreAlreadyInOrder() ?
> +      Iter::OrderState::eKnownOrdered : Iter::OrderState::eKnownUnordered;
> +    Iter iter(this, kPrincipalList, Iter::ChildFilter::eSkipPlaceholders,
> +              orderState);
> +    iter.SetGridItemCount(aGridItems->Length());

This 6 lines of iter creation/customization probably wants a brief explanatory comment. (since for the BaselineSharingGroup::eFirst version of this logic, up higher, we just directly use *aIter.)

@@ +6501,5 @@
> +    MOZ_ASSERT(!aGridItems->IsEmpty());
> +    auto gapAfterEndTrack =
> +      aCBSize - aCBBorderPaddingStart - aCBBorderPaddingEnd +
> +      aTracks.GridLineEdge(aFirstExcludedTrack, GridLineSide::eBeforeGridGap) -
> +      aTracks.GridLineEdge(aFragmentStartTrack, GridLineSide::eBeforeGridGap);

This math doesn't quite make sense to me. In particular:
 (1) "aCBSize - aCBBorderPaddingStart - aCBBorderPaddingEnd" seems like it's the grid's content-box size.
 (2) The aTracks.GridLineEdge(...) - aTracks.GridLineEdge(...) expression seems like the distance between this fragment's first line & its last line.

And we're adding those two quantities together.  I don't immediately see why those two quantities would produce a "gapAfterEndTrack"...  Maybe break the arithmetic down slightly differently, and/or add a comment to explain what's going on here?

	Mats Palmgren (inactive) Assignee
	Comment 36 • 8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #34)
> Could you fix this to consistently return the same central baseline?

Good point.

> > +        if (grid) {
> > +          isOrthogonal == isInlineAxis ?
> > +            grid->GetBBaseline(BaselineSharingGroup::eFirst, &baseline) :
> > +            grid->GetIBaseline(BaselineSharingGroup::eFirst, &baseline);
> 
> It's a bit deceiving to have "isOrthogonal = [choice between two functions
> which happen to always return true]". 

Well, as I said, this chunk is a bit temporary at the moment.
The idea was to remove the "if (grid)" and do this for all
frame types, which could return false here if they didn't
offer a baseline in the particular axis.  I think I'll leave
it as is for now (and add a XXX comment) -- I'll revisit this
code anyway before release.

> Maybe broaden the message to "about to use an unknown baseline", to be
> clearer about what's bad here?

Sure.

> Add a comment to explain this subtraction -- e.g.:
>    // Convert to distance from border-box end.

OK.

> > +      // NOTE since we messed with our child list here, we intentionally
> > +      // make aState.mIter invalid to avoid any use of it after this point.
> > +      aState.mIter.Invalidate();
> 
> I wonder if it'd be better to group this change into an earlier patch (in
> part 1 perhaps -- that's where we create this Invalidate() method, after
> all).

Sure, I've moved it to part 1.

> > +    if (gridReflowInput.mStartRow != len &&
> > +        gridReflowInput.mNextFragmentStartRow == len) {
> > +      baselines = BaselineSet(baselines | BaselineSet::eLast);
> 
> After staring at this a bit, I think the intent is something like:
>   // Only compute first-baseline if this is the first (nonempty) fragment,
> and
>   // only compute last-baseline if this is the last (nonempty) fragment.

The fragment containing the first/last row to be precise.
(BTW, I filed bug 1306499 to investigate maybe dropping these
conditions later.)

> > +      for (; !iter->AtEnd(); iter->Next()) {
> > +        auto child = **iter;
> > +        for (const auto& info : gridReflowInput.mGridItems) {
> > +          if (info.mFrame == child) {
> > +            gridItems->AppendElement(info);
> 
> Could you explain this loop? It looks like it's doing a double nested
> iteration over our grid items  (For each item, we walk across
> gridReflowInput.mGridItems)... Why?

Because the child frame list would now be out-of-sync with mGridItems
if we pushed an item.  So there is no longer a relation between
the child list and the mGridItems array, so we're rebuilding a new
array here.

> Isn't this nested loop going to be
> O(n^2) in the number of items, and (if so) can we avoid that?

O(n^2) in the number of items *in this fragment*, yes.
I don't see any immediate solution to this problem though.
It's similar to bug 1252186.  Is it OK if I add file a follow-up
bug for this performance problem too? (like bug 1252186 it only
occurs in fragmentainers)

	Mats Palmgren (inactive) Assignee
	Comment 37 • 8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #35)
> I think the "if (grid)" check here is missing a "&&
> aGridOrderItem.mIsInEdgeTrack"

Good catch, thanks.

> > +  if (!(aBaselineSet & BaselineSet::eFirst)) {
> > +    mBaseline[axis][BaselineSharingGroup::eFirst] =
> > +      ::SynthesizeBaselineFromBorderBox(BaselineSharingGroup::eFirst, aWM,
> 
> This logic seems backwards. It looks like it's saying:
> 
>   if (the caller did *not* request eFirst baseline) { 
>     Synthesize something for our eFirst baseline.
>   }

aBaselineSet indicates which baseline(s) should be taken from
an edge track baseline-aligned group, or if none is present,
from an item in in the edge track.  The absense of a bit means
we shouldn't bother with that and just synthesize a baseline
from the container's border-box.  (These bits come from the "does
this fragment contain the first/last track checks" above.)

> A code-comment would be helpful, in any case.... (This goes for the eLast
> version of this clause further down, too.)

I added a @param in the header:
@param aBaselineSet which baseline(s) to derive from a baseline-group or
  items; a baseline not included is synthesized from the border-box instead


> I tend to think we should get rid of this stray "return", and put everything
> after this into an "else if".

Sure.

> This 6 lines of iter creation/customization probably wants a brief
> explanatory comment.

// For finding items for the last-baseline we need to create a reverse
// iterator ('aIter' is the forward iterator from the GridReflowInput).


> > +    auto gapAfterEndTrack =
> > +      aCBSize - aCBBorderPaddingStart - aCBBorderPaddingEnd +
> > +      aTracks.GridLineEdge(aFirstExcludedTrack, GridLineSide::eBeforeGridGap) -
> > +      aTracks.GridLineEdge(aFragmentStartTrack, GridLineSide::eBeforeGridGap);
> And we're adding those two quantities together.  I don't immediately see why
> those two quantities would produce a "gapAfterEndTrack"...

Oops, yeah, this is definitely wrong... which is odd because the reftests
have plenty of last-baseline aligned groups which *should* fall into this
branch.  So, there must be a second bug somewhere else too...

	Mats Palmgren (inactive) Assignee
	Comment 38 • 8 years ago

Here's a fix that bug.  It's a "typo" in bug 1221525 part 3.
It caused us to fall into to the middle "else-if" block in part 4,
which gets the baseline from the last item instead.

So, the reftests are lacking a test that has a last-baseline aligned group
in the last track *and* the last item there has a different baseline from
the group *and* that baseline is propagated to the container and used to
last-baseline align it with something outside the container, for example
if the container itself is an item inside an outer grid.
I'll add a reftest that does that.

	Daniel Holbert [:dholbert]
	Comment 39 • 8 years ago
disregard-first-half

(In reply to Mats Palmgren (:mats) from comment #36)
> > > +        if (grid) {
> > > +          isOrthogonal == isInlineAxis ?
> > > +            grid->GetBBaseline(BaselineSharingGroup::eFirst, &baseline) :
> > > +            grid->GetIBaseline(BaselineSharingGroup::eFirst, &baseline);
> > 
> > It's a bit deceiving to have "isOrthogonal = [choice between two functions
> > which happen to always return true]". 
> 
> Well, as I said, this chunk is a bit temporary at the moment.
> The idea was to remove the "if (grid)" and do this for all
> frame types, which could return false here if they didn't
> offer a baseline in the particular axis.

But I don't see why it makes sense to store that "false" return value (which represents "baseline lookup failed") in a variable called "isOrthogonal".  That's my main hangup. I don't see the connection between the variable name and the success/failure code being stored in it.

I'm assuming it makes sense to you -- could you add a comment to better-explain that?

(And separately -- please do add an XXX comment about this becoming eventually-generic [non-"if(grid)"], as you suggested, too.)

> O(n^2) in the number of items *in this fragment*, yes.
> I don't see any immediate solution to this problem though.
> It's similar to bug 1252186.  Is it OK if I add file a follow-up
> bug for this performance problem too? (like bug 1252186 it only
> occurs in fragmentainers)

Sure.

	Daniel Holbert [:dholbert]
	Comment 40 • 8 years ago

D'oh.... Sorry, I was totally reading "isOrthogonal == isInlineAxis ?" as having a single equals.  Just now noticed that it's a double equals.

So it's not capturing the return value after all -- it's deciding which baseline function to call. Got it.

Disregard my concerns about that chunk -- though, we do eventually need to capture the result of that whole expression in something. (And an XXX comment about generifying it would still be merited.)

	Daniel Holbert [:dholbert]
	Comment 41 • 8 years ago

Comment on attachment 8796510 [details] [diff] [review]
part 4 - [css-grid] Implement Grid Container Baselines.

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

r=me with the following:

::: layout/generic/nsGridContainerFrame.cpp
@@ +4100,5 @@
>        if (state & ItemState::eFirstBaseline) {
> +        if (grid) {
> +          isOrthogonal == isInlineAxis ?
> +            grid->GetBBaseline(BaselineSharingGroup::eFirst, &baseline) :
> +            grid->GetIBaseline(BaselineSharingGroup::eFirst, &baseline);

So right now, I think this unfortunately looks too much like a more-common usage-pattern for ternary expressions, which is to say it looks too much like the following:
  foo = bar ?
    a :
    b;

It only differs from that common pattern in the usage of "=" vs "==" in the first line.  That's how I ended up misreading it so badly, and I worry others will misread it / stumble over it as well.

To reduce the likelihood of this mis-reading, please either:
 (1) wrap the boolean condition in parens, e.g. "(isOrthogonal == isInlineAxis) ?".  This prevents it from looking so much like "isOrthogonal = [some-complex expression]".

...OR:
 (2) Just use "if {  ... } else { ... }" instead of ternary formulation.

(This happens twice; however you address this, please address both instances.)

::: layout/generic/nsGridContainerFrame.h
@@ +295,5 @@
> +  /**
> +   * Calculate this grid container's baselines.
> +   * @param aBaselineSet which baseline(s) to derive from a baseline-group or
> +   *   items; a baseline not included is synthesized from the border-box instead
> +   * @param aFragmentStartTrack is the first track in this fragment in the same

Two nits:
 (1) Don't indent "items;" -- you don't indent the 2nd lines of the other @param chunks further down in this comment, at least. (or for other nearby functions, like for FindFirstItemInGridOrder below this)

 (2) Please add a period at the end of the sentence, i.e. after "border-box instead". That *would* push it over 80 characters, except that you'll be deindenting it now, which gives you a few spare characters to work with. :)

	Mats Palmgren (inactive) Assignee
	Comment 42 • 8 years ago

> I think this unfortunately looks too much like a more-common usage-pattern for ternary expressions

Ah, now I see what you mean.  OK, I'll re-phrase that somehow.

Do I have an r+ on the bug fix in part 5 as well?

	Daniel Holbert [:dholbert]
	Comment 43 • 8 years ago

Comment on attachment 8796511 [details] [diff] [review]
part 5 - [css-grid] Fix a bug in the is-this-the-last-track check.

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

Yup, sorry -- forgot about part 5.  It seems OK to me; r+

	Pulsebot
	Comment 44 • 8 years ago

Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb3080bc1aad
part 1 - [css-grid] Make GridItemCSSOrderIterator use nsFrameList iterators internally and make the specific type (forward/reverse) a template param.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/33bd454be96a
part 2 - [css-grid] Add methods for finding the first/last grid item in Grid Order in this fragment.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d2110357137
part 3 - [css-grid] Add a couple of members to record fragmentation state.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e1c048cf01b
part 4 - [css-grid] Implement Grid Container Baselines.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/69e356ace553
part 5 - [css-grid] Fix a bug in the is-this-the-last-track check.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8b0d217e5a4
part 6 - [css-grid] Reftests for grid container baselines.

	Phil Ringnalda (:philor)
	Comment 45 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/bb3080bc1aad
https://hg.mozilla.org/mozilla-central/rev/33bd454be96a
https://hg.mozilla.org/mozilla-central/rev/5d2110357137
https://hg.mozilla.org/mozilla-central/rev/1e1c048cf01b
https://hg.mozilla.org/mozilla-central/rev/69e356ace553
https://hg.mozilla.org/mozilla-central/rev/c8b0d217e5a4