741587 - Browser API: Handle alert/prompt/confirm in <iframe mozbrowser>

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

My plan is to handle these completely in gecko, rather than forwarding them up to the browser app.

My reasoning is that these functions are rarely called, and while it's important that they work, it's not critical that a browser app be able to override them arbitrarily.  We can change this down the road, if we want.

The alerts will be modal to the <iframe mozbrowser>.

Comment 1

[[:fabrice] Fabrice Desré]

I agree with keeping these in gecko. Some points to keep in mind:
- we'll need a virtual keyboard for some of them, and the vkb is part of the homescreen.
- it would be nice if the homescreen could provide a CSS resource to style the dialogs (à la userChrome.css, loaded from a <link rel=...> ).

Comment 2

[Justin Lebar (not reading bugmail)]

> - it would be nice if the homescreen could provide a CSS resource to style the dialogs (à la 
> userChrome.css, loaded from a <link rel=...> ).

I don't like this from the perspective of a web API -- it means that the HTML structure of our dialog box becomes part of the standard.  So long as these dialogs are in Gecko, I'd rather them be styled by a chrome stylesheet.  Otherwise, we might as well let the browser app override the dialogs arbitrarily.

Comment 3

[[:fabrice] Fabrice Desré]

We don't have to expose all the structure of the dialog boxes. I think that it's legitimate for homescreen authors to want prompts that blend in well with their look and feel.

What we need to be sure is that they have no way to hide stuff that should be displayed and can't sniff input.

Comment 4

[Justin Lebar (not reading bugmail)]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> We don't have to expose all the structure of the dialog boxes.

If you allow me to use arbitrary CSS to style the form, then I am aware of the form's HTML structure, right?  I can do things like div > div#bar > span.foo { color: green; }.

I don't think we want to write a spec which says "this is exactly how the dialog must be laid out."  Nor do I think we want to write a spec which says "the dialog's exact layout is undefined, and authors must rely only on the following properties".

> I think that
> it's legitimate for homescreen authors to want prompts that blend in well
> with their look and feel.

If so, I think this is an argument for not implementing these prompts in Gecko.  I was hoping that we wouldn't have to do this, because these prompts are not particularly important.

> What we need to be sure is that they have no way to hide stuff that should
> be displayed and can't sniff input.

This is major scope creep, and anyway hard to do.  The browser can already sniff input by displaying a fake alert, so I don't think we should protect against that.  And the if the browser hides things, then it's just breaking itself.

Comment 5

[Justin Lebar (not reading bugmail)]

> What we need to be sure is that they have no way to hide stuff that should
> be displayed and can't sniff input.

It's basically impossible to protect against a malicious browser, in general.

Comment 6

[Justin Lebar (not reading bugmail)]

Hm, there's actually a fair bit of code here.

Mostly for my reference, it lives in tabprompts.xmil and is powered by CommonDialog.jsm.  This is all invoked by nsPrompter.js :: getTabModalPrompt.

Hooking into the getTabModalPromptBox call in nsPrompter::getTabModalPrompt is easy.  But then what do we do with it?  Do we forward the raw request for a tab-modal box to the webpage?  If you open CommonDialog.jsm, you'll see that there are actually a lot of different prompts -- all of that would have to be re-implemented, and by each browser.

If we instead tried to use this XUL overlay, I think we'd need to place it inside a chrome window.  Which means we'd need to insert a chrome window directly inside the iframe mozbrowser.  I don't know how easy or hard that would be, but I bet someone somewhere assumes that chrome windows have chrome parents.  :-/

But I'm totally new to all this.  Justin, can you provide any insight here?  What do you think is the right way to handle alerts inside mozbrowser?

Comment 7

[Justin Dolske [:Dolske]]

Hmmm... Yeah, there's a significant amount of code involved, as there are a number of quirks about how dialogs work that can impact web content.

