809126 - Showing download size on the updates downloads prompt

Status: VERIFIED FIXED

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

Description

[Etienne Segonzac (:etienne)]

Par of the spec, not implemented yet.

The user should be able to see approximately how big the downloads will be.
(only approximately because we don't know for hosted apps)

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, we should show a minimum size, and if we don't know the actual size indicate that.

Comment 2

[Jason Smith [:jsmith]]

I'm guessing this is part of the app updates feature. Moving it into the new apps management component.

Comment 3

[Etienne Segonzac (:etienne)]

Attachment: Patch proposal v1

Comment 4

[Julien Wajsberg [:julienw]]

Comment on attachment 679373 [details] [diff] [review]
Patch proposal v1

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

I made lots of small comments but the most important things are :

- the name that gets displayed using innerHTML.
- use AppInstallManager's humanizeSize

::: apps/system/js/updatable.js
@@ +14,4 @@
>    this._mgmt = navigator.mozApps.mgmt;
>    this.target = target;
>  
>    if (target === 'system') {

maybe that's me and my Java background, but I think we can do better than such spaghetti code. Could we have a SystemUpdatable with a different constructor function ?

then it could have a different set of arguments, we don't need to have _system, etc.

::: apps/system/js/update_manager.js
@@ +140,5 @@
> +      if (updatable.target === 'system')
> +        return -1;
> +      if (otherUpdatable.target === 'system')
> +        return 1;
> +

See, you don't even use the ._system property here ;) Is it used somewhere already ?

@@ +141,5 @@
> +        return -1;
> +      if (otherUpdatable.target === 'system')
> +        return 1;
> +
> +      return 0;

Also, maybe you could sort alphabetically when it's not system.

@@ +147,5 @@
> +
> +    this.updatesQueue.forEach(function updatableIterator(updatable) {
> +      var size = updatable.size ? this._humanizeSize(updatable.size) : '';
> +      updateList += '<li>' +
> +                      updatable.name +

what happens if the name has dangerous characters ?

You really should use createTextNode for this one.

@@ +363,5 @@
>      event.initCustomEvent('mozContentEvent', true, true, data);
>      window.dispatchEvent(event);
> +  },
> +
> +  _humanizeSize: function um_humanizeSize(bytes) {

I'm not comfortable with having this here as it is already in AppInstallManager.

I think you can use directly the one in AppInstallManager. We should try to share as much code as possible between these modules.

::: apps/system/locales/system.en-US.properties
@@ +82,5 @@
> +updates[one]=Update
> +updates[two]=Updates
> +updates[few]=Updates
> +updates[many]=Updates
> +updates[other]=Updates

do we really need all these besides one and two ?

sorry, I don't know well how l10n works, but I think this is only necessary when the language actually needs them, which is not the case of english.

::: apps/system/style/update_manager/update_manager.css
@@ +107,5 @@
>  #update-manager-toaster.displayed {
>    -moz-transform: translateY(0);
>  }
> +
> +#updates-download-dialog {

I'm wondering how much of this could be shared between different system dialogs. I kind of dislike IDs in CSS for this reason.

@@ +135,5 @@
> +  list-style: none;
> +  padding-left: 0;
> +}
> +
> +#updates-download-dialog span {

I think you should use a class for this span, like ".download-size", remove the ID, and it could maybe even be reused by another dialog

::: apps/system/style/zindex.css
@@ +113,1 @@
>  #screen > [data-z-index-level="app"] > iframe.appWindow.inlineActivity {

shouldn't we have a specific class for that, like "on-top-dialog" or "modal-dialog" ? Because this seems clunky to me.

(is "clunky" actually an english word ?)

::: apps/system/test/unit/mock_updatable.js
@@ +2,2 @@
>    this.target = aTarget;
> +  this.size = downloadSize;

see my previous comment about Updatable

::: apps/system/test/unit/updatable_test.js
@@ +109,5 @@
> +
> +      test('should return null for hosted apps', function() {
> +        mockApp.updateManifest = null;
> +        subject = new Updatable(mockApp);
> +        assert.equal(null, subject.size);

you can use assert.isNull for that

@@ +115,5 @@
> +
> +      test('should update size on download available', function() {
> +        mockApp.updateManifest = null;
> +        subject = new Updatable(mockApp);
> +        assert.equal(null, subject.size);

same same

::: apps/system/test/unit/update_manager_test.js
@@ +527,5 @@
>          }, tinyTimeout * 2);
>        });
>      });
> +
> +    suite('humanizeSize', function() {

useless if you use AppInstallManagers humanizeSize

@@ +558,4 @@
>          UpdateManager.updatesQueue = [uAppWithDownloadAvailable];
>          UpdateManager.init();
>  
> +        UpdateManager.startAllDownloads({

maybe you should use a real ClickEvent here ?

@@ +633,5 @@
>        });
>  
>        test('should handle confirmation', function() {
> +        var defaultPrevented = false;
> +        UpdateManager.startAllDownloads({

I think you can use a ClickEvent and then use the standard event.defaultPrevented property on it.

Comment 5

[Etienne Segonzac (:etienne)]

Comment on attachment 679373 [details] [diff] [review]
Patch proposal v1

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

The funny thing is System update and App updates shared 3 lines of code :)

::: apps/system/js/updatable.js
@@ +14,4 @@
>    this._mgmt = navigator.mozApps.mgmt;
>    this.target = target;
>  
>    if (target === 'system') {

Yep, I have to bite the bullet.

::: apps/system/locales/system.en-US.properties
@@ +82,5 @@
> +updates[one]=Update
> +updates[two]=Updates
> +updates[few]=Updates
> +updates[many]=Updates
> +updates[other]=Updates

We need to ask Kaze :)

::: apps/system/style/update_manager/update_manager.css
@@ +107,5 @@
>  #update-manager-toaster.displayed {
>    -moz-transform: translateY(0);
>  }
> +
> +#updates-download-dialog {

We are sharing most of the stuff in the confirm.css building block :)
Removing the duplicates, my bad.

@@ +135,5 @@
> +  list-style: none;
> +  padding-left: 0;
> +}
> +
> +#updates-download-dialog span {

We're trying really hard to keep modules independents at all level.

What's shared should be in confirm.css, here we should have only update manager specific code.

Making since this statement is true, but this is why I'll keep the IDs.

::: apps/system/test/unit/update_manager_test.js
@@ +527,5 @@
>          }, tinyTimeout * 2);
>        });
>      });
> +
> +    suite('humanizeSize', function() {

I'm strongly against creating dependencies between the UpdateManager and the AppInstallManager for a simple utility function like this.

That said if you have a smart idea for the naming/scope of a module in charge of this kind of stuff I'll probably strongly approve :)

To be clear imho:
Module that makes sense > Small code duplication > Dependency between modules > having a util.js

@@ +558,4 @@
>          UpdateManager.updatesQueue = [uAppWithDownloadAvailable];
>          UpdateManager.init();
>  
> +        UpdateManager.startAllDownloads({

done

@@ +633,5 @@
>        });
>  
>        test('should handle confirmation', function() {
> +        var defaultPrevented = false;
> +        UpdateManager.startAllDownloads({

And you're right!

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 679373 [details] [diff] [review]
Patch proposal v1

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

::: apps/system/locales/system.en-US.properties
@@ +82,5 @@
> +updates[one]=Update
> +updates[two]=Updates
> +updates[few]=Updates
> +updates[many]=Updates
> +updates[other]=Updates

I did and he said that Mozilla L10n team wants us to do that, even if our L10n library doesn't mandate it.

Comment 7

[Etienne Segonzac (:etienne)]

Attachment: Patch proposal v2

Huuuge followup :)

Comment 8

[Etienne Segonzac (:etienne)]

Attachment: Patch proposal v3

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 679521 [details] [diff] [review]
Patch proposal v3

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

ok for me

::: apps/system/js/update_manager.js
@@ +144,5 @@
> +
> +      if (updatable.name < otherUpdatable.name)
> +        return -1;
> +      if (updatable.name > otherUpdatable.name)
> +        return 1;

this compare function won't be correct with lower/upper case, and also with accentuated characters.

But let's say it will be sufficient enough for now.

@@ +151,5 @@
> +
> +    this.downloadDialogList.innerHTML = '';
> +    this.updatesQueue.forEach(function updatableIterator(updatable) {
> +      var listItem = document.createElement('li');
> +      listItem.textContent = updatable.name;

I would have used a document.createTextNode but this seems safe.

Comment 10

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/pull/6290

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 679521 [details] [diff] [review]
Patch proposal v3

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

Sounds a good enough review :)

Comment 12

[Etienne Segonzac (:etienne)]

Cool

https://github.com/mozilla-b2g/gaia/pull/6290

Comment 14

[cbarker]

Verified Fixed in Unagi Build 20121231070201