Closed
Bug 1407103
Opened 6 years ago
Closed 6 years ago
Convert wstring attributes to AString in widget/nsIPrint*.idl
Categories
(Core :: Widget, enhancement)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(1 file)
98.09 KB,
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•6 years ago
|
||
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)
Comment 2•6 years ago
|
||
(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•6 years ago
|
||
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+
![]() |
Assignee | |
Comment 4•6 years ago
|
||
> 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.
![]() |
Assignee | |
Comment 5•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/396ff2a152d77a0c28f09fefc8b0a96659f2d4ba Bug 1407103 - Convert wstring attributes to AString in widget/nsIPrint*.idl. r=bobowen.
![]() |
||
Comment 6•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/396ff2a152d7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•