815203 - [system] handle the app install restart when the homescreen starts it

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System, defect, P1)

Description

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

In Bug 802596, we want to restart a failed Webapp installation.

This is the system part of Bug 802596.

- system listens for |progress| events to display the notification in the notification center
- system must keep its listeners on the app object on |downloaderror| event
- system must set listeners at init

Comment 1

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

BB+ because it depends on Bug 802596 which is BB+ as well.

Comment 2

[Dietrich Ayala (:dietrich)]

Julien, how long do you estimate it will take for your fix for this?

Comment 3

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Today for a first patch, tomorrow or friday for landing.

Comment 4

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Attachment: patch v1

Needs the patch for Bug 814687 for the reboot handling, but should work otherwise without it.

Comment 5

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Comment on attachment 686701 [details] [diff] [review]
patch v1

asking for Francisco's review as well because he works on the homescreen stuff.

Comment 6

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 686701 [details] [diff] [review]
patch v1

Looking good to me, tried with the branch that contains the changes for the homescreen and works like a charm.

Impressive job Julien!

Comment 7

[Etienne Segonzac (:etienne)]

Comment on attachment 686701 [details] [diff] [review]
patch v1

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

Biggest issues are:
- the app state duplication
- the readability of the test

But it should be reasonably easy to fix! :)

::: apps/system/js/app_install_manager.js
@@ +30,4 @@
>      }).bind(this));
>  
>      window.addEventListener('applicationinstall',
> +      function ai_handleApplicationInstallEvent(e) {

Won't fight for it, but having this declared on the singleton like handleApplicationReady (next to it) might be a bit more readable.

@@ +176,5 @@
>  
> +  onDownloadStart: function ai_onDownloadStart(app) {
> +    if (! this.isDownloading(app)) {
> +      var appInfo = this.appInfos[app.manifestURL];
> +      appInfo.downloading = true;

If you cannot use the .downloading property on the app itself for this there is probably a bug number that should be mentioned in a comment.

We shouldn't duplicate in gaia states that gecko already has.
If there's a bug let's file it and reference it here.

@@ +193,5 @@
> +  },
> +
> +  isDownloading: function ai_isDownloading(app) {
> +    var appInfo = this.appInfos[app.manifestURL];
> +    return appInfo && appInfo.downloading;

yep, the only place we're using it (the downloading flag) we have an app object handy, confirming my previous comment.

@@ +222,2 @@
>  
> +    if (appInfo.installNotification) {

why not use this.getNotification here?

@@ +289,5 @@
>      }
>      progressNode.textContent = message;
>    },
>  
>    removeNotification: function ai_removeNotification(app) {

same here, you have a nice this.getNotification(app) you know? :)

::: apps/system/test/unit/app_install_manager_test.js
@@ +425,5 @@
> +      });
> +
> +      test('downloadsuccess > should do nothing', function() {
> +        var method1 = 'decExternalNotifications',
> +            method2 = 'decSystemDownloads';

nit, renaming -> notifMethod, statusBarMethod? or maybe splitting in 2 tests.

@@ +426,5 @@
> +
> +      test('downloadsuccess > should do nothing', function() {
> +        var method1 = 'decExternalNotifications',
> +            method2 = 'decSystemDownloads';
> +        mocksHelper.teardown();

Don't feel good about calling teardown (and a global one) in the middle of test...

I think it may be a pointer that we should organize the tests differently.
Or at least we should teardown the specific mock.

@@ +491,5 @@
> +            assert.equal(fakeNotif.childElementCount, 0);
> +            assert.ok(MockNotificationScreen.wasMethodCalled[method]);
> +          });
> +
> +          test('downloadsuccess > should remove only its handlers',

test('should not remove the progress handler') ?

Looks like that's what you're testing.

@@ +556,5 @@
> +                assert.equal(fakeNotif.childElementCount, 0);
> +              });
> +
> +              noProgressSuite();
> +              firstProgressSuite(/*withError*/ false);

If you want to push for the dry-ness (and I guess you do :)) we need to do something to make the test output more readable (by changing the test name based on the |withError| parameter).

Currently we have

first progress
  - progress
  - quantified progress
  - downloadError
    * first progress
      + progress
      + quantified progress

We probably want something like

on first progress
  - indeterminate progress
  - quantified progress
  - on download error
    * on first progress after error
      + indeterminate progress
      + quantified progress

The firstProgressSuite() also needs to be rename, it does *way* more than that :)
And maybe renaming the |withError| parameter to |afterError|? (would have made it easier to understand for me at least).

