Closed Bug 1487296 Opened 1 year ago Closed 1 year ago

Merge nsISHistoryInternal into nsISHistory

Categories

(Core :: DOM: Navigation, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(3 files, 1 obsolete file)

With legacy add-ons gone, there is no benefit to keeping them separate.
Mostly:
- Fix indenting.
- Remove trailing whitespace.
- Use `aFoo` arguments.
Attachment #9005097 - Flags: review?(nika)
With legacy add-ons gone, there is no benefit to keeping them separate.
Attachment #9005098 - Flags: review?(nika)
Attached patch Remove nsISHistoryInternal usage (obsolete) — Splinter Review
jorgk: as usual, this is untested, but I think it's all that is needed.
Attachment #9005129 - Flags: review?(jorgk)
Thanks Nick, I'll take care of it when the time comes. Not a huge patch this time ;-)
Priority: -- → P3
Attachment #9005097 - Flags: review?(nika) → review+
Comment on attachment 9005098 [details] [diff] [review]
Merge nsISHistoryInternal into nsISHistory

Review of attachment 9005098 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/shistory/nsISHistory.idl
@@ +178,5 @@
> +   *
> +   * @param aIndex            The index at which the entry should be replaced.
> +   * @param aReplaceEntry     The replacement entry for the index.
> +   */
> +   void replaceEntry(in long aIndex, in nsISHEntry aReplaceEntry);

nit: indenting.

@@ +191,5 @@
> +   *                          interface.
> +   *
> +   * @return                  Whether the operation can proceed.
> +   */
> +   boolean notifyOnHistoryReload(in nsIURI aReloadURI,

nit: something's up with the indenting here.
Attachment #9005098 - Flags: review?(nika) → review+
Comment on attachment 9005129 [details] [diff] [review]
Remove nsISHistoryInternal usage

Nick, the change is in suite/, that's for SeaMonkey. That's pretty broken on trunk, but if we want to be nice, we can help them and not break it even more.

Somehow you missed one call site:
https://searchfox.org/comm-central/search?q=nsISHistoryInternal&case=false&regexp=false&path=suite%2F
suite/browser/tabbrowser.xml
2011	.createInstance(Ci.nsISHistoryInternal);
suite/components/sessionstore/nsSessionStore.js
2606	history.QueryInterface(Ci.nsISHistoryInternal);

Should we fix both? Clearing review for now since it stated bugging me as overdue after only two days(?).
Attachment #9005129 - Flags: review?(jorgk)
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a18c6a79fb9
Fix nsIHistory.idl formatting. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2ac39e3e6b7
Merge nsISHistoryInternal into nsISHistory. r=nika
Attachment #9005129 - Attachment is obsolete: true
Jorg: good catch! I've updated the patch to adjust the tabbrowser.xml usage as well. TBH, I don't know how that code even works, because the string "@mozilla.org/browser/shistory;1" doesn't have any XPCOM component associated with it. But there is another code location that is similar, but using nsISHistory instead of nsISHistoryInternal:

https://searchfox.org/comm-central/source/suite/components/helpviewer/content/help.js#131

so I've just copied that. Hopefully it'll work.
Comment on attachment 9005922 [details] [diff] [review]
Remove nsISHistoryInternal usage

Thanks Nick, I don't think we'll make it worse.
Attachment #9005922 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/46b2f0832f60
Remove nsISHistoryInternal usage in C-C's suite/ code. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
FRG, you might want to take a look at what got landed here.
Flags: needinfo?(frgrahl)
Thanks jorgk. Looks good.
Flags: needinfo?(frgrahl)
You need to log in before you can comment on or make changes to this bug.