Add mozbrowser{loadstart,loadend,locationchange} events for <iframe mozbrowser>

RESOLVED FIXED in Firefox 12

Status

()

defect
RESOLVED FIXED
8 years ago
3 months ago

People

(Reporter: benfrancis, Assigned: justin.lebar+bug)

Tracking

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

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox12 fixed)

Details

(Whiteboard: [qa-])

Attachments

(5 attachments, 10 obsolete attachments)

478 bytes, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
24.87 KB, patch
smaug
: review+
Details | Diff | Splinter Review
18.45 KB, patch
smaug
: review+
Details | Diff | Splinter Review
28.62 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.02 KB, patch
Details | Diff | Splinter Review
Reporter

Description

8 years ago
For the implementation of a web browser as a web app, it is necessary for a specially privileged container of an iFrame to be able to listen for certain cross-origin events.

For example:
* Page starts loading (onlocationchange?)
* Page finishes loading (onload?)
* URL changes (onlocationchange, onpopstate?)
* Page title changes
* Favicon URL changes

https://wiki.mozilla.org/WebAPI/EmbeddedBrowserAPI
Since the iframe may be in another process, we can't just expose the event (or allow it to bubble up like events normally bubble up).  It'll have to be...something different.
This is the same discussion what was happening ~18 months ago (event handling in E10s),
and at that time we ended up creating message manager.
We can't forward DOM events automatically. That would be just too slow, and event targeting wouldn't
make sense.
Reporter

Comment 3

8 years ago
Good point Justin.

Also, do you think handling Window.open falls within the scope of this discussion, as the container may want to be informed when content inside the iFrame calls this method so that it can choose to handle it in some way...
I think window.open is a separate issue.

In general, there are about five billion issues associated with making a DOM browser.  I'm working on 'em, but it may not be helpful to file bugs on everything right at the moment.
Yeah full DOM-event semantics seem like the wrong tool for the job here.  But notifying parent processes of these events through another mechanism is quite easy.
Reporter

Comment 6

8 years ago
Feel free to rename this bug to better reflect the actual work that needs doing to achieve this, I just wanted to kick off the discussion :)
Summary: Allow events to bubble up to a privileged cross-origin container of an iFrame → Add iframe.{add,remove}ContentStateListener(), for listening to cross-origin locationchange events
Scoping this bug to just remote locationchange events.
Posted patch Part 1, v1 (obsolete) — Splinter Review
Depends on: 708176
Attachment #583547 - Flags: review?(bugs)
Summary: Add iframe.{add,remove}ContentStateListener(), for listening to cross-origin locationchange events → Add iframe.{add,remove}ContentStateListener(), for listening to cross-origin locationchange, load start/stop events
Attachment #583624 - Flags: review?(bugs)
Ben, you should be able to play with this by pulling my branch off github [1] and merging with whatever remaining changes haven't been checked into m-c (if there are any).

I have a tendency to rebase my remote branches (rather than merging them), so...beware.  :)

[1] https://github.com/jlebar/mozilla-central/tree/710231-remote-events
Attachment #583547 - Attachment description: Patch v1 → Part 1, v1
Reporter

Comment 11

