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

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 2

[Daniel Holbert [:dholbert]]

(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.)

Comment 3

[Daniel Holbert [:dholbert]]

(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.)

Comment 4

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=50dec32593c7

Comment 5

[Daniel Holbert [:dholbert]]

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.)

Comment 6

[Daniel Holbert [:dholbert]]

s/noticed he's/noticed roc's/

Comment 7

[Karl Tomlinson (:karlt)]

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

Comment 8

[Daniel Holbert [:dholbert]]

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.

Comment 9

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7439bc62b8b

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f7439bc62b8b