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

Status: RESOLVED FIXED

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

Description

[Ben Francis [:benfrancis]]

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

Comment 1

[Justin Lebar (not reading bugmail)]

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.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 3

[Ben Francis [:benfrancis]]

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...

Comment 4

[Justin Lebar (not reading bugmail)]

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.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 6

[Ben Francis [:benfrancis]]

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 :)

Comment 7

[Justin Lebar (not reading bugmail)]

Scoping this bug to just remote locationchange events.

Comment 8

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v1

Comment 9

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v1: Add 'load' (start/stop) listener.

Comment 10

[Justin Lebar (not reading bugmail)]

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

Comment 11

[Ben Francis [:benfrancis]]

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?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

I should review the patches.

Comment 13

[Ben Francis [:benfrancis]]

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.

Comment 14

[Ben Francis [:benfrancis]]

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.

Comment 15

[Ben Francis [:benfrancis]]

Attachment: Part 3, v1: Enable browser frames for Gaia in B2G

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 16

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

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 18

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 19

[Ben Francis [:benfrancis]]

Attachment: Part 3, v2: Enable browser frames for Gaia in B2G

Gaia is moving to port 7777 so updating patch accordingly.

Comment 20

[Ben Francis [:benfrancis]]

Attachment: Part 4, v3: Enable browser frames for Gaia in B2G

Hmm, and now it's changing to 6666.

Comment 21

[Justin Lebar (not reading bugmail)]

> 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?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Sounds right.

Comment 23

[Justin Lebar (not reading bugmail)]

> 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.

Comment 24

[Justin Lebar (not reading bugmail)]

Attachment: Part 1a: Address review comments. (Always push a context)

I'm sorry about the rename noise in these patches.  It's proving to be difficult to unravel.

Comment 25

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v1.1 (no major changes)

Re-requesting review per comment 18.

Comment 26

[Justin Lebar (not reading bugmail)]

Attachment: Part 2a - Push cx before firing events.

Comment 27

[Justin Lebar (not reading bugmail)]

Attachment: Part 2b: Rename nsIDOMMozInnerStateCallback to nsIDOMMozContentStateCallback.

Again, I'm sorry about this rename mess.  The many renames became tangled and I've been unable to untangle them.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

I mean, having something like
<iframe browser requested-properties="location">

And then there could be event "propertychange" which had name and value properties.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

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 :(

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 587346 [details] [diff] [review]
Part 2, v1.1 (no major changes)

ok, I guess nsCxPusher is in 2a

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 33

[Ben Francis [:benfrancis]]

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.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

I wouldn't want to land an API to m-c if we know it will be removed very soon.

Comment 35

[Justin Lebar (not reading bugmail)]

> <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 36

[Justin Lebar (not reading bugmail)]

Comment on attachment 587340 [details] [diff] [review]
Part 1a: Address review comments. (Always push a context)

f? smaug re comment 35.

Comment 37

[Justin Lebar (not reading bugmail)]

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.

Comment 38

[Ben Francis [:benfrancis]]

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.

Comment 39

[Justin Lebar (not reading bugmail)]

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.)

Comment 40

[Ben Francis [:benfrancis]]

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.

Comment 41

[Justin Lebar (not reading bugmail)]

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!

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 43

[Justin Lebar (not reading bugmail)]

> 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.

Comment 44

[Ben Francis [:benfrancis]]

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.

Comment 45

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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?

Comment 46

[Justin Lebar (not reading bugmail)]

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

[Robert Kaiser]

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?)

Comment 48

[Ben Francis [:benfrancis]]

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?

Comment 49

[Justin Lebar (not reading bugmail)]

Yes, please file a new bug.  The smaller the scope of this one, the sooner we'll be able to get it done.

Comment 50

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v1: Move nsGenericHTMLFrameElement into its own file.

Comment 51

[Justin Lebar (not reading bugmail)]

Attachment: Part 3, v1: Add new events

Comment 52

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v1: Back out bug 708176

Comment 53

[Justin Lebar (not reading bugmail)]

Comment on attachment 586474 [details] [diff] [review]
Part 4, v3: Enable browser frames for Gaia in B2G

r=me on this change.

Comment 54

[Justin Lebar (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=74e16e293bd4

Comment 55

[Justin Lebar (not reading bugmail)]

Some crashes on try because of a missing null-check.  New push: https://tbpl.mozilla.org/?tree=Try&rev=f791c17cbb42

Comment 56

[Mozilla RelEng Bot]

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/[e-mail]-74e16e293bd4

Comment 57

[Mozilla RelEng Bot]

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/[e-mail]-f791c17cbb42

Comment 58

[Justin Lebar (not reading bugmail)]

Attachment: Part 3, v1.1: Add new events

Fixing test failure, and adding missed domclassinfo entries.

Comment 59

[Ben Francis [:benfrancis]]

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.

Comment 60

[Mozilla RelEng Bot]

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/[e-mail]-03cad637d1a7

Comment 61

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 62

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 63

[Justin Lebar (not reading bugmail)]

> 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

Comment 64

[Justin Lebar (not reading bugmail)]

> 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.  :)

Comment 65

[Ben Francis [:benfrancis]]

excellent :)

Comment 66

[Olli Pettay [:smaug][[e-mail]]]

Yes, but the patch uses adds onmozbrowserpropertychange atom, and sets
onmozbrowserpropertychange handler. Why?

Comment 67

[Justin Lebar (not reading bugmail)]

Ah, that's because the atom and the test which uses it are vestiges of days gone by!

Comment 68

[Justin Lebar (not reading bugmail)]

Attachment: Part 3, v2: Add new events

Comment 69

[Justin Lebar (not reading bugmail)]

Attachment: Interdiff part 3 v1.1 --> v2

Comment 70

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a262cdf67c82
https://hg.mozilla.org/integration/mozilla-inbound/rev/b50727e80564
https://hg.mozilla.org/integration/mozilla-inbound/rev/c145acbf470d

Comment 71

[Justin Lebar (not reading bugmail)]

And part 4, enabling for b2g: https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b1cbb631fa

Comment 72

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a262cdf67c82
https://hg.mozilla.org/mozilla-central/rev/b50727e80564
https://hg.mozilla.org/mozilla-central/rev/c145acbf470d
https://hg.mozilla.org/mozilla-central/rev/f9b1cbb631fa

Comment 73

[Ben Francis [:benfrancis]]

w00t!

https://github.com/andreasgal/gaia/pull/207

Comment 74

[Jeremie Patonnier :Jeremie]

Documentation available here:
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/mozbrowserloadend
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/mozbrowserloadstart
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/mozbrowserlocationchange

Comment 75

[Mike Kaply [:mkaply]]

Is there a privilege that is needed for these beyond "browser"?

Comment 76

[Mike Kaply [:mkaply]]

(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.