Closed Bug 802600 Opened 12 years ago Closed 12 years ago

[Apps] Cancel App Download from Notification Center

Categories

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

defect

Tracking

(blocking-basecamp:+)

VERIFIED FIXED
B2G C1 (to 19nov)
blocking-basecamp +

People

(Reporter: benfrancis, Assigned: julienw)

References

()

Details

(Keywords: feature, Whiteboard: [LOE:S][label:story])

Attachments

(1 file, 4 obsolete files)

As a user, once I confirm an app's installation, if that app requires a download in order to complete installation, I want to be able to cancel that download once it is in progress, from the Notification Center progress indicator, so that I can control my phone's network activity and defer the full installation until a later point in time.
Notification center functionality is nice, but not required.
blocking-basecamp: ? → -
Priority: -- → P2
Josh and I spent some time discussing the various scenarios, and it's not clear that we can do without notification center part of the spec.

Also, remaining required feature work listed at http://j.mp/VWpueM is now blocking+ to ensure visibility and tracking, per drivers' decision.
blocking-basecamp: - → +
Priority: P2 → --
Priority: -- → P1
QA Contact: jsmith
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.
Target Milestone: --- → B2G C1 (to 19nov)
Keywords: feature
Component: Gaia → Gaia::Apps Management
Component: Gaia::Apps Management → Gaia
Component: Gaia → Gaia::System
Ben,

do you know if this bug should use the same dialog than bug 802583?
No this is the "cancel download" dialog as opposed to the "cancel install" dialog. See page 10 of https://www.dropbox.com/sh/b0kyykhzcfkpm8b/fdA0gFkf5U/Gaia_AppInstall_20121011.pdf

This confused me too at first!
(In reply to Ben Francis [:benfrancis] from comment #6)
> No this is the "cancel download" dialog as opposed to the "cancel install"
> dialog. See page 10 of
> https://www.dropbox.com/sh/b0kyykhzcfkpm8b/fdA0gFkf5U/
> Gaia_AppInstall_20121011.pdf
> 
> This confused me too at first!

Thanks for the quick answer :)
Blocks: 802612
Depends on: 802598
Attached patch patch (obsolete) — Splinter Review
Attached patch patch (obsolete) — Splinter Review
also fixed some gjslint problems in various files.
Attachment #683083 - Attachment is obsolete: true
Attachment #683085 - Flags: review?(etienne)
Comment on attachment 683085 [details] [diff] [review]
patch

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

::: apps/system/index.html
@@ +689,5 @@
> +          </menu>
> +        </section>
> +      </form>
> +
> +      <form id="updates-download-dialog" data-type="confirm" role="dialog" data-z-index-level="updates-download-dialog">

80 cols or not? :)

::: apps/system/js/app_install_manager.js
@@ +275,5 @@
> +    var currentNode = e.target;
> +    while (!currentNode.classList.contains('fake-notification') &&
> +        currentNode !== this.notifContainer) {
> +      currentNode = currentNode.parentNode;
> +    }

definitively need some |pointer-events:none| goodness!

https://developer.mozilla.org/en-US/docs/CSS/pointer-events

