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

RESOLVED FIXED in mozilla38

Status

()

Core
Widget
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla38
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8551942 [details] [diff] [review]
fix v1
Attachment #8551942 - Flags: review?(roc)
(Assignee)

Comment 2

4 years ago
(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.)
(Assignee)

Comment 3

4 years ago
(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.)
(Assignee)

Updated

4 years ago
Blocks: 1123844
(Assignee)

Updated

4 years ago
No longer blocks: 1123844
(Assignee)

Comment 5

4 years ago
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)
(Assignee)

Comment 6

4 years ago
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+
(Assignee)

Comment 8

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.