Closed
Bug 1123836
Opened 9 years ago
Closed 9 years ago
GetPath() impls in nsDeviceContextSpec* are unused/useless & should be removed
Categories
(Core :: Widget, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(1 file)
5.70 KB,
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Attachment #8551942 -
Flags: review?(roc)
Assignee | ||
Comment 2•9 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•9 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 | ||
Comment 4•9 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=50dec32593c7
Assignee | ||
Comment 5•9 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•9 years ago
|
||
s/noticed he's/noticed roc's/
Comment 7•9 years ago
|
||
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
Updated•9 years ago
|
Attachment #8551942 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 8•9 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.
Assignee | ||
Comment 9•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7439bc62b8b
Flags: in-testsuite-
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f7439bc62b8b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•