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

RESOLVED FIXED in Firefox 12

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: benfrancis, Assigned: Justin Lebar (not reading bugmail))

Tracking

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

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

Firefox Tracking Flags

(firefox12 fixed)

Details

(Whiteboard: [qa-])

Attachments

(5 attachments, 10 obsolete attachments)

478 bytes, patch
Justin Lebar (not reading bugmail)
: 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

6 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
(Assignee)

Comment 1

6 years ago
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

6 years ago
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

6 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...
(Assignee)

Comment 4

6 years ago
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

6 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 :)
(Assignee)

Updated

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

Comment 7

6 years ago
Scoping this bug to just remote locationchange events.
(Assignee)

Comment 8

6 years ago
Created attachment 583547 [details] [diff] [review]
Part 1, v1
(Assignee)

Updated

6 years ago
Depends on: 708176
(Assignee)

Updated

6 years ago
Attachment #583547 - Flags: review?(bugs)
(Assignee)

Comment 9

6 years ago
Created attachment 583624 [details] [diff] [review]
Part 2, v1: Add 'load' (start/stop) listener.
(Assignee)

Updated

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

Updated

6 years ago
Attachment #583624 - Flags: review?(bugs)
(Assignee)

Comment 10

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

Updated

6 years ago
Attachment #583547 - Attachment description: Patch v1 → Part 1, v1
(Reporter)

Comment 11

6 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

6 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

6 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

6 years ago
Created attachment 584550 [details] [diff] [review]
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 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+
Blocks: 715782
(Reporter)

Comment 19

6 years ago
Created attachment 586379 [details] [diff] [review]
Part 3, v2: Enable browser frames for Gaia in B2G

Gaia is moving to port 7777 so updating patch accordingly.
Attachment #584550 - Attachment is obsolete: true
(Reporter)

Comment 20

6 years ago
Created attachment 586474 [details] [diff] [review]
Part 4, v3: Enable browser frames for Gaia in B2G

Hmm, and now it's changing to 6666.
Attachment #586379 - Attachment is obsolete: true
(Assignee)

Comment 21

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

Comment 23

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

Comment 24

6 years ago
Created attachment 587340 [details] [diff] [review]
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.
Attachment #587340 - Flags: review?(bugs)
(Assignee)

Comment 25

6 years ago
Created attachment 587346 [details] [diff] [review]
Part 2, v1.1 (no major changes)

Re-requesting review per comment 18.
Attachment #587346 - Flags: review?(bugs)
(Assignee)

Comment 26

6 years ago
Created attachment 587347 [details] [diff] [review]
Part 2a - Push cx before firing events.
Attachment #587347 - Flags: review?(bugs)
(Assignee)

Comment 27

6 years ago
Created attachment 587348 [details] [diff] [review]
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.
Attachment #587348 - Flags: review?(bugs)
(Assignee)

Updated

6 years ago
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-

Updated

6 years ago
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

6 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.
(Assignee)

Comment 35

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

Comment 36

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

Comment 37

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

Updated

6 years ago
Attachment #587340 - Flags: feedback?(bugs)
(Reporter)

Comment 38

6 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.
(Assignee)

Updated

5 years ago
Attachment #583547 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #587340 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #587346 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #587347 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #587348 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 718908
(Assignee)

Comment 39

5 years ago
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

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

Comment 41

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

Comment 43

5 years ago
> 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

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

Comment 46

5 years ago
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

5 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

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

Comment 49

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

Comment 50

5 years ago
Created attachment 589971 [details] [diff] [review]
Part 2, v1: Move nsGenericHTMLFrameElement into its own file.
Attachment #589971 - Flags: review?
(Assignee)

Comment 51

5 years ago
Created attachment 589972 [details] [diff] [review]
Part 3, v1: Add new events
(Assignee)

Comment 52

5 years ago
Created attachment 589975 [details] [diff] [review]
Part 1, v1: Back out bug 708176
Attachment #589975 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #589975 - Attachment description: Part 1, v1: Back out → Part 1, v1: Back out bug 708176
Attachment #589975 - Flags: review? → review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #589972 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #589971 - Flags: review? → review?(bugs)
(Assignee)

Comment 53

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

Comment 54

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=74e16e293bd4
(Assignee)

Comment 55

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

Comment 56

5 years ago
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

Comment 57

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

Comment 58

5 years ago
Created attachment 590106 [details] [diff] [review]
Part 3, v1.1: Add new events

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

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

Comment 60

5 years ago
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

Updated

5 years ago
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

Updated

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

Comment 63

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

Comment 64

5 years ago
> 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

5 years ago
excellent :)
Yes, but the patch uses adds onmozbrowserpropertychange atom, and sets
onmozbrowserpropertychange handler. Why?
(Assignee)

Comment 67

5 years ago
Ah, that's because the atom and the test which uses it are vestiges of days gone by!
(Assignee)

Comment 68

5 years ago
Created attachment 590216 [details] [diff] [review]
Part 3, v2: Add new events
Attachment #590106 - Attachment is obsolete: true
Attachment #590216 - Flags: review?(bugs)
(Assignee)

Comment 69

5 years ago
Created attachment 590217 [details] [diff] [review]
Interdiff part  3 v1.1 --> v2
Assignee: nobody → justin.lebar+bug
Status: NEW → ASSIGNED

Updated

5 years ago
Attachment #590216 - Flags: review?(bugs) → review+
(Assignee)

Comment 70

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

Comment 71

5 years ago
And part 4, enabling for b2g: https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b1cbb631fa
(Assignee)

Updated

5 years ago
status-firefox12: --- → fixed
(Assignee)

Updated

5 years ago
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
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
(Assignee)

Updated

5 years ago
Depends on: 720157
(Reporter)

Comment 73

5 years ago
w00t!

https://github.com/andreasgal/gaia/pull/207
Whiteboard: [qa-]
Keywords: dev-doc-needed
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
Keywords: dev-doc-needed → dev-doc-complete
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.
You need to log in before you can comment on or make changes to this bug.