I tried to make this as reusable as possible, with an eye towards XUL Fennec's fork of prompting (which didn't [doesn't?] work right)... The basic idea was that the app's front end would take care of _where_ in the chrome DOM the prompt would go -- see getTabModalPromptBox() in tabbrowser.xml -- and that that everything in the Toolkit prompt implementation would be self-contained.

In Firefox the <xul:tabmodalprompt> gets inserted into a <xul:stack> that's wrapping the <xul:browser> in a tab. For prompts not associated with a content window or otherwise unsuitable for tabmodality [*], we just use the old-skool modal prompt window. See commonDialog.xul/.js -- they've been greatly refactored as part of the tab-modal work.

Does B2G currently support XUL+XBL? If so I'd think this should be fairly easy to adapt -- happy to help. If not [and long-term that's the plan, afaik] it will take a bit more work... We don't have XBL-like thing for the web yet, but we can probably convert a bunch of the XUL to HTML, and figure out a minimally-hacky way to reuse some blob of HTML/JS as our basic usage of XBL currently does.

Also, you need this for both webpages in B2G (ie B2G's browser) as well as B2G apps/chrome itself? Could I get a link to the HTML/XUL involved there?

[*] there are a few things still not compatible with tab-modal windows, even from content, due to potential reentrancy. EG, the current onbeforeunload alert; various platform code still assumes that as it starts to close a tab, nothing else will attempt to close the same tab while the prompt's showing.

Comment 8

[Justin Dolske [:Dolske]]

(In reply to Justin Lebar [:jlebar] from comment #6)

> But I'm totally new to all this.  Justin, can you provide any insight here? 
> What do you think is the right way to handle alerts inside mozbrowser?

On this part specifically -- the current Firefox tab structure (stack wrapping a browser) comes in handy for other things too. So something like that (stack wrapping an iframe?) would be good.

Another thought is to do what video controls do... The <video> element as a native anonymous content frame over it where the <videocontrols> binding lives. It has a few downsides, but means the overlay for the controls (or in your case, prompt) is entirely self-contained.

Beyond that, though, you probably want a layout hacker to help with ideas. :)

Comment 9

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

Attachment: wip-0.1

The proposal add 3 new events on the <iframe mozbrowser>: mozbrowser[alert|confirm|prompt] 

The above patch delegate the handling,styling and localization of prompts to the embedder (Gaia/OWD/...).


The embedder is supposed to call showModalDialog in it's mozbrowser[alert|confirm|prompt] event handler to show a UI and block the calling thread until it returns.

For example:

 var frame = document.getElementById('iframe-mozbrowser');
 frame.addEventListener('mozbrowserprompt', function(evt) {
   evt.detail.resultValue = showModalDialog('prompt.html');
 });

Comment 10

[Justin Lebar (not reading bugmail)]

Comment on attachment 615735 [details] [diff] [review]
wip-0.1

If showModalDialog blocks the content page, this is great.

Comment 11

[Justin Lebar (not reading bugmail)]

Actually...

We're calling showModalDialog in the browser itself.  But we want to show the dialog in the browsee.  We want to block the browsee, but not the browser.  It doesn't look like this patch accomplishes that.

Comment 12

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

Attachment: wip-0.2

(In reply to Justin Lebar [:jlebar] from comment #11)
> Actually...
> 
> We're calling showModalDialog in the browser itself.  But we want to show
> the dialog in the browsee.  We want to block the browsee, but not the
> browser.  It doesn't look like this patch accomplishes that.

Correct, the current patch blocks both the browsee and the browser.

I'm uploading a new patch that uses nsIThread.processNextEvent instead (it does not handle the case where the browsee window is closed, etc...).

This is a different subject but I wonder if the same approach could not be used for window.open too? I have built a hack on top of this patch to let us prototype some on Gaia and it seems to work well.

Comment 13

[Justin Lebar (not reading bugmail)]

> This is a different subject but I wonder if the same approach could not be used for 
> window.open too? I have built a hack on top of this patch to let us prototype some on 
> Gaia and it seems to work well.

Yes, I think so.  Until we go to multi-process, and then I'm not entirely sure how it's going to work.  We may be able to keep the same semantics, creating an event in the parent process and passing the result down to the child process.

Comment 14

[Justin Lebar (not reading bugmail)]

Comment on attachment 616185 [details] [diff] [review]
wip-0.2

It's your own little nested event loop!  :)

I have no idea if this is right.  I imagine the right solution is more complex than this; nsGlobalWindow::ShowModalDialog goes through a lot of trouble.  But if it works, it works!

Comment 15

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

Comment on attachment 616185 [details] [diff] [review]
wip-0.2

(In reply to Justin Lebar [:jlebar] from comment #14)
> Comment on attachment 616185 [details] [diff] [review]
> wip-0.2
> 
> It's your own little nested event loop!  :)
> 
> I have no idea if this is right.  I imagine the right solution is more
> complex than this; nsGlobalWindow::ShowModalDialog goes through a lot of
> trouble.  But if it works, it works!

I have mostly look Fennec's code and also toolkit's code from 
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/src/nsPrompter.js#414

There the workflow seems like to be very straighforward:
 nsIPrompt::alert -> this.openPrompt -> openTabPrompt.

Let's ask more feedback to be sure this is the right way.

Comment 16

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

Attachment: Patch

This patch dispatch an event when there is a window.[alert|confirm|prompt] but also window.[open|close]. I can remove the open/close parts if needed.

I have not used nsIDOMWindowUtils::EnterModalStateWithWindow because it looks for the top window (afaict) and so it doesn't work very well with nested <iframe mozbrowser>. This sounds like an issue that will resolved themselves with e10s but it probably need a followup because e10s can be turned off.

Justin, I think the problem with window.open with multi-process could be resolved by resolving window.currentOuterWindowId on the frame script side assuming windows of the same application leaves on the same process? But that's out of the scope of this bug.

Comment 17

[Justin Lebar (not reading bugmail)]

I'm sorry for taking so long to look at this.  It's top of my list for tomorrow.

Comment 18

[Justin Lebar (not reading bugmail)]

Comment on attachment 616924 [details] [diff] [review]
Patch

I have nits on the comments, but we can handle those after Dolske decides whether he likes the patch.

> +    // Let's make sure the object is created in the scope of the requesting
> +    // window and do not leak any scope informations.
> +    let obj = Cu.createObjectIn(win);

What exactly is this rigamarole preventing, as opposed to defining the function within the second Object.defineProperty?

Whatever the reason (I'm sure it's a good one), I'd prefer

> Object.defineProperty(XPCNativeWrapper.unwrap(win), name, {
>    get: function() {
>-     return obj[name];
>+     return obj.func;
>    }
>  });

so it's clear that the name is not significant here.  Unless it somehow is.  :)

Also, does it work if you define the property on unwrap(window.prototype)?

Comment 19

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

(In reply to Justin Lebar [:jlebar] from comment #18)
> Comment on attachment 616924 [details] [diff] [review]
> Patch
> 
> I have nits on the comments, but we can handle those after Dolske decides
> whether he likes the patch.

Really? There are mistakes in my english, I'm surprised! ;)

> > +    // Let's make sure the object is created in the scope of the requesting
> > +    // window and do not leak any scope informations.
> > +    let obj = Cu.createObjectIn(win);
> 
> What exactly is this rigamarole preventing, as opposed to defining the
> function within the second Object.defineProperty?

When trying to coalesce the object into a regular string (dump(alert)) there was an xpconnect assertion with defineProperty. So I asked mrbkap and he brings me to this method. (CC'ing him).


Also more generally I don't really like to have to return the value in event.detail.result but for the moment I don't see any other good way of doing that. The future web activities (bug 715814) has a new event interface:
interface nsIDOMMozActivityEvent : nsIDOMEvent {
    readonly attribute nsIDOMActivity activity;

    // data will go to request.result.
    void postResult(in jsval data);

    // data will go to request.error.
    void postError(in DOMString data);
}

I wonder if the couple postResult(in jsval data) / postError(in DOMString data) can fit here? (CC'ing smaug).

Comment 20

[Justin Lebar (not reading bugmail)]

Comment on attachment 616924 [details] [diff] [review]
Patch

This needs to be re-done now that we landed bug 749018.  I'll take care of it.

It also needs tests...

Comment 21

[Justin Dolske [:Dolske]]

I'm confused, where/how is the actual prompt being shown?

Comment 22

[Justin Lebar (not reading bugmail)]

(In reply to Justin Dolske [:Dolske] from comment #21)
> I'm confused, where/how is the actual prompt being shown?

Suppose parent.html contains <iframe id='frame' mozbrowser src=child.html>, and child.html calls alert().

We fire an event at frame, which parent listens to.  It's then parent's responsibility to show the prompt.

Comment 23

[Justin Lebar (not reading bugmail)]

This may be too late, but I have a (kind of OK) alert() and a (totally bogus) open() implementation on my m-c clone.  There's no close() -- I'd need to talk to someone about how they need that to work, exactly.

  https://github.com/jlebar/mozilla-central/tree/mozbrowser-alert-741587

Comment 24

[Justin Lebar (not reading bugmail)]

Attachment: WIP v1

This has alert (blocking), open, and close.  But it only notices when you try to close yourself, not when you try to close other windows.

Comment 25

[Justin Lebar (not reading bugmail)]

Attachment: WIP v1.1 (rebased atop hg e6a9572b48f)

Comment 26

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

For things like .open() TabChild/Parent have already ProvideWindow and there is also
OpenDialog. (I vaguely remember that someone broke the latter when changing modal dialog handling.)

Comment 27

[Etienne Segonzac (:etienne)]

Attachment: Adds to the wip

Comment 28

[Justin Lebar (not reading bugmail)]

Bobby,

Vivien's earlier patch did the following to override a method on a window (this is basically _overrideMethod in attachment 616924 [details] [diff] [review]):
'
  function overrideMethod(win, fnName, fnImpl) {
    let obj = Cu.createObjectIn(win);
    Object.defineProperty(obj, 'method', {
      enumerable: true, configurable: true, writable: true,
      value: function() {
	// Do some other stuff.
        fnImpl();
      }.bind(this)
    });

    Cu.makeObjectPropsNormal(obj);

    Object.defineProperty(XPCNativeWrapper.unwrap(win), fnName, {
      get: function() { return obj.method; }
    });

Can you help me understand what exactly this rigmarole is preventing, as opposed to

  function overrideMethod(win, fnName, fnImpl) {
    Object.defineProperty(XPCNativeWrapper.unwrap(win), fnName, {
      get: function() { return fnImpl; }
     });
   }

?  (Simply doing unwrap(win).fnName = fnImpl doesn't work; the method is not visible to content.)

Comment 29

[Justin Lebar (not reading bugmail)]

Also, supposing I don't want |win| to be able to access |window.fnName| anymore (e.g., I want to override window.alert), I guess I need to do the same thing on unwrap(win.prototype).  Anything else?

Comment 30

[Justin Lebar (not reading bugmail)]

Now that I've worked on both alert and open some, I think there's no reason to do them in the same bug.  I think open can use a sync call, while alert (and prompt and confirm) need a nested event loop.

I'll split out the open work into a separate bug.

Comment 31

[Bobby Holley (:bholley)]

(In reply to Justin Lebar [:jlebar] from comment #28)
> Bobby,
> 
> Vivien's earlier patch did the following to override a method on a window
> (this is basically _overrideMethod in attachment 616924 [details] [diff] [review]
> [review]):
> '
>   function overrideMethod(win, fnName, fnImpl) {
>     let obj = Cu.createObjectIn(win);
>     Object.defineProperty(obj, 'method', {
>       enumerable: true, configurable: true, writable: true,
>       value: function() {
> 	// Do some other stuff.
>         fnImpl();
>       }.bind(this)
>     });
> 
>     Cu.makeObjectPropsNormal(obj);
> 
>     Object.defineProperty(XPCNativeWrapper.unwrap(win), fnName, {
>       get: function() { return obj.method; }
>     });
> 
> Can you help me understand what exactly this rigmarole is preventing, as
> opposed to
 
>   function overrideMethod(win, fnName, fnImpl) {
>     Object.defineProperty(XPCNativeWrapper.unwrap(win), fnName, {
>       get: function() { return fnImpl; }
>      });
>    }
> 

This won't work, because your |this| binding is incorrect. I've noticed you did this elsewhere, too. Why are you trying to define methods properties with a getter? It's not the way this stuff is supposed to be done. Methods should be value descriptors, and if you want to make them unchangeable you should set writable and configurable to |false| in Object.defineProperty.

> ?  (Simply doing unwrap(win).fnName = fnImpl doesn't work; the method is not
> visible to content.)

Hm, really? It should, I think. I do it in chrome mochitests all the time:
https://hg.mozilla.org/mozilla-central/rev/66223f04fb55#l2.32

(In reply to Justin Lebar [:jlebar] from comment #29)
> Also, supposing I don't want |win| to be able to access |window.fnName|
> anymore (e.g., I want to override window.alert), I guess I need to do the
> same thing on unwrap(win.prototype).  Anything else?

Do you mean "able to access" in a security sense? If so, we need to talk about this.

Comment 32

[Justin Lebar (not reading bugmail)]

>> ?  (Simply doing unwrap(win).fnName = fnImpl doesn't work; the method is not
>> visible to content.)
>
> Hm, really? It should, I think. 

Maybe it has to do with window specialness?  I recall that expandos are stored on the inner window, so they go away when we navigate?  (I think actually we explicitly clear them when destroying the inner window; I observed something like that here.)  Whereas Object.defineProperty goes on the outer window and doesn't get cleared?

> Do you mean "able to access" in a security sense?

Yes; it would be a big problem if a page called the vanilla "alert" method, for example.

> If so, we need to talk about this.

Okay, but please understand that all of this needs to be done some time ago.  If we need to do this the Right Way by modifying XPCOM and blocking access via wrappers (or something), I'm almost surely going to need your help, and right away.

It sounds like you're saying that neither of the pieces of code in comment 28 is right, so I might as well go with the second one for now until we figure out the Right Way; is that right?

Comment 33

[Justin Lebar (not reading bugmail)]

The window.open bug is 742944, in case anyone is looking for it.

Comment 34

[Justin Lebar (not reading bugmail)]

Attachment: WIP v2

This doesn't use Object.defineProperty (which we've decided is unworkable); instead it wraps the nsIPromptFactory.

alert-only at the moment, and the test only works when run individually (instead of as part of the suite), but it's coming along...

Comment 35

[Justin Lebar (not reading bugmail)]

Attachment: WIP v3

Now the test works when run as part of the suite.  Still alert-only, but getting there...

Comment 36

[Justin Lebar (not reading bugmail)]

Attachment: WIP v4

confirm() seems to work, but prompt appears to be returning "" for some reason.  Not sure yet what's going on, but we're getting close.

Comment 37

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

Comment 38

[Justin Lebar (not reading bugmail)]

Attachment: Tests, v1

Comment 39

[Justin Lebar (not reading bugmail)]

Note the dependency on bug 754997.

Comment 40

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

Comment on attachment 625705 [details] [diff] [review]
Patch, v1


>+  inline nsGlobalWindow *GetScriptableTop()
>+  {
>+    nsCOMPtr<nsIDOMWindow> top;
>+    GetScriptableTop(getter_AddRefs(top));
>+    if (top)
>+      return static_cast<nsGlobalWindow *>(top.get());
if (expr) {
  stmt;
}


Bug I don't know what GetScriptableTop does.
Which patch adds it?

The JS part needs review also from someone more familiar 
with JS modules etc.

Comment 41

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

Comment on attachment 625705 [details] [diff] [review]
Patch, v1

Ok, found the scriptabletop

Comment 42

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

Comment on attachment 625705 [details] [diff] [review]
Patch, v1

Hmm, actually, it isn't clear to me how the event handling works.
Could you explain.

Comment 43

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

Comment on attachment 625705 [details] [diff] [review]
Patch, v1

>+  handleShowModalPrompt: function(frameElement, data) {
>+    let detail = data.json;
>+    debug('handleShowPrompt ' + JSON.stringify(detail));
>+
>+    // Fire a startModalMsg event on the iframe. 
Er, what event?

 If we prevent default on it,
>+    // then the child will spin and wait until the frame element receives a
>+    // unblock-modal-prompt message.
And it gets that message from where?

> nsDOMWindowUtils::LeaveModalStateWithWindow(nsIDOMWindow *aWindow)
> {
>   nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
>   NS_ENSURE_STATE(window);
> 
>-  NS_ENSURE_ARG_POINTER(aWindow);
>   window->LeaveModalState(aWindow);
>   return NS_OK;
> }
This isn't needed. There is LeaveModalState()

Yes, this all needs some more documentation in the code.

Comment 44

[Justin Lebar (not reading bugmail)]

> >+    // Fire a startModalMsg event on the iframe. 
> Er, what event?

It should be showmodalprompt.

> >+    // then the child will spin and wait until the frame element receives a
> >+    // unblock-modal-prompt message.
> And it gets that message from where?

I guess I should say, we spin until *we* receive an unblock-modal-prompt message.  Which happens when content calls e.detail.unblock().

> This isn't needed. There is LeaveModalState()

It's noscript.

Comment 45

[Justin Lebar (not reading bugmail)]

For those following along at home, the main question blocking review atm is whether we need to provide a default implementation of alert in this patch.  I don't think it's necessary at the moment (and I don't think we know enough to know whether we'll ever want it), and smaug is thinking about it.

Comment 46

[Justin Lebar (not reading bugmail)]

Comment on attachment 625705 [details] [diff] [review]
Patch, v1

Vivien, can you look at the JS bits here?

Comment 47

[Justin Lebar (not reading bugmail)]

Comment on attachment 625706 [details] [diff] [review]
Tests, v1

This incorrectly contains test_browserFrameOpen.html.  I've moved it out of the patch locally.

Comment 48

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

Comment on attachment 625705 [details] [diff] [review]
Patch, v1

Review of attachment 625705 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/BrowserElementChild.js
@@ +47,5 @@
>      debug("Starting up.");
>      sendAsyncMsg("hello");
>  
> +    BrowserElementPromptService.mapWindowToBrowserElementChild(content, this);
> +

I feel uncomfortable by having BrowserElementPromptService referenced here.
I wonder if there is a chance to create a cycle if someone decide to store a reference as an internal property of BrowserElementChild?

If yes, what about moving this call outside of BrowserElementChild and next to the end of the file by doing: BrowserElementPromptService.mapWindowToBrowserElementChild(content, api);

@@ +114,5 @@
> +    args.windowID = { outer: utils.outerWindowID,
> +                      inner: this._tryGetInnerWindowID(win) };
> +    sendAsyncMsg('showmodalprompt', args);
> +
> +    let returnValue = this._waitForResult(win, args.promptType);

args.promptType seems to be never used

@@ +144,5 @@
> +    this._windowIDDict[outerWindowID] = Cu.getWeakReference(win);
> +
> +    // We can't use nsIDOMWindowUtils::EnterModalStateWithWindow because that
> +    // doesn't respect <iframe mozbrowser> boundaries, so we suppress event
> +    // handling and timeouts ourselves.

This comment seems deprecated based on your comment in enterModalState in nsGlobalWindow.cpp

@@ +176,5 @@
> +      }
> +
> +      //debug("NESTED EVENT LOOP event start");
> +      thread.processNextEvent(/* mayWait = */ true);
> +      //debug("NESTED EVENT LOOP event end");

nit: Remove the debugs leftover

::: dom/base/BrowserElementPromptService.jsm
@@ +1,5 @@
> +/* 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/. */
> +
> +"use strict"

nit: add a ;

@@ +9,5 @@
> +let Cc = Components.classes;
> +let Cr = Components.results;
> +let Cm = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);
> +
> +let EXPORTED_SYMBOLS = ['BrowserElementPromptService'];

nit: Seems like there is a mix of ' and "

Comment 49

[Justin Lebar (not reading bugmail)]

Thanks for the review!

> I feel uncomfortable by having BrowserElementPromptService referenced here.
> I wonder if there is a chance to create a cycle if someone decide to store a reference as an 
> internal property of BrowserElementChild?

Surely creating a cycle -- which this code doesn't do -- would be OK, since the JS GC handles cycles?

Comment 50

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

(In reply to Justin Lebar [:jlebar] from comment #49)
> Thanks for the review!
> 
> > I feel uncomfortable by having BrowserElementPromptService referenced here.
> > I wonder if there is a chance to create a cycle if someone decide to store a reference as an 
> > internal property of BrowserElementChild?
> 
> Surely creating a cycle -- which this code doesn't do -- would be OK, since
> the JS GC handles cycles?

Sounds good.

Comment 51

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Incorporates Vivien's comments plus Olli's first round of comments.  Also rebased to tip.  Interdiff available upon request.

Comment 52

[Justin Lebar (not reading bugmail)]

Olli, would you be able to get to this review faster if I split up this patch into the part I think you need to review?  There are event-ish changes that I'd really like you in particular to look at, but at the same time, this patch rots every time someone touches these files, and rebasing it is a waste of time.

Comment 53

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

I get to it when I get to it. Today, or if not, kick me.
Sorry for the slight delay.

Comment 54

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

Comment on attachment 627969 [details] [diff] [review]
Patch v2


>+  _waitForResult: function(win) {
>+    debug("_waitForResult(" + win + ")");
>+    let utils = win.QueryInterface(Ci.nsIInterfaceRequestor)
>+                   .getInterface(Ci.nsIDOMWindowUtils);
>+
>+    let outerWindowID = utils.outerWindowID;
>+    let innerWindowID = this._tryGetInnerWindowID(win);
>+    if (innerWindowID === null) {
>+      // I have no idea what waiting for a result means when there's no inner
>+      // window, so let's just bail.
>+      debug("_waitForResult: No inner window. Bailing.");
>+      return;
>+    }
>+
>+    this._windowIDDict[outerWindowID] = Cu.getWeakReference(win);
>+
>+    debug("Entering modal state (outerWindowID=" + outerWindowID + ", " +
>+                                "innerWindowID=" + innerWindowID + ")");
>+
>+    // In theory, we're supposed to pass |modalStateWin| back to
>+    // leaveModalStateWithWindow.  But in practice, the window is always null,
>+    // because it's the window associated with this script context, which
>+    // doesn't have a window.  But we'll play along anyway in case this
>+    // changes.
I don't understand this. You should know which window is about enter/leave modalstate.

>+  handleShowModalPrompt: function(frameElement, data) {
>+    // Fire a showmodalprmopt event on the iframe.  When this method is called,
>+    // the child is spinning in a nested event loop waiting for an
>+    // unblock-modal-prompt message.
>+    //
>+    // If the showmodalprompt event has preventDefault() called on it, we'll
>+    // send the unblock-modal-prompt message to the child as soon as the event
>+    // is done dispatching.
>+    //
>+    // Otherwise, we'll unblock the child when event.detail.unblock() is
>+    // called.
So, this comment is wrong.

Could I see still one more patch.

Comment 55

[Justin Lebar (not reading bugmail)]

(Sorry for not responding earlier; today was "mark all bugmail as spam" day.)

> I don't understand this. You should know which window is about enter/leave modalstate.

AIUI it's just a quirk of the enterModalStateWithWindow function.  That function looks at the topmost script context and returns its window.  For this script, it returns null.

We of course know which window is entering/leaving modalstate; that's |win|.

> Could I see still one more patch.

Yes, absolutely!

Comment 56

[Justin Lebar (not reading bugmail)]

>> Could I see still one more patch.
>
> Yes, absolutely!

Circling back here, comment 54 asked for two changes, but I addressed one of them in comment 55.  The other change is comment-only: s/has preventDefault() called/does not have preventDefault() called/.

I'll post a new patch, but it's just going to have that one change...

Comment 57

[Justin Lebar (not reading bugmail)]

Attachment: Patch v3

Comment 58

[Justin Lebar (not reading bugmail)]

Patch v2 --> patch v3

$ interdiff <(curl -L https://bugzilla.mozilla.org/attachment.cgi?id=627969) <(curl -L https://bugzilla.mozilla.org/attachment.cgi?id=630058)

>diff -u b/dom/base/BrowserElementParent.js b/dom/base/BrowserElementParent.js
>--- b/dom/base/BrowserElementParent.js
>+++ b/dom/base/BrowserElementParent.js
>@@ -114,7 +114,7 @@
>     addMessageListener("iconchange", this._fireEventFromMsg);
>     addMessageListener("get-mozapp-manifest-url", this._sendMozAppManifestURL);
>     addMessageListener("keyevent", this._fireKeyEvent);
>-    addMessageListener("showmodalprompt", this.handleShowModalPrompt);
>+    addMessageListener("showmodalprompt", this._handleShowModalPrompt);
>     mm.addMessageListener('browser-element-api:got-screenshot',
>                           this._recvGotScreenshot.bind(this));
> 
>@@ -146,17 +146,17 @@
>     frameElement.dispatchEvent(evt);
>   },
> 
>-  handleShowModalPrompt: function(frameElement, data) {
>+  _handleShowModalPrompt: function(frameElement, data) {
>     // Fire a showmodalprmopt event on the iframe.  When this method is called,
>     // the child is spinning in a nested event loop waiting for an
>     // unblock-modal-prompt message.
>     //
>-    // If the showmodalprompt event has preventDefault() called on it, we'll
>-    // send the unblock-modal-prompt message to the child as soon as the event
>-    // is done dispatching.
>+    // If the embedder calls preventDefault() on the showmodalprompt event,
>+    // we'll block the child until event.detail.unblock() is called.
>     //
>-    // Otherwise, we'll unblock the child when event.detail.unblock() is
>-    // called.
>+    // Otherwise, if preventDefault() is not called, we'll send the
>+    // unblock-modal-prompt message to the child as soon as the event is done
>+    // dispatching.
> 
>     let detail = data.json;
>     debug('handleShowPrompt ' + JSON.stringify(detail));

Comment 59

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

Comment on attachment 630058 [details] [diff] [review]
Patch v3


>+    frameElement.dispatchEvent(evt);
>+
>+    if (!evt.defaultPrevented) {


Maybe just 
if (frameElement..dispatchEvent(evt)) {

>+  alert: function(title, text) {
>+    this._browserElementChild.showModalPrompt(
>+      this._win, {promptType: "alert", title: title, message: text});
space after { and before } to keep this more readable. Same also elsewhere

>+  inline nsGlobalWindow *GetScriptableTop()
nsGlobalWindow*

>+  {
>+    nsCOMPtr<nsIDOMWindow> top;
>+    GetScriptableTop(getter_AddRefs(top));
>+    if (top)
>+      return static_cast<nsGlobalWindow *>(top.get());
if (expr) {
  stmt;
}

Comment 60

[Justin Lebar (not reading bugmail)]

{ foo: bar } and yet [foo, bar].  What a strange world we live in!

Thanks for the review.  :)

Comment 61

[Justin Lebar (not reading bugmail)]

(In reply to Justin Lebar [:jlebar] from comment #60)
> { foo: bar } and yet [foo, bar].  What a strange world we live in!

I believe in matching existing style in the module, which is {foo: bar}.  (See e.g. BrowserElementParent.js.)  So if it's OK with you, I'm going to use that.

Comment 62

[Justin Lebar (not reading bugmail)]

Landed for FF16:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc726c476bb5
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cb6215dea02

I'll handle the spacing issues as a follow-up if need be.

Comment 63

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

No need to.
{foo: bar} just looks wrong to me, but if that style is used elsewhere, then ok.
I don't know if Gecko coding style says anything about that.
(I'm not too familiar with JS coding style.)

Comment 64

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/fc726c476bb5
https://hg.mozilla.org/mozilla-central/rev/9cb6215dea02

(Merged by Ed Morley)

Comment 65

[Serge Gautherie (:sgautherie)]

Comment on attachment 630058 [details] [diff] [review]
Patch v3

Review of attachment 630058 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/installer/removed-files.in
@@ +872,5 @@
>    components/amWebInstallListener.js
>    components/browser.xpt
> +  components/BrowserElementParent.js
> +  components/BrowserElementParent.manifest
> +  components/BrowserElementPromptService.jsm

This line should be
modules/BrowserElementPromptService.jsm

Comment 66

[Justin Lebar (not reading bugmail)]

> This line should be
> modules/BrowserElementPromptService.jsm

If you say so!  Are you going to push that change?