Status

()

enhancement
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(3 attachments)

No description provided.
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.
Attachment #9005122 - Flags: review?(nika)
As per the previous commit, they are now subsumed by OnHistoryGotoIndex.
Attachment #9005123 - Flags: review?(nika)
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.)
Attachment #9005124 - Flags: review?(nika)
> (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?
Flags: needinfo?(nika)
Attachment #9005122 - Flags: review?(nika) → review+
Attachment #9005123 - Flags: review?(nika) → review+
Attachment #9005124 - Flags: review?(nika) → review+
(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.
Flags: needinfo?(nika)
> 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.
Pushed by nnethercote@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/b1eb8b58455a
https://hg.mozilla.org/mozilla-central/rev/4007042881de
https://hg.mozilla.org/mozilla-central/rev/ad4d77a3b69b
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.