::: apps/system/style/app_install_manager/app_install_manager.css
@@ +1,3 @@
>  /* dialog */
> +/* FIXME we should be able to use .app-install-dialog however
> + * confirm.css is conflicting with our style.

isn't
|form[role="dialog"][data-type="confirm"].app-install-dialog| working?

::: apps/system/test/unit/app_install_manager_test.js
@@ +10,3 @@
>  
>  requireApp('system/js/app_install_manager.js');
>  

var mocksForAppInstallManager = [...]

@@ +55,4 @@
>        };
>      };
>  
> +    mocksHelper = new MocksHelper(

new MocksHelper(mocksForAppInstallManager)

@@ +377,5 @@
>        });
>      });
>    });
>  
> +  suite('duringInstall >', function() {

I'm not against the ">" but I'd like to stay consistent at least at the test file level.

@@ +587,5 @@
> +
> +      test('tapping the message should display the dialog', function() {
> +        fakeNotif.querySelector('.fake-notification .message').click();
> +        assert.isTrue(fakeDownloadCancelDialog.classList.contains('visible'));
> +      });

Maybe we should have a |pointer-event:none;| css rule on the message.

@@ +624,5 @@
> +        assert.isFalse(fakeDownloadCancelDialog.classList.contains('visible'));
> +        assert.ok(mockApp.mCancelCalled);
> +      });
> +
> +      test('accepting should hide the dialog but not call cancelDownload if app is uninstalled', function() {

this file needs linting :)

::: apps/system/test/unit/mock_applications.js
@@ +18,5 @@
> +  }
> +
> +  return {
> +    getByManifestURL: getByManifestURL,
> +    registerMockApp: registerMockApp,

how about prefixing the mock-specific methods with "m"?
that's the rationale behind "mTeardown()" btw.

@@ +19,5 @@
> +
> +  return {
> +    getByManifestURL: getByManifestURL,
> +    registerMockApp: registerMockApp,
> +    unregisterMockApp: unregisterMockApp,

same

::: apps/system/test/unit/mock_notification_screen.js
@@ +20,5 @@
>      this.wasMethodCalled = {};
> +    var self = this;
> +    this.mockMethods.forEach(function(methodName) {
> +      self[methodName].wasCalled = false;
> +    });

forEach(function(){}, this)

::: apps/system/test/unit/mocks_helper.js
@@ +4,5 @@
> +};
> +
> +MocksHelper.prototype = {
> +
> +  setup: function mh_setup() {

do we still need this?

@@ +8,5 @@
> +  setup: function mh_setup() {
> +  },
> +
> +  suiteSetup: function mh_suiteSetup() {
> +    var self = this;

not needed anymore

@@ +20,5 @@
> +      window[objName] = window[mockName];
> +    }, this);
> +  },
> +
> +  suiteTeardown: function mj_suiteTeardown() {

mj -> mh

::: apps/system/test/unit/notifications_test.js
@@ +47,4 @@
>      document.body.appendChild(fakeLockScreenContainer);
>      document.body.appendChild(fakeButton);
>  
> +    mocksHelper.setup();

so this does nothing right?
Comment on attachment 683085 [details] [diff] [review]
patch

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

a patch will follow soon

::: apps/system/js/app_install_manager.js
@@ +275,5 @@
> +    var currentNode = e.target;
> +    while (!currentNode.classList.contains('fake-notification') &&
> +        currentNode !== this.notifContainer) {
> +      currentNode = currentNode.parentNode;
> +    }

ah nice, seems better ;)

::: apps/system/style/app_install_manager/app_install_manager.css
@@ +1,3 @@
>  /* dialog */
> +/* FIXME we should be able to use .app-install-dialog however
> + * confirm.css is conflicting with our style.

is that really better ?

that's specificity war and we should be able to do better. Have to see what is really different here from what's in confirm.css.

::: apps/system/test/unit/app_install_manager_test.js
@@ +10,3 @@
>  
>  requireApp('system/js/app_install_manager.js');
>  

done (and also in notifications_test.js)

@@ +55,4 @@
>        };
>      };
>  
> +    mocksHelper = new MocksHelper(

done

@@ +377,5 @@
>        });
>      });
>    });
>  
> +  suite('duringInstall >', function() {

done

@@ +587,5 @@
> +
> +      test('tapping the message should display the dialog', function() {
> +        fakeNotif.querySelector('.fake-notification .message').click();
> +        assert.isTrue(fakeDownloadCancelDialog.classList.contains('visible'));
> +      });

I added the CSS rule and removed this test. Do you have an idea of how we could test that this works ? in integration tests ?

@@ +624,5 @@
> +        assert.isFalse(fakeDownloadCancelDialog.classList.contains('visible'));
> +        assert.ok(mockApp.mCancelCalled);
> +      });
> +
> +      test('accepting should hide the dialog but not call cancelDownload if app is uninstalled', function() {

done

::: apps/system/test/unit/mock_applications.js
@@ +18,5 @@
> +  }
> +
> +  return {
> +    getByManifestURL: getByManifestURL,
> +    registerMockApp: registerMockApp,

yes good idea.

::: apps/system/test/unit/mocks_helper.js
@@ +4,5 @@
> +};
> +
> +MocksHelper.prototype = {
> +
> +  setup: function mh_setup() {

I removed "init" but I think "setup" is nice to have so that tests can use it now and we may painlessly use it later if necessary.

@@ +8,5 @@
> +  setup: function mh_setup() {
> +  },
> +
> +  suiteSetup: function mh_suiteSetup() {
> +    var self = this;

right

@@ +20,5 @@
> +      window[objName] = window[mockName];
> +    }, this);
> +  },
> +
> +  suiteTeardown: function mj_suiteTeardown() {

done

::: apps/system/test/unit/notifications_test.js
@@ +47,4 @@
>      document.body.appendChild(fakeLockScreenContainer);
>      document.body.appendChild(fakeButton);
>  
> +    mocksHelper.setup();

yes.. but it sounds consistent to have a "handler" for each step of the test.
Attached patch patch v2 (obsolete) — Splinter Review
see also the diff patch
Attachment #683085 - Attachment is obsolete: true
Attachment #683085 - Flags: review?(etienne)
Attachment #683167 - Flags: review?(etienne)
Attached patch diff to v1 (obsolete) — Splinter Review
Comment on attachment 683167 [details] [diff] [review]
patch v2

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

Thanks for the follow up!
Attachment #683167 - Flags: review?(etienne) → review+
Attached patch patch v3Splinter Review
slightly modified a comment from an oral discussion with etienne

keeping r=etienne from v2
Attachment #683167 - Attachment is obsolete: true
Attachment #683168 - Attachment is obsolete: true
merged https://github.com/mozilla-b2g/gaia/pull/6506
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [LOE:S][label:story] → [LOE:S][label:story][qa-]
Whiteboard: [LOE:S][label:story][qa-] → [LOE:S][label:story]
Keywords: verifyme
When selecting "stop download", the application loading process stops. Verified as fixed. Using Unagi, build 20130103070201
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: