Closed Bug 1123836 Opened 6 years ago Closed 6 years ago

GetPath() impls in nsDeviceContextSpec* are unused/useless & should be removed

Categories

(Core :: Widget, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file)

Several nsDeviceContextSpec[PLATFORM] implementations have this method...
 NS_IMETHOD GetPath (const char** aPath);
...which looks like it's implementing an interface method, but is not.  (and hence is unused externally)

MXR search showing this code:
https://mxr.mozilla.org/mozilla-central/search?string=GetPath&find=nsDeviceContextSpec

These methods can all go away.

HISTORICAL NOTE (feel free to ignore):
It looks like this dates back to this change in nsDeviceContextSpecG...
{
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/widget/src/gtk2&command=DIFF_FRAMESET&file=nsDeviceContextSpecG.h&rev2=1.4&rev1=1.3
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/widget/src/gtk2&command=DIFF_FRAMESET&file=nsDeviceContextSpecG.cpp&rev2=1.5&rev1=1.4
}
...which was just adding a bunch of accessors (in a needlessly-XPCOM-flavored way), AFAICT. And then nsDeviceContextSpecAndroid and nsDeviceContextSpecQt cribbed from that.
Attached patch fix v1Splinter Review
Attachment #8551942 - Flags: review?(roc)
(nsDeviceContextSpecGTK::GetPath() has one caller -- GetSurfaceForPrinter -- but that's another nsDeviceContextSpecGTK method, which can just use 'mPath' instead of calling GetPath, as shown in the patch.)
(FWIW, I found this bug while annotating all of the "NS_IMETHOD" methods in nsDeviceContextSpecG.h as MOZ_OVERRIDE. That triggered a compile error for GetPath(), because it doesn't actually override anything.)
No longer blocks: 1123844
Comment on attachment 8551942 [details] [diff] [review]
fix v1

karl, would you be up for reviewing this? I just noticed he's got quite a review queue, so it'd be better if I could farm this out onto someone else. (I originally tagged him since this is nominally a cross-/widget change, but really Widget|GTK is the only place this method is used, and the other impls seem to just be cribbed from that & unused.)
Attachment #8551942 - Flags: review?(roc) → review?(karlt)
s/noticed he's/noticed roc's/
The checkin comment, with no bug, was
"Added UNIX support for postscript printing, fixed many bugs.
 Added nsIDeviceContextSpecPS interface so we can do PS on any platoform, get
 information from the print dialog."

I think these missed being included in --disable-postscript in bug 80625 and
so didn't get removed at the same time as removal of nsDeviceContextSpecPS in
these changesets
https://hg.mozilla.org/mozilla-central/rev/ec737b4efe18
https://hg.mozilla.org/mozilla-central/rev/693f69afaff5
Depends on: 383889
Attachment #8551942 - Flags: review?(karlt) → review+
Ah, good observation! That second cset (693f69afaff5) did remove an invocation of GetPath() on a nsIDeviceContextSpecPS* pointer. That must be what this API was for.

Anyway -- thanks for the review & archeology! I'll land this when the tree reopens.
https://hg.mozilla.org/mozilla-central/rev/f7439bc62b8b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.