Ben Francis [:benfrancis] Reporter
	Description • 13 years ago

When creating a browser as a webapp it is common to want to know whether there is a page to go back to or a page to go forward to in the session history, to decide whether to display back/forward buttons or whether to disable them.

Privacy concerns were made about a previous proposal to allow content to access the index within session history https://bugzilla.mozilla.org/show_bug.cgi?id=708179 so this is an alternative proposal to add canGoBack and canGoForward properties to browser elements/mozbrowser iframes which reveal less information.

This is needed for the Gaia browser to be able to use the platform's session history instead of re-implementing the History API inside the app.

	Mounir Lamouri (:mounir)
	Comment 1 • 13 years ago

If we do that, for the sack of the platform, can we do that on a different element than iframe?

	Justin Lebar (not reading bugmail) Assignee
	Comment 2 • 13 years ago

This would be an easy one for you to start with, Ben.  Let me know if you want me to step you through it.

	Justin Lebar (not reading bugmail) Assignee
	Comment 3 • 12 years ago

It doesn't make much sense to do CanGoBack without GoBack, so I'll probably do both in this bug.

	Justin Lebar (not reading bugmail) Assignee
	Comment 4 • 12 years ago

Vivien, smaug, et al.: Using a DOMRequest feels pretty awful here:

  iframe.getCanGoBack().onsuccess = function(e) {
    var canGoBack = e.target.result;
    ...
  }

What about if we just use a callback:

  iframe.getCanGoBack(function(canGoBack) {
    ...
  });

For the sake of consistency, I'd probably change our screenshot code too.

	Ben Francis [:benfrancis] Reporter
	Comment 6 • 12 years ago

Yeah, canGoBack(callback) was the original suggestion on https://wiki.mozilla.org/WebAPI/BrowserAPI

What's the reason for adding the get prefix? Is this for consistency with something else?

	Justin Lebar (not reading bugmail) Assignee
	Comment 7 • 12 years ago

The |get| suggests to me that we're going through a callback; you can't do |iframe.canGoBack|.  But it's not a big deal either way.

We do need to figure out if we're going to use a DOMRequest, though.

	Justin Lebar (not reading bugmail) Assignee
	Comment 10 • 12 years ago

I left these using a DOMRequest because we don't seem to have quorum to decide whether we want to change it.

Next up: go{Back,Forward}().

Note that canGoBack (and, goBack) only works properly OOP.  It's more work to get it functioning properly in-process, and since we're not planning to run anything in-process, I don't think it's worth bothering with atm.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 11 • 12 years ago

