1088071 - Move the final variant of nsRenderingContext::DrawString to nsLayoutUtils

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Wrap the callers of the final nsRenderingContext::DrawString method up in an nsLayoutUtils method

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Move the nsRenderingContext::DrawString code to nsLayoutUtils::DrawStringWithoutBidi

Comment 3

[Jonathan Watt [:jwatt]]

I've exposed some nsRenderingContext methods for this, but those methods will be moved themselves shortly.

Comment 4

[Simon Montagu :smontagu]

Comment on attachment 8510347 [details] [diff] [review]
part 1 - Wrap the callers of the final nsRenderingContext::DrawString method up in an nsLayoutUtils method

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

How about something like DrawUnidirectionalString or DrawSingleDirectionString instead of DrawStringWithoutBidi?

Comment 5

[Jonathan Watt [:jwatt]]

Someone else can choose the name. I have no strong opinions there.

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 8510348 [details] [diff] [review]
part 2 - Move the nsRenderingContext::DrawString code to nsLayoutUtils::DrawStringWithoutBidi

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

::: layout/base/nsLayoutUtils.h
@@ +1325,5 @@
>                           nsStyleContext*       aStyleContext = nullptr);
>  
> +  /**
> +   * Supports only LTR or RTL. Bidi (mixed direction) is not supported.
> +   */

So it looks like the only two callers of this method are nsImageFrame::DisplayAltText (though if the text includes RTL characters, or the presContext has BidiEnabled, it'll go via nsBidiPresUtils::RenderText instead), and nsDisplayMathMLError::Paint, which just uses it to draw a hard-coded ASCII error string.

Given this, I wonder if we need any kind of RTL support at all in this method? Can we strip it down and make it just DrawLTRString?

Comment 7

[Jonathan Watt [:jwatt]]

Don't forget the nsLayoutUtils::DrawString caller.

I'm not adverse to stripping it down, but I'd rather do that as a follow-up.

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 8510347 [details] [diff] [review]
part 1 - Wrap the callers of the final nsRenderingContext::DrawString method up in an nsLayoutUtils method

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

OK, but let's go with one of Simon's naming suggestions to replace DrawStringWithoutBidi. I guess I'd lean towards DrawUnidirectionalString, though all of the options are rather long... DrawUniDirString, anyone?

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 8510348 [details] [diff] [review]
part 2 - Move the nsRenderingContext::DrawString code to nsLayoutUtils::DrawStringWithoutBidi

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

Fair enough. I still suspect we don't need RTL support here at all, though, and could simplify it accordingly; could you file a followup for us to check on that, please?

Comment 10

[Jonathan Watt [:jwatt]]

Surely to reflect "Bidi" it should be "Unidi". ;)

(DrawUniDirString seems fine to me.)

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 8510347 [details] [diff] [review]
part 1 - Wrap the callers of the final nsRenderingContext::DrawString method up in an nsLayoutUtils method

https://hg.mozilla.org/integration/mozilla-inbound/rev/449a78f0febc

Comment 12

[Jonathan Watt [:jwatt]]

Comment on attachment 8510348 [details] [diff] [review]
part 2 - Move the nsRenderingContext::DrawString code to nsLayoutUtils::DrawStringWithoutBidi

https://hg.mozilla.org/integration/mozilla-inbound/rev/90cd5968c9ce

Comment 13

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Kew (:jfkthame) from comment #9)
> could you file a followup for us to check
> on that, please?

Bug 1088596.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/449a78f0febc
https://hg.mozilla.org/mozilla-central/rev/90cd5968c9ce