Closed Bug 1100071 Opened 10 years ago Closed 10 years ago

Use utility function to check RTL embedding level

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: smontagu, Assigned: smontagu)

References

Details

Attachments

(3 files, 5 obsolete files)

As suggested by dholbert in bug 1097894 comment 5:

> Seems like we'd benefit from a utility function (or macro) to abstract away
> our "embeddinglevel & 1 == 1" checks into something like
> "IsEmbeddingLevelRTL(level)".
A bit of mission creep here, but while I was doing this it made sense to add a few more macros for common patterns...
Attachment #8523563 - Flags: review?(dholbert)
... and not to pass the nsBidiLevel at all to callers that only need to know whether the direction is LTR or RTL.
Attachment #8523564 - Flags: review?(dholbert)
Last one, honestly! I realized that the name SetDirectionFromBidiLevel is now inaccurate.
Attachment #8523721 - Flags: review?(dholbert)
Attachment #8523561 - Flags: review?(dholbert) → review+
Comment on attachment 8523563 [details] [diff] [review]
Patch 2: add macros for testing direction of level, and for comparing direction of two levels

># HG changeset patch
># User Simon Montagu <smontagu@smontagu.org>
># Date 1416152843 -7200
>#      Sun Nov 16 17:47:23 2014 +0200
># Node ID 244bdc555c59988e2c29b25d7f642f107d217686
># Parent  a1b6acf09877d821bc9acdc4659c4545ef68dcf3
>Bug 1100071 patch 2: add macros for common tests whether bidi level is odd and whether two bidi levels have the same parity
>
>diff --git a/dom/canvas/CanvasRenderingContext2D.cpp b/dom/canvas/CanvasRenderingContext2D.cpp
>--- a/dom/canvas/CanvasRenderingContext2D.cpp
>+++ b/dom/canvas/CanvasRenderingContext2D.cpp
>-  virtual void SetText(const char16_t* text, int32_t length, nsBidiDirection direction)
>+  virtual void SetText(const char16_t* aText, int32_t aLength, bool aIsRTL)
[...]
>+++ b/layout/base/nsBidiPresUtils.h
>@@ -102,25 +102,25 @@ public:
>-     * 
>+     *
>      * @param aText The string of text.
>      * @param aLength The length of the string of text.
>-     * @param aDirection The direction of the text. The string will never have
>-     *  mixed direction.
>+     * @param aIsRTL True if the direction of the text is right-to-left.
>+     *   (The string will never have mixed direction)
>      */
>     virtual void SetText(const char16_t*   aText,
>-                         int32_t            aLength,
>-                         nsBidiDirection    aDirection) = 0;
>+                         int32_t           aLength,
>+                         bool              aIsRTL) = 0;

I'm not sure we should switch this nsBidiDirection arg to be a bool.  I recall being advised (by roc, I think?) to prefer enum args over boolean args, and I think it's a very good best-practice. (Particularly in a case like this, where we already have an appropriate enum type, already being used for this arg).

The main reasons to prefer enums are, IIRC:
 - It's easy to forget what the polarity of a boolean argument is (whether "true" means "LTR" or "RTL"); you have to go to the function definition to find out.
 - In cases where callers pass literal values (true/false) for bools, e.g. DoStuff(arg1, arg2, true, arg4), it's unclear what concept the "true" is even about (not clear that it has anything to do with bidi/directionality). In contrast, the literal values for an enum are self-documenting (e.g. NSBIDI_RTL in this case).

So: if it's not too much more complicated, I think I'd prefer that this patch changes to:
 (A) leave the SetText() function-signatures as-is (still taking a nsBidiDirection)
 (B) adjust the callers (which in current m-c do hacky things like passing in "nsBidiDirection(level & 1)") to pass in NS_BIDI_DIRECTION_FROM_LEVEL(level), or something like that.

How does that sound?
(NS_BIDI_DIRECTION_FROM_LEVEL would be something like: IS_LEVEL_RTL(level) ? NSBIDI_RTL : NSBIDI_LTR)
Comment on attachment 8523564 [details] [diff] [review]
Patch 3: don't pass around bidi levels when all we need is a boolean for RTL/LTR

