Closed
Bug 815203
Opened 12 years ago
Closed 12 years ago
[system] handle the app install restart when the homescreen starts it
Categories
(Firefox OS Graveyard :: Gaia::System, defect, P1)
Tracking
(blocking-basecamp:+)
People
(Reporter: julienw, Assigned: julienw)
References
Details
(Keywords: feature)
Attachments
(1 file, 2 obsolete files)
29.42 KB,
patch
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•12 years ago
|
||
BB+ because it depends on Bug 802596 which is BB+ as well.
Blocks: 802596
blocking-basecamp: --- → ?
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → felash
Updated•12 years ago
|
blocking-basecamp: ? → +
Priority: -- → P1
Target Milestone: --- → B2G C2 (20nov-10dec)
Comment 2•12 years ago
|
||
Julien, how long do you estimate it will take for your fix for this?
Assignee | ||
Comment 3•12 years ago
|
||
Today for a first patch, tomorrow or friday for landing.
Assignee | ||
Comment 4•12 years ago
|
||
Needs the patch for Bug 814687 for the reboot handling, but should work otherwise without it.
Attachment #686701 -
Flags: review?(etienne)
Assignee | ||
Comment 5•12 years ago
|
||
Comment on attachment 686701 [details] [diff] [review] patch v1 asking for Francisco's review as well because he works on the homescreen stuff.
Attachment #686701 -
Flags: review?(francisco.jordano)
Assignee | ||
Updated•12 years ago
|
Flags: in-testsuite+
Comment 6•12 years ago
|
||
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!
Attachment #686701 -
Flags: review?(francisco.jordano) → review+
Comment 7•12 years ago
|
||
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
Attachment #686701 -
Flags: review?(etienne) → review-
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•12 years ago
|
||
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•12 years ago
|
||
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...
Assignee | ||
Comment 10•12 years ago
|
||
fixed nearly all that Étienne reported.
Attachment #686701 -
Attachment is obsolete: true
Attachment #687757 -
Flags: review?(etienne)
Comment 11•12 years ago
|
||
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').
Attachment #687757 -
Flags: review?(etienne) → review+
Assignee | ||
Comment 12•12 years ago
|
||
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| ?
Assignee | ||
Comment 13•12 years ago
|
||
latest patch which addresses the comments from Etienne v2 was already r=etienne so landing v3
Attachment #687757 -
Attachment is obsolete: true
Assignee | ||
Comment 14•12 years ago
|
||
commit https://github.com/mozilla-b2g/gaia/commit/73f6968811b8b660e14e46924b9b40704bd9ddab
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 15•11 years ago
|
||
Verified during app install test run on 1/8.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in
before you can comment on or make changes to this bug.
Description
•