1087982 - Get rid of the ASCII variants of nsRenderingContext::DrawString/GetWidth

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Get rid of the useless null checks in nsRenderingContext::GetMaxChunkLength/GetWidth

In the case of GetMaxChunkLength, after all calls we go on to deref mFontMetrics anyway. In the case of GetWidth the other GetWidth instance that it calls if mFontMetrics is null goes on to reref without checking it for null. So the null checks in this file are bogus.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Get rid of the pass-char-by-value variant of nsRenderingContext::GetWidth

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: part 3 - Get rid of the ASCII variants of nsRenderingContext::DrawString/GetWidth

We know that the single caller of these functions has a maximum string length of 16. Therefore all the length checks inherent to these nsRenderingContext methods are redundant. We can just use the nsFontMetrics object directly.

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 8510269 [details] [diff] [review]
part 2 - Get rid of the pass-char-by-value variant of nsRenderingContext::GetWidth

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

You're on a code-removal spree, aren't you? :)

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 8510270 [details] [diff] [review]
part 3 - Get rid of the ASCII variants of nsRenderingContext::DrawString/GetWidth

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

::: layout/base/nsPresShell.cpp
@@ +10110,5 @@
>        sprintf(buf, "%d", counter->mCount);
>        nscoord x = 0, y = fm->MaxAscent();
>        nscoord width, height = fm->MaxHeight();
> +      fm->SetTextRunRTL(false);
> +      width = fm->GetWidth(buf, strlen(buf), aRenderingContext);;

While you're here, how about assigning the return value of sprintf() just above to a local |len| variable, and avoiding the need to call strlen() here?

@@ +10143,2 @@
>        aRenderingContext->ThebesContext()->SetColor(color);
> +      fm->DrawString(buf, strlen(buf), x, y, aRenderingContext);

Then these two strlen()s will also be unnecessary.

Comment 6

[Jonathan Watt [:jwatt]]

Comment on attachment 8510265 [details] [diff] [review]
part 1 - Get rid of the useless null checks in nsRenderingContext::GetMaxChunkLength/GetWidth

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e7b73241e03

Comment 7

[Jonathan Watt [:jwatt]]

Comment on attachment 8510269 [details] [diff] [review]
part 2 - Get rid of the pass-char-by-value variant of nsRenderingContext::GetWidth

https://hg.mozilla.org/integration/mozilla-inbound/rev/0085911c35b4

Comment 8

[Jonathan Watt [:jwatt]]

Comment on attachment 8510270 [details] [diff] [review]
part 3 - Get rid of the ASCII variants of nsRenderingContext::DrawString/GetWidth

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b6b6c03d82b

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e7b73241e03
https://hg.mozilla.org/mozilla-central/rev/0085911c35b4
https://hg.mozilla.org/mozilla-central/rev/2b6b6c03d82b