741717 - Browser API : Add a reload method to browser elements

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Francis [:benfrancis]]

Currently if you want to refresh a page loaded inside a browser element/mozbrowser iframe you have to reset the src attribute which seems to work but is a little clunky.

It would be nice to add a reload() method to the browser element, similar to the one <xul:browser> elements hav,e as a more explicit API for doing this.

<xul:browser> also has a reloadWithFlags() method which allows you to do things like bypass the cache and force a full reload, we could consider adding something like this too.

Comment 1

[Justin Lebar (not reading bugmail)]

Dale and I originally thought that we could just reset src to accomplish this.  But of course that would truncate session history, which we don't want!  So we do need this method.

Comment 2

[Mounir Lamouri (:mounir)]

Can't we use window.location.reload() or would that truncate session history too?

Comment 3

[Justin Lebar (not reading bugmail)]

(In reply to Mounir Lamouri (:mounir) from comment #2)
> Can't we use window.location.reload() or would that truncate session history
> too?

For an OOP iframe, iframe.contentWindow is null.

Comment 4

[Dale Harvey (:daleharvey)]

Attachment: WIP: Add a reload method to browser elements

I have a strange problem in that this breaks the SecurityChange event (in process)

238 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/browser-element/mochitest/test_browserElement_inproc_SecurityChange.html | undefined - got secure, expected insecure

trying to investigate, I also likely need more tests but I cant think of what else to test.

Comment 5

[Justin Lebar (not reading bugmail)]

>+  _recvReload: function(data) {
>+    try {
>+      let nsIWebNavigation = docShell.QueryInterface(Ci.nsIWebNavigation);
>+      nsIWebNavigation.reload(nsIWebNavigation.LOAD_FLAGS_NONE);

FYI, we'd send FLAGS_NONE for a "normal" reload, and BYPASS_PROXY | BYPASS_CACHE for a shift-reload (see browser/base/content/browser.js::BrowserReloadSkipCache()).  On a phone, we don't have a shift-reload, so we might want normal reload to act like shift reload, or perhaps we should let the embedder ask for a normal/shift reload.  But I think this is fine for now.

>+    } catch(e) {
>+      // Silently swallow errors; these can happen if a used cancels reload
>+    }

Can you elaborate on how you got an error here?

>+/* Any copyright is dedicated to the public domain.
>+   http://creativecommons.org/publicdomain/zero/1.0/ */
>+
>+// Bug 741717 - Test the reload ability of <iframe mozbrowser>.

I think this test is sufficient if we don't add shift-reload semantics.

I have no idea why this is breaking the securitychange test.  :-/  Does the test still break if you don't reload here?  Does the test still break if you put the SimpleTest.finish() behind a long-ish setTimeout?

Comment 6

[Dale Harvey (:daleharvey)]

Attachment: Bug 751717: Add a reload method to browser elements

Currently figuring out how this breaks security change notifications

Comment 7

[Dale Harvey (:daleharvey)]

Security (location) related bug is filed seperately

https://bugzilla.mozilla.org/show_bug.cgi?id=741717

Will rebase and push this to try with in process tests disabled.

Comment 8

[Dale Harvey (:daleharvey)]

Attachment: Bug 751717 - Add a reload method to browser elements. r=jlebar

Justin

I am having problems pushing this to try, 

https://github.com/mozilla/mozilla-central/commits/master and
http://hg.mozilla.org/mozilla-central/

are diverged (I dont understand why) and git push-to-try is trying to push what looks like a few hundred commits

Although I can import the patch to mercurial, when I try to qpush I get a bunch of conflicts, will figure out hg eventually but in the meantime if you could push that would help me, thanks, sorry for the bother.

Comment 9

[Dale Harvey (:daleharvey)]

>>+    } catch(e) {
>>+      // Silently swallow errors; these can happen if a used cancels reload
>>+    }

> Can you elaborate on how you got an error here?

I didnt get an error, but since the its specified that it may through, I thought it would be prudent to catch

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIWebNavigation.idl#241

Comment 10

[Justin Lebar (not reading bugmail)]

> I am having problems pushing this to try, 

I did

  $ hg qpop -a
  $ hg pull -u
  $ hg qimport  <(curl -L https://bug741717.bugzilla.mozilla.org/attachment.cgi?id=642453)
  $ hg qpush
  $ hg qref -e
  (add trychooser bits)
  $ hg push -f ssh://hg.mozilla.org

> Although I can import the patch to mercurial, when I try to qpush I get a bunch of conflicts,

Weird; I didn't get any conflicts.  Maybe your hg tree is not at tip.  That happens sometimes, particularly if you pushed some patches onto hg and then got conflicts.

  $ hg qpop -a; hg pull; hg up -C

may fix it.

> [git and hg] are diverged (I dont understand why) and git push-to-try is trying to push what looks 
> like a few hundred commits

git push-to-try tries to push all the commits in |git qapplied|; it's not looking at hg at all for that information.  It works by comparing HEAD to "upstream".  But in the version in the master branch, "upstream" is always "origin/master".  IOW, your problems may be solved if you |git fetch|.

Alternatively, you can |git push-to-hg --tip| your patch (using an explicit rev list).  You can't give git push-to-try an explicit rev list at the moment because it's hard for me to distinguish between a revision and trychooser params.  But I should figure this out, because I want it too...

Sorry, that's probably all TMI.  Here's your push.  :)

https://tbpl.mozilla.org/?tree=Try&rev=7de45592fbef

Comment 11

[Justin Lebar (not reading bugmail)]

> I didnt get an error, but since the its specified that it may through, I thought it would be prudent 
> to catch

Ah...this is apparently the "do you want to re-send the POST data?" prompt (search for NS_BINDING_ABORTED in nsDocShell.cpp; there's only one occurrence).  I wonder what happens if you try to refresh a POST page.  My guess is that it is not pretty.  :)  Swallowing the exception seems right here, but we should probably file a follow-up on that dialog.

Comment 12

[Justin Lebar (not reading bugmail)]

(btw, the bug number in the commit is wrong.)

The try push is leaking in mochitest-other, but that is not your fault [1], so we're good here.

[1] https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=2220334ff2d0

Comment 13

[Dale Harvey (:daleharvey)]

Attachment: Bug 741717 - Add a reload method to browser elements. r=jlebar

Comment 14

[Justin Lebar (not reading bugmail)]

Comment on attachment 642653 [details] [diff] [review]
Bug 741717 - Add a reload method to browser elements. r=jlebar

> +  _reload: function(hardReload) {

May the API Gods forgive us all.

r=me with a few nits on the test:

+  var bodyCount = parseInt(data.json.data, 10);
+  switch (loadedEvents) {
+  case 1:
+    iframe.reload();
+    break;
+  case 2:
+    ok(true, 'reload was triggered');
+    lastLoadNum = bodyCount;
+    iframe.reload(true);
+    break;

Are you intentionally not testing that reload() doesn't do a hard-reload?  In this case, you're sending |Cache-Control: max-age=0|, so I would expect reload() /would/ do a hard-reload.  But shouldn't we test it doing a soft one instead?

+   response.write('<html><body onload="opener.onChildLoad()" ' +
+                  'onunload="parseInt(\'0\')">' +

What is this onunload handler for?  If it's there for a good reason, could you please put a comment?

Comment 15

[Dale Harvey (:daleharvey)]

I was intentionally not testing soft reloads since its behavior wasnt predictable for me, it would sometimes load from cache and sometimes load the page, but as the comment below it looks like it may have been due to me resetting away some of my changes

+   response.write('<html><body onload="opener.onChildLoad()" ' +
+                  'onunload="parseInt(\'0\')">' +

accidentally reverted some of my older changes

Will reattach shortly

Comment 16

[Dale Harvey (:daleharvey)]

> Are you intentionally not testing that reload() doesn't do a hard-reload?  
> In this case, you're sending |Cache-Control: max-age=0|, so I would expect 
> reload() /would/ do a hard-reload.  But shouldn't we test it doing a
> soft one instead?

I was intentionally not testing softreloads behaviour as I have never been able to get a reliable load from cache working, even currently using 

   response.setHeader('Cache-Control', 'public, max-age=60');

a soft reload goes to the server, I would have argued that we didnt test softreload behaviour if I didnt find a bug in my patch where it was always hardreloading :) trying to figure out the caching now.

Comment 17

[Dale Harvey (:daleharvey)]

Attachment: Bug 741717 - Add a reload method to browser elements. r=jlebar

Now tests soft + hard reload

I took the lets out of the try catch since we dont want to be silently failing if there is bugs there, just catching reload throws

Comment 18

[Justin Lebar (not reading bugmail)]

Comment on attachment 642812 [details] [diff] [review]
Bug 741717 - Add a reload method to browser elements. r=jlebar

> +++ b/dom/browser-element/mochitest/file_bug741717.sjs

Oh gosh, this looks horrible.  I'm sorry!

>+    let nsIWebNavigation = docShell.QueryInterface(Ci.nsIWebNavigation);

Nit: Please call this webNav (or something), so it's clearly different from Ci.nsIWebNavigation.

>+    let hardReloadFlags = nsIWebNavigation.LOAD_FLAGS_BYPASS_PROXY |
>+      nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE;

Nit: I think it would be a bit clearer if this was |const|.  Also, you may want to indent the second line so nsIWebNavigation lines up on both lines.

>+    let reloadFlags = data.json.hardReload ? hardReloadFlags :
>+      nsIWebNavigation.LOAD_FLAGS_NONE;
>+    try {
>+      nsIWebNavigation.reload(reloadFlags);
>+    } catch(e) {
>+      // Silently swallow errors; these can happen if a used cancels reload
>+    }
>+  },

Comment 19

[Dale Harvey (:daleharvey)]

Attachment: Bug 741717 - Add a reload method to browser elements. r=jlebar

Comment 20

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72c79a019f5a

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/72c79a019f5a

Comment 22

[Jeremie Patonnier :Jeremie]

Documentation available here:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement.reload