735577 - Convert reflow API to use logical not physical terms

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Brian Birtles (:birtles)]

In preparation for implementing vertical text (writing-mode) we want to rename parts of our reflow code to use logical terms where appropriate.

In CSS parsing we'll still have "top" and "bottom" (i.e. physical terms) and in frame classes we also want to use physical coordinates. However, for much of reflow it makes sense to use logical coordinates. For example, when performing border-collapsing we want to deal with "before" and "after" rather than "top" and "bottom" so that when we implement vertical text we can choose the appropriate value.

It's not quite clear exactly how the boundaries will work here. One proposal from fantasai, summarised by dbaron is "fully converting nsHTMLReflowState and nsHTMLReflowMetrics, and adding logical alternative setters/getters to nsIFrame"

Note, however, that there are a lot of special cases. For example, images do not rotate, width remains physical. dbaron has identified the following preliminary list of such cases: "At a first glance these might be nsImageFrame, nsObjectFrame, nsHTMLCanvasFrame, nsVideoFrame, nsImageBoxFrame, nsImageControlFrame, nsProgressFrame (I think), nsGfxCheckboxControlFrame (and probably nsGfxRadioControlFrame too), the interfaces to SVG (nsSVGOuterSVGFrame and nsSVGForeignObjectFrame)"

fantasai has suggested: "This can be thought of as an instance of orthogonal flows: just assume that systems in physical coordinates are horizontal LTR. If it helps, we can have dual name mappings, so physical systems can use continue to use physical names--we just need to be careful to not use those names in logical layout systems since they'll be wrong."

Comment 1

[O. Atsushi (Torisugari)]

