1407103 - Convert wstring attributes to AString in widget/nsIPrint*.idl

Status: RESOLVED FIXED

(Core :: Widget, enhancement)

Description

[Nicholas Nethercote [inactive]]

This avoids a lot of mismatches between nsAString and char16_t*, thus removing
many getter_Copies() and ToNewUnicode() and get() calls, and generally making
things simpler.

Note: the patch removes GetDefaultPrinterNameFromGlobalPrinters() by simply
inlining it at its two callsites, which is easy with the changed types.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Convert wstring attributes to AString in widget/nsIPrint*.idl

Bob, I'm not sure if you're the right person to review this. Please forward on
to somebody else if necessary.

Comment 2

[Bob Owen (:bobowen)]

(In reply to Nicholas Nethercote [:njn] from comment #1)
> Created attachment 8916856 [details] [diff] [review]
> Convert wstring attributes to AString in widget/nsIPrint*.idl
> 
> Bob, I'm not sure if you're the right person to review this. Please forward
> on
> to somebody else if necessary.

I'm not sure either, but I'm happy to take it on. :-)
There's quite a bit to get through, so I'll try and look at it in the morning.

Comment 3

[Bob Owen (:bobowen)]

Comment on attachment 8916856 [details] [diff] [review]
Convert wstring attributes to AString in widget/nsIPrint*.idl

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

Nice clean-up, thanks.
The world feels a little bit safer.

::: widget/nsPrintOptionsImpl.cpp
@@ +143,1 @@
>    data->title() = title;

Can we get rid of these local vars entirely and just pass data->title() etc.?

@@ +535,1 @@
>        DUMP_STR(kReadStr, kPrintPaperName, str.get());

I was wondering if we could get rid of the get() here as well.

However, this only does something when DEBUG_rods_X is defined.
Unsurprisingly this is broken, so maybe we should rip this out at the same time or perhaps a follow-up.

::: widget/nsPrintSettingsImpl.cpp
@@ +559,5 @@
>    return NS_OK;
>  }
>  
> +nsresult
> +nsPrintSettings::GetMarginStrs(nsAString& aTitle,

[Get|Set]MarginStrs only seem to be used in the getters and setters below.
Seems to me it would be clearer if they just did the thing rather than using these.

::: widget/windows/nsDeviceContextSpecWin.cpp
@@ +202,3 @@
>    }
>  
> +  NS_ASSERTION(!printerName.IsEmpty(), "We have to have a printer name");

Isn't this a slightly different assertion?
I actually think we'd get an OOM before the old assertion could happen.

Comment 4

[Nicholas Nethercote [inactive]]

> Can we get rid of these local vars entirely and just pass data->title() etc.?

Good suggestion, I'll do it.


> However, this only does something when DEBUG_rods_X is defined.
> Unsurprisingly this is broken, so maybe we should rip this out at the same
> time or perhaps a follow-up.

Good catch. That's touching enough lines of code that I'll do it as a follow-up.


> [Get|Set]MarginStrs only seem to be used in the getters and setters below.
> Seems to me it would be clearer if they just did the thing rather than using
> these.

True! I'll do that.


> Isn't this a slightly different assertion?
> I actually think we'd get an OOM before the old assertion could happen.

True again. In a lot of code like this working out the distinction between "no string" and "empty string" can be a real pain; sometimes it's significant, sometimes not. I'll remove the assertion.


Thank you for the very helpful review.

Comment 5

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/396ff2a152d77a0c28f09fefc8b0a96659f2d4ba
Bug 1407103 - Convert wstring attributes to AString in widget/nsIPrint*.idl. r=bobowen.

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/396ff2a152d7