566866 - "restart now" gives Quit dialog instead of Restart dialog

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[John P Baker]

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

Comment 1

[John P Baker]

(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

Comment 2

[John P Baker]

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);

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Same on OS X. Moving to all/all.

Comment 4

[John P Baker]

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.

Comment 5

[John P Baker]

Attachment: patch

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

>    if (aQuitType != "restart")
>      aQuitType = "quit";

Comment 6

[Simon Bünzli]

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).

Comment 7

[John P Baker]

Comment on attachment 446686 [details] [diff] [review]
patch

I filed bug 567721 on also using "shutdown" on quit-application-requested notifications.

Comment 8

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/a89c055ff932

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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?

Comment 10

[John P Baker]

https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=6906 - but that is the old extension manager. [bug 385425 comment #22]

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(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.

Comment 12

[Dave Townsend [:mossop]]

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.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 15

[Dave Townsend [:mossop]]

This dialog is no longer shown by default so I don't think it is worth testing it.