Last Comment Bug 757182 - Handle window.close in <iframe mozbrowser>
: Handle window.close in <iframe mozbrowser>
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on: 758297 764718
Blocks: 744451 762049
  Show dependency treegraph
 
Reported: 2012-05-21 13:21 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-06-20 10:03 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (6.92 KB, patch)
2012-05-22 12:25 PDT, Justin Lebar (not reading bugmail)
justin.lebar+bug: review-
Details | Diff | Splinter Review
Patch v2 (7.81 KB, patch)
2012-06-06 14:29 PDT, Justin Lebar (not reading bugmail)
bugs: review-
Details | Diff | Splinter Review
Patch v3 (7.24 KB, patch)
2012-06-07 08:27 PDT, Justin Lebar (not reading bugmail)
bugs: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2012-05-21 13:21:35 PDT
close is the much simpler friend of window.open.
Comment 1 Justin Lebar (not reading bugmail) 2012-05-22 12:25:19 PDT
Created attachment 626128 [details] [diff] [review]
Patch v1

DOM changes, so you get to review.  :)
Comment 2 Olli Pettay [:smaug] 2012-05-31 11:20:20 PDT
Comment on attachment 626128 [details] [diff] [review]
Patch v1

So why do we need DOMWindowClosing and DOMWindowClose?
Couldn't BrowserElementChild listen for the latter, perhaps in
system event group and check whether preventDefault was called.
Comment 3 Justin Lebar (not reading bugmail) 2012-05-31 22:38:20 PDT
> Couldn't BrowserElementChild listen for the latter, perhaps in
> system event group and check whether preventDefault was called.

My concern was that someone could preventDefault on the event after I checked.

But I think this was borne out of ignorance of the event model.  The right thing to do is to register a capturing listener in the system event group, right?  Nobody can call prevent default after the bubbling phase?
Comment 4 Justin Lebar (not reading bugmail) 2012-06-04 21:45:43 PDT
> The right thing to do is to register a capturing listener in the system event group, right?

Ah, now I remember why I did things this way.

Our event listener needs to fire after everyone who might call preventDefault has a chance to call preventDefault, because we do not want to fire a windowclosed event on the iframe if anybody canceled the close.

If we say that chrome may preventDefault during the system bubbling phase, then my listener has to run after that.  That would be during the system capture phase, but of course I can't call preventDefault there myself!

In other words, if I were to use the system bubbling phase here, that would be an assertion that I am the only one who may ever call preventDefault() during that phase.  Which seems pretty wrong to me.

What's the right way to handle this in the event model?
Comment 5 Justin Lebar (not reading bugmail) 2012-06-04 21:46:01 PDT
Comment on attachment 626128 [details] [diff] [review]
Patch v1

f? for comment 4.
Comment 6 Olli Pettay [:smaug] 2012-06-05 01:47:43 PDT
(In reply to Justin Lebar [:jlebar] from comment #4)
> > The right thing to do is to register a capturing listener in the system event group, right?
> 
> Ah, now I remember why I did things this way.
> 
> Our event listener needs to fire after everyone who might call
> preventDefault has a chance to call preventDefault,
So, you need system event group.

> because we do not want
> to fire a windowclosed event on the iframe if anybody canceled the close.
> 
> If we say that chrome may preventDefault during the system bubbling phase,
> then my listener has to run after that.
There is no way to do that. You can add listener to bubble phase, but if something
higher up in the event target chain calls preventDefault(), that happens after you have
handled the event.
 
> What's the right way to handle this in the event model?
System event group is the best we have for this. Other option is to change the code so
that you dispatch the event manually and check the return value of dispatchEvent.
Comment 7 Justin Lebar (not reading bugmail) 2012-06-05 07:56:36 PDT
> Other option is to change the code so
> that you dispatch the event manually and check the return value of dispatchEvent.

Could you elaborate on what you mean here?  I don't understand.
Comment 8 Olli Pettay [:smaug] 2012-06-06 07:54:14 PDT
Dispatch whatever event you want in your code and check the return value of dispatchEvent()
(it indicates whether preventDefault was called)
Comment 9 Justin Lebar (not reading bugmail) 2012-06-06 08:23:46 PDT
(In reply to Olli Pettay [:smaug] from comment #8)
> Dispatch whatever event you want in your code and check the return value of
> dispatchEvent()
> (it indicates whether preventDefault was called)

Isn't that what this patch does?
Comment 10 Justin Lebar (not reading bugmail) 2012-06-06 12:12:30 PDT
Comment on attachment 626128 [details] [diff] [review]
Patch v1

...and back to r? because I don't understand how comment 8 is different from what this patch is doing.
Comment 11 Justin Lebar (not reading bugmail) 2012-06-06 14:26:29 PDT
Comment on attachment 626128 [details] [diff] [review]
Patch v1

Actually, this doesn't quite work in-process.  Patch in a moment.
Comment 12 Justin Lebar (not reading bugmail) 2012-06-06 14:29:07 PDT
Created attachment 630722 [details] [diff] [review]
Patch v2

Works in-process too, now.

(I'm fixing the tests so they always run in- and out-of-process, so I won't overlook things like this, in bug 762049.)
Comment 13 Olli Pettay [:smaug] 2012-06-07 05:21:58 PDT
Comment on attachment 630722 [details] [diff] [review]
Patch v2

So, how does DOMWindowClosing help with anything? There can still be
listener in system group/bubble phase after your listener.
(When I said dispatch your own event, I mean the the JS code would need
to dispatch it and check the return value)

Just use the existing event.
Comment 14 Justin Lebar (not reading bugmail) 2012-06-07 08:00:25 PDT
> (When I said dispatch your own event, I mean the the JS code would need
> to dispatch it and check the return value)

I'm still not sure what you mean.  What JS code, and when?

> So, how does DOMWindowClosing help with anything? There can still be
> listener in system group/bubble phase after your listener.

I guess I did not explain this well in the code:

> +  // <iframe mozbrowser> needs a chance to handle this close event after
> +  // DOMWindowClose.  At this point, the window is going to close; it's just a
> +  // matter of whether we're going to call FinalClose() or if someone else is
> +  // going to promise to close the window some other way.

My thought was that DOMWindowClosing means "the window is going away -- nothing you can do about it".  The only question is whether we're going to call FinalClose().

> Just use the existing event.

Okay.
Comment 15 Justin Lebar (not reading bugmail) 2012-06-07 08:27:06 PDT
Created attachment 630993 [details] [diff] [review]
Patch v3
Comment 16 Olli Pettay [:smaug] 2012-06-07 09:08:34 PDT
Comment on attachment 630993 [details] [diff] [review]
Patch v3


>+    els.addSystemEventListener(global, 'DOMWindowClose',
>+                               this._closeHandler.bind(this),
>+                               /* useCapture = */ true);
You probably want bubble phase here.
Comment 17 Justin Lebar (not reading bugmail) 2012-06-07 10:09:10 PDT
> You probably want bubble phase here.

Yes, thanks.
Comment 18 Justin Lebar (not reading bugmail) 2012-06-07 10:29:33 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d138bc9b28e
Comment 19 Etienne Segonzac (:etienne) 2012-06-07 10:32:28 PDT
I may only get to it on Monday but believe me: I can't wait to test this! :)
Comment 20 Graeme McCutcheon [:graememcc] 2012-06-08 04:21:02 PDT
https://hg.mozilla.org/mozilla-central/rev/0d138bc9b28e

(Merged by Ed Morley)

Note You need to log in before you can comment on or make changes to this bug.