978610 - [e10s] Make window.close() work

Status: RESOLVED FIXED

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

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: window-close

I mostly just copies what we do in non-e10s code. See the DOMWindowClose event.

Comment 1

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

How does b2g deal with this stuff?

Comment 2

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

Comment on attachment 8384313 [details] [diff] [review]
window-close

We need to coordinate this kind of changes with b2g. And have some tests.

Comment 3

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

Comment on attachment 8384313 [details] [diff] [review]
window-close

And DOMTabClose is, well, tab specific. We will use remote stuff for other
remote browsers too.
So perhaps DOMRemoteBrowserClose.

Also, isn't that this.tabs.length thing doing the wrong thing here.
It prevent us to close the last tab/window.

DOMWindowClose doesn't call event.preventDefault which means nsGlobalWindow
doesn't try to close itself.

I think it would be better to mimic the non-OOP behavior.
TabChild could catch DOMWindowClose and forward it synchronously to
parent and parent then deals it the same way as it does local
DOMWindowClose.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: window-close v2

Does this look right?

Comment 5

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

Comment on attachment 8385604 [details] [diff] [review]
window-close v2

>+TabParent::RecvCloseWindow()
>+{
>+  nsContentUtils::DispatchTrustedEvent(mFrameElement->OwnerDoc(), mFrameElement,
>+                                       NS_LITERAL_STRING("DOMRemoteWindowClose"),
>+                                       true, true);
Null check mFrameElement.

But this needs a toolkit/ review, since I'm not sure whether calling window.close() is the right thing.
I believe BrowserTryToCloseWindow() or closeWindow(some params) should be used.

Also, I don't know how b2g app closing works.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8385604 [details] [diff] [review]
window-close v2

> But this needs a toolkit/ review, since I'm not sure whether calling
> window.close() is the right thing.
> I believe BrowserTryToCloseWindow() or closeWindow(some params) should be used.

I don't see where non-e10s code would call those functions, so I don't think we should call them in e10s either. When there's only one tab, the non-e10s code just seems to handle everything in nsGlobalWindow, which I think is what window.close() does.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: window-close v3

It turns out this patch does break b2g, so I added a branch to avoid that.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: window-close v4

Oops, wrong patch.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: window-close v5

Getting this patch to work on b2g has really been painful. The IsBrowserOrApp checks didn't work and I don't know why. Anyway, this patch does everything in frontend code. Sorry for the churn, Felipe.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8391381 [details] [diff] [review]
window-close v5

Never mind. This one doesn't work either.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: window-close v6

This one is really conservative. I don't think there's any way it should affect b2g.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8391466 [details] [diff] [review]
window-close v6

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

do you think bug 924260 needs to be addressed together?

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/18e9c7c020bd

You mean fixing the assertion mentioned in bug 924260? That seems separate I think.

Comment 14

[Bob Owen (:bobowen)]

(In reply to Bill McCloskey (:billm) from comment #13)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/18e9c7c020bd
> 
> You mean fixing the assertion mentioned in bug 924260? That seems separate I
> think.

Yeah, I'm just starting to look at sandboxing / e10s stuff, so I'm happy to pick up bug 924260 as a separate piece.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/18e9c7c020bd

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

billm, can you describe how QA can test and verify this fix? Thanks!

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We have tests to check this and they're running on tinderbox now, so there's no need to verify.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Thanks Bill!