Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Browser API : Add a reload method to browser elements

RESOLVED FIXED in mozilla17

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: benfrancis, Assigned: daleharvey)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla17
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Summary: Browser API - Add a reload method to browser elements → Browser API : Add a reload method to browser elements
Keywords: dev-doc-needed
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.
Can't we use window.location.reload() or would that truncate session history too?
(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.
(Assignee)

Comment 4

5 years ago
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.
Assignee: nobody → dale
Status: NEW → ASSIGNED
Attachment #639160 - Flags: feedback?(justin.lebar+bug)
>+  _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?
Attachment #639160 - Flags: feedback?(justin.lebar+bug) → feedback+
(Assignee)

Comment 6

5 years ago
Created attachment 641610 [details] [diff] [review]
Bug 751717: Add a reload method to browser elements

Currently figuring out how this breaks security change notifications
Attachment #639160 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
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.
Depends on: 774100
(Assignee)

Comment 8

5 years ago
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.
Attachment #641610 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
>>+    } 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
> 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
> 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.
(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
(Assignee)

Comment 13

5 years ago
Created attachment 642653 [details] [diff] [review]
Bug 741717 - Add a reload method to browser elements. r=jlebar
Attachment #642453 - Attachment is obsolete: true
Attachment #642653 - Flags: review?(justin.lebar+bug)
Blocks: 709759
Attachment #642653 - Attachment is patch: true
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?
Attachment #642653 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 15

5 years ago
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
(Assignee)

Comment 16

5 years ago
> 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.
(Assignee)

Comment 17

5 years ago
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
Attachment #642653 - Attachment is obsolete: true
Attachment #642812 - Flags: review?(justin.lebar+bug)
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
>+    }
>+  },
Attachment #642812 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 19

5 years ago
Created attachment 643005 [details] [diff] [review]
Bug 741717 - Add a reload method to browser elements. r=jlebar
Attachment #642812 - Attachment is obsolete: true
Attachment #643005 - Flags: checkin?(justin.lebar+bug)
https://hg.mozilla.org/integration/mozilla-inbound/rev/72c79a019f5a
Attachment #643005 - Flags: checkin?(justin.lebar+bug) → checkin+
https://hg.mozilla.org/mozilla-central/rev/72c79a019f5a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Documentation available here:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement.reload
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.