1104711 - LogicalSize.ConvertTo can skip conversion when source and target modes are not orthogonal

Status: RESOLVED FIXED

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

Description

[Simon Montagu :smontagu]

All the ConvertTo() methods in WritingModes.h swap coordinates whenever the source and target writing modes are different. For LogicalSize this can be improved: when converting between two horizontal or two vertical modes the result will be the same so we can skip the conversion.

Comment 1

[Simon Montagu :smontagu]

Attachment: Patch

The simple patch that I envisaged for this doesn't work because the writing modes are still different so we assert all over the place (https://tbpl.mozilla.org/?tree=Try&rev=0773e5d2181b).

We could in theory take this a step further by relaxing the CHECK_WRITING_MODE assertions on LogicalSize to check only for the same axes and not the same directions, but I don't think that's a great idea because the assertions are valuable for catching logical errors even when the results of calculations aren't affected.

Alternatively we could do what this patch does, but maybe it's not a significant win any more and we can just WONTFIX this. WDYT?

Comment 2

[Jonathan Kew [:jfkthame]]

Comment on attachment 8529707 [details] [diff] [review]
Patch

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

My gut feeling is that this version should be better that the current code, as it does the conversion (or non-conversion) more directly, so we may as well do it. (I suppose micro-benchmarking and/or comparing the compiled code that we get in an optimized build would be the way to really know.)

::: layout/generic/WritingModes.h
@@ +769,5 @@
>      CHECK_WRITING_MODE(aFromMode);
> +    return aToMode == aFromMode
> +            ? *this : aToMode.IsOrthogonalTo(aFromMode)
> +                       ? LogicalSize(aToMode, BSize(), ISize())
> +                       : LogicalSize(aToMode, ISize(), BSize());

I suppose we could even do something like

  return aToMode == aFromMode
           ? *this
           : aToMode.IsOrthogonalTo(aFromMode)
             ? LogicalSize(aToMode, BSize(), ISize())
             :
#ifdef DEBUG // ensure correct writing-mode in the result
               LogicalSize(aToMode, ISize(), BSize())
#else // in non-DEBUG builds, LogicalSize does not store the writing mode,
      // so we can simply return the existing value
               *this
#endif
               ;

but maybe that's too tricky to be worth it for the marginal gain?

Comment 3

[Simon Montagu :smontagu]

Attachment: Patch v.2

Yes, I like that idea, but I decided to implement it slightly differently, thus:

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 8530308 [details] [diff] [review]
Patch v.2

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

Yes, that's more readable than what I wrote!

Comment 5

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74861ffc991f

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/74861ffc991f