Last Comment Bug 741717 - Browser API : Add a reload method to browser elements
: Browser API : Add a reload method 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)
: mozilla17
Assigned To: Dale Harvey (:daleharvey)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 774100
Blocks: browser-api 709759
  Show dependency treegraph
 
Reported: 2012-04-03 00:49 PDT by Ben Francis [:benfrancis]
Modified: 2013-06-27 06:19 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP: Add a reload method to browser elements (7.75 KB, patch)
2012-07-04 12:43 PDT, Dale Harvey (:daleharvey)
justin.lebar+bug: feedback+
Details | Diff | Splinter Review
Bug 751717: Add a reload method to browser elements (9.24 KB, patch)
2012-07-12 14:52 PDT, Dale Harvey (:daleharvey)
no flags Details | Diff | Splinter Review
Bug 751717 - Add a reload method to browser elements. r=jlebar (9.40 KB, patch)
2012-07-15 18:01 PDT, Dale Harvey (:daleharvey)
no flags Details | Diff | Splinter Review
Bug 741717 - Add a reload method to browser elements. r=jlebar (9.40 KB, patch)
2012-07-16 11:19 PDT, Dale Harvey (:daleharvey)
justin.lebar+bug: review+
Details | Diff | Splinter Review
Bug 741717 - Add a reload method to browser elements. r=jlebar (9.86 KB, patch)
2012-07-16 17:50 PDT, Dale Harvey (:daleharvey)
justin.lebar+bug: review+
Details | Diff | Splinter Review
Bug 741717 - Add a reload method to browser elements. r=jlebar (9.77 KB, patch)
2012-07-17 09:42 PDT, Dale Harvey (:daleharvey)
justin.lebar+bug: checkin+
Details | Diff | Splinter Review

Description Ben Francis [:benfrancis] 2012-04-03 00:49:53 PDT
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) 2012-06-26 10:43:33 PDT
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) 2012-06-26 12:01:09 PDT
Can't we use window.location.reload() or would that truncate session history too?
Comment 3 Justin Lebar (not reading bugmail) 2012-06-27 01:04:33 PDT
(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) 2012-07-04 12:43:01 PDT
Created attachment 639160 [details] [diff] [review]
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) 2012-07-05 07:24:42 PDT
>+  _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) 2012-07-12 14:52:26 PDT
Created attachment 641610 [details] [diff] [review]
Bug 751717: Add a reload method to browser elements

Currently figuring out how this breaks security change notifications
Comment 7 Dale Harvey (:daleharvey) 2012-07-15 12:31:18 PDT
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) 2012-07-15 18:01:49 PDT
Created attachment 642453 [details] [diff] [review]
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) 2012-07-15 18:25:01 PDT
>>+    } 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) 2012-07-15 21:41:57 PDT
> 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) 2012-07-15 21:44:12 PDT
> 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) 2012-07-16 10:30:11 PDT
(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) 2012-07-16 11:19:45 PDT
Created attachment 642653 [details] [diff] [review]
Bug 741717 - Add a reload method to browser elements. r=jlebar
Comment 14 Justin Lebar (not reading bugmail) 2012-07-16 12:46:11 PDT
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) 2012-07-16 12:52:58 PDT
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) 2012-07-16 15:59:33 PDT
> 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) 2012-07-16 17:50:06 PDT
Created attachment 642812 [details] [diff] [review]
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) 2012-07-17 09:08:03 PDT
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) 2012-07-17 09:42:50 PDT
Created attachment 643005 [details] [diff] [review]
Bug 741717 - Add a reload method to browser elements. r=jlebar
Comment 20 Justin Lebar (not reading bugmail) 2012-07-17 12:54:51 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/72c79a019f5a
Comment 21 Ed Morley [:emorley] 2012-07-18 05:56:14 PDT
https://hg.mozilla.org/mozilla-central/rev/72c79a019f5a
Comment 22 Jeremie Patonnier :Jeremie 2013-06-27 06:19:53 PDT
Documentation available here:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement.reload

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