740720 - B2G Updates: Show users a prompt to defer applying a Gecko update

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P2)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

In the current system, updates are applied no matter what the user is doing (calling 911, etc.).  That's bad.

A first improvement to that is letting users defer a pending update.  To do that, we need to add code here

http://mxr.mozilla.org/mozilla-central/source/b2g/components/UpdatePrompt.js#37

that shows users a dialog like

  A Gecko update is available.  Gecko needs to restart to apply the update.  Gecko will restart automatically in 1 minute if you do nothing.

along with two buttons

  [ Wait 10 minutes ]  [ Restart now ]

The state machine is
  state update-available: 
    user clicks "Wait 10 minutes" -> goto wait
    user click "Restart now" -> goto apply-update
    1 minute elapses -> goto apply-update
  state wait:
    10 minutes elapse -> goto update-available
  state apply-update:
    initiate gecko restart, using existing code

Comment 1

[Jim Straus]

Attachment: Changes to gaia

The changes to gaia.  Receives an event from gecko, handles the UI and when finally accepted, posts an event back to gecko.

Comment 2

[Jim Straus]

Attachment: Changes to gecko

Changes to gecko.  Sends event to gaia.  The actual update occurs when gaia sends an event back.

Comment 3

[Jim Straus]

Note, I'm not sure I like this UI.  It seems more intrusive than it needs to be.  I would like to see an update trigger a notification and then an update management app could control doing the actual update.  This way, the user isn't forced to do the update, but can defer it for as long as desired.  I often will wait several days on updating my phone to see what the community thinks of an update before applying it.  But doing this would be another bug.

Comment 4

[Jim Straus]

Also note that if we develop a more general way for gecko to communicate with gaia (as has been discussed), this bug could be changed to make use of that.  Or the code here could be used as the basis for creating such.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is way more code than this bug was spec'd for.  Just popping up a dialog from chrome would have been fine for now.

Also, I see a typo in the second patch that makes me wonder if you tested this.

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 611880 [details] [diff] [review]
Changes to gecko

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

This looks untested : the event you send to content must be a mozChromeEvent, with the payload in the detail field. Look at how this is done for the mozNotifications and mozApps APIs.

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 611877 [details] [diff] [review]
Changes to gaia

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

This is gaia code, and should be submitted as a pull request to gaia.

Comment 8

[Jim Straus]

Attachment: Changes to gecko

I initiated a pull request for the gaia portion.  First time I've done that, so hopefully it is right.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 612531 [details] [diff] [review]
Changes to gecko

>diff --git a/b2g/chrome/content/shell.js b/b2g/chrome/content/shell.js
>-
>+  
>   start: function shell_init() {
>+    if (false) { // ENABLE TO TEST FAILURE CASE
>+      window.setTimeout(function() {
>+        let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
>+        appStartup.quit(appStartup.eForceQuit)
>+      }, 60000);
>+    }
>+

nit: remove this code and also your text editor seems to add some unneeded spaces.

>diff --git a/b2g/components/UpdatePrompt.js b/b2g/components/UpdatePrompt.js
>   showUpdateDownloaded: function UP_showUpdateDownloaded(aUpdate, aBackground) {
>-    // FIXME/bug 737598: we should let the user request that the

The comment seems broken, there is an extra '/' after the FIXME and it seems to miss the end of the comment

>+    let browser = Services.wm.getMostRecentWindow("navigator:browser");
>+    browser.content.addEventListener('updates-now', this.eventHandler);

Listen for the mozContentEvent instead and check for the type of event. Also just use |this| as a second parameter and rename the |eventHandler| method |handleEvent|. This will let use really use |this| in handleEvent. Otherwise |this| point to the callback function and don't do what you expect.

>+    let evt = browser.content.document.createEvent('CustomEvent');
>+    evt.initCustomEvent('mozChromeEvent', true, true,
>+                        {'type': 'updates-request', 'update': aUpdate});

What kind of object is aUpdate? Does it contains any chrome information we don't want to expose to Gaia?

>+    browser.content.dispatchEvent(evt);
>+    // send updates-available message to homescreen, this function becomes the insallupates callback.

nit: what is that?

>+  },
>+
>+  eventHandler: function (e) {
>+    browser.content.removeEventListener('updates-now', this.eventHandler);
>+    installUpdae(e.aUpdate, false);
>+  },

See the commennt above. Also it seems that there is typo in installUpdate...

>+
>+  installUpdate: function UP_showUpdateDownloaded(aUpdate, aBackground) {
>+   // FIXME/bug 737598: we should let the user request that the
>     // update be applied later, e.g. if they're in the middle of a
>     // phone call ;).

The indentation seems strange

> 
>     log("Update downloaded, restarting to apply it");
> 
>     // If not cleanly shut down within 5 seconds, this process will
>     // explode.

Why?

>     this._setSelfDestructTimer(5000);

Comment 10

[Jim Straus]

Attachment: Changes to gecko

Fixed the left over debugging code and comments and spacing.  Fixed the listen type.  An update consists of: type, name, displayVersion, appVersion, platformVersion, prevousAppVersion, buildID, detailsURL, billboardURL, licenseURL, serviceURL, channel, showPrompt, showNeverForVersion, showSurvey, isSecurityUpdate, and a patch.  Except for the patch the rest of it seems possible to use in the UI.  Certainly not dangerous.  The patch is potentially problematic, though anyone can see it by going to the update server.  I'll open another bug to hide it after this one is through.

Comment 11

[[:fabrice] Fabrice Desré]

Comment on attachment 612618 [details] [diff] [review]
Changes to gecko

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

::: b2g/chrome/content/shell.js
@@ +78,5 @@
>      } catch (e) {}
>  
>      return Services.prefs.getCharPref('browser.homescreenURL');
>    },
> +  

nit: don't add whitespace

::: b2g/components/UpdatePrompt.js
@@ +31,5 @@
>    showUpdateAvailable: function UP_showUpdateAvailable(aUpdate) { },
>  
>    showUpdateDownloaded: function UP_showUpdateDownloaded(aUpdate, aBackground) {
> +    let browser = Services.wm.getMostRecentWindow("navigator:browser");
> +    browser.content.addEventListener('mozContentEvent', this.handleEvent);

this should be just |this|

nit: be consistent with quotes usage (double quotes everywhere)

@@ +34,5 @@
> +    let browser = Services.wm.getMostRecentWindow("navigator:browser");
> +    browser.content.addEventListener('mozContentEvent', this.handleEvent);
> +    let evt = browser.content.document.createEvent('CustomEvent');
> +    evt.initCustomEvent('mozChromeEvent', true, true,
> +                        {'type': 'updates-request', 'update': aUpdate});

We need to trim down what we send to the UI here: aUpdate is really fat (see http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsIUpdateService.idl#122)

@@ +40,5 @@
> +  },
> +
> +  handleEvent: function (e) {
> +    if (e.detail.type == 'updates-now') {
> +      browser.content.removeEventListener('updates-now', this.handleEvent);

|browser| is out of scope there...

@@ +45,5 @@
> +      this.installUpdate(e.detail.aUpdate, false);
> +    }
> +  },
> +
> +  installUpdate: function UP_showUpdateDownloaded(aUpdate, aBackground) {

nit: UP_showUpdateDownloaded should be UP_installUpdate

Comment 12

[Jim Straus]

Attachment: Changes to gecko

Hopefully this is cleaned up to your requirements.  Also note the pull request to gaia for that side of things.

Comment 13

[Jim Straus]

Attachment: Changes to gecko

Noticed an error removing a listener.

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 613380 [details] [diff] [review]
Changes to gecko

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

I'm still not convinced that sending the full update object to the content is the right thing to do.
Can you check what native fennec is doing here?

::: b2g/components/UpdatePrompt.js
@@ +43,5 @@
> +    if (e.detail.type == "updates-now") {
> +      this.installUpdate(e.detail.aUpdate, false);
> +    }
> +    let browser = Services.wm.getMostRecentWindow("navigator:browser");
> +    browser.content.removeEventListener("mozContentEvent", this.handleEvent);

this should be just |this| instead of |this.handleEvent|

Comment 15

[Jim Straus]

Attachment: Changes to gecko

Sorry, missed the this.handleEvent.  Fennecc and Firefox just pass around the update object, but the prompt is handled purely in the chrome.  I assume we'll have a more full-featured chrome (we need to not send the permission prompt to the content), so once that is done, this will need to be changed to use the chrome dialog, which makes passing the update object secure.

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 614073 [details] [diff] [review]
Changes to gecko

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

Fennec does not pass the update object around, it just keeps it in scope for a notification observer (see http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/UpdatePrompt.js#97)

Also, this code should be in showUpdateAvailable() if I understand fennec's code and the idl at http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsIUpdateService.idl#488 correctly. This patch displays the notification after the download, but does not prevent it, right?

Comment 17

[Jim Straus]

Attachment: Changes to gecko

Ok, not passing an update object to gaia, though it still has some of the same slots.  I passed information that the Android sends to notification even though our UI currently doesn't use it.

In reading toolkit/mozapps/update/nsIUpdateService.idl, showUpdateAvailable is called when an update is available, but before downloading.  showUpdateDownloaded is used when the update is downloaded and is ready to be installed.  I believe the latter is what we want (since we're about to restart gecko when allowed).

Comment 18

[[:fabrice] Fabrice Desré]

(In reply to Jim Straus from comment #17)

> In reading toolkit/mozapps/update/nsIUpdateService.idl, showUpdateAvailable
> is called when an update is available, but before downloading. 
> showUpdateDownloaded is used when the update is downloaded and is ready to
> be installed.  I believe the latter is what we want (since we're about to
> restart gecko when allowed).

We need more input on what we want to do here. Personally I would be upset if I can't prevent download of the update and can only choose to install or not. 

A safe way is to do the full dance, asking user confirmation on each step.

Comment 19

[Jim Straus]

I agree, but that should be a different bug. I'll open a new one in the morning.

Comment 20

[Jim Straus]

Attachment: Changes to gecko

Small change to the gUpdate

Comment 21

[[:fabrice] Fabrice Desré]

Jim, can you update this patch? I'll have some time to review

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 616762 [details] [diff] [review]
Changes to gecko

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

::: b2g/components/UpdatePrompt.js
@@ +9,5 @@
>  const Ci = Components.interfaces;
>  const Cu = Components.utils;
>  
> +const UPDATE_NOTIFICATION_ICON = "drawable://alert_download_progress";
> +

What is the drawable:// protocol ?
In any case the front-end should be able to customize the icon.

@@ +19,5 @@
>    VERBOSE ?
>    function log_dump(msg) { dump("UpdatePrompt: "+ msg +"\n"); } :
>    function log_noop(msg) { };
>  
> +var gUpdate = null;

I don't understand why you need this variable. It seems like it is used only in the scope of showUpdateDownloaded.

@@ -29,5 @@
>    // nsIUpdatePrompt
> -
> -  // FIXME/bug 737601: we should have users opt-in to downloading
> -  // updates when on a billed pipe.  Initially, opt-in for 3g, but
> -  // that doesn't cover all cases.

Keep the comment.

@@ +52,5 @@
> +                           "name": aUpdate.name}});
> +    browser.content.dispatchEvent(evt);
> +  },
> +
> +  handleEvent: function (e) {

nit: add a name to your anonymous function (UP_handleEvent)

@@ +54,5 @@
> +  },
> +
> +  handleEvent: function (e) {
> +    let browser = Services.wm.getMostRecentWindow("navigator:browser");
> +    browser.content.removeEventListener("mozContentEvent", this.handleEvent);

this.handleEvent -> this

@@ +58,5 @@
> +    browser.content.removeEventListener("mozContentEvent", this.handleEvent);
> +
> +    if (e.detail.type == "updates-now") {
> +      this.installUpdate(gUpdate, false);
> +      gUpdate = null;

Use |aUpdate| instead of gUpdate|.

Also it seems like your code ignore the aBackground parameter since you pass |false| to installUpdate.

Comment 23

[Jim Straus]

Attachment: Changes to gecko

Thanks for the feedback.  Since it looks like bug 741587 will land soon, I changed how the prompt works.  Once the confirm dialog is available, I can build a variant that takes a timeout.  This will make the dialog more secure, since it will live entirely in the chrome.
I know the part dealing with the update works from previous testing.  I'll have to test the actual dialog when that code is done.

Comment 24

[Justin Lebar (not reading bugmail)]

I don't understand how this relates to bug 741587.  That bug is about calling confirm() from inside an <iframe mozbrowser>.  But that's not what you're doing here, right?

Comment 25

[Jim Straus]

Cool.  Looks like it just closed.  In my reading of it (and let me know if I'm wrong), bug 741587 supplies a confirm (prompt, alert) from the chrome, not gaia.  My intent was to use such instead of creating new gaia dialogs for everything.  For this bug specifically, I need something like a confirm but that times out.  In looking at the last patch, it looked like it would be relatively straight forward to create such using the infrastructure you've already created.

Comment 26

[Justin Lebar (not reading bugmail)]

> In my reading of it (and let me know if I'm wrong), bug 741587 supplies a confirm (prompt, alert) 
> from the chrome, not gaia.

Well, one of us is misunderstanding the other.  I'm not sure which yet.

Bug 741587 handles the following situation:

embedder.html:
  <iframe id='iframe' mozbrowser src="foo.html">
  <script>
    document.getElementById('iframe').addEventListener('mozbrowsershowmodalprompt', function(e) {
      console.debug('Got dialog with message ' + e.detail.message);
    });
  </script>

foo.html:
  <script>alert("foo");</script>

Notice that the patch does not supply any UI for the alert.  It only lets the alert bubble up from foo.html to embedder.html.  It's up to the embedder to create the UI.

Comment 27

[JP Rosevear [:jpr]]

Josh, do we have designs for the UX facing parts of updates?

Comment 28

[Josh Carpenter [:jcarpenter]]

(In reply to JP Rosevear [:jpr] from comment #27)
> Josh, do we have designs for the UX facing parts of updates?

Not yet, JP. I've Chris Lee for a written breakdown of our policies. Normally I'd be happy to go and gather the reqs, but we're spread too thin at the moment. Am hoping to get the details next week so I can turn around IxD specs by Friday. Visual will then need to take a pass, but I'm hoping the work there is minor.

Comment 29

[Andreas Gal :gal]

Whats the status here? We need a new reviewer.

Comment 30

[Andreas Gal :gal]

Renom if you think we can't ship a v1 without this.

Comment 31

[Jason Smith [:jsmith]]

Per IRC conversations with a few other folks, I think the best course of action if there is disagreement on whether this blocks or not is to do the following:

- Move the blocking-basecamp flag to ? for re-evaluation
- Indicate a rationale for why you disagree

Comment 32

[Justin Lebar (not reading bugmail)]

Comment on attachment 631511 [details] [diff] [review]
Changes to gecko

(In reply to Andreas Gal :gal from comment #29)
> We need a new reviewer.

Vivien looked at the last version...

Comment 33

[Andrew Overholt [:overholt]]

Josh, should this be a blocker?

Comment 34

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 631511 [details] [diff] [review]
Changes to gecko

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

The patch try to use .properties strings from the platform instead of delegating this tasks to Gaia. Also some of the methods called does not exists, not sure where they are defined ?

::: b2g/components/UpdatePrompt.js
@@ +38,5 @@
> +    let title = updateBundle.GetStringFromName("updatesfound_" + aUpdate.type
> +               + ".title");
> +    let deferButton = updateBundle.GetStringFromName("updatesfound_defer");
> +    let nowButton = updateBundle.GetStringFromName("updatesfound_now");
> +

Dialogs should be displayed by Gaia using the mozChromeEvent/mozContentEvent events.

@@ +42,5 @@
> +
> +    if (title && deferButton && nowButton) {
> +      let browser = Services.wm.getMostRecentWindow("navigator:browser");
> +      let timeout = 60 * 1000;  // 1 minute
> +      // FIXME: Bug 741587  When landed, add in confirmTimeout modal dialog.

Bug 741587 has been fixed already.

@@ +44,5 @@
> +      let browser = Services.wm.getMostRecentWindow("navigator:browser");
> +      let timeout = 60 * 1000;  // 1 minute
> +      // FIXME: Bug 741587  When landed, add in confirmTimeout modal dialog.
> +      // Dialog returns the title for the selected button or the string "_timeout"
> +      let result = browser.confirmTimeout(title, deferButton, nowButton, timeout);

What is browser.confirmTimeout? Why is it not returning a boolean?

@@ +51,5 @@
> +      // force update now, hopefully fixing the strings as well.
> +      result = true;
> +    }
> +    if (result == deferButton ||| result == "_timeout") {
> +      this.installUpdate(aUpdate);

what is |installUpdate|?

@@ +55,5 @@
> +      this.installUpdate(aUpdate);
> +    } else {
> +      browser.setTimeout(function() {
> +        this.installUpdate(aUpdate);
> +      }, 10*60*1000);  // 10 minutes

|this| is not what you think it is here.

::: toolkit/locales/en-US/chrome/mozapps/update/updates.properties
@@ +78,1 @@
>  

Those strings should be part of Gaia.

Comment 35

[Marshall Culpepper [:marshall_law]]

Hey guys, I should have a new patch for this soon (along with a new Gaia PR)

Comment 36

[Josh Carpenter [:jcarpenter]]

> Josh, should this be a blocker? 

We're still establishing Update requirements (see current compilation here: https://wiki.mozilla.org/Gaia/System/Updates) so I cannot judge whether this blocks or not. I will say that forced downloads over constrained / expensive Brazilian bandwidth is totally verboten. At a bare minimum we would need to defer download (whether forced or prompted) until WiFi=true.

Comment 37

[Marshall Culpepper [:marshall_law]]

Attachment: UpdatePrompt - v1

A clean start for UpdatePrompt changes - matching Gaia pull request coming shortly.

Comment 38

[Marshall Culpepper [:marshall_law]]

Note: this implementation follows the original state machine from cjones. If UX requirements change, this patch can be updated.

Comment 39

[Marshall Culpepper [:marshall_law]]

Matching gaia pull request: https://github.com/mozilla-b2g/gaia/pull/3447

Comment 40

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 651902 [details] [diff] [review]
UpdatePrompt - v1

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

::: b2g/components/UpdatePrompt.js
@@ +20,5 @@
>  
> +const PROMPT_TIMEOUT = Services.prefs.getIntPref("b2g.update.prompt-timeout")
> +                       || 30 * 1000; // 30 seconds
> +const WAIT_TIMEOUT = Services.prefs.getIntPref("b2g.update.wait-timeout")
> +                     || 10 * 60 * 1000; // 10 minutes

You don't really need the default value here. Also is 10 minutes enough? It seems a bit annoying...

@@ +69,5 @@
> +      type: "update-prompt",
> +      update: this._update,
> +      waitTimeout: WAIT_TIMEOUT,
> +      promptTimeout: PROMPT_TIMEOUT
> +    });

You can use shell.sendChromeEvent({
  type: "update-prompt",
  ...
});

Ideally sendChromeEvent would take a callback so you don't have to worry about the mozContentEvent logic but that's out of the scope of this bug.

@@ +116,5 @@
>      log("Update downloaded, restarting to apply it");
>  
>      // If not cleanly shut down within 5 seconds, this process will
>      // explode.
> +    this._selfDestructTimer = this.createTimer(5000);

For consistency maybe you want to add a SELFDESTRUCT_TIMEOUT = 5 * 1000; next to the other constants.

@@ +141,5 @@
> +      this.restartProcess();
> +    } else if (timer == this._waitTimer) {
> +      this._waitTimer = null;
> +      this.showUpdatePrompt();
> +    }

switch/case?

Comment 41

[Marshall Culpepper [:marshall_law]]

(In reply to Josh Carpenter [:jcarpenter] from comment #36)
> We're still establishing Update requirements (see current compilation here:
> https://wiki.mozilla.org/Gaia/System/Updates) so I cannot judge whether this
> blocks or not.

I've answered the majority of the open questions for Gecko updates -- is there anything else specific you need?

> I will say that forced downloads over constrained / expensive
> Brazilian bandwidth is totally verboten. At a bare minimum we would need to
> defer download (whether forced or prompted) until WiFi=true.

Agreed -- this is just the first iteration that deals with the best case scenario (i.e. the data won't cost the user anything, but they may be in the middle of doing something)

FYI, update opt-in over a billed connection will be dealt with in Bug 736701

Comment 42

[Marshall Culpepper [:marshall_law]]

(In reply to Marshall Culpepper [:marshall_law] from comment #41)
> FYI, update opt-in over a billed connection will be dealt with in Bug 736701

D'oh! That was supposed to be Bug 737601 :)

Comment 43

[Josh Carpenter [:jcarpenter]]

> I've answered the majority of the open questions for Gecko updates -- is there anything else specific you need?

Thanks Marshall, I have not had a chance to review your feedback yet (was on PTO Thursday eve—Sunday). Am still catching up. Will let you know if there are any questions.

Comment 44

[Marshall Culpepper [:marshall_law]]

(In reply to Vivien Nicolas (:vingtetun) from comment #40)

> You can use shell.sendChromeEvent({

Is |shell| accessible from the UpdatePrompt code?

> @@ +141,5 @@
> > +      this.restartProcess();
> > +    } else if (timer == this._waitTimer) {
> > +      this._waitTimer = null;
> > +      this.showUpdatePrompt();
> > +    }
> 
> switch/case?

Thanks for the suggestion, I actually wasn't aware that case statements could have expressions in Javascript. You learn something new every day :)

Comment 45

[Marshall Culpepper [:marshall_law]]

(In reply to Vivien Nicolas (:vingtetun) from comment #40)
> Also is 10 minutes enough? It seems a bit annoying...

That's probably true -- I was just using the value suggested in cjones' initial comment :) I'm open to making this a longer timeout, if you have a suggestion (or a know of a UX requirement) that would be good.

Comment 46

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Marshall Culpepper [:marshall_law] from comment #44)
> (In reply to Vivien Nicolas (:vingtetun) from comment #40)
> 
> > You can use shell.sendChromeEvent({
>
> Is |shell| accessible from the UpdatePrompt code?
> 

Not directly but the code above access the browser window that contains the 'shell' object. So it will be |browser.shell.sendChromeEvent|.



> > @@ +141,5 @@
> > > +      this.restartProcess();
> > > +    } else if (timer == this._waitTimer) {
> > > +      this._waitTimer = null;
> > > +      this.showUpdatePrompt();
> > > +    }
> > 
> > switch/case?
> 
> Thanks for the suggestion, I actually wasn't aware that case statements
> could have expressions in Javascript. You learn something new every day :)

Well, not expressions like foo === bar but
switch(timer) {
  case this._updateTimer:
    break;

   case this._resumeTimer:
     break;

   ...
}

Comment 47

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Marshall Culpepper [:marshall_law] from comment #45)
> (In reply to Vivien Nicolas (:vingtetun) from comment #40)
> > Also is 10 minutes enough? It seems a bit annoying...
> 
> That's probably true -- I was just using the value suggested in cjones'
> initial comment :) I'm open to making this a longer timeout, if you have a
> suggestion (or a know of a UX requirement) that would be good.

Let's delegate this to Josh Carpenter.

Comment 48

[Josh Carpenter [:jcarpenter]]

Commented here: https://bugzilla.mozilla.org/show_bug.cgi?id=740722#c10

Quoting myself:

I've posted a draft update process proposal to the following:

https://wiki.mozilla.org/Gaia/System/Updates#Draft_process_for_atomic_Gecko.2BGaia_updates

That reflects my current understanding of the technical requirements and constraints, plus some nice-to-haves thrown in for good measure. 

Abridged flow diagram here (minus some details, and the Manual update flows):

https://wiki.mozilla.org/images/4/46/SystemUpdates_Flow1.pdf

WRT to the specific question raised above, re: delays, the process proposes both Silent and Manual Installs, and nuanced behavior behind each. For the Manual Install prompt (received by user once update download is complete, and the device in On and Unlocked), I imagine the simplest options would be "Now" or "Later", with the second deferring the update until the conditions for a Silent Install were in place (details here: https://wiki.mozilla.org/Gaia/System/Updates#6._Install)

That's my first take on it, anyways.

Comment 49

[Marshall Culpepper [:marshall_law]]

Attachment: UpdatePrompt - v2

Renamed/refactored a bit to more easily support other prompts and messages.
Updated to address review comments, added a new property for the self destruct
timeout.

Comment 50

[[:fabrice] Fabrice Desré]

Comment on attachment 657260 [details] [diff] [review]
UpdatePrompt - v2

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

Nothing major, but I'd like to take another look.

::: b2g/app/b2g.js
@@ +442,5 @@
> +// Timeout before the update prompt automatically installs the update
> +pref("b2g.update.apply-prompt-timeout", 60000); // milliseconds
> +// Optional timeout the user can wait before getting another update prompt
> +pref("b2g.update.apply-wait-timeout", 1800000); // milliseconds
> +// Amount of time the updater waits for the proess to exit cleanly before

nit: process

::: b2g/components/UpdatePrompt.js
@@ +43,5 @@
>    // that doesn't cover all cases.
>    checkForUpdates: function UP_checkForUpdates() { },
>    showUpdateAvailable: function UP_showUpdateAvailable(aUpdate) { },
>  
> +  showUpdateDownloaded: function UP_showUpdateDownloaded(update, background) {

All params must be aSomething

@@ +56,5 @@
> +    // a prompt for some reason
> +    this._applyPromptTimer = this.createTimer(APPLY_PROMPT_TIMEOUT);
> +  },
> +
> +  sendUpdateEvent: function UP_sendUpdateEvent(type, update, callback, detail) {

Why do you need the detail parameter? It's not set by the only caller.

@@ +73,5 @@
> +    }
> +
> +    if (!detail) {
> +      detail = {};
> +    }

That can be written as detail = detail || { }

@@ +135,5 @@
> +        // postpone applying an update
> +        this._applyWaitTimer = this.createTimer(APPLY_WAIT_TIMEOUT);
> +        break;
> +      case "restart":
> +      default:

What are the "default" cases that we don't expect?

@@ +151,3 @@
>  
> +    let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"]
> +                     .getService(Ci.nsIAppStartup);

nit: align .getService() with [

Comment 51

[Marshall Culpepper [:marshall_law]]

(In reply to Fabrice Desré [:fabrice] from comment #50)
> ::: b2g/components/UpdatePrompt.js
> @@ +43,5 @@
> >    // that doesn't cover all cases.
> >    checkForUpdates: function UP_checkForUpdates() { },
> >    showUpdateAvailable: function UP_showUpdateAvailable(aUpdate) { },
> >  
> > +  showUpdateDownloaded: function UP_showUpdateDownloaded(update, background) {
> 
> All params must be aSomething

*sigh* none of the JS code I saw in b2g/components, or nsUpdateService.js use aSomething style function parameters, which is why I changed this :) I should've looked harder I guess.

This is pretty annoying because there are obviously high level Mozilla coding standards, and then in some cases you are expected to follow the local standards of a subdirectory or module.

I don't personally care which one we're obeying, it's just never clear which one is "right". I guess ultimately, the reviewer's opinion is all that matters :)

> @@ +56,5 @@
> > +    // a prompt for some reason
> > +    this._applyPromptTimer = this.createTimer(APPLY_PROMPT_TIMEOUT);
> > +  },
> > +
> > +  sendUpdateEvent: function UP_sendUpdateEvent(type, update, callback, detail) {
> 
> Why do you need the detail parameter? It's not set by the only caller.

Looks like I messed up the splitting of this patch, it is used by the update progress patch here:
https://bugzilla.mozilla.org/attachment.cgi?id=657263&action=diff#a/b2g/components/UpdatePrompt.js_sec5

Should I move this part of the patch over to Bug 787383?

Comment 52

[Marshall Culpepper [:marshall_law]]

Attachment: UpdatePrompt - v3

Addressed review comments, removed the "default" case in the prompt result,
since we don't need to handle this.

Comment 53

[Marshall Culpepper [:marshall_law]]

Attachment: UpdatePrompt - v4

Apologies for review spam, I accidentally the debugging log message ;)

Comment 54

[Andrew Overholt [:overholt]]

Needed for dogfooding so blocker.

Comment 55

[[:fabrice] Fabrice Desré]

Comment on attachment 658674 [details] [diff] [review]
UpdatePrompt - v4

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

r=me with nits addressed

::: b2g/app/b2g.js
@@ +437,5 @@
>  #ifdef MOZ_UPDATER
> +// Timeout before the update prompt automatically installs the update
> +pref("b2g.update.apply-prompt-timeout", 60000); // milliseconds
> +// Optional timeout the user can wait before getting another update prompt
> +pref("b2g.update.apply-wait-timeout", 1800000); // milliseconds

That looks reasonnable, but did you check with UX on these values?

::: b2g/components/UpdatePrompt.js
@@ +114,5 @@
> +        self._update = null;
> +      }
> +    }
> +
> +    content.addEventListener("mozContentEvent", UP_handleContentEvent);

that could be written as:
content.addEventListener("mozContentEvent", (function UP_handleContentEvent(aEvent) {
  if (!aEvent.detail) {
    return;
  }
		 
 let detail = aEvent.detail;
 if (detail.type == resultType) {
   aCallback.call(this, detail);
   content.removeEventListener("mozContentEvent", UP_handleContentEvent);
   this._update = null;
  }
}).bind(this));

and get rid of the |let self = this| line

Comment 56

[Marshall Culpepper [:marshall_law]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b140990b46c

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b140990b46c