Closed Bug 714234 Opened 9 years ago Closed 9 years ago

Handle window.close() via DOMWindowClose

Categories

(Firefox for Android :: General, defect)

x86
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 12
Tracking Status
firefox11 --- fixed
firefox12 --- fixed

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
While working on the orange failures in bug 712761, we discovered that Fennec crashes if a DOM window tries to close via window.close()

This might be the real reasons for the orange failures.

This patch borrows a bit from the original XUL Fennec bug to support DOMWindowCLose (bug 504352).

The big difference here is Java has the smarts to handle closing a tab and selecting a new one. Therefore we do handle and squelch the DOMWindowClose event in browser.js and then send a request to close the tab to Java via a message.

In Java we merely call Tabs.closeTab(aTab) which then fires more messages and cleans up the tab for us. Tested using the testcase in bug 504352 and it works fine.
Attachment #584922 - Flags: review?(mbrubeck)
Attachment #584922 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 584922 [details] [diff] [review]
patch

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

I'd prefer to put the listener in Tabs.java rather than GeckoApp.java, but otherwise looks good.
Attachment #584922 - Flags: feedback?(bugmail.mozilla) → feedback+
Comment on attachment 584922 [details] [diff] [review]
patch

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

Stealing the review. Looks fine. Why are we using DOMWindowClosed as the message here rather than something like Tab:Close? I see other DOM* messages, so maybe its on purpose?

I think we could probably move a few of these Tab:* messages into Tabs.java, but I'm fine doing that in a follow up.
Attachment #584922 - Flags: review?(mbrubeck) → review+
(In reply to Wesley Johnston (:wesj) from comment #2)
> Comment on attachment 584922 [details] [diff] [review]
> patch
> 
> Review of attachment 584922 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Stealing the review. Looks fine. Why are we using DOMWindowClosed as the
> message here rather than something like Tab:Close? I see other DOM*
> messages, so maybe its on purpose?

Tab:Close is only used to let Java know a Tab was destroyed. It is not used to "start" the Tab close process. The complete "Tab close" process requires selecting a fallback Tab, based on parent or index, then sending Tab:Select and Tab:Close messages to JS. JS then sends back acknowledgement messages to Java letting Java know that a new tab was selected and the desired tab was destroyed.

> I think we could probably move a few of these Tab:* messages into Tabs.java,
> but I'm fine doing that in a follow up.

I agree with this followup cleanup/refactor
Comment on attachment 584922 [details] [diff] [review]
patch

Needed for Aurora as a core feature (handling window.close) and needed because it blocks bug 712761.
Attachment #584922 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/a579327de2c2
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Comment on attachment 584922 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approving for Aurora.
Attachment #584922 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on:

Mozilla/5.0 (Android;Linux armv7l;rv:11.0a2)Gecko/20120130
Firefox/11.0a2 Fennec/11.0a2
Device: Samsung Galaxy S
OS: Android 2.2

Mozilla/5.0 (Android;Linux armv7l;rv:12.0a1)Gecko/20120130
Firefox/12.0a1 Fennec/12.0a1
Device: Samsung Galaxy S
OS: Android 2.2
Status: RESOLVED → VERIFIED
Assignee: nobody → mark.finkle
You need to log in before you can comment on or make changes to this bug.