1031241 - rename nsIFrame::Get{Min,Pref}Width methods to Get{Min,Pref}ISize to indicate that they participate in reflow using logical coordinates

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jonathan Kew [:jfkthame]]

As per bug 1028716 comment 13, I think we should consider doing this rename. As we're working on converting layout methods to work with logical coordinates, it's really helpful if the names indicate the correct coordinate system, so that inappropriate combinations of physical and logical are easy to see when reading the code.

The downside, as per bug 789096 comment 71, is that this obscures the association with CSS properties that have existing "physical" names, but then get treated as "logical" in vertical writing modes. In an ideal world, we'd revise the names of the CSS properties; but as we're stuck with those, there's going to be a disconnect somewhere. :(

My inclination for Get{Min,Pref}Width is that we should rename; these are key players in (logical) reflow, and I think the code will be constantly jarring if we stick with the "physical" names. But WDYT?

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Rename Get{Min,Pref}Width to Get{Min,Pref}ISize throughout layout.

Comment 2

[Jonathan Kew [:jfkthame]]

Comment on attachment 8447085 [details] [diff] [review]
Rename Get{Min,Pref}Width to Get{Min,Pref}ISize throughout layout.

This patch was generated by running

find layout/ -name *.cpp -or -name *.h -exec perl -p -i -e 's/\bGetMinWidth\b/GetMinISize/g;s/\bGetPrefWidth\b/GetPrefISize/g;' {} \;

over my tree, and adding a comment in nsIFrame.h.

As such, this just changes the method names; in many cases, there will also be local variables, etc. (both within the implementations of these methods and in the code that uses them) that we will want to update to reflect their logical-coord nature. But that can be done piecemeal as we work through the various classes.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Jonathan Kew (:jfkthame) from comment #0)
> The downside, as per bug 789096 comment 71, is that this obscures the
> association with CSS properties that have existing "physical" names, but

That's actually less of an issue here because these methods don't have any interesting correspondence to CSS properties.  They're not really related to the min-width or max-width properties.

(I'm hoping to review the patch on Monday or sooner; sorry for not getting to it today.)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8447085 [details] [diff] [review]
Rename Get{Min,Pref}Width to Get{Min,Pref}ISize throughout layout.

nsIFrame.h:

>   /**
>-   * Get the intrinsic minimum width of the frame.  This must be less
>-   * than or equal to the intrinsic width.
>+   * Get the intrinsic minimum inline size of the frame.  This must be less
>+   * than or equal to the intrinsic inline size.

Given CSS terminology solidification that's happened since these comments were written, I'd suggest describing this one as the min-content intrinsic inline size, and the other as the max-content intrinsic inline size.  (Throughout these comments.)  Maybe even put "measure" in parentheses after inline size, although I'm somewhat hoping the measure/extent distinction goes away, so maybe not.

>+   *
>+   * ===========
>+   * NAMING NOTE:

No big "=======" and "NAMING NOTE", please.

>+   * This used to be named GetMinWidth, reflecting its connection to CSS
>+   * properties named (*-)width, but is in fact a "logical width" or inline

There really wasn't a connection to the properties named *-width; it was just an intrinsic width.

>+   * size, which in vertical writing modes really the height.
>+   * The method name was changed to GetMinISize to make it easier to keep
>+   * track of whether calling code is working in terms of logical or
>+   * physical coordinates, as well as to clarify for classes implementing
>+   * the method that they should be returning a writing-mode-based logical
>+   * value and not a physical width.
>+   *
>+   * The same applies to GetPrefISize (formerly GetPrefWidth) below.
>+   *
>+   * Many comments, local variables, etc., may not be fully up-to-date
>+   * with this transition; caveat hacker!
>+   * ===========

I'd actually drop this entire comment between ======, I think.  Given the current state of the code, you could put such a comment pretty much anywhere in layout, but the plan is to fix that over a timespan of months to a year, no?

>   /**
>-   * Get the intrinsic width of the frame.  This must be greater than or
>-   * equal to the intrinsic minimum width.
>+   * Get the intrinsic inline size of the frame.  This must be greater than or
>+   * equal to the intrinsic minimum inline size.

Same terminology as above, I think.

r=dbaon with that

Comment 5

[Jonathan Kew [:jfkthame]]

Thanks for the comments; will update accordingly.

:smontagu, it'd be good to have confirmation that you're comfortable with this change, too, given how it'll cut across the logicalization work throughout layout. Any merge needed with your current work should be trivial, I presume.

Comment 6

[Simon Montagu :smontagu]

Comment on attachment 8447085 [details] [diff] [review]
Rename Get{Min,Pref}Width to Get{Min,Pref}ISize throughout layout.

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

Yes, I'm happy with this change

Comment 7

[Jonathan Kew [:jfkthame]]

While we're doing this, I think it makes sense to extend this renaming to include a bunch of additional types and methods that are all connected with intrinsic sizes. E.g. types such as nsLayoutUtils::IntrinsicWidthType, nsIFrame::InlineMinWidthData, and nsIFrame::IntrinsicWidthOffsetData; and methods like MarkIntrinsicWidthsDirty(), GetIntrinsicWidth(), AddInlinePrefWidth(), etc.

I'll attach several more patches that update a bunch more of these names. These are all generated by simple global replacements across the /layout/ tree, and have no effect on behavior; they just serve to extend the use of logical-coordinate terminology a bit further across the code.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - More renaming, e.g. where {Min,Pref}Width occurs within longer type and function names.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: pt 3 - Also rename Intrinsic{Width,Height} to Intrinsic{I,B}Size.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: pt 4 - Rename the {MIN,PREF}_WIDTH constants used to identify the type of intrinsic inline-size.

Comment 11

[Jonathan Kew [:jfkthame]]

I notice there are a few minor conflicts here with code that's touched in bug 789096 patch 9. So even if reviews are completed, I propose to hold off on landing anything here until that patch is safely in the tree; it'll be trivial to fix/rebase the patches here as needed, given that they're simple search-and-replace changes.

Comment 12

[Simon Montagu :smontagu]

Comment on attachment 8456129 [details] [diff] [review]
pt 2 - More renaming, e.g. where {Min,Pref}Width occurs within longer type and function names.

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

::: layout/base/nsLayoutUtils.cpp
@@ +4413,4 @@
>  
>  /* static */ nscoord
>  nsLayoutUtils::MinWidthFromInline(nsIFrame* aFrame,
>                                    nsRenderingContext* aRenderingContext)

This method name seems to have slipped through the cracks :)

Comment 13

[Jonathan Kew [:jfkthame]]

Rebased and pushed, with the method mentioned in comment 12 added:

https://hg.mozilla.org/integration/mozilla-inbound/rev/beb29103be69
https://hg.mozilla.org/integration/mozilla-inbound/rev/91005c6cc198
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea66ed3c2ca1
https://hg.mozilla.org/integration/mozilla-inbound/rev/4563a79a7968

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/beb29103be69
https://hg.mozilla.org/mozilla-central/rev/91005c6cc198
https://hg.mozilla.org/mozilla-central/rev/ea66ed3c2ca1
https://hg.mozilla.org/mozilla-central/rev/4563a79a7968