"restart now" gives Quit dialog instead of Restart dialog

VERIFIED FIXED in mozilla1.9.3a5

Status

()

VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: john.p.baker, Assigned: john.p.baker)

Tracking

Trunk
mozilla1.9.3a5
Points:
---
Bug Flags:
in-testsuite -
in-litmus +

Firefox Tracking Flags

(blocking2.0 beta1+)

Details

(Whiteboard: [rewrite][in-litmus-bug-week])

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
1. Tools/Add-ons
2. Enable/Disable an installed extension
3. Click adjacent "Restart now" button

Result:
Get a "Do you want Minefield to save your tabs for the next time it starts?" dialogue.
Selecting "Save and Quit" or "Quit" _both_ restore tabs on restart.

Expected Result:
Either (a) No dialogue or (b) dialogue with just two options.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a5pre) Gecko/20100519 Minefield/3.7a5pre
(Assignee)

Comment 1

9 years ago
(In reply to comment #0)
> Expected Result:
> Either (a) No dialogue or (b) dialogue with just two options.

(b) Correct dialogue title and buttons as bug 385425
(Assignee)

Comment 2

9 years ago
If I have followed this correctly the problem is moving from

-  os.notifyObservers(cancelQuit, "quit-application-requested", "restart");

to

+        Application.restart();
which eventually does (_quitWithFlags)
672     this._obs.notifyObservers(cancelQuit, "quit-application-requested", null);
Summary: Enable/Disable followed by "restart now" gives "Do you want Minefield to save your tabs for the next time it starts?" → "restart now" gives Quit dialogue instead of Restart dialogue
Same on OS X. Moving to all/all.
Blocks: 550048
OS: Windows 2000 → All
Hardware: x86 → All
Whiteboard: [rewrite]
(Assignee)

Comment 4

9 years ago
At first glance

- this._obs.notifyObservers(cancelQuit, "quit-application-requested", null);
+ this._obs.notifyObservers(cancelQuit, "quit-application-requested",
+                           aFlags & Components.interfaces.nsIAppStartup.eRestart ? "restart" : null);

seems sufficient.
blocking2.0: --- → beta1+
(Assignee)

Comment 5

9 years ago
Created attachment 446686 [details] [diff] [review]
patch

It was tempting to use | : "quit"| but that type is only used internally in nsBrowserGlue.js

>    if (aQuitType != "restart")
>      aQuitType = "quit";
Attachment #446686 - Flags: feedback?(zeniko)

Comment 6

9 years ago
Comment on attachment 446686 [details] [diff] [review]
patch

Looks fine to me. Thanks for the patch.

While you're at it, you might also want to update https://developer.mozilla.org/En/Observer_Notifications to note the meaning of the data argument (which for consistency with "quit-application" should read "shutdown" in the non-restarting case).
Attachment #446686 - Flags: feedback?(zeniko) → feedback+
(Assignee)

Comment 7

9 years ago
Comment on attachment 446686 [details] [diff] [review]
patch

I filed bug 567721 on also using "shutdown" on quit-application-requested notifications.
Attachment #446686 - Flags: review?(mark.finkle)
Attachment #446686 - Flags: review?(mark.finkle) → review+
(Assignee)

Updated

9 years ago
Assignee: nobody → john.p.baker
Keywords: checkin-needed
Summary: "restart now" gives Quit dialogue instead of Restart dialogue → "restart now" gives Quit dialog instead of Restart dialog
http://hg.mozilla.org/mozilla-central/rev/a89c055ff932
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100528 Minefield/3.7a5pre

We should have a manual test here, but what could be tested automatically? Is there anything?
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-litmus?
(Assignee)

Comment 10

9 years ago
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=6906 - but that is the old extension manager. [bug 385425 comment #22]
(In reply to comment #10)
> https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=6906 - but that is
> the old extension manager. [bug 385425 comment #22]

Correct. And I need feedback from Dave what's possible with an automated test right now. Depending on his information I have to update this test.
Blar, can you figure out whether we can automatically test this? It seems possible on the surface to me but you've got more of an idea of where the UI tests are going right now.
(In reply to comment #12)
> Blar, can you figure out whether we can automatically test this? It seems
> possible on the surface to me but you've got more of an idea of where the UI
> tests are going right now.

Er, I guess... though given that this was a FUEL bug (the new EM just happened to make it more obvious), it makes more sense to me for the FUEL tests to cover this.
Added Litmus test:
https://litmus.mozilla.org/show_test.cgi?id=10920

(In reply to comment #12)
> Blar, can you figure out whether we can automatically test this? It seems
> possible on the surface to me but you've got more of an idea of where the UI
> tests are going right now.

Leaving the in-testsuite? request until we get a response from Blair.
Flags: in-litmus? → in-litmus+
Whiteboard: [rewrite] → [rewrite][in-litmus-bug-week]
This dialog is no longer shown by default so I don't think it is worth testing it.
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.