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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Capella [:capella]]

Attachment: wesjBug.diff

Wasn't sure it was correct to call it a regression, as add-ons expect things can possible change ;-)

Comment 1

[Wesley Johnston (:wesj)]

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

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8461719 [details] [diff] [review]
wesjBug.diff

flipping back wesj's r+

Comment 4

[Mark Capella [:capella]]

Attachment: 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

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 6

[Aaron Train [:aaronmt]]

Guess this fixes bug 1041562?

Comment 7

[Mark Capella [:capella]]

yes

Comment 9

[Aaron Train [:aaronmt]]

Need the 33+ carried forward from bug 1041562

Comment 10

[Wesley Johnston (:wesj)]

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

Comment 11

[Mark Capella [:capella]]

TRY push ... 
https://tbpl.mozilla.org/?tree=Try&rev=bd75ffd52c8d

Comment 12

[Mark Capella [:capella]]

on into fx-team https://hg.mozilla.org/integration/fx-team/rev/79576fbc8625

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/79576fbc8625

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Looks like this needs an uplift

Comment 15

[Mark Capella [:capella]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3ecca2fb311d

Comment 17

[Cristina Madaras, QA [:CristinaM]]

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