8 years ago
I have a branch of Gaia with a working browser (https://github.com/krellian/gaia/tree/browser) which uses jlebar's mozilla-central branch mentioned above :)

My Gaia patch is here https://github.com/krellian/gaia/commit/638d6be0004cdf06b42f26d387dc94c3de909be0

What needs to happen next to get this landed?
I should review the patches.
Reporter

Comment 13

8 years ago
Great, OK.

I forgot to mention, I couldn't get the prefs working so I had to comment out the pref checks to get my browser to work. I'm probably just doing something wrong but I'd be grateful if someone else could test that part.
Reporter

Comment 14

8 years ago
I can confirm that the prefs do in fact work.

I was using <iframe browser> but it expects <iframe mozbrowser>

I've updated my Gaia branch accordingly.
Reporter

Comment 15

8 years ago
Added patch to enable browser frames for Gaia in B2G which means whitelisting the whole of localhost:8888 where Gaia is currently served from.
Comment on attachment 583547 [details] [diff] [review]
Part 1, v1


>+BrowserFrameProgressListener::OnLocationChange(nsIWebProgress* aWebProgress,
>+                                               nsIRequest* aRequest,
>+                                               nsIURI* aURI,
>+                                               PRUint32 aFlags)
>+{
>+  nsCAutoString spec;
>+  aURI->GetSpec(spec);
>+
>+  NS_ConvertUTF8toUTF16 spec8(spec);
You mean spec16

>+NS_IMETHODIMP
>+nsGenericHTMLFrameElement::MozAddContentStateListener(const nsAString &aProperty,
>+                                                      nsIDOMContentStateCallback *aCallback)
>+{
>+  nsresult rv = BrowserFrameSecurityCheck();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (!mProgressListener) {
>+    nsCOMPtr<nsIDOMWindow> contentWindow;
>+    GetContentWindow(getter_AddRefs(contentWindow));
>+    mProgressListener = new BrowserFrameProgressListener();
>+    mProgressListener->Init(contentWindow);
>+  }
>+
>+  return mProgressListener->AddListener(aProperty, aCallback);
This should probably have similar check that what event listeners have. The same
listener is added only once.
Attachment #583547 - Flags: review?(bugs) → review-
Comment on attachment 583547 [details] [diff] [review]
Part 1, v1

>+BrowserFrameProgressListener::AddListener(const nsAString &aProperty,
>+                                          nsIDOMContentStateCallback *aCallback)
>+{
>+  if (!aProperty.EqualsLiteral("location")) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  // This would be slow if we had a lot of callbacks, but this is a privileged
>+  // DOM API, so we can expect that code won't go crazy and add a ton.
>+  if (!mLocationChangeCallbacks.Contains(aCallback)) {
>+    mLocationChangeCallbacks.AppendElement(aCallback);
>+  }
Ah, it is here.


>+BrowserFrameProgressListener::OnLocationChange(nsIWebProgress* aWebProgress,
>+                                               nsIRequest* aRequest,
>+                                               nsIURI* aURI,
>+                                               PRUint32 aFlags)
>+{
>+  nsCAutoString spec;
>+  aURI->GetSpec(spec);
>+
>+  NS_ConvertUTF8toUTF16 spec8(spec);
>+
>+  for (PRUint32 i = 0; i < mLocationChangeCallbacks.Length(); i++) {
>+    mLocationChangeCallbacks[i]->Callback(spec8);
>+  }
You need to push right cx to stack before calling callback.
Use nsCxPusher.
Comment on attachment 583624 [details] [diff] [review]
Part 2, v1: Add 'load' (start/stop) listener.


>+NS_IMETHODIMP
>+BrowserFrameProgressListener::OnStateChange(nsIWebProgress* aProgress,
>+                                            nsIRequest* aRequest,
>+                                            PRUint32 aProgressStateFlags,
>+                                            nsresult aStatus)
>+{
>+  if (!(aProgressStateFlags & STATE_IS_WINDOW)) {
>+    return NS_OK;
>+  }
>+
>+  nsAutoString status;
>+  if (aProgressStateFlags & STATE_START) {
>+    status.AssignLiteral("start");
>+  }
>+  else if (aProgressStateFlags & STATE_STOP) {
} else if () {


I'd like to re-review this after part1 has been updated.
Attachment #583624 - Flags: review?(bugs) → review+
Reporter

Comment 19

8 years ago
Gaia is moving to port 7777 so updating patch accordingly.
Attachment #584550 - Attachment is obsolete: true
Reporter

Comment 20

8 years ago
Hmm, and now it's changing to 6666.
Attachment #586379 - Attachment is obsolete: true
> You need to push right cx to stack before calling callback.
> Use nsCxPusher.

I need to push the cx corresponding to the window which originally registered the callback, right?  So that means I need to keep a weak ref to the window?
Sounds right.
> I need to push the cx corresponding to the window which originally registered the callback, right?  
> So that means I need to keep a weak ref to the window?

On IRC, we clarified that I need to keep a ref to the window *containing the iframe*, not the window which registered the callback.
I'm sorry about the rename noise in these patches.  It's proving to be difficult to unravel.
Attachment #587340 - Flags: review?(bugs)
Re-requesting review per comment 18.
Attachment #587346 - Flags: review?(bugs)
Attachment #587347 - Flags: review?(bugs)
Again, I'm sorry about this rename mess.  The many renames became tangled and I've been unable to untangle them.
Attachment #587348 - Flags: review?(bugs)
Attachment #583624 - Attachment is obsolete: true
Comment on attachment 587340 [details] [diff] [review]
Part 1a: Address review comments. (Always push a context)

> BrowserFrameProgressListener::AddListener(const nsAString &aProperty,
>-                                          nsIDOMContentStateCallback *aCallback)
>+                                          nsIDOMMozInnerStateCallback *aCallback)
Nit, I'd prefer nsIDOMMozInnerStateCallback* aCallback
And least use consistent coding style.

> BrowserFrameProgressListener::OnLocationChange(nsIWebProgress* aWebProgress,
>                                                nsIRequest* aRequest,
>                                                nsIURI* aURI,
>                                                PRUint32 aFlags)
> {
Could you add IsSafeToRunScript() check here. Though, I wonder if callbacks should fire either async or using
a scriptrunner.


>+  nsCOMPtr<nsIDOMEventTarget> frameElement = do_QueryReferent(mFrameElement);
>+  NS_ENSURE_TRUE(frameElement, NS_ERROR_FAILURE);
>+
>   nsCAutoString spec;
>   aURI->GetSpec(spec);
> 
>   NS_ConvertUTF8toUTF16 spec16(spec);
> 
>+  nsCxPusher pusher;
>   for (PRUint32 i = 0; i < mLocationChangeCallbacks.Length(); i++) {
>+    NS_ENSURE_TRUE(pusher.RePush(frameElement), NS_ERROR_FAILURE);
I like NS_ENSURE_STATE



> false, or if the calling window is
>    * not privileged, this function fails.
fails means what? throws an exception? no-op?



I still wonder if this all could be done using normal event listeners.
Attachment #587340 - Flags: review?(bugs) → review+
I mean, having something like
<iframe browser requested-properties="location">

And then there could be event "propertychange" which had name and value properties.
Comment on attachment 587346 [details] [diff] [review]
Part 2, v1.1 (no major changes)


>+  for (PRUint32 i = 0; i < mLoadStateCallbacks.Length(); i++) {
>+    mLoadStateCallbacks[i]->Callback(status);
>+  }

No cx pushing here?

Maybe it is added in some other patch, but hard to review patches split to so many parts :(
Attachment #587346 - Flags: review?(bugs) → review-
Attachment #587347 - Flags: review?(bugs) → review+
Comment on attachment 587346 [details] [diff] [review]
Part 2, v1.1 (no major changes)

ok, I guess nsCxPusher is in 2a
Attachment #587346 - Flags: review- → review+
Comment on attachment 587348 [details] [diff] [review]
Part 2b: Rename nsIDOMMozInnerStateCallback to nsIDOMMozContentStateCallback.

r+

But would the event based API be more natural to web devs?
And it would use common code paths, so hopefully less code would be needed.
Attachment #587348 - Flags: review?(bugs) → review+
Reporter

Comment 33

8 years ago
smaug discussed this with me in IRC and he has made what seems like a sensible suggestion for <iframe browser> to fire events which can be listened for just by using a standard event listener on the iframe.

However, it would be nice to see this patch land first so we can prototype something to find out what's still missing.
I wouldn't want to land an API to m-c if we know it will be removed very soon.
> <iframe browser requested-properties="location">
>
> And then there could be event "propertychange" which had name and value properties.

One kind of crummy thing about doing it this way is that I'd have to write a function with a big switch statement myself.  That is, I'd need to do:

function iframeListener(e) {
  if (e.name == 'location') {
    iframeLocationChanged(e.value);
  }
  else if (e.name == 'load') {
    iframeLoadStateChanged(e.value);
  }
  ...
}

whereas right now, iframeLocationChanged and iframeLoadChanged are called directly.

Also, with the current API, you specify the events you want once, whereas with this proposed API, you'd have to specify them twice.

I guess I don't feel so strongly about it either way, but I'm not sure that the current API is obviously worse...

> Maybe it is added in some other patch, but hard to review patches split to so many parts :(

Sorry; I was trying to keep part 2 unchanged from the patch you'd r+'ed earlier.  But I can always fold patches together if you ask!  That's much easier than splitting them apart.  :)
Comment on attachment 587340 [details] [diff] [review]
Part 1a: Address review comments. (Always push a context)

f? smaug re comment 35.
Attachment #587340 - Flags: feedback?(bugs)
smaug convinced me on IRC that we should go with the event-based approach.

I'm going to do it for now without filtering of events in the <iframe> element.
Attachment #587340 - Flags: feedback?(bugs)
Reporter

Comment 38

8 years ago
jlebar: Sorry about the time you spent in this cul-de-sac of API exploration but I think it's been worthwhile and we did prove it worked!

<iframe browser> with all the events turned on sounds like a great starting point for this new path.

I wonder if later <iframe browser> could be an alternative to <iframe app> where by default it just makes window.top stop at the iframe and ignores x-frame-options. Then you could turn on additional features like <iframe browser="allow-events allow-navigation">

smaug may also have convinced me that we don't need to get properties directly and that event listening is enough. The only use case of getting content from inside the iframe I can think of that isn't covered by events is session restore.
Attachment #583547 - Attachment is obsolete: true
Attachment #587340 - Attachment is obsolete: true
Attachment #587346 - Attachment is obsolete: true
Attachment #587347 - Attachment is obsolete: true
Attachment #587348 - Attachment is obsolete: true
Depends on: 718908
We need to resolve bug 718908 first.  (I guess we could proceed without that fix, but then this isn't going to work when you have two iframes on a page, and it might break in other circumstances.  Anyway, I don't want to land code which doesn't work, even if the bug isn't in the code I'm landing.)
Reporter

Comment 40

8 years ago
In a recent email I saw that the proposed new API would be used like...

iframe.addEventListener("mozBrowserPropertyChange",
 function(e) {
   // e.property may be either "location" or "loading".  If
e.property is location, e.value is the iframe's new location.  If
e.property is "loading", e.value is either "start" or "stop".
});

I'm just curious, why not have a different type of event per property...

iframe.addEventListener("locationchange", function(e) { });
iframe.addEventListener("loadingchange", function(e) { });
iframe.addEventListener("titlechange", function(e) { });

or

iframe.addEventListener("browserlocationchange", function(e) { });
iframe.addEventListener("browserloadingchange", function(e) { });
iframe.addEventListener("browsertitlechange", function(e) { });

?

I can't think of many use cases where you'd want a single event listener for all of these events.
Yeah, it's kind of annoying to have just one event; this is the problem I identified in comment 35.

On the other hand, it takes a fairly large amount of code to add a new event to Gecko, and writing all that code every time we want to add a new property would be unpleasant.  Also, Smaug wants to pollute the global namespace as little as possible, and one event is less pollution than N.

I don't much care either way, although if we want this to land, we need to agree on an API eventually!
An event doesn't pollute global namespace. An event interface does.

I don't strongly care whether you dispatch mozBrowserPropertyChange which has property to tell which property
has change, or whether you dispatch mozbrowserlocationchange, mozbrowserloadingchange etc.

Adding a new event is like
createEvent();
initEvent("newevent")
dispatch()

Adding new event interface does require some C++ code, but it isn't really much nowadays.
> Adding new event interface does require some C++ code, but it isn't really much nowadays.

Right; the problem isn't *firing* the new event, which is easy, but adding the new interface.

Just so we're clear, here's the diffstat of my current patch, minus the code to actually fire the event:

 content/base/src/nsGkAtomList.h                    |    2 +
 content/events/public/nsEventNameList.h            |    4 +
 content/events/public/nsIPrivateDOMEvent.h         |    2 +
 content/events/src/Makefile.in                     |    1 +
 content/events/src/nsDOMEvent.cpp                  |    7 +-
 content/events/src/nsDOMEvent.h                    |    3 +-
 .../src/nsDOMMozBrowserPropertyChangeEvent.cpp     |   76 +++++++
 .../src/nsDOMMozBrowserPropertyChangeEvent.h       |   44 ++++
 content/events/src/nsEventDispatcher.cpp           |    2 +
 dom/base/nsDOMClassInfo.cpp                        |   11 +
 dom/base/nsDOMClassInfoClasses.h                   |    1 +
 dom/interfaces/core/nsIInlineEventHandlers.idl     |    1 +
 dom/interfaces/events/Makefile.in                  |    1 +
 .../events/nsIDOMMozBrowserPropertyChangeEvent.idl |   28 +++
 js/xpconnect/src/dictionary_helper_gen.conf        |    4 +-
 widget/nsGUIEvent.h                                |    1 +
 widget/xpwidgets/nsBaseWidget.cpp                  |    1 +

Maybe this is better than it used to be, but it still takes me half a day to get it all right.
Reporter

Comment 44

8 years ago
So following our conversation on IRC it seems we can have separate events which can be listened for with a standard event listener, with the iframe as the target.

Something like:

loadstart
loadend
locationchange
titlechange
securitychange
iconchange

...maybe with some prefixes for namespacing, whether that be "browser" or "DOM"/"content" like Fennec currently uses.

loadstart and loadend may be able to use the existing ProgressEvent interface, one or more new interfaces may be needed for the other events which need to contain strings, or the CustomEvent interface could be used.
(In reply to Ben Francis from comment #44)
> So following our conversation on IRC it seems we can have separate events
> which can be listened for with a standard event listener, with the iframe as
> the target.
> 
> Something like:
> 
> loadstart
> loadend

does loadstart and loadend will thrown for xhr and sub-windows as well? Or will it be only for the root window?
In theory, I think it should be "start the throbber" and "stop the throbber".  So yes, for xhr and sub-windows.  But in the first implementation.

Comment 47

8 years ago
Just out of interest, are we comparing behavior with XUL <browser> and matching how it does things and/or taking our lessons from that? (IIRC it uses some kind of progress listener for the progress of loading a web page - do we deep that too complicated for HTML-based browsers?)
Reporter

Comment 48

8 years ago
A full progress listener would be great (though potentially difficult to calculate), but loadstart and loadend is enough for a Fennec clone in HTML. I have been using the browser tag as a reference and have taken some ideas from that for proposed additions to the History API.

jlebar: I'm going to need at least a titlechange event as well for the Demo Phone milestone (iconchange would be nice too but not essential). Would you like me to file a separate bug as this one is currently scoped to loadstart, loadend and locationchange?
Yes, please file a new bug.  The smaller the scope of this one, the sooner we'll be able to get it done.
Attachment #589975 - Attachment description: Part 1, v1: Back out → Part 1, v1: Back out bug 708176
Attachment #589975 - Flags: review? → review?(bugs)
Attachment #589972 - Flags: review?(bugs)
Attachment #589971 - Flags: review? → review?(bugs)
Comment on attachment 586474 [details] [diff] [review]
Part 4, v3: Enable browser frames for Gaia in B2G

r=me on this change.
Attachment #586474 - Attachment description: Part 3, v3: Enable browser frames for Gaia in B2G → Part 4, v3: Enable browser frames for Gaia in B2G
Attachment #586474 - Flags: review+
Some crashes on try because of a missing null-check.  New push: https://tbpl.mozilla.org/?tree=Try&rev=f791c17cbb42
Try run for 74e16e293bd4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=74e16e293bd4
Results (out of 102 total builds):
    exception: 24
    success: 29
    warnings: 10
    failure: 39
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-74e16e293bd4
Try run for f791c17cbb42 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f791c17cbb42
Results (out of 207 total builds):
    success: 168
    warnings: 38
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-f791c17cbb42
Posted patch Part 3, v1.1: Add new events (obsolete) — Splinter Review
Fixing test failure, and adding missed domclassinfo entries.
Attachment #589972 - Attachment is obsolete: true
Attachment #589972 - Flags: review?(bugs)
Attachment #590106 - Flags: review?(bugs)
Reporter

Comment 59

8 years ago
This is looking great :)

I was just wondering, do you know whether a mozbrowserlocationchange event will get fired if history.pushState or history.replaceState are called? Do you think it should?

If it doesn't then we won't be able to update the address bar with new URLs set using these methods.
Try run for 03cad637d1a7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=03cad637d1a7
Results (out of 208 total builds):
    success: 180
    warnings: 26
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-03cad637d1a7
Attachment #589971 - Flags: review?(bugs) → review+
Comment on attachment 589971 [details] [diff] [review]
Part 2, v1: Move nsGenericHTMLFrameElement into its own file.


>+++ b/content/html/content/src/nsGenericHTMLFrameElement.h
>@@ -0,0 +1,73 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim:set tw=80 expandtab softtabstop=2 ts=2 sw=2: */
>+
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "nsGenericHTMLElement.h"
>+#include "nsIDOMHTMLFrameElement.h"
>+




This .h should have #ifndef / #define before includes
Attachment #589975 - Flags: review?(bugs) → review+
Comment on attachment 590106 [details] [diff] [review]
Part 3, v1.1: Add new events


>+nsGenericHTMLFrameElement::MaybeFireBrowserEvent(
>+  const nsAString &aEventName,
>+  const nsAString &aEventType,
>+  const nsAString &aValue /* = EmptyString() */)
>+{
>+  MOZ_ASSERT(aEventType.EqualsLiteral("event") ||
>+             aEventType.EqualsLiteral("customevent"));
>+  MOZ_ASSERT_IF(aEventType.EqualsLiteral("event"),
>+                aValue.IsEmpty());
>+
>+  if (!BrowserFrameSecurityCheck()) {
>+    return NS_OK;
>+  }
>+
>+  nsAutoString eventName;
>+  eventName.AppendLiteral("mozbrowser");
>+  eventName.Append(aEventName);
>+
>+  nsCOMPtr<nsIDOMEvent> domEvent;
>+  nsEventDispatcher::CreateEvent(GetPresContext(), nsnull,
>+                                 aEventType, getter_AddRefs(domEvent));
>+
>+  nsCOMPtr<nsIPrivateDOMEvent> privateEvent = do_QueryInterface(domEvent);
>+  NS_ENSURE_STATE(privateEvent);
>+
>+  nsresult rv = privateEvent->SetTrusted(true);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (aEventType.EqualsLiteral("customevent")) {
>+    nsCOMPtr<nsIDOMCustomEvent> customEvent = do_QueryInterface(domEvent);
>+    NS_ENSURE_STATE(customEvent);
>+
>+    nsCOMPtr<nsIWritableVariant> value = new nsVariant();
>+    value->SetAsAString(aValue);
>+
>+    rv = customEvent->InitCustomEvent(eventName,
>+                                      /* bubbles = */ false,
>+                                      /* cancelable = */ false,
>+                                      value);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+  else {
>+    rv = domEvent->InitEvent(eventName,
>+                             /* bubbles = */ false,
>+                             /* cancelable = */ false);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+
>+  return nsEventDispatcher::DispatchDOMEvent(
>+    NS_ISUPPORTS_CAST(nsIFrameLoaderOwner*, this),
>+    nsnull, domEvent, GetPresContext(), nsnull);
>+

This should dispatch the event asynchronously using nsAsyncDOMEvent and its
PostDOMEvent. It is quite scary to dispatch stuff synchronously to content within
webprogress listener methods. Also, when iframe runs in a different process, 
this all becomes async anyway.



>+nsGenericHTMLFrameElement::OnStatusChange(nsIWebProgress* aWebProgress,
>+                                             nsIRequest* aRequest,
>+                                             nsresult aStatus,
>+                                             const PRUnichar* aMessage)
Fix indentation



>+{
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsGenericHTMLFrameElement::OnSecurityChange(nsIWebProgress *aWebProgress,
>+                                               nsIRequest *aRequest,
>+                                               PRUint32 state)
same here


Is the patch missing some part, since I don't understand how iframe.onmozbrowserpropertychange gets mapped to event listener.
Though, I'd prefer using .addEventListener for now.
Attachment #590106 - Flags: review?(bugs) → review-
> This should dispatch the event asynchronously using nsAsyncDOMEvent and its
> PostDOMEvent.

Yes, of course!

> Is the patch missing some part, since I don't understand how iframe.onmozbrowserpropertychange gets 
> mapped to event listener.

AIUI you did not want iframe.onmozbrowserX properties:

  <jlebar>	smaug, And you explicitly *don't* want a onmozbrowsertitlechange property, right?
  <smaug>	yeah, it would be good to avoid that, IMO
> I was just wondering, do you know whether a mozbrowserlocationchange event will get fired if 
> history.pushState or history.replaceState are called?

Yes.  Or at least, I wrote that code, and it *should* work.  :)
Reporter

Comment 65

8 years ago
excellent :)
Yes, but the patch uses adds onmozbrowserpropertychange atom, and sets
onmozbrowserpropertychange handler. Why?
Ah, that's because the atom and the test which uses it are vestiges of days gone by!
Attachment #590106 - Attachment is obsolete: true
Attachment #590216 - Flags: review?(bugs)
Assignee: nobody → justin.lebar+bug
Status: NEW → ASSIGNED
Attachment #590216 - Flags: review?(bugs) → review+
Summary: Add iframe.{add,remove}ContentStateListener(), for listening to cross-origin locationchange, load start/stop events → Add mozbrowser{loadstart,loadend,locationchange} events for <iframe mozbrowser>
Depends on: 720069
Depends on: 720157
Whiteboard: [qa-]
Is there a privilege that is needed for these beyond "browser"?
(In reply to Mike Kaply (:mkaply) from comment #75)
> Is there a privilege that is needed for these beyond "browser"?

To answer my own question, no.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.