Closed Bug 935094 Opened 11 years ago Closed 11 years ago

[Download Manager] Download notifications

Categories

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

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.3 Sprint 6 - 12/6

People

(Reporter: crdlc, Assigned: crdlc)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files, 1 obsolete file)

* Download has started, complete, failed and stopped
No longer depends on: 937116
No longer depends on: 935070
Description: * Banner shows info related to download events * Utility tray will house the download 'notifications'
Assignee: nobody → crdlc
Status: NEW → ASSIGNED
Summary: [Download Manager] Banner shows info related to download events → [Download Manager] Download notifications
Component: Gaia → Gaia::System
Depends on: 938023
Blocks: 940290
Blocks: 940294
Attached file Patch v1 (deleted) —
I would like to start reviewing the patch for notifications although we don't have the API yet. I will attach a mockup in order to watch a demo in the device how it works
Attachment #8334554 - Flags: review?(francisco.jordano)
Attached file download_manager_mockup.js (obsolete) (deleted) —
This is a mockup to see how it works. You have to add this line in the index.html before download_manager library <script defer src="js/download/download_manager_mockup.js"></script>
Alive, could you tell me who is the best person to review this patch? I added to Francisco Jordano because is one of the Download Manager developers and he has all the context. Maybe you want to include to somebody to do another review... Thanks a lot
Flags: needinfo?(alive)
Some minor comments on github. r+ for the downloads mechanims, we will need to find a system peer to agree with the changes on system. Works perfectly, tried on b2g desktop and the download gets updated, we need to verify the download launcher, but trying it on b2g desktop doesn't help.
Attachment #8334554 - Flags: review?(francisco.jordano) → review+
I think all system app peers could review it. Based on this modifies notification I'll recommend etienne.
Flags: needinfo?(alive)
Attachment #8334554 - Flags: review?(etienne)
Cool, I gonna review your comment in a couple of hours. (In reply to Francisco Jordano [:arcturus] (on PTO till 18/11) from comment #7) > Some minor comments on github. > > r+ for the downloads mechanims, we will need to find a system peer to agree > with the changes on system. > > Works perfectly, tried on b2g desktop and the download gets updated, we need > to verify the download launcher, but trying it on b2g desktop doesn't help. The launcher is out of this patch thought, it is performed by DownloadLauncher in shared. Although we will have to review that for sure
Whiteboard: [systemsfe]
Attachment #8334558 - Attachment is obsolete: true
Comment on attachment 8334554 [details] Patch v1 I had a quick chat with Julien, since the notification work here looks a lot like what he had to build for the app installs I'm forwarding him the review.
Attachment #8334554 - Flags: review?(etienne) → review?(felash)
Hi Julien, how are you in Taipei? I would like to say that I received the r+ from Jordano seven days ago and I am pretty sure that you are busy reviewing sms/mms app and other stuffs but we need this patch for v1.3. Do you think that I could move the review to another person? is it needed? Thanks in advance. Happy working week
Flags: needinfo?(felash)
Hey Cristian, Yeah you're right, I wasn't working Friday, and yesterday and today I took more time writing code than reviewing. Now I've started reviewing and I definitely want to look at your patch either today or tomorrow (probably today, as you still have an afternoon of work ;) ). Thanks for the heads up!
Flags: needinfo?(felash)
Depends on: 942140
Comment on attachment 8334554 [details] Patch v1 (I put r- because there is a r+ on the patch and I don't want that someone accidentally thinks it's ready to go in) Reviewed this yesterday. I'm concerned that we use the Notification API only halfly because it's not ready to do what we want. I'd rather want to take what we did for the app install manager and generify it. Not having a Visual Design doesn't help here. The App Install Manager used a progress bar in its notification, therefore I'll assume we'll have one here too for consistency. I'll raise a mail soon on the WHATWG mailing list so that at one point we can have the necessary capabilities in the standard Notification API. In the mean time I think we should avoid it if we need any feature that it does not provide.
Attachment #8334554 - Flags: review?(felash) → review-
Comment on attachment 8334554 [details] Patch v1 Hi Julien, I addressed all your comments. According to amylee.design@gmail.com the visual design is already implemented. Because he just gave me the correct icons (attached in the patch) and the provided with icons and styles (padding, margin, layout, etc) for download list. About hacking the NotificationScreen, I deleted the stringified object sent in new Notification. It is not used anymore. Right now, the patch use the NotificationScreen lib as usual, I only modified addNotification in order to share common parts with updateNotification. Basically the design of regular notifications is the same than download notifications so I don't see to use fake/ad-hoc notifications when we have the common mechanism thought Could you take a look to the new implementation? Thanks a lot
Attachment #8334554 - Flags: review- → review?
Comment on attachment 8334554 [details] Patch v1 r=me with the questions answered and moving the event binding out of the ondownloadstart event handler. I'd like that someone in Gregor's team hsa a look too because they own this code now. Thanks Cristian!
Attachment #8334554 - Flags: review? → review?(anygregor)
Hi Julien, When I was in the latest meeting about download manager in SFO (Gregor was there) we agreed with a review by ourself (in this case Jordano) and another one by a peer of the app (in this case, you ;)). Then, if we have the r+ yours and Jordano's so I think that when travis will be ok if you are comfortable we could land, right? Thanks
I discussed with Francisco yesterday that I wanted to give Gregor the opportunity to have a look during the night. No comment so let's move this to r+ and land. I'd still like Gregor to look at this once he comes back from PTO.
Attachment #8334554 - Flags: review?(anygregor) → review+
I will address the Gregor's comments, thanks
Thanks Gregor for your comments: FYI: How to disabled downloads from the system: The code is ready to disable the download notifications from the system doing: 1) Remove the downloads permission form the manifest or.. 2) If the API is not available in Gecko nothing happens neither or 3) Backout jeje :) Thanks for the reviews mates
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 Sprint 6 - 12/6
Hi, had to backout/revert this change in https://github.com/mozilla-b2g/gaia/commit/4ad01a3200951315fc8e62904b114e1ea17a86ea because it was breaking Gaia UI Tests on TBPL with https://tbpl.mozilla.org/php/getParsedLog.php?id=31255710&tree=B2g-Inbound and seems it caused a java exception
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I can see the fail @app://system.gaiamobile.org/shared/js/download/download_formatter.js, line 78 and I guess it is because the DownloadAPI is not providing with the startTime field defined in the thought
I was testing in the latest build and I can see: 1) When the device is starting the first time, we receive a weird download notification: No id, no startTime totalBytes 0 currentBytes 0 url about:blank path /mnt/sdcard/downloads/ATpWEich.html Do you what it is? I can check if the download object does not have the identifier, if so return..
Do you have idea about my previous comment? Thanks a lot mate
Flags: needinfo?(fabrice)
More info, when I receive this download, if I check typeof download.startTime === 'undefined' I have this error: [JavaScript Error: "NS_ERROR_UNEXPECTED
I've filled this bug 944682 in order to follow this issue detected with my patch
Depends on: 944682
Flags: needinfo?(fabrice)
Blocks: 944639
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Blocks: 956276
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: