Remove class "nsNavHistoryFullVisitResultNode" because it's never used or instantiated, and remove its interface nsINavHistoryFullVisitResultNode

RESOLVED FIXED in mozilla21

Status

()

Toolkit
Places
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

({addon-compat})

Trunk
mozilla21
addon-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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

5 years ago
Created attachment 707275 [details] [diff] [review]
part 1: remove nsNavHistoryFullVisitResultNode
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #707275 - Flags: review?(mak77)
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 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

5 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

5 years ago
Created attachment 707457 [details] [diff] [review]
part 2: remove interface nsINavHistoryFullVisitResultNode
Attachment #707457 - Flags: review?(mak77)
(Assignee)

Updated

5 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

5 years ago
Created attachment 707460 [details] [diff] [review]
part 2 v2: remove interface nsINavHistoryFullVisitResultNode and uses of RESULT_TYPE_FULL_VISIT

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

5 years ago
Attachment #707275 - Flags: review?(mak77) → review+
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

5 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-
This shouldn't affect add-ons due to its incomplete state, but just for burocracy setting addon-compat :)
Keywords: addon-compat
(Assignee)

Comment 11

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

Updated

5 years ago
Depends on: 836094
https://hg.mozilla.org/mozilla-central/rev/66bedf7bbf02
https://hg.mozilla.org/mozilla-central/rev/41b0df3dd995
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.