Closed Bug 1043539 Opened 6 years ago Closed 6 years ago

Tweak Browser:Quit to maintain existing support for add-ons

Categories

(Firefox for Android :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 34
Tracking Status
firefox33 --- verified
firefox34 --- verified
fennec 33+ ---

People

(Reporter: capella, Assigned: capella)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch wesjBug.diff (obsolete) — Splinter Review
Wasn't sure it was correct to call it a regression, as add-ons expect things can possible change ;-)
Attachment #8461719 - Flags: review?(wjohnston)
Comment on attachment 8461719 [details] [diff] [review]
wesjBug.diff

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

+ but put back the aItems check unless you need to get rid of it.

::: mobile/android/chrome/content/browser.js
@@ -1346,5 @@
>    },
>  
>    sanitize: function (aItems, callback) {
> -    if (!aItems) {
> -      return;

Why'd you remove this?

@@ +1571,5 @@
>          break;
>  
>        case "Browser:Quit":
>          Services.console.logStringMessage(aData);
> +        this.quit(aData ? JSON.parse(aData) : "");

I'd use null instead of "";
Attachment #8461719 - Flags: review?(wjohnston) → review+
Comment on attachment 8461719 [details] [diff] [review]
wesjBug.diff

Just a drive-by

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>   sanitize: function (aItems, callback) {
>-    if (!aItems) {
>-      return;
>-    }
>-

Why this? Wouldn't it be good to keep the shortcut?

>       case "Browser:Quit":
>         Services.console.logStringMessage(aData);
>-        this.quit(JSON.parse(aData));
>+        this.quit(aData ? JSON.parse(aData) : "");
>         break;

Can you remove the | Services.console.logStringMessage(aData); | while you are here?

I wonder if this is safe enough? Maybe:

>       case "Browser:Quit": {
>         let sanitize = {};
>         try {
>           sanitize = JSON.parse(aData);
>         } catch (e) {}
>         this.quit(sanitize);
>         break;
>       }


If you want to keep the easier way, at least change the "" to {}:
>+        this.quit(aData ? JSON.parse(aData) : {});

We should try to keep the types equivalent and JSON.parse returns an object. Also quit(aClear) defaults aClear to an object.
Attachment #8461719 - Flags: review+ → review?(wjohnston)
Comment on attachment 8461719 [details] [diff] [review]
wesjBug.diff

flipping back wesj's r+
Attachment #8461719 - Flags: review?(wjohnston) → review+
Attached patch wesjBug.diffSplinter Review
I corrected both nits ... and am still removing if (!aItems) {return); so that the intermediate quit() method hits the callback http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=b5dbedb3dead&mark=1137#1115 ... and ping wesj for review? just to be sure vs. carryover
Assignee: nobody → markcapella
Attachment #8461719 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8461739 - Flags: review?(wjohnston)
(In reply to Mark Capella [:capella] from comment #4)
> Created attachment 8461739 [details] [diff] [review]
> wesjBug.diff
> 
> I corrected both nits ... and am still removing if (!aItems) {return); so
> that the intermediate quit() method hits the callback
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js?rev=b5dbedb3dead&mark=1137#1115 ... and ping wesj for review?
> just to be sure vs. carryover

Good catch
Guess this fixes bug 1041562?
yes
Duplicate of this bug: 1041562
Need the 33+ carried forward from bug 1041562
Comment on attachment 8461739 [details] [diff] [review]
wesjBug.diff

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

I knew you must have a reason for that! Thanks :)
Attachment #8461739 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/79576fbc8625
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Looks like this needs an uplift
tracking-fennec: --- → 33+
Comment on attachment 8461739 [details] [diff] [review]
wesjBug.diff

Approval Request Comment: Fix a small regression

[Feature/regressing bug #]: bug1001309 - https://hg.mozilla.org/integration/fx-team/rev/00b81ba0835a

[User impact if declined]: Existing Add-ons the add a "Quit" option to the main menu wll be broken ... "QuitNow, CleanQuit"
[Describe test coverage new/current, TBPL]: UI tested locally and again after release to m-c, haven't noticed independent Q/A verification yet
[Risks and why]: Minor but annoying ... simple section of the code-base, lo-risk as we'll be baking in m-a for a bit
[String/UUID change made/needed]: None
Attachment #8461739 - Flags: approval-mozilla-aurora?
Attachment #8461739 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed using the steps from Bug 1041562 with both add-ons, QuitNow and CleanQuit.
Builds:
Aurora 33.0a2 (2014-07-29)
Nightly 34.0a1 (2014-07-29)

Device: Asus Transformer Pad TF300T Android 4.2.1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.