Last Comment Bug 741755 - Browser API: Add canGoBack and canGoForward properties to browser elements
: Browser API: Add canGoBack and canGoForward properties to browser elements
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
: 708179 (view as bug list)
Depends on: 764130 764232 764248
Blocks: browser-api
  Show dependency treegraph
 
Reported: 2012-04-03 06:25 PDT by Ben Francis [:benfrancis]
Modified: 2013-06-27 06:18 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Add getCanGoBack, getCanGoForward methods (6.99 KB, patch)
2012-06-13 13:34 PDT, Justin Lebar (not reading bugmail)
mounir: review+
Details | Diff | Splinter Review
Part 2: Add tests for getCanGo{Back,Forward} (4.94 KB, patch)
2012-06-13 13:34 PDT, Justin Lebar (not reading bugmail)
mounir: review+
Details | Diff | Splinter Review
Part 3: Add goBack/goForward. (3.91 KB, patch)
2012-06-13 15:05 PDT, Justin Lebar (not reading bugmail)
bugs: review+
Details | Diff | Splinter Review
Part 4: Tests for goBack / goForward (5.92 KB, patch)
2012-06-13 15:05 PDT, Justin Lebar (not reading bugmail)
bugs: review+
Details | Diff | Splinter Review

Description Ben Francis [:benfrancis] 2012-04-03 06:25:19 PDT
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.
Comment 1 Mounir Lamouri (:mounir) 2012-04-03 06:43:06 PDT
If we do that, for the sack of the platform, can we do that on a different element than iframe?
Comment 2 Justin Lebar (not reading bugmail) 2012-04-03 13:39:11 PDT
This would be an easy one for you to start with, Ben.  Let me know if you want me to step you through it.
Comment 3 Justin Lebar (not reading bugmail) 2012-06-12 09:44:46 PDT
It doesn't make much sense to do CanGoBack without GoBack, so I'll probably do both in this bug.
Comment 4 Justin Lebar (not reading bugmail) 2012-06-12 18:11:43 PDT
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.
Comment 5 Ben Francis [:benfrancis] 2012-06-13 04:51:56 PDT
*** Bug 708179 has been marked as a duplicate of this bug. ***
Comment 6 Ben Francis [:benfrancis] 2012-06-13 04:56:09 PDT
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?
Comment 7 Justin Lebar (not reading bugmail) 2012-06-13 04:58:31 PDT
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.
Comment 8 Justin Lebar (not reading bugmail) 2012-06-13 13:34:01 PDT
Created attachment 632848 [details] [diff] [review]
Part 1: Add getCanGoBack, getCanGoForward methods
Comment 9 Justin Lebar (not reading bugmail) 2012-06-13 13:34:14 PDT
Created attachment 632849 [details] [diff] [review]
Part 2: Add tests for getCanGo{Back,Forward}
Comment 10 Justin Lebar (not reading bugmail) 2012-06-13 13:36:50 PDT
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.
Comment 11 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-06-13 14:07:12 PDT
(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 :/
Comment 12 Justin Lebar (not reading bugmail) 2012-06-13 14:24:52 PDT
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...
Comment 13 Justin Lebar (not reading bugmail) 2012-06-13 15:05:50 PDT
Created attachment 632901 [details] [diff] [review]
Part 3: Add goBack/goForward.
Comment 14 Justin Lebar (not reading bugmail) 2012-06-13 15:05:54 PDT
Created attachment 632902 [details] [diff] [review]
Part 4: Tests for goBack / goForward
Comment 15 Justin Lebar (not reading bugmail) 2012-06-13 15:11:31 PDT
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.
Comment 16 Mounir Lamouri (:mounir) 2012-06-18 07:01:00 PDT
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?
Comment 17 Justin Lebar (not reading bugmail) 2012-06-18 07:06:35 PDT
> 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...
Comment 18 Mounir Lamouri (:mounir) 2012-06-19 05:53:41 PDT
(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.
Comment 19 Justin Lebar (not reading bugmail) 2012-06-19 07:41:37 PDT
> 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.
Comment 20 Mounir Lamouri (:mounir) 2012-06-19 09:03:49 PDT
(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?
Comment 21 Justin Lebar (not reading bugmail) 2012-06-19 09:14:14 PDT
> 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).
Comment 22 Justin Lebar (not reading bugmail) 2012-06-19 09:27:01 PDT
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.
Comment 23 Justin Lebar (not reading bugmail) 2012-06-19 09:28:13 PDT
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.
Comment 24 Justin Lebar (not reading bugmail) 2012-06-20 10:36:00 PDT
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.
Comment 25 Mounir Lamouri (:mounir) 2012-06-21 10:15:16 PDT
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
Comment 26 Justin Lebar (not reading bugmail) 2012-06-21 10:19:59 PDT
> 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?
Comment 27 Mounir Lamouri (:mounir) 2012-06-21 10:22:00 PDT
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
Comment 28 Mounir Lamouri (:mounir) 2012-06-21 10:22:29 PDT
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?
Comment 29 Mounir Lamouri (:mounir) 2012-06-21 10:24:38 PDT
(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.
Comment 30 Justin Lebar (not reading bugmail) 2012-06-21 10:41:53 PDT
> 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().
Comment 31 Mounir Lamouri (:mounir) 2012-06-21 10:49:51 PDT
(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 ;)

Note You need to log in before you can comment on or make changes to this bug.