(In reply to Brian Birtles (:birtles) from comment #0)
> Note, however, that there are a lot of special cases. For example, images do
> not rotate, width remains physical. dbaron has identified the following
> preliminary list of such cases: "At a first glance these might be
> nsImageFrame, nsObjectFrame, nsHTMLCanvasFrame, nsVideoFrame,
> nsImageBoxFrame, nsImageControlFrame, nsProgressFrame (I think),
> nsGfxCheckboxControlFrame (and probably nsGfxRadioControlFrame too), the
> interfaces to SVG (nsSVGOuterSVGFrame and nsSVGForeignObjectFrame)"

And MathML. Since, for example, |nsBoundingMetrics| is now exclusively used by MathML, it does not seem easy to judge them by C++ class names.

And I personally don't understand the merit of the dual-names plan. Most classes need one absolute position. This information, or we should call it "origin", is just a spot. Rest of information, e.g. width, height, etc. is relative values from the origin. Instead of "horizontal width", we should call it "the distance in the direction parallel to text progression" or "the distance in the direction perpendicular to line progression".

Comment 2

[Brian Birtles (:birtles)]

(In reply to O. Atsushi (Torisugari) from comment #1)
> And MathML. Since, for example, |nsBoundingMetrics| is now exclusively used
> by MathML, it does not seem easy to judge them by C++ class names.

Sorry, I possibly mis-represented dbaron there since he did mention MathML but fantasai responded that MathML should be logical so I removed it from the end of list.

Comment 3

[fantasai]

Attachment: nsWritingModes.h

Here's a first cut at an nsWritingMode struct, which can be used to pass around all the critical data and do rapid lookups of things like IsVertical() and the absolute inline flow direction.

I've also sketched out what logical equivalents of nsPoint, nsSize, nsRect, nsMargin might look like. This part is very incomplete, but you can get the idea.

nsIFrame will need to get some additional methods like
  nsLogicalRect GetLogicalRect(nsWritingMode aWritingmode);
It's important that it not implicitly use its own writing mode; if the parent is looking up it's child's size, for example, it will want the coords in its (the parent's) space, whereas if the child is looking up its own size, it will want the coords in its (the child's) space.

The logical structs will be used in nsHTMLReflowState, nsHTMLReflowMetrics, and all the Reflow methods and their helpers. (The rest of layout/ will stay physical.)

A tricky aspect is incremental logicalization. The suggestion here is to have e.g. nsLogicalRect inherit from nsRect, so that everything will still compile and work, but gradually shift to using logical coords. There are some disadvantages to this: math is less efficient when it has to jump through switch statements, and nsHTMLReflowStatus will be carrying around a lot of redundant nsWritingMode structs. (At least they're only 1 int in size!)

It might be worth completing the conversion of most of layout to use logical geometric methods first, before implementing vertical writing mode support. There's a lot fewer switches (only the start/end ones, and a lot of places that operate in that axis already have an explicit if/else to check for bidi, so no perf hit plus we get a maintainability benefit). Then we can switch over to using native-logical geometric classes before we implement writing-mode, which would otherwise invoke 3/4-way switch clauses for almost every geometric operation.

An open question is if we want two variants of the logical rects, one natively using flow-relative coords in the block axis (increase towards logical bottom of the block), and one natively using line-relative coords in the block axis (increase towards logical bottom of the line). In the block axis, line-relative coords are sometimes the same and sometimes opposite the flow-relative coords. Flow-relative coords are used for most of layout, but within a line box, layout is line-relative. This confusing case is triggered when 'writing-mode: vertical-lr' and 'text-orientation' has its initial value.

Thoughts?

Comment 4

[Simon Montagu :smontagu]

Attachment: w-i-p patch #1: fixes to build errors in nsWritingModes.h

Comment 5

[Simon Montagu :smontagu]

Attachment: w-i-p patch #2: Update nsWritingMode to use writing mode from style system

Comment 6

[Simon Montagu :smontagu]

Attachment: w-i-p patch #3: Fill out the writing mode API more

Comment 7

[Simon Montagu :smontagu]

Attachment: w-i-p patch #4: Logicalize more layout headers

Comment 8

[fantasai]

Comment on attachment 767640 [details] [diff] [review]
w-i-p patch #1: fixes to build errors in nsWritingModes.h

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

Noticing the typo I made... and wondering if maybe GetInlineDir is clearer than GetTextDir? What do you think? Otherwise looks good to me. r+ ;)

Comment 9

[fantasai]

Oh, and I'm thinking we should call nsLogicalRect as nsFlowCoordRect, to be clearer about the coordinate system it's in. Also, in case we need nsLineCoordRect, which I'm suspecting (though not yet 100% certain) that we will...

Comment 10

[fantasai]

Comment on attachment 767642 [details] [diff] [review]
w-i-p patch #2: Update nsWritingMode to use writing mode from style system

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

+    switch (aWritingStyleContext->mWritingMode) {
+
+      case NS_STYLE_WRITING_MODE_HORIZONTAL_TB:
+      default:
+        mWritingMode =
+          (NS_STYLE_DIRECTION_RTL == aWritingStyleContext->mDirection) ?
+            eInlineFlowMask & eBidiMask : 0;
+
+      case NS_STYLE_WRITING_MODE_VERTICAL_LR:
+        mWritingMode = eBlockFlowMask & eOrientationMask;
+
+      case NS_STYLE_WRITING_MODE_VERTICAL_RL:
+        mWritingMode = eOrientationMask;

We can have RTL in vertical text. So it should express the following:

  mWritingMode |= eOrientationMask if (writing-mode: vertical-lr | vertical-rl);
  mWritingMode |= eInlineFlowMask if (direction: rtl); //XXX needs update when text-orientation added
  mWritingMode |= eBlockFlowMask if (writing-mode: vertical-lr);
  mWritingMode |= eLineOrientMask if (writing-mode: vertical-lr); //XXX needs update when text-orientation added
  mWritingMode |= eBidiMask if (direction: rtl);

r- on this one

Comment 11

[fantasai]

Comment on attachment 767643 [details] [diff] [review]
w-i-p patch #3: Fill out the writing mode API more

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

AdjustBSizeBy() ... Maybe say Increase or Grow rather than Adjust, just to be clear it's adding aDelta?

+  nscoord IStartEnd() const { return LeftRight(); }
+  nscoord BBeforeAfter() const { return TopBottom(); }

These need to check the writing mode, since they swap in vertical text, right? ;)

Also s/BeforeAfter/StartEnd/. Likewise for BBefore() and BAfter().

+  nscoord& BAfter()
+  {
+    switch (mWritingMode.GetBlockDir()) {
+      case nsWritingMode::eBlockTB:
+      default:
+        return bottom;
...
+  void AdjustIStartBy(nscoord aDelta) { IStart() += aDelta; }

Wait, if we can do this kind of magic, why have Get/Set/Adjust at all? Seems better to just expose IStart() & friends.

Also, you need to add GetPhysical*() and ConvertTo(nsWritingMode outMode) methods to these structures.

Comment 12

[fantasai]

Comment on attachment 767642 [details] [diff] [review]
w-i-p patch #2: Update nsWritingMode to use writing mode from style system

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

Oh, also, on this patch... nsLogicalRect's methods need to check mWritingMode where currently they're not, because I optimized it for what we have implemented currently (no vertical text).

As I wrote in comment 3, though, I think it might be best to do logicalization assuming no support for vertical text, move over to flow-coord-native geometric structures, and then add in support for vertical. Otherwise we have to take the perf hit of every operation in our reflow arithmetic falling into a case switch. Which is why I commented out the checks for vertical modes in my sketch of nsLogicalRect::IStart().

Comment 13

[fantasai]

Comment on attachment 767644 [details] [diff] [review]
w-i-p patch #4: Logicalize more layout headers

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

+  nsLogicalMargin LogicalBorderPadding() const {
+    return nsLogicalMargin(0, BorderPadding());
+  }

We want nsBlockReflowState to be flow-coord-native. Is it possible to have an mFlowCoordBorderPadding member with mBorderPadding being a reference to its internal nsMargin? That would be best. Conversion would look like
  1. Add mFlowCoordBorderPadding with mBorderPadding referencing its internal nsMargin (guaranteed to be in sync).
  2. Convert layout to use mFlowCoordBorderPadding instead of mBorderPadding.
  3. Remove mBorderPadding.

We might then consider either removing "FlowCoord" or shortening it to FC or something else. (FC could be confused with float continuations, though, so may want to consider a longer name for those if we go that route.)

Similarly, for nsFlowAreaRect, I think we want to pass in the nsWritingMode to the constructor and make mRect be an nsFlowCoordRect. I'd suggest doing this as a separate patch, where we update all of the interactions with nsFlowAreaRect to be in logical coords.

If we can do reference magic on nsHTMLReflowMetrics.h, let's do that there, too, and drop Set*().
  nscoord& ISize() { return width; }

   // Callers using this constructor must call InitOffsets on their own.
   nsCSSOffsetState(nsIFrame *aFrame, nsRenderingContext *aRenderingContext)
     : frame(aFrame)
     , rendContext(aRenderingContext)
+    , mWritingMode(0)

Seems to me mWritingMode should be a required parameter. No? Otherwise we'll forget to handle it in places.
Also, I don't think nsWritingMode has a constructor that takes an int, so while this will compile, I expect it'll die on a null dereference as soon as you run it.

+  nscoord ComputeInlineSizeValue(nscoord aContainingBlockInlineSize,

I think we should be consistent about using InlineSize or ISize everywhere. (I went with ISize because it's shorter, and we're going to be using these such terms everywhere, so it'll really make a difference on how many awkward mid-statement line breaks we have to deal with.)

+  nsLogicalRect GetLogicalRect() const {
+    return nsLogicalRect(nsWritingMode(StyleVisibility()), mRect);
+  }
+  nsLogicalSize GetLogicalSize() const {
+    return nsLogicalSize(nsWritingMode(StyleVisibility()), GetSize());
+  }

As I mentioned in comment 3, it's very important that we require the caller to pass in the writing mode, and not assume our own. It needs to pass in the writing mode of the coordinate system it's working in, which may or may not match the writing mode of this frame!!!!

We need to do everything we can to prevent people from trying to do arithmetic with logical structs that are in different coord spaces, i.e. that derive from different writing modes. A given function should only ever do arithmetic using a single nsWritingMode. Requiring an explicit style lookup for every acquisition of a different writing mode adds some friction against doing that; whereas folding it into a function call makes it really easy to screw up.

Comment 14

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

(In reply to fantasai from comment #13)
> +  nsLogicalRect GetLogicalRect() const {
> +    return nsLogicalRect(nsWritingMode(StyleVisibility()), mRect);
> +  }
> +  nsLogicalSize GetLogicalSize() const {
> +    return nsLogicalSize(nsWritingMode(StyleVisibility()), GetSize());
> +  }
> 
> As I mentioned in comment 3, it's very important that we require the caller
> to pass in the writing mode, and not assume our own. It needs to pass in the
> writing mode of the coordinate system it's working in, which may or may not
> match the writing mode of this frame!!!!

I disagree with this.  I think the main use case for GetLogicalRect should be in the *parent's* logical writing mode, since it's in the parent's coordinate space.  On the other hand, I think the main use case for GetLogicalSize is in the frame's writing mode.  I'm not sure if it would be confusing to have this distinction without an additional naming difference between the functions, though.

> We need to do everything we can to prevent people from trying to do
> arithmetic with logical structs that are in different coord spaces, i.e.
> that derive from different writing modes. A given function should only ever
> do arithmetic using a single nsWritingMode. Requiring an explicit style
> lookup for every acquisition of a different writing mode adds some friction
> against doing that; whereas folding it into a function call makes it really
> easy to screw up.

We already have this problem with origins; I don't think writing modes make it significantly worse.  If anything, I think we should use the type system for this; I think requiring extra arguments just makes the code harder to write.

Comment 15

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

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #14)
> We already have this problem with origins; I don't think writing modes make
> it significantly worse.  If anything, I think we should use the type system
> for this; I think requiring extra arguments just makes the code harder to
> write.

To expand on this a little:  we frequently use coordinates relative to either the border-box or the padding-box of a particular frame.  We already need to be careful about what these are relative to.  I don't think that something that ensures we track writing mode correctly is worth the overhead in the long run, since it still doesn't address the slightly more complex problem of tracking which frame we're relative to.  I'd be more open to something that was temporary, and would help us do the conversion correctly, and then go away.  But ideally I'd love to have a way to have the compiler catch such mistakes using the type system (as we're doing more now for whether units are CSS pixels, device pixels, image pixels, etc.), though I don't see a way to do that.

On the other hand, I think it may well be worth using the type system to have the compiler enforce that we can't cross logical and physical coordinates.  (The current patch series prior to patch 3 does this only because it omits the "public" or "private" the declaration of the base class of nsLogicalRect, etc., and thus (for 'class') defaults to "private".  But I have no idea if that was intentional, and I suspect it wasn't.  Either way, that should *never* be omitted.  Then patch 3 changes these to struct and thus changes the default to "public".)  This might be done by using a type distinctly derived from BaseRect, or (though that's probably a bad idea) explicitly using private inheritance.

Comment 16

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

Also, roc reminded me that we have nsIFrame::GetContentRectRelativeToSelf.  That provides a symmetric way to expand to logical rects:
  nsIFrame::GetLogical{,Padding,Content}Rect are in the parent's writing mode
  nsIFrame::GetLogicalContentRectRelativeToSelf is in the frame's writing mode
(I wonder if we should use LogicalBorderRect rather than LogicalRect?  Probably not.)

And, again, I think it's important that the nsLogicalRect and nsRect types can't be implicitly converted from one to the other in either direction (and same for point, margin, size, etc.).

Comment 17

[Simon Montagu :smontagu]

Attachment: w-i-p patch #2 v.2: Update nsWritingMode to use writing mode from style system

Comment 18

[Simon Montagu :smontagu]

Attachment: w-i-p patch #3 v.2: Fill out the writing mode API more

Comment 19

[Simon Montagu :smontagu]

Attachment: w-i-p patch #4: Logicalize nsIFrame

This and the others attached today are still very w-i-p, but I've tried to adopt the suggestions in comment 13 and comment 15-16 particularly wrt to the conversion process.

Comment 20

[Simon Montagu :smontagu]

Attachment: w-i-p patch #4b: Use GetUsed{Margin|Border|Padding} in layout

Comment 21

[Jonathan Kew [:jfkthame]]

Attachment: Revised version of nsWritingModes.h

This is a reworking of nsWritingModes.h to move away from letting the nsLogical{Point,Size,Margin,Rect} types inherit from their physical-coordinate counterparts. This is very much a W-I-P; for one thing, I'm sure there are lots more methods that we'll want to expose on the logical classes and call through to the appropriate methods on their ns{Point,Size,Margin,Rect} members. However, comments on the general direction here would be very welcome.

I'm tagging smontagu and dbaron for feedback? at this time, but others are invited to chime in.

Comment 22

[Jonathan Kew [:jfkthame]]

BTW, I've been trying this version of nsWritingModes.h together with Simon's patches from http://smontagu.org/mozilla/verticalText/ dated July 30th, plus some minor fixups. So far, tests are looking good with patches 4a ("Logicalize nsIFrame.h") and 4b ("Implement GetUsed{Margin|Border|Padding} in layout"):

  https://tbpl.mozilla.org/?tree=Try&rev=648c5bc1435c

but with 4c ("Implement GetLogicalRect/Position/Size in layout") added, I'm getting a few test failures, so still need to track those down:

  https://tbpl.mozilla.org/?tree=Try&rev=c2061be8911c

Simon, do you have updated versions of patches 4a-4c that I should be using, by any chance?

Comment 23

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #22)
> Simon, do you have updated versions of patches 4a-4c that I should be using,
> by any chance?

I've updated the versions on http://smontagu.org/mozilla/verticalText/ but I don't think there are any radical differences since the last uplift, only more source files patched.

Comment 24

[Jonathan Kew [:jfkthame]]

Attachment: Updated version of nsWritingModes.h

Here's a further update to nsWritingModes.h. Tryserver run with this plus Simon's patches 4a-4c, and a few additional fixes (some minor adjustments to account for nsWritingModes.h API changes, plus a few bug-fixes in the layout logicalization):
  https://tbpl.mozilla.org/?tree=Try&rev=446b19aa7364

This should be down to no more than a couple of test failures at most, I hope, based on my local testing.

If this looks good, I'll clean up a bit more and post a patch for review soon, but preliminary feedback would be welcome already.

Comment 25

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - introduce nsWritingMode and logical-coordinate point/size/margin/rect classes

Here's a somewhat tidied-up version of nsWritingModes.h. It'll need additional methods to be implemented as we proceed with converting layout classes, I'm sure, but should work as a starting point. This should be functionally the same as the version that passed tryserver in comment 24 together with smontagu's patches 4a-4c, just with some internal cleanup.

Comment 26

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

Comment on attachment 790988 [details] [diff] [review]
pt 1 - introduce nsWritingMode and logical-coordinate point/size/margin/rect classes

For the include guards, use one trailing _ rather than three.  (Two or
more _s is technically reserved for the implementation.)

>+// If WRITING_MODE_TRANSITIONAL_CODE is TRUE, we will have constructors and
>+// typecast operators that allow certain implicit conversions between logical-
>+// and physical-coordinate types. If FALSE (or undefined), these conversions
>+// will not be allowed, and will generate compile-time errors; callers should
>+// use explicit constructors and conversion methods for code clarity.

Hmmm.  I dislike the "TRUE" and "FALSE" in all-caps, since we don't use
TRUE and FALSE macros.  Similar for the next few paragraphs.  Could you
write this using constructs like "When using
WRITING_MODE_TRANSITIONAL_CODE, ..."?

>+#ifndef WRITING_MODE_TRANSITIONAL_CODE
>+#  define WRITING_MODE_TRANSITIONAL_CODE 1
>+#endif

Skip the #ifndef, and outdent.  It's not ok for these to be defined
anywhere else.

(And same around ASSERT_ON_WRITING_MODE_MISMATCH.)

>+  /***********************************************************
>+   * This is an immutable class representing a writing mode. *
>+   * It efficiently stores the writing mode and can rapidly  *
>+   * compute interesting things about it for use in layout.  *
>+   *                                                         *
>+   * Writing modes are computed from the CSS 'direction',    *
>+   * 'writing-mode', and 'text-orientation' properties.      *
>+   * See CSS3 Writing Modes for more information             *
>+   *   http://www.w3.org/TR/css3-writing-modes/              *
>+   ***********************************************************/

Move this above "class nsWritingMode", and switch to doc-comment style as in
https://hg.mozilla.org/mozilla-central/file/b7f636fada9f/layout/style/nsRuleNode.h#l190

>+  /**
>+   * Return true if LTR. (Convenience method)
>+   */
>+  bool IsLTR() const { return eBidiLTR == (mWritingMode & eBidiMask); }

This seems a little dangerous in terms of naming?  Maybe IsBidiLTR?

>+  /**
>+   * Block-axis flow-relative to line-relative factor
>+   */
>+  int F2L() const { return (mWritingMode & eLineOrientMask) ? -1 : 1; }

This seems extremely cryptic to me.  I don't actually know what the "F"
and "L" stand for.  Could it have a better name?

(Is the intent that it be used in multiplication?  If not, why are the
return values 1 and -1?)

Also, maybe it should call IsLineInverted() to make it clearer that anybody
looking for a boolean should just call IsLineInverted, rather than testing
this result against 1 or -1.

>+  nsWritingMode(const nsStyleVisibility* aWritingStyleContext)

aWritingStyleContext -> aStyleVisibility

And please do *not* null-check aStyleVisibility, and outdent.

I think the switch would be clearer without a fall-through, and just
using | rather than |=, and with all the values of writing mode explicitly
written (even if some are the same as default).  (And the default case
should really assert.)

>+/******************************************************************************
>+ * There are three sets of coordinate space:                                  *

No boxes of *s around the comments, please.

>+  nscoord& I() { return mPoint.x; }
>+  nscoord& B() { return mPoint.y; }

// inline-axis and // block-axis comments here would be useful.
(and in the other classes, with appropriate replacement of axis for
dimension or size)

>+  void MoveEndward(nscoord aDeltaI, nscoord aDeltaB)

I have mixed feelings about this name (whether we should use MoveBy)
instead, but inclined for now to think it's ok.  If we dislike it it
should be easy to change later due to its uniqueness.

>+  nsLogicalPoint operator+ (const nsLogicalPoint& aOther)

No space between function name and (.
(occurs later too, including in type conversion operators)

>+#if WRITING_MODE_TRANSITIONAL_CODE

Prefer #ifdef over #if (local style).

(throughout)

nsLogicalMargin is missing non-const accessors.


I wonder if you can get away without the WRITING_MODE_TRANSITIONAL_CODE
in nsLogicalPoint::operator+, nsLogicalRect::operator+, +=, -, and -=,
MoveBy, Inflate, and Deflate.  (They're all assertion-guarded, I think,
though I didn't check closely.)


r=dbaron with that

Comment 27

[Jonathan Kew [:jfkthame]]

smontagu: I'll post an updated nsWritingModes.h patch soon taking dbaron's comments into account, but would also appreciate hearing any feedback you have on this, as it underlies all the other patches you're working on.

Oops - also, just noticed that using bzexport had reassigned this bug to me; sorry, didn't mean to steal it! Reverting that.

Comment 28

[Simon Montagu :smontagu]

Comment on attachment 789792 [details]
Updated version of nsWritingModes.h

What feedback do you want from me? If I encounter things that I think need to change for future work I'll file bugs and change them. On the general design I'll defer to dbaron and fantasai anyway

Comment 29

[Jonathan Kew [:jfkthame]]

OK, that's fine - I wanted to check whether you had any concerns about the direction this was going, given that you're the one who has been writing the patches that will actually begin to use this.

I expect we'll want to extend the logical-class APIs to provide additional convenience methods; we can do that on an as-needed basis, I think.

Comment 30

[Jonathan Kew [:jfkthame]]

(In reply to David Baron [:dbaron] (needinfo? me) from comment #26)

> >+  /**
> >+   * Block-axis flow-relative to line-relative factor
> >+   */
> >+  int F2L() const { return (mWritingMode & eLineOrientMask) ? -1 : 1; }
> 
> This seems extremely cryptic to me.  I don't actually know what the "F"
> and "L" stand for.  Could it have a better name?

How about FlowRelativeToLineRelativeFactor()? Verbose, I know, but any code that intends to make repeated use of it should assign it to a local variable anyway.


> (Is the intent that it be used in multiplication?  If not, why are the
> return values 1 and -1?)

That was my assumption. (This was inherited from earlier iterations of nsWritingModes.h, but it seemed reasonable to me. We've got similar usages of a "direction" value that is set to either 1 or -1 in some bidi code, for example.)


> I wonder if you can get away without the WRITING_MODE_TRANSITIONAL_CODE
> in nsLogicalPoint::operator+, nsLogicalRect::operator+, +=, -, and -=,
> MoveBy, Inflate, and Deflate.  (They're all assertion-guarded, I think,
> though I didn't check closely.)

I'd prefer to leave this in place for now, if you're willing; the idea is that with WRITING_MODE_TRANSITIONAL_CODE enabled, these classes will attempt to paper over places where the code hasn't been fully converted yet, and incorrectly mixes physical and logical forms. We'd need to turn off the assertions (making them fall back to NS_WARNING) if we land such partially-converted code in the tree, so as to keep tests green, and in that case we'd want these functions to do the appropriate conversions for us.

Once layout is fully converted, and builds and passes tests even with WRITING_MODE_TRANSITIONAL_CODE disabled, we'll rip this stuff out and require that future code is written with clean logical-coordinate support from the start. The assertions will remain in place (non-optionally!) so as to catch future errors.

Comment 31

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - introduce nsWritingMode and logical-coordinate point/size/margin/rect classes.

Updated as per comment 26, modulo comment 30; carrying forward r=dbaron.

Comment 32

[Simon Montagu :smontagu]

Attachment: Rename existing nsIFrame methods

We thought that it would make more sense to add a step before patch #4. This renames existing nsIFrame methods that we are going to add "logical" versions of to "physical".

The patch was produced by running the following command in layout and accessible, with some manual editing to add missing cases in widget, dom and content and remove false positives:

find . -path '*/.hg*' -prune -o -name \*.cpp -print -o -name \*.h -print | xargs sed -i -e "s/[[:<:]]GetRect[[:>:]]/GetPhysicalRect/g
s/[[:<:]]GetUsedMargin[[:>:]]/GetUsedPhysicalMargin/g
s/[[:<:]]GetUsedBorder[[:>:]]/GetUsedPhysicalBorder/g
s/[[:<:]]GetUsedPadding[[:>:]]/GetUsedPhysicalPadding/g
s/[[:<:]]GetUsedBorderAndPadding[[:>:]]/GetUsedPhysicalBorderAndPadding/g
s/[[:<:]]GetPosition[[:>:]]/GetPhysicalPosition/g
s/[[:<:]]GetSize[[:>:]]/GetPhysicalSize/g
s/[[:<:]]GetBaseLine[[:>:]]/GetPhysicalBaseline/g
s/[[:<:]]GetNormalPosition[[:>:]]/GetNormalPhysicalPosition/g"

Comment 33

[Simon Montagu :smontagu]

Attachment: Rename existing nsIFrame methods v.2

Updated to current trunk and added a couple more changes:
GetPaddingRect -> GetPhysicalPaddingRect
GetBaseline -> GetPhysicalBaseline

Comment 34

[Simon Montagu :smontagu]

Attachment: Add logical nsIFrame methods

Comment 35

[Simon Montagu :smontagu]

Attachment: Use GetUsedLogical{Margin|Border|Padding} in layout

Comment 36

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

(In reply to Simon Montagu :smontagu from comment #33)
> GetBaseline -> GetPhysicalBaseline

I'm a little bit queasy about "GetPhysicalBaseline"; baselines seem like they're more complicated than just having a physical/logical variant, perhaps?  Though maybe not.  What would the distinction be, exactly?

Comment 37

[Jonathan Kew [:jfkthame]]

Comment on attachment 812223 [details] [diff] [review]
Add logical nsIFrame methods

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

A couple of drive-by comments...

::: layout/generic/nsIFrame.h
@@ +852,5 @@
> +  }
> +  nsLogicalPoint GetLogicalPosition(nsWritingMode aWritingMode) const {
> +    nsLogicalRect logicalRect = GetLogicalRect(aWritingMode);
> +    return nsLogicalPoint(aWritingMode,
> +                          logicalRect.IStart(), logicalRect.BStart());

nsWritingModes.h defines the Origin() method on nsLogicalRect, so this can be simplified to
  return GetLogicalRect(aWritingMode).Origin();

@@ +854,5 @@
> +    nsLogicalRect logicalRect = GetLogicalRect(aWritingMode);
> +    return nsLogicalPoint(aWritingMode,
> +                          logicalRect.IStart(), logicalRect.BStart());
> +  }
> +  nsSize GetLogicalSize(nsWritingMode aWritingMode) const {

Shouldn't this return an nsLogicalSize?

@@ +856,5 @@
> +                          logicalRect.IStart(), logicalRect.BStart());
> +  }
> +  nsSize GetLogicalSize(nsWritingMode aWritingMode) const {
> +    nsLogicalRect logicalRect = GetLogicalRect(aWritingMode);
> +    return nsSize(logicalRect.ISize(), logicalRect.BSize());

There's also a Size() method on nsLogicalRect, so you can just do
  return GetLogicalRect(aWritingMode).Size();
here.

Comment 38

[Jonathan Kew [:jfkthame]]

Attachment: proposed new version of WritingModes.h

Here is a proposed rewrite of WritingModes.h, based on ideas discussed in Paris. This changes the API of the logical-geometry classes significantly, so will require updates to all the patches built on top of this, but I think it's a firmer foundation.

Some key updates in this version:

- There's a preprocessor symbol WRITING_MODE_VERTICAL_ENABLED; if this is undefined, then WritingMode.IsVertical() is hardcoded to false, etc., so that a lot of the potential extra branches involved in checking for vertical mode should be optimized away at compile time.

- All methods on Logical{Point,Size,Margin,Rect} classes take a WritingMode parameter. In DEBUG builds, they use this to check that modes are not being incorrectly mixed. This should help to ensure correctness, but it adds "clutter" at every call site; perhaps we will want to drop this from at least the simplest methods (e.g. logical-coordinate accessors) eventually. All methods that depend internally on the writing mode (e.g. physical-coord conversions) will need to keep this parameter, though.

- In non-DEBUG builds, the Logical* classes don't actually store their writing mode; it's assumed that the caller is combining them correctly, converting modes where necessary, etc. Therefore, the compiler should be able to completely optimize away the WritingMode parameter from the simple (logical-geometry) methods that don't actually use it.

- LogicalPoint and LogicalRect methods that may need to convert LTR/RTL take an additional parameter, the "container width" of the coordinate space being used. This allows us to address the fact that the origin for RTL layout purposes is in effect moved to the right-hand edge of the rect.

I'm still unsure, though, whether this last item is the best way to handle the issue, or whether we should simply negate the x-coordinates when switching directions, and leave the frame to explicitly deal with the origin translation outside these classes.

Early feedback welcome; I'm not posting actual patches for review until I have more of the dependent patches updated to a buildable state again.

Comment 39

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - introduce mozilla::WritingMode and logical-coordinate Point/Size/Margin/Rect classes.

OK, here's a new version of WritingModes.h that I think we can try to work with. Key points here: (a) The caller must always pass the appropriate WritingMode to logical-coordinate classes, although many methods will only use this for DEBUG-build checks, so it should be optimized away in release builds. (b) As long as WRITING_MODE_VERTICAL_ENABLED is not defined, IsVertical() is hardcoded to false, eliminating many of the conditionals in accessor functions, so the overhead for switching to logical classes should be minimal. For now, we can #define this locally for testing purposes in our builds. (c) All methods that convert horizontal coordinates between logical and physical space need to be passed a 'container width' parameter that defines the width of the coordinate space for bidi reflection purposes. Does this seem like a workable setup?

Comment 40

[Jonathan Kew [:jfkthame]]

Comment on attachment 8344686 [details] [diff] [review]
pt 1 - introduce mozilla::WritingMode and logical-coordinate Point/Size/Margin/Rect classes.

:dbaron, this is significantly changed from the version you reviewed previously, so if you're able to have another look at it I'd be appreciative!

(Oops, I just noticed a stray debugging printf in there - will clean that up.)

Comment 41

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - add WritingMode() method to nsIFrame.

This is trivial, but seems like a separate piece.

Comment 42

[Jonathan Kew [:jfkthame]]

Attachment: pt 3.1 - make width/height/offset/margin fields in nsHTMLReflowState private, and provide accessor functions.

To begin the conversion process for nsHTMLReflowState, this just makes various 'physical' fields private, and adds accessor functions. Note that this patch will not compile alone; it requires the following patch to also be applied.

Comment 43

[Jonathan Kew [:jfkthame]]

Attachment: pt 3.2 - update the rest of layout code to use the new accessors on nsHTMLReflowState

Here are the corresponding updates to the rest of the code, mostly the result of running a bunch of 'sed' changes over the entire layout/ tree. Patches 3.1 + 3.2 should have no net effect on the build, but provide encapsulation that we'll need to begin the actual conversion within nsHTMLReflowState.

Comment 44

[Jonathan Kew [:jfkthame]]

Attachment: pt 3.3 - provide logical-coordinate APIs in nsHTMLReflowState, parallel to the existing physical APIs

The beginnings of logical-coordinate API for reflow-state, based on the frame's writing-mode. (Nothing actually uses this at this stage.)

Comment 45

[Simon Montagu :smontagu]

Comment on attachment 8344686 [details] [diff] [review]
pt 1 - introduce mozilla::WritingMode and logical-coordinate Point/Size/Margin/Rect classes.

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

::: layout/generic/WritingModes.h
@@ +420,5 @@
> +/* XXX I don't like this name; it's not clear (to me) what "Endward" means
> + * in relation to bidi-rtl directionality.
> + * Let's see whether we really need something like this...
> + */
> +  void MoveEndward(WritingMode aWritingMode, nscoord aDeltaI, nscoord aDeltaB)

right, I don't think we do either :)

@@ +1070,5 @@
> +
> +  // When setting the Width of a rect, we keep its physical X-coord fixed
> +  // and modify XMax. This means that in the RTL case, we'll be moving
> +  // the IStart, so that IEnd remains constant.
> +  void SetWidth(WritingMode aWritingMode, nscoord aWidth)

I'm not sure about this (but willing to be convinced). Obviously whichever way we do it is going to have some upsides and some downsides, but I think I prefer keeping the IStart constant in both RTL and LTR

Comment 46

[Simon Montagu :smontagu]

Comment on attachment 8344716 [details] [diff] [review]
pt 3.2 - update the rest of layout code to use the new accessors on nsHTMLReflowState

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

rs=me (I haven't read it from alpha to omega, since it's a sed patch)

Comment 47

[Simon Montagu :smontagu]

Comment on attachment 8344748 [details] [diff] [review]
pt 3.3 - provide logical-coordinate APIs in nsHTMLReflowState, parallel to the existing physical APIs

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

::: layout/generic/nsHTMLReflowState.h
@@ +151,1 @@
>    {

I'm not sure if we can get away with taking the writing mode from aFrame rather than adding it as a constructor argument -- especially in the child frame reflow constructor it may need to be the parent frame's writing mode, no?

Comment 48

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #45)
> Comment on attachment 8344686 [details] [diff] [review]
> pt 1 - introduce mozilla::WritingMode and logical-coordinate
> Point/Size/Margin/Rect classes.

> @@ +1070,5 @@
> > +
> > +  // When setting the Width of a rect, we keep its physical X-coord fixed
> > +  // and modify XMax. This means that in the RTL case, we'll be moving
> > +  // the IStart, so that IEnd remains constant.
> > +  void SetWidth(WritingMode aWritingMode, nscoord aWidth)
> 
> I'm not sure about this (but willing to be convinced). Obviously whichever
> way we do it is going to have some upsides and some downsides, but I think I
> prefer keeping the IStart constant in both RTL and LTR

If the caller is working with logical coordinates, it will modify the ISize, not the Width; then IStart will indeed remain constant in any writing mode (and so IEnd will move).

But for not-yet-logicalized code that is manipulating the (logical) rect via its physical accessors (X, Width, etc), the expected behavior should match what a physical nsRect would do: i.e. calling SetWidth() leaves X() unchanged and modifies XMax().

Comment 49

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #47)
> Comment on attachment 8344748 [details] [diff] [review]
> pt 3.3 - provide logical-coordinate APIs in nsHTMLReflowState, parallel to
> the existing physical APIs
> 
> Review of attachment 8344748 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsHTMLReflowState.h
> @@ +151,1 @@
> >    {
> 
> I'm not sure if we can get away with taking the writing mode from aFrame
> rather than adding it as a constructor argument -- especially in the child
> frame reflow constructor it may need to be the parent frame's writing mode,
> no?

It's not totally clear to me yet. But if parent and child writing modes differ, won't the child frame want the reflow state to work in terms of -its- writing mode, in order to reflow its own contents appropriately?

The child will also end up returning reflow metrics in terms of its writing mode (not its parent's); the parent will then need to convert this to its own writing mode in order to compute its own size, place the child, etc.

Comment 50

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - introduce mozilla::WritingMode and logical-coordinate Point/Size/Margin/Rect classes.

Tidied up WritingModes.h a little, removing some (commented-out) APIs that we probably don't want anyhow. Carrying forward r=smontagu; dbaron, if you have time to look this over as well, I'd appreciate hearing any input or concerns before we land it.

Comment 51

[Jonathan Kew [:jfkthame]]

Flagging needinfo? on dbaron (see comment 50). If you're too busy to look at this for now, please let me know; in that case, I think we should get it into the tree anyhow as a starting-point, and continue to iterate as needed after the initial landing.

Comment 52

[Jonathan Kew [:jfkthame]]

Attachment: pt 4.1 - use accessor functions for some physical-coordinate fields in nsHTMLReflowMetrics, in preparation for logicalization

First step for nsHTMLReflowMetrics: make the width/height/ascent fields private, and use accessors. For ascent, I've called the accessors [Set]TopAscent, as once we have vertical support we'll potentially have an 'ascent'-like concept on the left or right instead, depending on the writing mode; so for physical accessors, we'll need to clarify which side of the metrics box the 'ascent' applies to. (I'm assuming any given frame will only expose an 'ascent' value for one side - whichever its block-direction starts from - so we'll only need a single field in the reflowMetrics structure, but which side it represents will depend on the writing mode.)

Comment 53

[Jonathan Kew [:jfkthame]]

Attachment: pt 4.2 - convert width and height fields in nsHTMLReflowMetrics to logical coordinates

This converts the reflow-metrics class to use logical instead of physical coords, mapping them to the physical accessors appropriately depending on its writing mode. By itself, this should have no effect - and as long as vertical support is not enabled in WritingModes.h, there should be no runtime cost, as IsVertical() is a compile-time constant - but it means we can start to use the logical APIs when we begin converting the frame classes' Reflow methods.

Comment 54

[Simon Montagu :smontagu]

Comment on attachment 8344748 [details] [diff] [review]
pt 3.3 - provide logical-coordinate APIs in nsHTMLReflowState, parallel to the existing physical APIs

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

::: layout/generic/nsHTMLReflowState.h
@@ +124,5 @@
> +  LogicalMargin ComputedLogicalPadding() const
> +    { return LogicalMargin(mWritingMode, mComputedPadding); }
> +
> +  WritingMode GetWritingMode() const { return mWritingMode; }
> +

Nit: can you make this, and other accessors, just WritingMode()?

Comment 55

[Simon Montagu :smontagu]

I have a couple of comments after beginning to experiment with using the logical API.

If the logical structs aren't going to store the writing mode, a lot of the time we won't be gaining anything by storing the coordinates either, and it could be more efficient to have static conversion methods rather than having to create an ad hoc logical struct and converting to physical.

Comment 56

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #54)

> Nit: can you make this, and other accessors, just WritingMode()?

We could (that's how I originally wrote it), but this leads to a few cases where the compiler needs help with the ambiguity between WritingMode (the type) and WritingMode (the method), resulting in compile errors such as

 0:11.67 ../../dist/include/WritingModes.h:475:3: error: must use 'class' tag to refer to type 'WritingMode' in this scope
 0:11.67   WritingMode mWritingMode;
 0:11.67   ^
 0:11.67   class

...

 1:04.04 layout/generic/nsFrame.cpp:7460:11: error: must use 'class' tag to refer to type 'WritingMode' in this scope
 1:04.04     const WritingMode wm = aState.OuterReflowState() ?
 1:04.04           ^
 1:04.04           class
 1:04.05 layout/generic/nsFrame.cpp:7648:9: error: must use 'class' tag to refer to type 'WritingMode' in this scope
 1:04.05   const WritingMode wm = aState.OuterReflowState() ?
 1:04.05         ^
 1:04.05         class

So we'd need to fix these cases by explicitly writing "class WritingMode" or using "mozilla::WritingMode", etc.

It seemed to me it'll be simpler for authors if we consistently name the accessors "GetWritingMode" and avoid this issue altogether, rather than requiring the extra syntax in certain cases. But we could go either way - WDYT? dbaron, any opinion?

Comment 57

[Jonathan Kew [:jfkthame]]

Attachment: [sed patch] replace GetWritingMode() with WritingMode() throughout.

This is a sed-generated patch that simply replaces all the GetWritingMode() accessors with WritingMode(). By itself, this won't compile; see the following patch for fixups.

Comment 58

[Jonathan Kew [:jfkthame]]

Attachment: [fixups] manual tweaks to get the WritingMode()-accessor patch to actually compile

And here are the fixes I had to do in order to actually compile with the WritingMode() accessors - mainly adding a few explicit 'class' tags. Do we want to go this way, or keep the GetWritingMode() name and avoid the occasional ambiguities?

Comment 59

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #55)
> I have a couple of comments after beginning to experiment with using the
> logical API.
> 
> If the logical structs aren't going to store the writing mode, a lot of the
> time we won't be gaining anything by storing the coordinates either, and it
> could be more efficient to have static conversion methods rather than having
> to create an ad hoc logical struct and converting to physical.

Yes, in some cases that might be the most sensible approach. It may depend how frequently we end up needing to do logical/physical conversions once the migration is completed.

Perhaps we should add a collection of static conversion methods for now, at least, and then see what usage patterns are most frequent as we go forwards. Right now we're likely to have a lot of "mixed" cases where we're just beginning to adapt code, and often interfacing with old, physical-coord code and having to do conversions, but that may become less common once we're further along and can stay in logical coordinates more of the time.

Comment 60

[Jonathan Kew [:jfkthame]]

Pushed the reviewed patches to inbound, in preference to letting them bit-rot. We can still consider whether to do the s/GetWritingMode()/WritingMode()/ change for all the accessors (comment 54, comments 56-68).

dbaron, smontagu: do you want us to pursue that, or stay with the Get... variant? See discussion above. Leaving this bug open until we decide which way to go there.

https://hg.mozilla.org/integration/mozilla-inbound/rev/865efac0aafc - pt1
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3bccb687bd0 - pt2
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcba7f58a8e7 - pt3.1
https://hg.mozilla.org/integration/mozilla-inbound/rev/c73264fcffc3 - pt3.2
https://hg.mozilla.org/integration/mozilla-inbound/rev/10592ba2f695 - pt3.3
https://hg.mozilla.org/integration/mozilla-inbound/rev/72591e9d49a8 - pt4.1
https://hg.mozilla.org/integration/mozilla-inbound/rev/07f8896db19c - pt4.2

[please keep bug open when merging to m-c]

Comment 61

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/865efac0aafc
https://hg.mozilla.org/mozilla-central/rev/d3bccb687bd0
https://hg.mozilla.org/mozilla-central/rev/bcba7f58a8e7
https://hg.mozilla.org/mozilla-central/rev/c73264fcffc3
https://hg.mozilla.org/mozilla-central/rev/10592ba2f695
https://hg.mozilla.org/mozilla-central/rev/72591e9d49a8
https://hg.mozilla.org/mozilla-central/rev/07f8896db19c

Comment 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We have a large number of call sites that do "nsHTMLReflowMetrics metrics(reflowState.WritingMode())". Seems to me it would save a little bit of code to allow (or require) nsHTMLReflowMetrics' constructor to take an nsHTMLReflowState directly (the reflow state we're computing metrics for).

Comment 63

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Also, if you're going to use ISize and BSize, please document at the common places where the name is exposed (e.g. nsHTMLReflowMetrics::ISize/BSize) what these names mean.

Comment 64

[Simon Montagu :smontagu]

I suppose if I have to choose between those two alternatives I say leave the Get... version. If we had stayed with "nsWritingMode" for the class name the question wouldn't have arisen ;-)

Comment 65

[Jonathan Kew [:jfkthame]]

Attachment: followup 1 - allow reflow-state to be passed to the reflow-metrics constructor to get the writing mode.

Re comment 62: sounds good, here's a patch to do this. The old constructor that takes a WritingMode is still used by a few callsites (in table and mathml code), though I'm not sure they're all correct; we should be able to figure that out in due course as we begin to actually use the writing-mode.

Comment 66

[Jonathan Kew [:jfkthame]]

Attachment: followup 2 - additional comments describing ISize/BSize in nsHTMLReflow{Metrics,State}.h

And here are some extra comments, as per comment 63.

Comment 67

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8351717 [details] [diff] [review]
followup 2 - additional comments describing ISize/BSize in nsHTMLReflow{Metrics,State}.h

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

Thanks!!!

Comment 68

[Jonathan Kew [:jfkthame]]

Pushed the 2 followups above:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b6f20bda157
https://hg.mozilla.org/integration/mozilla-inbound/rev/125bec220126

Comment 69

[Frank Wein [:mcsmurf]]

Those checkins caused -inbound to go red.

Comment 70

[Jonathan Kew [:jfkthame]]

Ugh - that didn't go so well. Backed out followup 1 for burning everything:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f71a7dafeefe

I thought I'd built that successfully here, but it must have been a different patch. :(

Comment 71

[Jonathan Kew [:jfkthame]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #62)
> We have a large number of call sites that do "nsHTMLReflowMetrics
> metrics(reflowState.WritingMode())". Seems to me it would save a little bit
> of code to allow (or require) nsHTMLReflowMetrics' constructor to take an
> nsHTMLReflowState directly (the reflow state we're computing metrics for).

So this isn't as easy as it sounded at first. It means we need to #include nsHTMLReflowState.h before we can define the nsHTMLReflowMetrics constructor. That in turn requires nsIFrame.h, because there are a couple of (inline) methods that call nsIFrame methods. But nsIFrame.h #includes nsHTMLReflowMetrics.h. Circularity.

One way to work around this would be to remove some of the methods that are currently inlined in the headers, and define them out-of-line in the associated .cpp files instead. Then we can just forward-declare the class(es) involved instead of actually #including the complete headers. But the downside is that we may no longer benefit from inlining some of these methods (such as the nsHTMLReflowMetrics constructor).

So roc: want me to pursue this, even if it means we'll be calling an out-of-line constructor at all these callsites, or should we just drop it?

Comment 72

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think making the constructor out-of-line would be fine. It must already be reasonably big since it's initializing quite a few members.

Comment 73

[Simon Montagu :smontagu]

comment 64

Comment 74

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/125bec220126

Comment 75

[Jonathan Kew [:jfkthame]]

Attachment: followup 1 (v2) - allow reflow-state to be passed to the reflow-metrics constructor to get the writing mode.

OK, here's a version that actually compiles, using an out-of-line constructor so that we only need a forward declaration of nsHTMLReflowState in the nsHTMLReflowMetrics.h header.

Comment 76

[Simon Montagu :smontagu]

Attachment: 735577.followup.diff

Followup: add some methods I found useful going forward

Comment 77

[Jonathan Kew [:jfkthame]]

Comment on attachment 8370023 [details] [diff] [review]
735577.followup.diff

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

Looks mostly fine, but I do have a few questions, see below. Clearing r? for now, pending clarification of these issues, but happy to review again after changes and/or explanations.

::: layout/generic/WritingModes.h
@@ +195,5 @@
> +  // For unicode-bidi: plaintext, reset the direction of the writing mode from
> +  // the bidi paragraph level of the content
> +
> +  //XXX change uint8_t to UBiDiLevel after bug 924851
> +  void SetDirectionFromContent(uint8_t level)

I'd prefer to call this SetDirectionFromLevel, otherwise it sounds like you'd pass it the actual content (e.g. some text, or a DOM subtree?) and it would analyse that and set a direction accordingly.

@@ +198,5 @@
> +  //XXX change uint8_t to UBiDiLevel after bug 924851
> +  void SetDirectionFromContent(uint8_t level)
> +  {
> +    NS_ASSERTION(!IsVertical(),
> +                 "Setting bidi direction in vertical inline flow");

Is it necessarily an error to have bidi in vertical modes?

@@ +938,5 @@
>    {
> +    mRect.x = IStart(aWritingMode, aRect, aContainerWidth);
> +    mRect.y = BStart(aWritingMode, aRect, aContainerWidth);
> +    mRect.width = ISize(aWritingMode, aRect);
> +    mRect.height = BSize(aWritingMode, aRect);

I'd be curious whether this compiles to code that's equivalent to the old version (on all relevant compilers). On the surface, it looks less optimal, as it results in aWritingMode.IsVertical() being tested four times, instead of just once; but maybe that'll get optimized away?

I think this would be worth checking (with vertical support enabled, obviously, otherwise those tests should compile out altogether).

@@ +1231,5 @@
> +    }
> +  }
> +
> +  static nsRect LogicalToPhysical(WritingMode aWritingMode,
> +                                  nsRect aRect, nscoord aContainerWidth)

What's the use-case for this method? It seems confusing to me: it's taking an nsRect, which is normally the type used for *physical* coordinates, but interpreting it as logical. Offhand, ISTM that we shouldn't be using nsRects in such a way at all.

@@ +1239,5 @@
> +                             aContainerWidth);
> +  }
> +
> +  static nscoord IStart(WritingMode aWritingMode, nsRect aRect,
> +                        nscoord aContainerWidth) {

Local style in this file (at least - not sure if we've finished bikeshedding this stuff globally) puts the open-brace for the function definition on a new line, here and in the following methods.

Comment 78

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #77)
> > +  void SetDirectionFromContent(uint8_t level)
> 
> I'd prefer to call this SetDirectionFromLevel, otherwise it sounds like
> you'd pass it the actual content (e.g. some text, or a DOM subtree?) and it
> would analyse that and set a direction accordingly.

How about SetDirectionFromBidiLevel?
 
> Is it necessarily an error to have bidi in vertical modes?

Firstly, I've changed the message to "setting bidi auto-direction..." to make it clearer what the issue is. I don't know what it would mean to set LTR or RTL from the resolved bidi level of the content in a vertical-flow context. Maybe this should be at most a warning, since there's nothing to prevent content having both dir: auto (or unicode-bidi: plaintext) and writing-mode: vertical, but I really don't know what the expected behaviour in that case would be.

> > +  static nsRect LogicalToPhysical(WritingMode aWritingMode,
> > +                                  nsRect aRect, nscoord aContainerWidth)
> 
> What's the use-case for this method? It seems confusing to me: it's taking
> an nsRect, which is normally the type used for *physical* coordinates, but
> interpreting it as logical. Offhand, ISTM that we shouldn't be using nsRects
> in such a way at all.

You're probably right here, and I was being lazy and cutting corners. I'll rewrite the code that uses this.

Comment 79

[Simon Montagu :smontagu]

Attachment: Additional API in WritingMode

After rewriting part of the patch in bug 789096 this is the only addition to WritingModes.h that I end up needing.

Comment 80

[Simon Montagu :smontagu]

Attachment: Some logical getters/setters in nsIFrame

... plus this in nsIFrame

Comment 81

[Jonathan Kew [:jfkthame]]

Comment on attachment 8373052 [details] [diff] [review]
Some logical getters/setters in nsIFrame

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

::: layout/generic/nsIFrame.h
@@ +689,5 @@
> +  mozilla::LogicalRect GetLogicalRect(nscoord aContainerWidth) const {
> +    return mozilla::LogicalRect(GetWritingMode(), GetRect(), aContainerWidth);
> +  }
> +  mozilla::LogicalPoint GetLogicalPosition(nscoord aContainerWidth) const {
> +    return GetLogicalRect(aContainerWidth).Origin(GetWritingMode());

Depending how well this gets optimized, it might be worth rewriting to avoid computing all 4 values that define the rect, when all we really need is a point. But perhaps the compiler is smart enough to eliminate things that are ultimately discarded anyhow.

Let's not worry about it for now - just a thought to keep in the back of our minds in case it becomes an issue anywhere.

@@ +690,5 @@
> +    return mozilla::LogicalRect(GetWritingMode(), GetRect(), aContainerWidth);
> +  }
> +  mozilla::LogicalPoint GetLogicalPosition(nscoord aContainerWidth) const {
> +    return GetLogicalRect(aContainerWidth).Origin(GetWritingMode());
> +  }

Add comments to note that these return logical values in the frame's writing mode.

(In particular, this might NOT be the parent's writing mode, so we'll need to beware of mismatches there.)

@@ +712,5 @@
> +  /**
> +   * Set this frame's rect from a logical rect in its own writing direction
> +   */
> +  void SetRectFromLogicalRect(const mozilla::LogicalRect& aRect,
> +                              const nscoord aContainerWidth) {

I don't think we generally bother with const when passing params by value, only when we're passing references or pointers and want to make it clear to the caller that they won't be modified.

@@ +721,5 @@
> +   * (GetPhysicalRect will assert if the writing mode doesn't match)
> +   */
> +  void SetRectFromLogicalRect(const mozilla::WritingMode aWritingMode,
> +                              const mozilla::LogicalRect& aRect,
> +                              const nscoord aContainerWidth) {

Likewise, only aRect needs the const, IMO.

Comment 82

[Jonathan Kew [:jfkthame]]

Comment on attachment 8373051 [details] [diff] [review]
Additional API in WritingMode

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

::: layout/generic/WritingModes.h
@@ +198,5 @@
> +  //XXX change uint8_t to UBiDiLevel after bug 924851
> +  void SetDirectionFromBidiLevel(uint8_t level)
> +  {
> +    NS_ASSERTION(!IsVertical(),
> +                 "Setting bidi auto-direction in vertical inline flow");

I still wonder whether it's necessarily correct to assert here. Mightn't we potentially need this if there's bidi text within a writing-mode:vertical-* context?

Comment 83

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #82)
> Comment on attachment 8373051 [details] [diff] [review]
> > +    NS_ASSERTION(!IsVertical(),
> > +                 "Setting bidi auto-direction in vertical inline flow");
> 
> I still wonder whether it's necessarily correct to assert here. Mightn't we
> potentially need this if there's bidi text within a writing-mode:vertical-*
> context?

I still don't see the use case, but I'll remove the assertion and make the callers ignore unicode-bidi: plaintext in vertical writing mode.

Comment 84

[fantasai]

Bidi should be supported in full in vertical writing modes. As for use cases, consider table headers / captions.

Comment 85

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a63da21ad30b
https://hg.mozilla.org/integration/mozilla-inbound/rev/e41e1fd3c900

Comment 86

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a63da21ad30b
https://hg.mozilla.org/mozilla-central/rev/e41e1fd3c900

Comment 87

[Simon Montagu :smontagu]

Attachment: More logical getter/setters

Changes SetRectFromLogicalRect to SetRect as suggested in bug 789096 comment 13, and adds some more logical getters that I need down the road.

Comment 88

[Jonathan Kew [:jfkthame]]

Comment on attachment 8382342 [details] [diff] [review]
More logical getter/setters

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

LGTM.

The one thing to be wary of is that it might sometimes be more efficient - depending how good the compiler is at optimizing this stuff - for a caller to use (say) GetLogicalRect and then extract its IStart() and BStart() coordinates locally, rather than using the separate IStart() and BStart(), each of which hides a GetLogicalRect with its associated writing-mode check and remapping of coordinates.

But I don't think that's a reason *not* to provide the convenience APIs here; just something to keep in mind as we're using this stuff.

Comment 89

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca402c27bf50

Comment 90

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca402c27bf50

Comment 91

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8355004 [details] [diff] [review]
followup 1 (v2) - allow reflow-state to be passed to the reflow-metrics constructor to get the writing mode.

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

Why didn't I review this earlier, and why didn't you nag me? :-)

Comment 92

[Jonathan Kew [:jfkthame]]

Why indeed? :)

Rebased and pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63a4ad62401a

Comment 93

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/63a4ad62401a

Comment 94

[Simon Montagu :smontagu]

Attachment: More logical getter/setters

And some more stuff I need going forward, getters for frame logical margin/border/padding, and +/- operators for LogicalMargin. For the latter, do you think it's better to go on adding them incrementally as we use them, or shall we put in the whole set of +, -, += and -= for all the logical claases now?

Comment 95

[Jonathan Kew [:jfkthame]]

Comment on attachment 8401743 [details] [diff] [review]
More logical getter/setters

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

::: layout/generic/nsIFrame.h
@@ +810,5 @@
>    nsPoint GetNormalPosition() const;
> +  mozilla::LogicalPoint
> +  GetLogicalNormalPosition(nscoord aContainerWidth) const {
> +    return GetLogicalNormalPosition(GetWritingMode(), aContainerWidth);
> +  }

I realize it saves a bunch of verbosity for callers, but I'm a bit nervous of having methods like this that return logical-coordinate values with no check that the writing mode is what the caller expects.

This would be fine if used only by other methods on the same frame, working in the same writing mode, but could result in errors if another frame with a different WM calls it and then uses the resulting I- and B-coordinates without realizing they are incompatible.

Maybe as I look at the places this gets used, I'll become more comfortable with this, but I'd like to think about it a bit longer at least.

Comment 96

[Simon Montagu :smontagu]

Attachment: Logical getter/setters without the implicit WritingMode

I don't in fact use the versions of these methods with implicit writing mode anywhere in my upcoming patch queue, so I'm happy to move forward without them.

Comment 97

[Jonathan Kew [:jfkthame]]

Comment on attachment 8402359 [details] [diff] [review]
Logical getter/setters without the implicit WritingMode

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

::: layout/generic/nsIFrame.h
@@ +952,5 @@
>    }
> +  mozilla::LogicalMargin
> +  GetLogicalUsedBorderAndPadding(mozilla::WritingMode aWritingMode) const {
> +    return GetLogicalUsedBorder(aWritingMode) +
> +           GetLogicalUsedPadding(aWritingMode);

Wouldn't it seem more efficient to do

  return mozilla::LogicalMargin(aWritingMode, GetUsedBorderAndPadding());

so as to only do the physical/logical conversions once?

Comment 98

[Simon Montagu :smontagu]

Attachment: Logical getter/setters without the implicit WritingMode, updated

> Wouldn't it seem more efficient to do
> 
>   return mozilla::LogicalMargin(aWritingMode, GetUsedBorderAndPadding());
> 
> so as to only do the physical/logical conversions once?

Yes

Comment 99

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/886528e51951

Comment 100

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/886528e51951

Comment 101

[Simon Montagu :smontagu]

I think we don't need to keep this open any longer. We'll add more things to the API ad hoc in other bugs.