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

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Daniel Holbert [:dholbert]]

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

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1: remove nsNavHistoryFullVisitResultNode

Comment 2

[Marco Bonardo [:mak]]

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

Comment 3

[Marco Bonardo [:mak]]

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.

Comment 4

[Daniel Holbert [:dholbert]]

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

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: part 2: remove interface nsINavHistoryFullVisitResultNode

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: 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?)

Comment 7

[Marco Bonardo [:mak]]

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

Comment 8

[Marco Bonardo [:mak]]

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.

Comment 9

[Daniel Holbert [:dholbert]]

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

Comment 10

[Marco Bonardo [:mak]]

This shouldn't affect add-ons due to its incomplete state, but just for burocracy setting addon-compat :)

Comment 11

[Daniel Holbert [:dholbert]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/66bedf7bbf02
https://hg.mozilla.org/mozilla-central/rev/41b0df3dd995