Closed
Bug 835543
Opened 12 years ago
Closed 12 years ago
Remove class "nsNavHistoryFullVisitResultNode" because it's never used or instantiated, and remove its interface nsINavHistoryFullVisitResultNode
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
(Keywords: addon-compat)
Attachments
(2 files, 1 obsolete file)
3.73 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
7.78 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
I think the class nsNavHistoryFullVisitResultNode is completely unused.
Here are the instances of its name occurring in the codebase:
https://mxr.mozilla.org/mozilla-central/search?string=nsNavHistoryFullVisitResultNode
That turns up the class declaration and constructor declaration (in the header file), and the class's NS_IMPL_ISUPPORTS_INHERITED1 line and its constructor implementation (in the .cpp file).
Notably, there are no variable-declarations of this type (on the stack or the heap), and no derived classes, and no static helper-methods dangling off of this class. So, AFAICT, the class is completely unused.
Some hg archeology shows me that the last instance of this class was removed for bug 700296, here:
https://hg.mozilla.org/mozilla-central/rev/457b6bafbf3a#l8.207
Assignee | ||
Comment 1•12 years ago
|
||
Comment 2•12 years ago
|
||
yes, basically the plan was bug 409662, but it has never been completed and we don't plan to. I'm fine with killing this and wontfix bug 409662 (will do now).
btw, the class is currently implementing nsINavHistoryFullVisitResultNode, so that interface should also be removed, along with its consumers:
mxr.mozilla.org/mozilla-central/search?string=nsINavHistoryFullVisitResultNode
A couple notes on the removal:
1. the RESULT_TYPE_FULL_VISIT should be handled like we did with RESULT_TYPE_DYNAMIC_CONTAINER (see the comment above it)
2. asFullVisit appears twice, as a function and as a method, both should be removed
Blocks: 409662
Comment 3•12 years ago
|
||
Comment on attachment 707275 [details] [diff] [review]
part 1: remove nsNavHistoryFullVisitResultNode
Review of attachment 707275 [details] [diff] [review]:
-----------------------------------------------------------------
See comment 2, the removal is incomplete.
Attachment #707275 -
Flags: review?(mak77)
Assignee | ||
Comment 4•12 years ago
|
||
Comment on attachment 707275 [details] [diff] [review]
part 1: remove nsNavHistoryFullVisitResultNode
Rebranding the first patch as "part 1", then, since it's the first atomic part of this bug. (and we build & run correctly after it's applied. :) )
Attachment #707275 -
Attachment description: fix → part 1: remove nsNavHistoryFullVisitResultNode
Attachment #707275 -
Flags: review?(mak77)
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #707457 -
Flags: review?(mak77)
Assignee | ||
Updated•12 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Summary: Remove class "nsNavHistoryFullVisitResultNode" because it's never used or instantiated → Remove class "nsNavHistoryFullVisitResultNode" because it's never used or instantiated, and remove its interface nsINavHistoryFullVisitResultNode
Version: unspecified → Trunk
Assignee | ||
Comment 6•12 years ago
|
||
Now this also removes uses of RESULT_TYPE_FULL_VISIT, so that the "deprecated and unsupported" comment that I added is closer to the truth.
(I wonder if we should comment out those declarations, so that they can be placeholders for their "reserved" values without actually declaring those value-names and allowing code to accidentally use them?)
Attachment #707457 -
Attachment is obsolete: true
Attachment #707457 -
Flags: review?(mak77)
Attachment #707460 -
Flags: review?(mak77)
Comment 7•12 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #6)
> (I wonder if we should comment out those declarations, so that they can be
> placeholders for their "reserved" values without actually declaring those
> value-names and allowing code to accidentally use them?)
Yes, I thought the same yesterday, but didn't want to overload you with more stuff. If you are willing to do that, I'm definitely in favor of doing that.
Updated•12 years ago
|
Attachment #707275 -
Flags: review?(mak77) → review+
Comment 8•12 years ago
|
||
Comment on attachment 707460 [details] [diff] [review]
part 2 v2: remove interface nsINavHistoryFullVisitResultNode and uses of RESULT_TYPE_FULL_VISIT
Review of attachment 707460 [details] [diff] [review]:
-----------------------------------------------------------------
I'm happy to see this pointless complication die.
Attachment #707460 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 9•12 years ago
|
||
Cool, I commented-out the placeholder constants per comment 7 (and added newlines around them so they aren't just blurred into a giant comment block):
https://hg.mozilla.org/integration/mozilla-inbound/rev/66bedf7bbf02
https://hg.mozilla.org/integration/mozilla-inbound/rev/41b0df3dd995
Flags: in-testsuite-
Comment 10•12 years ago
|
||
This shouldn't affect add-ons due to its incomplete state, but just for burocracy setting addon-compat :)
Keywords: addon-compat
Assignee | ||
Comment 11•12 years ago
|
||
Also, I'm pretty sure the IDL change doesn't require revving any IIDs -- I'm completely removing one class (nsINavHistoryFullVisitResultNode), and removing constants from another class (nsINavHistoryResultNode), and I'm not tweaking any methods or anything. (And bz and Waldo both agree on that.)
Comment 12•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/66bedf7bbf02
https://hg.mozilla.org/mozilla-central/rev/41b0df3dd995
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in
before you can comment on or make changes to this bug.
Description
•