1487328 - Streamline nsSHistory

Status: RESOLVED FIXED

(Core :: DOM: Navigation, enhancement)

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Clarify which nsIWebNavigation methods in nsSHistory are unreachable

The only ones that are reachable are GotoIndex(), GetCurrentURI(), and
Reload(). This patch makes all the others crash.

Note that the reason that Go{Back,Forward}() are unreachable is because of this
behaviour in ChildHistory::Go():

  // XXX(nika): Should we turn Go(-1) and Go(1) to call GoForward and GoBack?
  // They technically fire different change events but I couldn't find anyone
  // who cares, so I'm inclined not to.

I.e. those functions are now subsumed by GotoIndex().

As for CanGo{Back,Forward}(), a try push plus code inspection confirm that they
are unused.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Remove nsISHistoryListener.OnHistoryGo{Back,Forward}

As per the previous commit, they are now subsumed by OnHistoryGotoIndex.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Make nsSHistory *not* implement nsIWebNavigation

nsSHistory currently implements nsIWebNavigation, but only three of the methods
are actually used, and the rest call MOZ_CRASH. This patch removes the
inheritance and changes the implementations of those three methods (Reload(),
GetCurrentURI(), and GotoIndex()) to pure C++.

There is one test, bug662200_window.xul, that calls Reload() from JS, which is
no longer possible. Fortunately, nsSHistory::ReloadCurrentEntry() -- which *is*
available from JS -- is similar enough to Reload(0) that it can be used
instead.

(The only difference between Reload(0) and ReloadCurrentEntry() is that the
former triggers the `OnHistoryReload` notification and the latter triggers
`OnHistoryGotoIndex`, which doesn't matter for this test.)

Comment 4

[Nicholas Nethercote [inactive]]

> (The only difference between Reload(0) and ReloadCurrentEntry() is that the
> former triggers the `OnHistoryReload` notification and the latter triggers
> `OnHistoryGotoIndex`, which doesn't matter for this test.)

It's possible that this difference is a bug, BTW. Both Reload() and ReloadCurrentEntry() call LoadEntry() with a `HIST_CMD_RELOAD` argument, which suggests that ReloadCurrentEntry() should trigger `OnHistoryReload` instead of `OnHistoryGotoIndex`.

Nika, do you have an opinion here?

Comment 5

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1256e30b6e2ff8d7bbcc712a42dfa7a12ee0224

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> > (The only difference between Reload(0) and ReloadCurrentEntry() is that the
> > former triggers the `OnHistoryReload` notification and the latter triggers
> > `OnHistoryGotoIndex`, which doesn't matter for this test.)
> 
> It's possible that this difference is a bug, BTW. Both Reload() and
> ReloadCurrentEntry() call LoadEntry() with a `HIST_CMD_RELOAD` argument,
> which suggests that ReloadCurrentEntry() should trigger `OnHistoryReload`
> instead of `OnHistoryGotoIndex`.
> 
> Nika, do you have an opinion here?

My initial impression is that it is a bug for exactly the reasons you mention. I'm curious as to what places call `ReloadCurrentEntry` and if any of them require it to be a GotoIndex instead of a Reload.

Comment 7

[Nicholas Nethercote [inactive]]

> My initial impression is that it is a bug for exactly the reasons you mention.
> I'm curious as to what places call `ReloadCurrentEntry` and if any of them require
> it to be a GotoIndex instead of a Reload.

I will investigate this in a follow-up.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1eb8b58455a
Clarify which nsIWebNavigation methods in nsSHistory are unreachable. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/4007042881de
Remove nsISHistoryListener.OnHistoryGo{Back,Forward}. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad4d77a3b69b
Make nsSHistory *not* implement nsIWebNavigation. r=nika

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/b1eb8b58455a
https://hg.mozilla.org/mozilla-central/rev/4007042881de
https://hg.mozilla.org/mozilla-central/rev/ad4d77a3b69b