Closed Bug 757182 Opened 12 years ago Closed 12 years ago

Handle window.close in <iframe mozbrowser>

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

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

References

Details

Attachments

(1 file, 2 obsolete files)

close is the much simpler friend of window.open.
Attached patch Patch v1 (obsolete) — Splinter Review
DOM changes, so you get to review.  :)
Attachment #626128 - Flags: review?(bugs)
Assignee: nobody → justin.lebar+bug
Depends on: 758297
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.
Attachment #626128 - Flags: review?(bugs) → review-
> 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?
> 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 on attachment 626128 [details] [diff] [review]
Patch v1

f? for comment 4.
Attachment #626128 - Flags: review- → feedback?(bugs)
(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.
> 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.
Dispatch whatever event you want in your code and check the return value of dispatchEvent()
(it indicates whether preventDefault was called)
(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 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.
Attachment #626128 - Flags: feedback?(bugs) → review?(bugs)
Comment on attachment 626128 [details] [diff] [review]
Patch v1

Actually, this doesn't quite work in-process.  Patch in a moment.
Attachment #626128 - Flags: review?(bugs) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
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.)
Attachment #626128 - Attachment is obsolete: true
Attachment #630722 - Flags: review?(bugs)
Blocks: 762049
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.
Attachment #630722 - Flags: review?(bugs) → review-
> (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.
Attached patch Patch v3Splinter Review
Attachment #630722 - Attachment is obsolete: true
Attachment #630993 - Flags: review?(bugs)
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.
Attachment #630993 - Flags: review?(bugs) → review+
> You probably want bubble phase here.

Yes, thanks.
I may only get to it on Monday but believe me: I can't wait to test this! :)
https://hg.mozilla.org/mozilla-central/rev/0d138bc9b28e

(Merged by Ed Morley)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 764718
You need to log in before you can comment on or make changes to this bug.