Last Comment Bug 835543 - Remove class "nsNavHistoryFullVisitResultNode" because it's never used or instantiated, and remove its interface nsINavHistoryFullVisitResultNode
: Remove class "nsNavHistoryFullVisitResultNode" because it's never used or ins...
Status: RESOLVED FIXED
: addon-compat
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla21
Assigned To: Daniel Holbert [:dholbert]
:
: Marco Bonardo [::mak]
Mentors:
Depends on: 836094
Blocks: 409662
  Show dependency treegraph
 
Reported: 2013-01-28 13:40 PST by Daniel Holbert [:dholbert]
Modified: 2013-01-30 05:21 PST (History)
2 users (show)
dholbert: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1: remove nsNavHistoryFullVisitResultNode (3.73 KB, patch)
2013-01-28 13:47 PST, Daniel Holbert [:dholbert]
mak77: review+
Details | Diff | Splinter Review
part 2: remove interface nsINavHistoryFullVisitResultNode (4.45 KB, patch)
2013-01-28 22:38 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
part 2 v2: remove interface nsINavHistoryFullVisitResultNode and uses of RESULT_TYPE_FULL_VISIT (7.78 KB, patch)
2013-01-28 22:49 PST, Daniel Holbert [:dholbert]
mak77: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2013-01-28 13:40:25 PST
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] 2013-01-28 13:47:14 PST
Created attachment 707275 [details] [diff] [review]
part 1: remove nsNavHistoryFullVisitResultNode
Comment 2 Marco Bonardo [::mak] 2013-01-28 18:34:30 PST
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] 2013-01-28 18:35:14 PST
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] 2013-01-28 22:27:58 PST
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] 2013-01-28 22:38:45 PST
Created attachment 707457 [details] [diff] [review]
part 2: remove interface nsINavHistoryFullVisitResultNode
Comment 6 Daniel Holbert [:dholbert] 2013-01-28 22:49:45 PST
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?)
Comment 7 Marco Bonardo [::mak] 2013-01-29 09:26:19 PST
(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] 2013-01-29 09:36:56 PST
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] 2013-01-29 14:45:01 PST
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] 2013-01-29 14:45:41 PST
This shouldn't affect add-ons due to its incomplete state, but just for burocracy setting addon-compat :)
Comment 11 Daniel Holbert [:dholbert] 2013-01-29 14:48:51 PST
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.)

Note You need to log in before you can comment on or make changes to this bug.