Closed Bug 1407103 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Widget, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

Details

Attachments

(1 file)

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.
Bob, I'm not sure if you're the right person to review this. Please forward on
to somebody else if necessary.
Attachment #8916856 - Flags: review?(bobowencode)
(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 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.
Attachment #8916856 - Flags: review?(bobowencode) → review+
> 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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/396ff2a152d77a0c28f09fefc8b0a96659f2d4ba
Bug 1407103 - Convert wstring attributes to AString in widget/nsIPrint*.idl. r=bobowen.
https://hg.mozilla.org/mozilla-central/rev/396ff2a152d7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.