>Bug 1100071 patch 3: don't pass around bidi levels when all we need is a boolean RTL/LTR, try: -b do -p all -u all -t none
[...]
>+++ b/layout/generic/WritingModes.h
>-  //XXX change uint8_t to UBiDiLevel after bug 924851
>-  void SetDirectionFromBidiLevel(uint8_t level)
>+  void SetDirectionFromBidiLevel(bool aIsRTL)
>   {
>-    if (level & 1) {
>-      // odd level, set RTL
>+    if (aIsRTL) {
>+      // set RTL
>       mWritingMode |= eBidiMask;
>     } else {
>-      // even level, set LTR
>+      // set LTR
>       mWritingMode &= ~eBidiMask;
>     }

My reservations in comment 6 apply here, as well.  I don't think we should move in this direction (converting arguments to bools) -- I'd rather we took a nsBidiLevel here (which is what the uint8_t arg really wants to be, of course), and then check "if (IS_LEVEL_RTL(aLevel))", instead of taking a "bool aIsRTL".

That applies to a lot of this patch, I think. :-/

(The new accessors like nsBidiPresUtils::IsParagraphRTL() seem like they might be useful, though, if we have usages for them beyond converting to a different type of input for other functions; not sure.)
(Also, FWIW, I'm not necessarily pushing back on the new local-var bools like "isRTLParagraph" -- those seem useful & don't have the drawbacks mentioned in comment 6. I'm just talking about the function-params.)
Yes, I guess those reasons for preferring enum to bool make sense.
Attachment #8523563 - Attachment is obsolete: true
Attachment #8523563 - Flags: review?(dholbert)
Attachment #8524767 - Flags: review?(dholbert)
> My reservations in comment 6 apply here, as well.  I don't think we should
> move in this direction (converting arguments to bools) -- I'd rather we took
> a nsBidiLevel here (which is what the uint8_t arg really wants to be, of
> course), and then check "if (IS_LEVEL_RTL(aLevel))", instead of taking a
> "bool aIsRTL".

I've changed most of the patch to pass nsBidiDirections instead of bools, but there are a few problems. The comment "//XXX change uint8_t to UBiDiLevel after bug 924851" should have reminded me that there were reasons why I didn't put nsBidiLevel there in the first place (other than laziness). If we use nsBidiLevel and #include nsBidi.h, there is a slew of warnings because of duplicate definitions of macros from ICU. That isn't an insoluble problem, of course, but since nsBidi.h is going to go away in bug 924851 anyway, it doesn't seem worth taking the trouble.

The same applies to another change that I tried to add, making nsILineIterator::GetDirection return an nsDirection instead of a boolean.

Or if you prefer we could defer this whole fix until after bug 924851.
Attachment #8524779 - Flags: review?(dholbert)
Comment on attachment 8523721 [details] [diff] [review]
Patch 4: change SetDirectionFromBidLevel to SetDirectionFromParagraph

This is no longer relevant.
Attachment #8523721 - Attachment is obsolete: true
Attachment #8523721 - Flags: review?(dholbert)
Attachment #8523564 - Attachment is obsolete: true
Attachment #8523564 - Flags: review?(dholbert)
Comment on attachment 8524767 [details] [diff] [review]
Patch 2 v.2: add macros for testing direction of level, and for comparing direction of two levels

>+++ b/intl/unicharutil/util/nsBidiUtils.h
> /**
>+ * Find the direction of an embedding level or paragraph level set by
>+ * the Unicode Bidi Algorithm. (Even levels are left-to-right, odd
>+ * levels right-to-left.
>+ */
>+
>+#define IS_LEVEL_RTL(level) (((level) & 1) == 1)
>+#define IS_SAME_DIRECTION(level1, level2) (((level1 ^ level2) & 1) == 0)
>+#define DIRECTION_FROM_LEVEL(level) ((IS_LEVEL_RTL(level)) \
>+                                     ? NSBIDI_RTL : NSBIDI_LTR)

Since that comment-block only applies to the first macro, add a newline after that first macro.
(And maybe a comment like "Other macros for reasoning about direction of nsBidiLevels:" after it).

>+++ b/layout/base/nsBidiPresUtils.cpp
>@@ -2002,55 +2003,55 @@ nsresult nsBidiPresUtils::ProcessText(co
[...]
>-      if (!(level & 1)) {
>+      if (!dir == NSBIDI_RTL) {
>         xOffset += width;
>       }

This wanted to be "dir !=", not "!dir ==". But really, this should just be checking if (dir == NSBIDI_LTR).

(It must be either LTR or RTL -- not MIXED -- because it comes from a call to DIRECTION_FROM_LEVEL that you're adding up above, and that macro only returns LTR or RTL. So checking is-not-RTL is equivalent to checking is-LTR.)

>diff --git a/layout/base/nsCaret.cpp b/layout/base/nsCaret.cpp
[...]
>           else if (aBidiLevel < levelBefore && aBidiLevel > levelAfter  // rule c11/12
>-                   && !((levelBefore ^ levelAfter) & 1)                 // before and after have the same parity
>-                   && ((aBidiLevel ^ levelAfter) & 1))                  // caret has different parity
>+                   && IS_SAME_DIRECTION(levelBefore, levelAfter)        // before and after have the same parity
>+                   && !IS_SAME_DIRECTION(aBidiLevel, levelAfter))      // caret has different parity

Nit: keep the comment aligned with stuff above it, on the last line there. (This is at line 761 -- there are earlier instances of this pattern which are correctly aligned.

>diff --git a/layout/generic/nsFrame.cpp b/layout/generic/nsFrame.cpp
>--- a/layout/generic/nsFrame.cpp
>+++ b/layout/generic/nsFrame.cpp
> // Determine movement direction relative to frame
> static bool IsMovingInFrameDirection(nsIFrame* frame, nsDirection aDirection, bool aVisual)
> {
>-  bool isReverseDirection = aVisual ?
>-    (NS_GET_EMBEDDING_LEVEL(frame) & 1) != (NS_GET_BASE_LEVEL(frame) & 1) : false;
>+  bool isReverseDirection = aVisual ? !IS_FRAME_IN_PARA_DIRECTION(frame)
>+                                    : false;

It seems broken for a ternary expression to have ": false" at the end. I think this just wants be a non-ternary boolean expression.

In particular, I think this could be simplified to:
  bool isReverseDirection = aVisual && !IS_FRAME_IN_PARA_DIRECTION(frame);

If you agree, let's do that simplification while you're here.

r=me with the above. Thanks!
Attachment #8524767 - Flags: review?(dholbert) → review+
[patch 2's changes to nsFrame::GetPointFromOffset layer on top of bug 1097894. --> setting explicit dependency.]
Depends on: 1097894
Comment on attachment 8524779 [details] [diff] [review]
Patch 3 v.2: don't pass around bidi levels when all we need is an nsBidiDirection

>diff --git a/layout/base/nsBidiPresUtils.h b/layout/base/nsBidiPresUtils.h
>--- a/layout/base/nsBidiPresUtils.h
>+++ b/layout/base/nsBidiPresUtils.h
>+  static nsBidiDirection ParagraphDirection(nsIFrame* aFrame) {
>+    return DIRECTION_FROM_LEVEL(GetFrameBaseLevel(aFrame));
>+  }
>+  static nsBidiDirection FrameDirection(nsIFrame* aFrame) {
>+    return DIRECTION_FROM_LEVEL(GetFrameEmbeddingLevel(aFrame));
>+  }

Please add a comment to document that these functions are guaranteed to return either NSBIDI_LTR or NSBIDI_RTL, and *not* NSBIDI_MIXED (the other valid nsBidiDirection value).

The callers in this patch implicitly depend on this, and we should document that to make the callers' assumptions more explicitly-legitimate.

>+  static bool IsFrameInParagraphDirection(nsIFrame* aFrame) {
>+    return ParagraphDirection(aFrame) == FrameDirection(aFrame);
>+  }

I suspect this means we can get rid of the IS_FRAME_IN_PARA_DIRECTION macro (added in part 2), and convert its callers to use this function?

Or, we can use drop this function and use the macro... but the function is probably better, for type-safety and to take advantage of the other functions you're adding here.

>diff --git a/layout/generic/WritingModes.h b/layout/generic/WritingModes.h
> #include "nsRect.h"
> #include "nsStyleContext.h"
>+#include "nsBidiUtils.h"
[...]
>   void SetDirectionFromBidiLevel(uint8_t level)
>   {
>-    if (level & 1) {
>-      // odd level, set RTL
>+    if (IS_LEVEL_RTL(level)) {
>+      // set RTL
>       mWritingMode |= eBidiMask;
>     } else {
>-      // even level, set LTR
>+      // set LTR
>       mWritingMode &= ~eBidiMask;

Nit: The changes to this file (WritingModes.h) seem like they belong in part 2.

(r=me on shifting them over there.)

>+++ b/layout/generic/nsFrame.cpp
>@@ -6644,20 +6644,22 @@ nsIFrame::PeekOffset(nsPeekOffsetStruct*
>-          nsBidiLevel embeddingLevel = nsBidiPresUtils::GetFrameEmbeddingLevel(baseFrame);
>-          // If the direction of the frame on the edge is opposite to that of the line,
>-          // we'll need to drill down to its opposite end, so reverse endOfLine.
>-          if (IS_LEVEL_RTL(embeddingLevel) == !lineIsRTL)
>+          bool frameIsRTL =
>+            (nsBidiPresUtils::FrameDirection(baseFrame) == NSBIDI_RTL);
>+          // If the direction of the frame on the edge is opposite to
>+          // that of the line, we'll need to drill down to its opposite
>+          // end, so reverse endOfLine.
>+          if (frameIsRTL != lineIsRTL)
>             endOfLine = !endOfLine;
>         }

Could you add curly-braces around the "if" condition's body, as long as you're changing the if condition? (to bring it into alignment with the coding style guide)

This formulation is human-error-prone:
      [...]
      if (foo)
        do something
    } //<--- closing brace for *some outer* block, not for "if (foo)"!!!

(The closing brace is open to misinterpretation, which can mean bugs, if someone wants to extend that "if (foo)" block and misinterprets the closing curly-brace.)

>+++ b/layout/generic/nsFrameList.cpp
>@@ -421,32 +420,30 @@ nsFrameList::GetNextVisualFor(nsIFrame* 
>     } else {
>-      // Just get the next or prev sibling, depending on block and frame direction.
>-      nsBidiLevel frameEmbeddingLevel = nsBidiPresUtils::GetFrameEmbeddingLevel(mFirstChild);
>-      if (IS_SAME_DIRECTION(frameEmbeddingLevel, baseLevel)) {
>+      if (nsBidiPresUtils::IsFrameInParagraphDirection(mFirstChild)) {
>         return aFrame ? aFrame->GetNextSibling() : mFirstChild;
>       } else {
>         return aFrame ? aFrame->GetPrevSibling() : LastChild();
>       }

I think the comment should probably stay (i.e. don't delete it), right? (You keep a similar comment around in a similar chunk higher up in this file, and it still seems to help explain the code, so I think it wants to stay.)

[Holding off on r+ because I'm curious about the IS_FRAME_IN_PARA_DIRECTION vs. IsFrameInParagraphDirection distinction / possible-consolidation.]
Updated to review comments from comment 13, carrying over r=dholbert
Attachment #8525182 - Flags: review+
Updated to review comments from comment 15

> I suspect this means we can get rid of the IS_FRAME_IN_PARA_DIRECTION macro
> (added in part 2), and convert its callers to use this function?
> 
> Or, we can use drop this function and use the macro... but the function is
> probably better, for type-safety and to take advantage of the other
> functions you're adding here.

Can we investigate that in a follow-up bug? The two aren't identical, because the macros just get the property from the frame passed in, but the functions drill down to the leaf (excuse the mixed metaphor) and get the real frame from floating first-letter placeholders. In theory that should always be more correct, but it could also be more expensive.
Attachment #8524767 - Attachment is obsolete: true
Attachment #8524779 - Attachment is obsolete: true
Attachment #8524779 - Flags: review?(dholbert)
Attachment #8525195 - Flags: review?(dholbert)
Comment on attachment 8525182 [details] [diff] [review]
Patch 2 v.3: add macros for testing direction of level, and for comparing direction of two levels

>diff --git a/layout/generic/nsIFrame.h b/layout/generic/nsIFrame.h
>--- a/layout/generic/nsIFrame.h
>+++ b/layout/generic/nsIFrame.h
>+#define IS_FRAME_RTL(frame) (IS_LEVEL_RTL(NS_GET_EMBEDDING_LEVEL(frame)))
>+#define IS_FRAME_IN_RTL_PARA(frame) (IS_LEVEL_RTL(NS_GET_BASE_LEVEL(frame)))
>+#define IS_FRAME_IN_PARA_DIRECTION(frame) \
>+  (IS_SAME_DIRECTION(NS_GET_EMBEDDING_LEVEL(frame), NS_GET_BASE_LEVEL(frame)))

Nit: looks like those first two macros (for frama/para being RTL) are new in this patch version -- and the first only has one usage, and the second (for RTL_PARA) is never used at all.

Maybe drop the unused one, and move the other one to the .cpp file where it's used? (unless you've got plans to add more usages)  nsIFrame.h is already pretty huge (and included-all-over-the-place), so it's probably better not to add unused / rarely-used macros to it, if we can avoid it.
(In reply to Simon Montagu :smontagu from comment #17)
> Can we investigate that in a follow-up bug? The two aren't identical,
> because the macros just get the property from the frame passed in, but the
> functions drill down to the leaf (excuse the mixed metaphor) and get the
> real frame from floating first-letter placeholders. In theory that should
> always be more correct, but it could also be more expensive.

Ah, I missed that difference -- thanks for clarifying.

Then yeah, let's punt on that -- but in the meantime, please rename one of them, so they're easier to distinguish.  (Right now they've got identical names (aside from "PARA" vs "Paragraph"), and that's bad, since they're not equivalent.)

The names don't have to be perfect / final (we can rename later if we come up with better names, if we don't end up merging them), but for now, they should at least be distinguishable... e.g. maybe name the macro one IS_FRAME_IN_PARA_DIRECTION_CHEAP, and mention in its documentation that it's "cheap" compared to IsFrameInParaDirection because it doesn't drill down to the bidi leaf frame?

(Also, in the spirit of comment 18 -- I'm noticing that IS_FRAME_IN_PARA_DIRECTION is only used in nsFrame.cpp. It should probably be #defined in that .cpp file, instead of in nsIFrame.h.)
(In reply to Daniel Holbert [:dholbert] from comment #19)
> and mention in its documentation that it's
> "cheap" compared to IsFrameInParaDirection because

(Sorry, I meant "compared to IsFrameInParagraphDirection")
Comment on attachment 8525195 [details] [diff] [review]
Patch 3 v.3: don't pass around bidi levels when all we need is an nsBidiDirection

r=me on patch 3, with some IS_FRAME_IN_PARA_DIRECTION vs. IsFrameInParagraphDirection name-disambiguation, per comment 19.

(Note that the actual changes for that disambiguation may *really* want to happen in "patch 2", not "patch 3", if it's the macro that gets renamed (e.g. adding _CHEAP and moving to the .cpp file, as suggested in comment 19))
Attachment #8525195 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #21)
> (Note that the actual changes for that disambiguation may *really* want to
> happen in "patch 2", not "patch 3", if it's the macro that gets renamed
> (e.g. adding _CHEAP and moving to the .cpp file, as suggested in comment 19))

The macro is only ever used negated, so what I think I'll do, as well as moving to the .cpp file and adding a comment, is turn it round and change the name, like so:

+// This is faster than nsBidiPresUtils::IsFrameInParagraphDirection,
+// because it uses the frame pointer passed in without drilling down to
+// the leaf frame.
+#define REVERSED_DIRECTION_FRAME(frame) \
+  (!IS_SAME_DIRECTION(NS_GET_EMBEDDING_LEVEL(frame), NS_GET_BASE_LEVEL(frame)))

Does that sound reasonable? I've also removed IS_FRAME_IN_RTL_PARA and just used 
 IS_LEVEL_RTL(NS_GET_EMBEDDING_LEVEL(firstFrame))
in nsTextFrame.cpp without bothering with the IS_FRAME_RTL macro. (I had already made those changes in the previous version but accidentally reverted them)
(In reply to Simon Montagu :smontagu from comment #22)
> Does that sound reasonable?

Yup -- thanks!

(minor nit: I would've maybe suggested giving the macro an "IS_" prefix -- e.g. IS_REVERSED_DIRECTION_FRAME -- so that it's more clearly boolean-returning. But, doesn't matter too much, particularly given that it's only exposed in a single .cpp file)

> I've also removed IS_FRAME_IN_RTL_PARA

Sounds good!

Thanks again for filing this & thoroughly following up here! (based on an offhand comment in bug 1097894) :)
You need to log in before you can comment on or make changes to this bug.