Closed Bug 1156176 Opened 9 years ago Closed 8 years ago

“Allow Firefox Developer Edition and Firefox to run at the same time” lacks "Restart later" option

Categories

(Firefox :: Settings UI, defect)

39 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: from_bugzilla3, Assigned: maurya1985)

References

Details

(Whiteboard: [good first bug])

Attachments

(2 files, 4 obsolete files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150413140831

Steps to reproduce:

1. Remember that, next time I'm forced to restart Firefox, I'll have to manually select "default" in the profile manager because Aurora became Developer Edition.
2. Open Preferences
3. Uncheck “Allow Firefox Developer Edition and Firefox to run at the same time”
4. Receive dialog asking for restart
5. Click Cancel because I've got something that can't be interrupted going on (eg. 4GiB GOG.com BIN file download).


Actual results:

6. Checkbox resets to checked
7. I get annoyed and decide to do it later
8. I forget
9. Return to step 1 (This has been happening for months now)


Expected results:

It should provide a "Restart now/Restart later/Cancel" choice so I can choose "Restart later", check this off my TODO list, and get on with my day.

(Since I don't want to have to think about this anymore, I asked PlayOnLinux to install the Windows build of the Developer Edition before I started writing this and then let `diff -r` solve the problem. I also set an immutable filesystem attribute on the resulting ~/.mozilla/firefox/ignore-dev-edition-profile for good measure.)
Changed the component to Firefox - General.
Severity: normal → enhancement
Component: Untriaged → General
Severity: enhancement → normal
Status: UNCONFIRMED → NEW
Component: General → Preferences
Ever confirmed: true
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [good first bug]
If this is assigned to nobody, I'd like to take this up.
Assignee: nobody → maurya1985
Status: NEW → ASSIGNED
Attachment #8710294 - Flags: review?(from_bugzilla2)
I have no experience with the Firefox codebase or its standards, so I'm not qualified to review patches.
Attachment #8710294 - Flags: review?(from_bugzilla2) → review?(past)
Attached image Capture.PNG
screenshot of the change
Attachment #8710341 - Flags: review?(past)
Comment on attachment 8710294 [details] [diff] [review]
delayed-restart-option-for-simultaneous-run-setting

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

Thanks for the patch. I have a few nits, but the main issue is that the button labels need to be localizable.

::: browser/components/preferences/in-content/main.js
@@ +200,5 @@
>        if (error) {
>          Cu.reportError("Failed to toggle separate profile mode: " + error);
>        }
>      }
> +	function createOrRemoveSpecialProfileFile(actionOnFulfill) {

This function is misaligned, presumably due to using hard-tabs for indentation. Please use soft-tabs (spaces).

About naming things, I'd rather have something like the following:

function createOrRemoveSpecialDevEditionFile(onSuccess)

@@ +224,3 @@
>  
> +	let prompts = Cc["@mozilla.org/embedcomp/prompt-service;1"]
> +                        .getService(Components.interfaces.nsIPromptService);

It's simpler to just say:

let prompts = Services.prompt;

Indentation is off here, too.

@@ +227,5 @@
> +	let check = {value: false};
> +	let flags = prompts.BUTTON_POS_0 * prompts.BUTTON_TITLE_IS_STRING +
> +            prompts.BUTTON_POS_1 * prompts.BUTTON_TITLE_CANCEL  +
> +            prompts.BUTTON_POS_2 * prompts.BUTTON_TITLE_IS_STRING;
> +    let button_index = Services.prompt.confirmEx(window, title, msg, flags, "Restart Now", "", "Restart Later", null, check)

The "Restart Now"/"Restart Later" strings need to be localizable. You have to put them in preferences.properties and load them using the same method as |msg| above.

@@ +253,5 @@
> +	    // Revert the checkbox in case we didn't quit
> +	    revertCheckbox();
> +	    break;
> +	  case RESTART_LATER_BUTTON_INDEX:
> +	    createOrRemoveSpecialProfileFile(undefined);

No need to pass |undefined| explicitly.
Attachment #8710294 - Flags: review?(past) → review-
Thanks for the review, Panos! I'll update after working on them. I did go through the coding style, but might have missed the indentation bit, so thanks for your patience in going thru them all :)
No worries, we tend to be quite deliberate in any change that goes into Firefox, no matter how small, but your patch is a great contribution nevertheless!
Attachment #8710294 - Attachment is obsolete: true
Comment on attachment 8711585 [details] [diff] [review]
Add Restart Later button for the setting that allows Dev Edition and Firefox to run at the same time

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

Just a couple of nits, but I'd like to make sure Jared is OK with this.

::: browser/components/preferences/in-content/main.js
@@ +227,5 @@
> +                  prompts.BUTTON_POS_1 * prompts.BUTTON_TITLE_CANCEL  +
> +                  prompts.BUTTON_POS_2 * prompts.BUTTON_TITLE_IS_STRING;
> +    let button0Title = bundle.getString("restartNowButton");
> +    let button2Title = bundle.getString("restartLaterButton");
> +    let button_index = Services.prompt.confirmEx(window, title, msg, flags,

Nit: you could reuse |prompts| here.

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties
@@ +168,5 @@
>  featureDisableRequiresRestart=%S must restart to disable this feature.
>  shouldRestartTitle=Restart %S
>  
> +restartNowButton=Restart Now
> +restartLaterButton=Restart Later

You could lose the "Button" part, we don't use that convention in this file.
Attachment #8711585 - Flags: review?(past)
Attachment #8711585 - Flags: review?(jaws)
Attachment #8711585 - Flags: review+
Comment on attachment 8711585 [details] [diff] [review]
Add Restart Later button for the setting that allows Dev Edition and Firefox to run at the same time

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

Untested but looks good to me. r=jaws

::: browser/components/preferences/in-content/main.js
@@ +201,5 @@
>          Cu.reportError("Failed to toggle separate profile mode: " + error);
>        }
>      }
> +    function createOrRemoveSpecialDevEditionFile(onSuccess) {
> +      Cu.import("resource://gre/modules/osfile.jsm");

You should define a lazy module getter at the top of this file.

@@ +228,5 @@
> +                  prompts.BUTTON_POS_2 * prompts.BUTTON_TITLE_IS_STRING;
> +    let button0Title = bundle.getString("restartNowButton");
> +    let button2Title = bundle.getString("restartLaterButton");
> +    let button_index = Services.prompt.confirmEx(window, title, msg, flags,
> +                         button0Title, "", button2Title, null, check)

Pass null in here instead of ""

@@ +236,5 @@
>  
> +    switch (button_index) {
> +      case CANCEL_BUTTON_INDEX:
> +        revertCheckbox();
> +        break;

This `break;` can be a `return;` as the other cases use return and there is no code following this switch statement.
Attachment #8711585 - Flags: review?(jaws) → review+
Instead of a lazyModuleGetter, you can keep the Cu.import in the function but please import it to a local scope instead of the global scope. I asked that it be moved to the top of the file, because as this function is currently written, osfile will be added to the global scope but not before this function is called. Some developer in the future may expect osfile to work in another function because of some specific code path they followed, whereas it wouldn't if they didn't follow that same code path.

So it should either be moved to the top of the file in a lazyModuleGetter or imported to a local scope within the function.
The Cu.import is also being done in the init function:

> #ifdef MOZ_DEV_EDITION
>     Cu.import("resource://gre/modules/osfile.jsm");
>     let uAppData = OS.Constants.Path.userApplicationDataDir;

So, this is importing the OSfile module into the global scope only when the branding is MOZ_DEV_EDITION. It's good to have it in the global scope since the createOrRemoveSpecialDevEditionFile() function will also need access to this module. There'd no alternative code path prior to this import since the current path is still in the init stage.
Attachment #8711585 - Attachment is obsolete: true
Attachment #8713501 - Flags: review?(jaws)
Comment on attachment 8713501 [details] [diff] [review]
delayed-restart-option-for-simultaneous-run-setting

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

Thanks for finding that osfile is getting imported there. Sticking with my response in comment #14, can you move the import you described in comment #15 to be a lazy module getter?
Attachment #8713501 - Flags: review?(jaws) → feedback+
The general case is bug 973014.
There's no point to produce various UI for each type of "this option requires restart" (IMO).
Depends on: 973014
Maurya, if you need help understanding how the lazy module getter works, let us know.
Panos, yes I need help understanding the usage of the lazyModuleGetter.
When I write this at the top of the file: 

XPCOMUtils.defineLazyModuleGetter(this, "OSFile", "resource://gre/modules/osfile.jsm");

how can I refer to the osfile module in a statement?
e.g., in let uAppData = OS.Constants.Path.userApplicationDataDir;

I tried a couple of possibilities like osfile.OS.Constants, osfile.Constants, etc. but without much luck.
With,
XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");

an `OS` variable is defined on `this`. That will let you do `OS.Constants.Path.userApplicationDataDir`.
Loading osfile module using lazyModuleGetter
Attachment #8713501 - Attachment is obsolete: true
Attachment #8720624 - Flags: review?(jaws)
Attachment #8720624 - Flags: review?(jaws) → review+
Jared, anything else before we move it to a commit?
I rebased your patch against fx-team tip and pushed it to our tryserver. I'll mark this as checkin-needed, and somebody will check in this patch soon. Your changes will show up in Nightly builds in a day or two after it gets checked in.

Thanks!
Attachment #8720624 - Attachment is obsolete: true
awesome!
https://hg.mozilla.org/mozilla-central/rev/967a62006b57
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
QA Whiteboard: [good first verify]
Bug 1201352, already marked as a duplicate of this bug (in comment 2 above), is not reflected in the fix from this bug. The target milestone for this bug was 47. I tested FF v47.0 > Edit > Preferences > Privacy > History > Firefox Will, changing the menu from Use Custom Settings For History to Remember History. I had done the same test in v45.0. With both versions, the dialog showed no Restart Later button and the Cancel button undid the menu change.

I tried to replicate the original poster's procedure in both v45.0 and v47.0, but the only Preferences menu item I have has no such option about a Developer Edition. I do not have Aurora. Since the poster reports Aurora as having been renamed, downloading and installing Aurora, if still available (it's no longer listed in the Software app and, for all I know, maybe it never was in openSuse's Software app listings), would be irrelevant. Maybe the poster and the people involved with the patch have a Firefox add-on I don't have.

Should bug 1201352 be reopened to support patching it?
(In reply to Nick Levinson from comment #28)
> Bug 1201352, already marked as a duplicate of this bug (in comment 2 above),
> is not reflected in the fix from this bug. The target milestone for this bug
> was 47. I tested FF v47.0 > Edit > Preferences > Privacy > History > Firefox
> Will, changing the menu from Use Custom Settings For History to Remember
> History. I had done the same test in v45.0. With both versions, the dialog
> showed no Restart Later button and the Cancel button undid the menu change.
> 
> I tried to replicate the original poster's procedure in both v45.0 and
> v47.0, but the only Preferences menu item I have has no such option about a
> Developer Edition. I do not have Aurora. Since the poster reports Aurora as
> having been renamed, downloading and installing Aurora, if still available
> (it's no longer listed in the Software app and, for all I know, maybe it
> never was in openSuse's Software app listings), would be irrelevant. Maybe
> the poster and the people involved with the patch have a Firefox add-on I
> don't have.
> 
> Should bug 1201352 be reopened to support patching it?

Firefox Developer Edition (formerly Aurora channel) generally isn't found in base distro repositories for the same reason Firefox Beta Channel, Chrome Beta Channel, and Chrome Canary aren't. It's technically a pre-beta release channel that's kept usable for day-to-day work by how thorough Firefox's automated testing is.

On Ubuntu, if you want its updates to be handled by the package manager rather than Firefox's own updater,  you have to install a PPA (private package archive, basically an additional package source).

As for the dialogs, the prompt for "General > Startup > Allow Firefox Developer Edition and Firefor to run at the same time" no longer appears for me at all (I assume it'll just work on next restart), but I can confirm that the dialog still appears for "Privacy > History > Firefox Developer Edition Will".

It does appear that bug 1201352 was marked as a duplicate of this in error.
The prompt doesn't show up because of the typo/bug in this patch - we define restartNow string, but look for restartNowButton.  It'll get fixed in bug 1285328.
See Also: → 1285328
You need to log in before you can comment on or make changes to this bug.