(In reply to Justin Lebar [:jlebar] from comment #7)
> The |get| suggests to me that we're going through a callback; you can't do
> |iframe.canGoBack|.  But it's not a big deal either way.
> 
> We do need to figure out if we're going to use a DOMRequest, though.

My main issue with DOMRequest in this case is that I don't think you will ever need the DOMRequest.onerror part. So I would suggest to use a callback but it make me sad to not have unified APIs :/

	Justin Lebar (not reading bugmail) Assignee
	Comment 12 • 12 years ago

We never use the onerror part for getScreenshot, either.  But I guess your point is that we might, at some time in the future?  If we made that change in the future, nobody's going to watching onerror anyway...

	Justin Lebar (not reading bugmail) Assignee
	Comment 15 • 12 years ago

This is all straightforward.  The only slightly tricky change is in the first patch; I consolidated the DOMRequest code used by screenshots with the DOMRequest code used here.

smaug, you get this r?, but if you can't get to it by Monday, we can bump it to Mounir.

	Mounir Lamouri (:mounir)
	Comment 16 • 12 years ago

Can't we have synchronous canGoBack and canGoForward? And async goBack()/goForward() returning DOMRequest? Feels like we could easily cache that information to get it quickly, can't we?

	Justin Lebar (not reading bugmail) Assignee
	Comment 17 • 12 years ago

> Can't we have synchronous canGoBack and canGoForward?

shistory doesn't have "can go {back,forward} changed" events, at the moment.

Caching is complicated because things that aren't navigations can change canGoBack/Forward.  For example, removing an iframe can change things.

But I realize that the API we have now is insufficient to write a good back button implementation -- because we're not sending canGoBack events, the embedder has no way to detect when it needs to query canGoBack, except when the top-level page navigates, and that's insufficient.  So maybe we should figure out how to send canGoBack notifications, and then we can also have a synchronous cache, if we want.

> And async goBack()/goForward() returning DOMRequest?

Is the goal to detect failure, in the case when you try to go back but can't?  I'm not sure we care so much...

	Mounir Lamouri (:mounir)
	Comment 18 • 12 years ago

(In reply to Justin Lebar [:jlebar] from comment #17)
> > Can't we have synchronous canGoBack and canGoForward?
> 
> shistory doesn't have "can go {back,forward} changed" events, at the moment.
> 
> Caching is complicated because things that aren't navigations can change
> canGoBack/Forward.  For example, removing an iframe can change things.
> 
> But I realize that the API we have now is insufficient to write a good back
> button implementation -- because we're not sending canGoBack events, the
> embedder has no way to detect when it needs to query canGoBack, except when
> the top-level page navigates, and that's insufficient.  So maybe we should
> figure out how to send canGoBack notifications, and then we can also have a
> synchronous cache, if we want.

Indeed. Does it really make sense to push .canGoBack and .canGoForward if it is not really fixing the use case we have? Right now it seems like it would be used -at best- like:
f.getCanGoBack().onsuccess = function() {
  f.goBack();
}
Which would be more verbose than:
f.goBack();

> > And async goBack()/goForward() returning DOMRequest?
> 
> Is the goal to detect failure, in the case when you try to go back but
> can't?  I'm not sure we care so much...

The goal is to know when the action is done. Given that you can also call that method without being allowed to go back/forward, being tell when that happens wouldn't hurt.

	Justin Lebar (not reading bugmail) Assignee
	Comment 19 • 12 years ago

> Right now it seems like it would be used -at best- like:
> f.getCanGoBack().onsuccess = function() {
>   f.goBack();
> }
> Which would be more verbose than:
> f.goBack();

Well, no.  If you wanted to go back, you'd just goBack(), and if it fails, then whatever.

> Given that you can also call that method without being allowed to go back/forward, being tell when 
> that happens wouldn't hurt.

I guess.  Note that we silently ignore history.back() calls when you can't go back.

	Mounir Lamouri (:mounir)
	Comment 20 • 12 years ago

(In reply to Justin Lebar [:jlebar] from comment #19)
> > Right now it seems like it would be used -at best- like:
> > f.getCanGoBack().onsuccess = function() {
> >   f.goBack();
> > }
> > Which would be more verbose than:
> > f.goBack();
> 
> Well, no.  If you wanted to go back, you'd just goBack(), and if it fails,
> then whatever.

That was actually my point (very badly formed): there is no reason to use getCanGoBack() because the developer doesn't get informed about a state change so the only reason to use it would be before going back which is actually useless.
Or do we expect developers to call the method regularly in a setInterval? Or after each location change?

	Justin Lebar (not reading bugmail) Assignee
	Comment 21 • 12 years ago

> there is no reason to use getCanGoBack() [...]
> Or do we expect developers to call the method regularly in a setInterval? Or after each location 
> change?

The purpose of getCanGoBack is mostly so that we can enable/disable the back button as appropriate.  But like I said earlier, we really need an event that says "back button will work now," because we don't want to use an interval, and the back button can become disabled after something that's not a locationchange (e.g. iframe removal).

	Justin Lebar (not reading bugmail) Assignee
	Comment 22 • 12 years ago

Comment on attachment 632848 [details] [diff] [review]
Part 1: Add getCanGoBack, getCanGoForward methods

Clearing review request; per discussion with Mounir, I think this needs to be reworked.

	Justin Lebar (not reading bugmail) Assignee
	Comment 23 • 12 years ago

Comment on attachment 632901 [details] [diff] [review]
Part 3: Add goBack/goForward.

We can still have goBack without canGoBack, though, so I'm leaving this r? here.  It would be nice to use real shistory in the B2G browser sooner rather than later.

	Justin Lebar (not reading bugmail) Assignee
	Comment 24 • 12 years ago

It looks like desktop Firefox queries CanGoBack/CanGoForward whenever it gets an onlocationchange event.  It's not clear to me whether we're talking onlocationchange in the top-level frame or onlocationchange in any frame.

But I think given that, we can add canGoBack to mozbrowser and have the Gaia browser query whenever it gets an onlocationchange event (which is only for the top-level browsing context).  It's not 100% correct, and we can fix it later, but it's more important that Gaia have something 95% correct, right now.

	Mounir Lamouri (:mounir)
	Comment 25 • 12 years ago

Comment on attachment 632848 [details] [diff] [review]
Part 1: Add getCanGoBack, getCanGoForward methods

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

::: dom/browser-element/BrowserElementChild.js
@@ +337,5 @@
>      }
>    },
>  
> +  _recvCanGoBack: function(data) {
> +    var webNav = docShell.QueryInterface(Ci.nsIWebNavigation);

Shouldn't you check that webNav isn't null?

@@ +345,5 @@
> +    });
> +  },
> +
> +  _recvCanGoForward: function(data) {
> +    var webNav = docShell.QueryInterface(Ci.nsIWebNavigation);

ditto

	Justin Lebar (not reading bugmail) Assignee
	Comment 26 • 12 years ago

> Shouldn't you check that webNav isn't null?

When QI fails in JS, don't you get an exception?  But don't all docshells implement nsIWebNavigation?

	Mounir Lamouri (:mounir)
	Comment 27 • 12 years ago

Comment on attachment 632849 [details] [diff] [review]
Part 2: Add tests for getCanGo{Back,Forward}

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

::: dom/browser-element/mochitest/browserElement_CanGoBack.js
@@ +15,5 @@
> +  // crosses the mozbrowser boundary.  It's like the mozbrowser wasn't there;
> +  // canGoBack reflects whether the top-level frame can go back, not whether the
> +  // iframe itself can go back.
> +  if (!browserElementTestHelpers.getOOPByDefaultPref()) {
> +    ok(true, "Nothing to do here.");

I would put:
ok(false, "We should not be there!");
Given that there is only an OOP test file.

@@ +37,5 @@
> +
> +function checkCanGoBackAndForward(canGoBack, canGoForward, nextTest) {
> +  var seenCanGoBackResult = false;
> +  iframe.getCanGoBack().onsuccess = function(e) {
> +    seenCanGoBackResult = true;

Can you add is(seenCanGoBackResult, false, "The success handler shouldn't be called twice.");
before seenCanGoBackResult = true;

@@ +44,5 @@
> +  };
> +
> +  var seenCanGoForwardResult = false;
> +  iframe.getCanGoForward().onsuccess = function(e) {
> +    seenCanGoForwardResult = true;

same here

	Mounir Lamouri (:mounir)
	Comment 28 • 12 years ago

Comment on attachment 632902 [details] [diff] [review]
Part 4: Tests for goBack / goForward

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

::: dom/browser-element/mochitest/browserElement_CanGoBack.js
@@ +89,5 @@
> +  addOneShotIframeEventListener('mozbrowserlocationchange', function(e) {
> +    is(e.detail, browserElementTestHelpers.emptyPage2);
> +    checkCanGoBackAndForward(true, false, SimpleTest.finish);
> +  });
> +  iframe.goForward();

Can you add a test that test the situation where you can go back *and* forward?

	Mounir Lamouri (:mounir)
	Comment 29 • 12 years ago

(In reply to Justin Lebar [:jlebar] from comment #26)
> > Shouldn't you check that webNav isn't null?
> 
> When QI fails in JS, don't you get an exception?

Maybe. I very rarely use JS in Firefox chrome.

> But don't all docshells implement nsIWebNavigation?

Sure. We can actually assume that if that changes, it will be caught by your tests.

Forget that comment.

	Justin Lebar (not reading bugmail) Assignee
	Comment 30 • 12 years ago

> Can you add a test that test the situation where you can go back *and* forward?

I will, but I'll add it to the second set of tests (part 4) -- it's easier to get into this situation when you have goBack().

	Mounir Lamouri (:mounir)
	Comment 31 • 12 years ago

(In reply to Justin Lebar [:jlebar] from comment #30)
> > Can you add a test that test the situation where you can go back *and* forward?
> 
> I will, but I'll add it to the second set of tests (part 4) -- it's easier
> to get into this situation when you have goBack().

The comment was made on part 4 actually ;)

	Justin Lebar (not reading bugmail) Assignee
	Comment 32 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9ce1a042083
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0f3399cea3
https://hg.mozilla.org/integration/mozilla-inbound/rev/b21ba17e03af
https://hg.mozilla.org/integration/mozilla-inbound/rev/31c540b02399

	Ed Morley [:emorley]
	Comment 33 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/c9ce1a042083
https://hg.mozilla.org/mozilla-central/rev/2a0f3399cea3
https://hg.mozilla.org/mozilla-central/rev/b21ba17e03af
https://hg.mozilla.org/mozilla-central/rev/31c540b02399

	Jeremie Patonnier :Jeremie
	Comment 34 • 11 years ago

Documentation available here:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement.getCanGoBack
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement.getCanGoForward
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement.goBack
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement.goForward