1452383 - [css-grid] Use LogicalAxis/LogicalSide more

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Mats Palmgren (inactive)]

Just a little cleanup to make these methods more "axis friendly".

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1 - Use LogicalAxis instead of passing member pointers (idempotent patch)

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 2 - Remove local enum LineRangeSide and use LogicalSide instead (idempotent patch)

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8965967 [details] [diff] [review]
part 1 - Use LogicalAxis instead of passing member pointers (idempotent patch)

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

Commit message nit:
> Bug 1452383 part 1 - Use LogicalAxis instead of passing member pointers (idempotent patch). r=dholbert

This should probably mention "grid", to make it easier to find in pushlogs.

(Maybe add [css-grid] like you sometimes do in commit messages IIRC, or add "...when resolving grid lines" at the end?)

::: layout/generic/nsGridContainerFrame.cpp
@@ +2221,5 @@
>     *             a specified line name, it's permitted to pass in zero which
>     *             will be treated as one.
>     * @param aFromIndex the zero-based index to start counting from
>     * @param aLineNameList the explicit named lines
> +   * @param aAxis the axis we're resolving names in

This comment is a little confusing right now, for two reasons:
 (1) it seems needlessly focused on names -- we might *use* this arg internally for matching names, but really from the caller's perspective, that doesn't matter, and really this is just the axis that corresponds to the line that we're resolving. Right?

 (2) It's not superficially obvious what the mapping is from "row/column" to "block/inline" is. (E.g. for row-names: a row is laid out / subdivided into grid-areas in the direction of the grid's inline axis, but the between-row bounding lines are stacked in the block axis. So there's a connection from "row" to both logical axes, and it's not superficially obvious what the right one to pass here would be.)

So: perhaps reword to avoid mentioning names, and to make it clearer what the mapping is from row-vs-col to block-vs.-inline?  Maybe even as direct as just:
   "@param aAxis  eLogicalAxisInline if we're resolving a column line,
                  or eLogicalAxisBlock if we're resolving a row line"

@@ +2259,5 @@
>     * @param aStyle the StylePosition() for the grid container
>     * @param aStart style data for the start line
>     * @param aEnd style data for the end line
>     * @param aLineNameList the explicit named lines
> +   * @param aAxis the axis we're resolving names in

(ditto)

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8965968 [details] [diff] [review]
part 2 - Remove local enum LineRangeSide and use LogicalSide instead (idempotent patch)

>    * Return a line number for (non-auto) aLine, per:
>    * http://dev.w3.org/csswg/css-grid/#line-placement
>    * @param aLine style data for the line (must be non-auto)
>    * @param aNth a number of lines to find from aFromIndex, negative if the
>    *             search should be in reverse order.  In the case aLine has
>    *             a specified line name, it's permitted to pass in zero which
>    *             will be treated as one.
>    * @param aFromIndex the zero-based index to start counting from
>    * @param aLineNameList the explicit named lines
>-   * @param aAxis the axis we're resolving names in
>+   * @param aSide the axis+edge we're resolving names for
>    * @param aExplicitGridEnd the last line in the explicit grid
>-   * @param aEdge indicates whether we are resolving a start or end line

Ah, so this is collapsing away the first instance of the @param whose documentation I was bikeshedding above. :)

Still, this new text could stand to be a bit clearer, too -- maybe just by adding an example to disambiguate? Something like:

  * @param aSide the axis+edge we're resolving names for (e.g. if we're
                  resolving a grid-row-start line, pass eLogicalSideBStart)

Comment 5

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] (away 4/24 - 5/11; use needinfo to be sure I see pings on return) from comment #3)
> This should probably mention "grid", to make it easier to find in pushlogs.

Added a [cdd-grid] prefix.

(In reply to Daniel Holbert [:dholbert] (away 4/24 - 5/11; use needinfo to be sure I see pings on return) from comment #4)
>   * @param aSide the axis+edge we're resolving names for (e.g. if we're
>                   resolving a grid-row-start line, pass eLogicalSideBStart)

Sure, I changed the comment to that ^

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87a5deedc146
part 1 - [css-grid] Use LogicalAxis instead of passing member pointers (idempotent patch).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b2650f9cc81
part 2 - [css-grid] Remove local enum LineRangeSide and use LogicalSide instead (idempotent patch).  r=dholbert

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/87a5deedc146
https://hg.mozilla.org/mozilla-central/rev/1b2650f9cc81