Closed
Bug 935094
Opened 11 years ago
Closed 11 years ago
[Download Manager] Download notifications
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
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
Assignee | ||
Updated•11 years ago
|
Blocks: fxos-download-mgr
Assignee | ||
Comment 1•11 years ago
|
||
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
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Component: Gaia → Gaia::System
Assignee | ||
Comment 4•11 years ago
|
||
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)
Assignee | ||
Comment 5•11 years ago
|
||
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>
Assignee | ||
Comment 6•11 years ago
|
||
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)
Comment 7•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #8334554 -
Flags: review?(francisco.jordano) → review+
Comment 8•11 years ago
|
||
I think all system app peers could review it. Based on this modifies notification I'll recommend etienne.
Flags: needinfo?(alive)
Assignee | ||
Updated•11 years ago
|
Attachment #8334554 -
Flags: review?(etienne)
Assignee | ||
Comment 9•11 years ago
|
||
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
Updated•11 years ago
|
Whiteboard: [systemsfe]
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #8334558 -
Attachment is obsolete: true
Comment 11•11 years ago
|
||
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)
Assignee | ||
Comment 12•11 years ago
|
||
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)
Comment 13•11 years ago
|
||
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)
Comment 14•11 years ago
|
||
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-
Assignee | ||
Comment 15•11 years ago
|
||
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 16•11 years ago
|
||
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)
Assignee | ||
Comment 17•11 years ago
|
||
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
Comment 18•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #8334554 -
Flags: review?(anygregor) → review+
Assignee | ||
Comment 19•11 years ago
|
||
I will address the Gregor's comments, thanks
Assignee | ||
Comment 20•11 years ago
|
||
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
Assignee | ||
Comment 21•11 years ago
|
||
Merged in master:
https://github.com/mozilla-b2g/gaia/commit/ca5b25713fa9163dc98c2feb4ad8fee035778625
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Target Milestone: --- → 1.3 Sprint 6 - 12/6
Comment 22•11 years ago
|
||
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 → ---
Assignee | ||
Comment 23•11 years ago
|
||
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
Assignee | ||
Comment 24•11 years ago
|
||
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..
Assignee | ||
Comment 25•11 years ago
|
||
Do you have idea about my previous comment? Thanks a lot mate
Flags: needinfo?(fabrice)
Assignee | ||
Comment 26•11 years ago
|
||
More info, when I receive this download, if I check
typeof download.startTime === 'undefined'
I have this error: [JavaScript Error: "NS_ERROR_UNEXPECTED
Assignee | ||
Comment 27•11 years ago
|
||
I've filled this bug 944682 in order to follow this issue detected with my patch
Updated•11 years ago
|
Flags: needinfo?(fabrice)
Assignee | ||
Comment 28•11 years ago
|
||
Merged in master
https://github.com/mozilla-b2g/gaia/commit/3544840f1756fa0c1f7a51f1634939f3e9a5c2e4
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•