@@ +589,2 @@
>  
> +      function firstProgressSuite(withError) {

the comments from the first firstProgressSuite() function apply here too obviously.

@@ +589,4 @@
>  
> +      function firstProgressSuite(withError) {
> +        suite('first progress >', function() {
> +          var newprogress = 5;

tiny nit: newProgress

@@ +593,3 @@
>  
> +          setup(function() {
> +            mocksHelper.teardown();

this feels wrong too.
setup calling teadown... Maybe it's time to use the setup() function on mocksHelper?

@@ +604,5 @@
>  
> +          test('notification should have a message', function() {
> +            var expectedText = 'downloadingAppMessage{"appName":"' +
> +              mockAppName + '"}';
> +          assert.equal(fakeNotif.querySelector('.message').textContent,

nit: indentation issue

Comment 8

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Comment on attachment 686701 [details] [diff] [review]
patch v1

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

::: apps/system/js/app_install_manager.js
@@ +30,4 @@
>      }).bind(this));
>  
>      window.addEventListener('applicationinstall',
> +      function ai_handleApplicationInstallEvent(e) {

We don't need a singleton because there is no need to reference it after.

I'll create a method in the AppInstallManager object though.

@@ +176,5 @@
>  
> +  onDownloadStart: function ai_onDownloadStart(app) {
> +    if (! this.isDownloading(app)) {
> +      var appInfo = this.appInfos[app.manifestURL];
> +      appInfo.downloading = true;

Well, the app.downloading property and our local downloading property are not exactly similar.

I'm using a local downloading property to know when I actually begin to download, because we only get a progress event which isn't different from other progress event.

I can do the same with the local installNotification property but it didn't seem so readable to me (and I dislike having a variable used for something other than it was meant to), that's why I created a local downloading property as well.

What do you think ?

@@ +222,2 @@
>  
> +    if (appInfo.installNotification) {

because I'm setting appInfo.installNotification later so I have to know how to access this property anyway.

I'm in the "notification-handling-subsystem" so I have the right to access my internal structures ;)

@@ +289,5 @@
>      }
>      progressNode.textContent = message;
>    },
>  
>    removeNotification: function ai_removeNotification(app) {

and same here :)

Comment 9

[Etienne Segonzac (:etienne)]

Comment on attachment 686701 [details] [diff] [review]
patch v1

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

::: apps/system/js/app_install_manager.js
@@ +30,4 @@
>      }).bind(this));
>  
>      window.addEventListener('applicationinstall',
> +      function ai_handleApplicationInstallEvent(e) {

Probably an understanding issue but that's exactly what I'm asking :)

@@ +176,5 @@
>  
> +  onDownloadStart: function ai_onDownloadStart(app) {
> +    if (! this.isDownloading(app)) {
> +      var appInfo = this.appInfos[app.manifestURL];
> +      appInfo.downloading = true;

Let's talk about it, we at least need to change the name. We can't have appInfo.downloading being _almost_ like app.downloading.

@@ +289,5 @@
>      }
>      progressNode.textContent = message;
>    },
>  
>    removeNotification: function ai_removeNotification(app) {

Well then why have the getNotificaiton function at all for 1 call to it...

Comment 10

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Attachment: patch v2

fixed nearly all that Étienne reported.

Comment 11

[Etienne Segonzac (:etienne)]

Comment on attachment 687757 [details] [diff] [review]
patch v2

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

I won't block because it's a tiny comment, but I'd *really* like for it to be addressed :)

::: apps/system/test/unit/app_install_manager_test.js
@@ +414,4 @@
>  
>      });
>  
> +    function noProgressSuite() {

I think it would be a huge readability win (for the test), if this suite started a suite block with something like suite('when no progress event is received').

Comment 12

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Comment on attachment 687757 [details] [diff] [review]
patch v2

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

::: apps/system/test/unit/app_install_manager_test.js
@@ +414,4 @@
>  
>      });
>  
> +    function noProgressSuite() {

I actually asked myself about this and I changed back and forth between these two possibilities.

Then I decided it would make more sense to not have a real suite (because each time we use this, we have other tests and it would be strange in the output).

Maybe rename |noProgressSuite| to |noProgressCommonTests| ?

Comment 13

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Attachment: patch v3

latest patch which addresses the comments from Etienne
v2 was already r=etienne so landing v3

Comment 14

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

commit https://github.com/mozilla-b2g/gaia/commit/73f6968811b8b660e14e46924b9b40704bd9ddab

Comment 15

[Jason Smith [:jsmith]]

Verified during app install test run on 1/8.