As a user, once I confirm an app's installation, if that app requires a download in order to complete installation, I want an indicator in the Status Bar to inform me that the download is in progress, so that I can confirm that the app has started downloading and is still in progress.

This is requirement AppInstall-013 https://docs.google.com/a/tola.me.uk/spreadsheet/ccc?key=0AtuYwL8qXqZmdGQtNHJ6S2NxZXlmYVctRS1xbEh3V0E#gid=73

Remaining required feature work listed at http://j.mp/VWpueM is now blocking+ to ensure visibility and tracking, per drivers' decision.

We're marking this bug with the C1 milestone since it follows the criteria of "unfinished feature work" (see https://etherpad.mozilla.org/b2g-convergence-schedule). If this work is not finished by Nov19, this bug will need an exception and will be called out at the upcoming Exec Review.

working on this

Comment on attachment 680234 [details] [diff] [review] patch + tests * rewritten update manager so that it registers the events on the app only when it really needs it (otherwise, we can get conflicts and event handlers that get overwritten) * added code for adding and removing the icon in the status bar (so it fixes Bug 802611 as well) * uses the code that landed in Bug 809275

Comment on attachment 680234 [details] [diff] [review] patch + tests Review of attachment 680234 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the nits addressed. ::: apps/system/js/app_install_manager.js @@ +89,5 @@ > > + handleApplicationInstall: function ai_handleApplicationInstall(e) { > + var app = e.detail.application; > + > + // JW: they're not the same but we'll manage. so yep, this is the kind of comments that won't fly well :) @@ +94,5 @@ > + var manifest = app.manifest || app.updateManifest; > + > + if (app.installState === 'installed') { > + // nothing more to do here, everything is already done > + // soon we'll still want to show the notification to the user we do ? (want to display a notification?) @@ +124,5 @@ > + app.onprogress = null; > + }, > + > + handleProgress: function ai_handleProgress(app, evt) { > + var manifest = app.manifest || app.updateManifest; looks like dead code :) ::: apps/system/js/updatable.js @@ +22,5 @@ > } > > AppUpdatable.prototype.download = function() { > + // JW: we add these callbacks only know to prevent interfering > + // with other modules (especially the AppInstallManager) initials *and* trailing spaces? ;) ::: apps/system/test/unit/app_install_manager_test.js @@ +275,5 @@ > + e = new CustomEvent('applicationinstall', { detail: {} }); > + }); > + > + > + function finishSetup() { calling it |dispatchEvent()| will probably be more readable. @@ +299,5 @@ > + finishSetup(); > + }); > + > + test('should not show the icon', function() { > + assert.isUndefined(MockStatusBar.wasMethodCalled["incSystemDownloads"]); so happy to know this exists, if only it worked with simple quotes ;) ::: apps/system/test/unit/mock_app.js @@ +1,4 @@ > var idGen = 0; > > +function MockApp(opts) { > + /* default values */ we know that ;) @@ +18,5 @@ > this.mId = idGen++; > this.mDownloadCalled = false; > this.mCancelCalled = false; > + > + /* overwrite with what the dev really wants */ I like it! |// overwrite default values with the one provided in the test| would be a bit clearer for me ::: apps/system/test/unit/mock_statusbar.js @@ +3,5 @@ > > + wasMethodCalled: {}, > + > + methodCalled: function msb_methodCalled(name) { > + this.wasMethodCalled[name] = this.wasMethodCalled[name] ? this.wasMethodCalled[name]++ : 1; looks like it's more than 80 chars :)

Comment on attachment 680234 [details] [diff] [review] patch + tests Review of attachment 680234 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/system/js/app_install_manager.js @@ +94,5 @@ > + var manifest = app.manifest || app.updateManifest; > + > + if (app.installState === 'installed') { > + // nothing more to do here, everything is already done > + // soon we'll still want to show the notification to the user yep, that's Bug 802598 @@ +124,5 @@ > + app.onprogress = null; > + }, > + > + handleProgress: function ai_handleProgress(app, evt) { > + var manifest = app.manifest || app.updateManifest; will be used in Bug 802598, should I remove it now ? ::: apps/system/test/unit/mock_app.js @@ +1,4 @@ > var idGen = 0; > > +function MockApp(opts) { > + /* default values */ should I remove this ? I like it, it's sort of a header for the first part of the constructor, and there is another header (next comment) for the last part of the constructor But if you think that's useless I can remove it.

Comment on attachment 680234 [details] [diff] [review] patch + tests Review of attachment 680234 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the nits addressed. ::: apps/system/js/app_install_manager.js @@ +89,5 @@ > > + handleApplicationInstall: function ai_handleApplicationInstall(e) { > + var app = e.detail.application; > + > + // JW: they're not the same but we'll manage. so yep, this is the kind of comments that won't fly well :) @@ +94,5 @@ > + var manifest = app.manifest || app.updateManifest; > + > + if (app.installState === 'installed') { > + // nothing more to do here, everything is already done > + // soon we'll still want to show the notification to the user we do ? (want to display a notification?) @@ +124,5 @@ > + app.onprogress = null; > + }, > + > + handleProgress: function ai_handleProgress(app, evt) { > + var manifest = app.manifest || app.updateManifest; yep :) ::: apps/system/js/updatable.js @@ +22,5 @@ > } > > AppUpdatable.prototype.download = function() { > + // JW: we add these callbacks only know to prevent interfering > + // with other modules (especially the AppInstallManager) initials *and* trailing spaces? ;) ::: apps/system/test/unit/app_install_manager_test.js @@ +275,5 @@ > + e = new CustomEvent('applicationinstall', { detail: {} }); > + }); > + > + > + function finishSetup() { calling it |dispatchEvent()| will probably be more readable. @@ +299,5 @@ > + finishSetup(); > + }); > + > + test('should not show the icon', function() { > + assert.isUndefined(MockStatusBar.wasMethodCalled["incSystemDownloads"]); so happy to know this exists, if only it worked with simple quotes ;) ::: apps/system/test/unit/mock_app.js @@ +1,4 @@ > var idGen = 0; > > +function MockApp(opts) { > + /* default values */ I can live with that (since it does not have initials ;)) @@ +18,5 @@ > this.mId = idGen++; > this.mDownloadCalled = false; > this.mCancelCalled = false; > + > + /* overwrite with what the dev really wants */ I like it! |// overwrite default values with the one provided in the test| would be a bit clearer for me ::: apps/system/test/unit/mock_statusbar.js @@ +3,5 @@ > > + wasMethodCalled: {}, > + > + methodCalled: function msb_methodCalled(name) { > + this.wasMethodCalled[name] = this.wasMethodCalled[name] ? this.wasMethodCalled[name]++ : 1; looks like it's more than 80 chars :)

Ben, I'd be more comfortable if you reviewed this patch too :-)

github pull request is https://github.com/mozilla-b2g/gaia/pull/6343

here is the download indicator from Peter

new pull request with the changed icon: https://github.com/mozilla-b2g/gaia/pull/6345 Changes from the attached patch is only the new icon checked in, the old icon removed, and the css referencing the new icon. Should I do a new patch for this trivial css change ?

(In reply to Julien Wajsberg [:julienw] [:everlong] from comment #14) > new pull request with the changed icon: > https://github.com/mozilla-b2g/gaia/pull/6345 > > Changes from the attached patch is only the new icon checked in, the old > icon removed, and the css referencing the new icon. Should I do a new patch > for this trivial css change ? You can also just put a pointer to the pull request, since it gets auto-updated.

BTW I'd like to talk about the "one bug - one patch" rule of thumb. Since nobody would show and then hide the download icon in 2 different patches this should have been only 1 bug.

merged https://github.com/mozilla-b2g/gaia/pull/6345

Great patch Julien :D ...now I just have to clean up all my merge conflicts ;) Etienne, the two bugs per patch thing is my fault, I filed a bug for every user story. I think really the problem is that the user stories are far too granular but we should discuss that separately. How come the code review was done in Bugzilla? I thought we were still using GitHub for that. I much prefer it. Btw Julien, Etienne may already have told you this but if you commit to the same branch your pull request is automatically updated, no need to send a new one.

yep, in this specific case, I really wanted that nobody merges the PR before I put this icon in it (since I didn't really know when I'd be able to do it) ;) That's why I closed+opened a new one. For the code review, well, I have no specific preference.

(In reply to Ben Francis [:benfrancis] from comment #18) > How come the code review was done in Bugzilla? I thought we were still using > GitHub for that. I much prefer it. > I checked with Dietrich and it's the developer choice, I'm fine with both personally.

I think there might be some race conditions or similar with the unit tests for this patch, they don't always pass. One way to reproduce this is to move the "hosted app without cache" test to underneath the "packaged app suite". You should see that three of the tests fail. Julien are you able to reproduce this? If not then feel free to re-close this bug.

Hey Ben, Here is a simple patch fixing the test. To use it and unblock things on your end without waiting for the review/merge just |git apply the-patch.patch| Cheers!

Comment on attachment 680981 [details] [diff] [review] Patch fixing the tests Review of attachment 680981 [details] [diff] [review]: ----------------------------------------------------------------- ok for me with this comment addressed. sorry 'bout that :) ::: apps/system/test/unit/mock_statusbar.js @@ +24,4 @@ > this.notificationsCount = null; > this.mNotificationsUpdated = false; > this.mNotificationUnread = false; > + this.wasMethodCalled = {}; Was this really necessary ? the property is already defined/declared above (because I consider it is a "public" property, I like to declare it before)

Comment on attachment 680981 [details] [diff] [review] Patch fixing the tests ok, forget my comment on the diff, that's ok. push it !

Verified with Unagi, build ID 20130103070201