Closed Bug 910161 Opened 7 years ago Closed 7 years ago

Remove nsIHistoryEntry and replace it with nsISHEntry

Categories

(Core Graveyard :: History: Global, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: ttaubert, Assigned: ttaubert)

Details

(Keywords: addon-compat)

Attachments

(1 file)

It seems like nsIHistoryEntry is a legacy interface that we actually don't use anymore. In most of the places it's QI'ed to nsISHEntry shortly after calling nsISHistory.GetEntryAtIndex().
Summary: Merge nsIHistoryEntry into nsIEntry or remove nsIHistoryEntry entirely → Merge nsIHistoryEntry into nsISHEntry or remove nsIHistoryEntry entirely
I have no clue about our interface retention policies and whether we can just remove nsIHistoryEntry? The patch does that.

If we can't remove the interface for backwards compatibility reasons, can we just make it an empty one extending nsISHEntry and mark it explicitly as something that's only around for bc?

I think that would reduce confusion when stumbling upon code that checks for and uses nsIHistoryEntry objects.
Attachment #796564 - Flags: feedback?(bzbarsky)
https://mxr.mozilla.org/addons/search?string=nsihistoryentry&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons suggests some addons are using it.  Removing it would break them.

Making nsIHistoryEntry empty would work if all those addons are doing no-op QIs (as in they already have an nsIHistoryEntry or nsISHEntry object in JS).  On the other hand, if they're relying on the interface flattening behavior of QI it would break them.

Jorge, how much do we need to worry about preserving compat with this set of addons?
Flags: needinfo?(jorge)
There are still a few of those that are still relevant (and Collusion is one of ours). If the fix on the add-on side is straightforward, like "replace nsIHistoryEntry with nsISHEntry", then it's fine to move forward without a compatibility shim. It's very easy for us to detect specific interface usage for our compatibility bumps, so we can notify the developers when the time is right.
Flags: needinfo?(jorge)
Keywords: addon-compat
Comment on attachment 796564 [details] [diff] [review]
remove nsIHistoryEntry

In that case, this looks reasonable.
Attachment #796564 - Flags: feedback?(bzbarsky) → feedback+
Attachment #796564 - Flags: review?(bzbarsky)
Summary: Merge nsIHistoryEntry into nsISHEntry or remove nsIHistoryEntry entirely → Remove nsIHistoryEntry and replace it with nsISHEntry
Comment on attachment 796564 [details] [diff] [review]
remove nsIHistoryEntry

r=me
Attachment #796564 - Flags: review?(bzbarsky) → review+
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/7fbbfe346cad
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.