852964 - Add prompts for the "quit-application-requested", "offline-requested", and "last-pb-context-exiting" notifications

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

The new JavaScript API for downloads should implement the same prompts that
the nsIDownloadManager observer provides to warn the user if any download will
be interrupted by the operation.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Andres, any updates here?

Comment 2

[Andres Hernandez [:andreshm]]

I was having some Fx compiling issues, but now I'm able to work on this. I'll paste a patch as soon as I have something working.

Comment 3

[Andres Hernandez [:andreshm]]

Attachment: WIP Patch

This is still a WIP patch. The prompts are ready, but I need some guidance on the following things. 

1. How I can get the current active download list? I need the private and public list. I tried using the following code async, without luck.
> Downloads.getPublicDownloadList().then(function(aList) { 
>   let downloads = yield list.getAll();
> }.bind(this));

2. Where is the best place to add the observers? Temporally I used a call at the end of the file, but the observers are added a bit late. 
> (function() { this._addObservers(); }).apply(DownloadIntegration);

3. If the user accepts the confirm dialog, do we need to cancel the downloads in these cases? Or this is something that is handled somewhere else?

Comment 4

[Raymond Lee [:raymondlee]]

(In reply to Andres Hernandez [:andreshm] from comment #3)
> Created attachment 782100 [details] [diff] [review]
> WIP Patch
> 
> This is still a WIP patch. The prompts are ready, but I need some guidance
> on the following things. 
> 
> 1. How I can get the current active download list? I need the private and
> public list. I tried using the following code async, without luck.
> > Downloads.getPublicDownloadList().then(function(aList) { 
> >   let downloads = yield list.getAll();
> > }.bind(this));

Have you tried the below?

Task.spawn(function() {
  let list = yield Downloads.getPublicDownloadList();
  let downloads = yield list.getAll();
    …
})

> 
> 2. Where is the best place to add the observers? Temporally I used a call at
> the end of the file, but the observers are added a bit late. 
> > (function() { this._addObservers(); }).apply(DownloadIntegration);

Lets see what Paolo suggests.

> 
> 3. If the user accepts the confirm dialog, do we need to cancel the
> downloads in these cases? Or this is something that is handled somewhere
> else?

Need feedback from Paolo

Comment 5

[:Paolo Amadini]

The difficulty here is that the observers should provide a synchronous response
before they return, but they cannot have one if they use a task or a promise.

To work around this, I think we should register a view on each DownloadList
object, and use it to keep state variables that indicate if a prompt is needed
at every point in time. The notification observers would just check those
variables synchronously.

I think we should move all the logic and state variables into a separate
DownloadObserver object, inside the DownloadIntegration module.

(In reply to Andres Hernandez [:andreshm] from comment #3)
> 1. How I can get the current active download list? I need the private and
> public list. I tried using the following code async, without luck.
> > Downloads.getPublicDownloadList().then(function(aList) { 
> >   let downloads = yield list.getAll();
> > }.bind(this));

The "yield" operator can only be used with "Task.jsm". My general recommendation
here, though, is to catch and report errors so they're logged at the and of a
promise chain, as shown in the documentation:

https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

This would also show the typo on the undeclared variable. For example:

  Downloads.getPublicDownloadList().then(function(aList) {
    // ...
  }).then(null, Cu.reportError);

The same should be done with Task.spawn, because it returns a promise:

  Task.spawn(function() { ... }).then(null, Cu.reportError);

But, if you return the promise to the caller in turn, you shouldn't catch errors.

> 2. Where is the best place to add the observers? Temporally I used a call at
> the end of the file, but the observers are added a bit late. 
> > (function() { this._addObservers(); }).apply(DownloadIntegration);

This should be done lazily, when the public or private download lists are
accessed for the first time, before DownloadStore reads the list of public
downloads from disk.

> 3. If the user accepts the confirm dialog, do we need to cancel the
> downloads in these cases? Or this is something that is handled somewhere
> else?

This is handled in bug 836443, no need to do this here.

Comment 6

[Andres Hernandez [:andreshm]]

Attachment: WIP Patch 2

I added the suggested changes. But still needs more testing and some minor fixes. 

A couple of questions:
1. What is the best way to activate the new JS downloads API? 
2. We need a xpcshell test for this right?

Comment 7

[:Paolo Amadini]

(In reply to Andres Hernandez [:andreshm] from comment #6)
> 1. What is the best way to activate the new JS downloads API?

The best way to try this is to apply the patches from bug 899107 and its two
dependencies, then switch the "browser.download.useJSTransfer" preference and
restart the browser.

> 2. We need a xpcshell test for this right?

We'll need a test that adds some private and some public downloads, of which some
keep partial data and some not, and check that we show or don't show the prompts
based on the types of download.

For now, however, we can start with reviewing a patch without the xpcshell test,
with manual testing only (that is needed anyway to verify that the prompts are
correct).

Comment 8

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

I wasn't able to test it further, since my local sourcecode of Mozilla is not running properly after latest updates. 
I'm getting this error: 
WARNING: Leaking the RDF Service.: file /.. ../Mozilla/Central/rdf/build/nsRDFModule.cpp, line 165
Assertion failure: !JSRuntime::hasLiveRuntimes() (forgot to destroy a runtime before shutting down), at /.. ../Mozilla/Central/js/src/jsapi.cpp:710

I used a Map object since it helps us to the get the size of the current downloads. Because, I can't increase or decrease a counter on the onDownloadChanged observer, since it can be called several times for the same download. Please let me know if there is a better way.

Comment 9

[:Paolo Amadini]

(In reply to Andres Hernandez [:andreshm] from comment #8)
> I'm getting this error: 
> WARNING: Leaking the RDF Service.: file /..
> ../Mozilla/Central/rdf/build/nsRDFModule.cpp, line 165
> Assertion failure: !JSRuntime::hasLiveRuntimes() (forgot to destroy a
> runtime before shutting down), at /.. ../Mozilla/Central/js/src/jsapi.cpp:710

I don't know what this error means. Pulling the latest revision, doing a rebuild
from scratch ("mach clobber"), or using a new profile might all help.

> I used a Map object since it helps us to the get the size of the current
> downloads. Because, I can't increase or decrease a counter on the
> onDownloadChanged observer, since it can be called several times for the
> same download. Please let me know if there is a better way.

The collection is a good idea to track downloads. Since you don't have any
relevant associated data, this can be implemented using a Set.

Comment 10

[:Paolo Amadini]

Comment on attachment 785539 [details] [diff] [review]
Patch v1

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

::: toolkit/components/jsdownloads/src/Downloads.jsm
@@ +151,1 @@
>              yield DownloadIntegration.loadPersistent(list);

So, I think the code flow here is re-entering getPublicDownloadList. The effect is that this registers a handler on _promisePublicDownloadList, but observers are not really registered until this function returns, after persistent data has been loaded.

We should do something like "yield DownloadIntegration.addListObservers(list, false);", where the second argument is "aIsPrivate".

This way, observers are registered before loading, and also the backreference to the "Downloads.jsm" module in DownloadIntegration can be removed.

Comment 11

[:Paolo Amadini]

Comment on attachment 785539 [details] [diff] [review]
Patch v1

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

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +511,5 @@
> +   */
> +  addViews: function DO_addViews() {
> +    // Register a public download list view.
> +    Downloads.getPublicDownloadList().then(function (aList) {
> +      let publicView = {

Since this view code is duplicate, we may have a single function that takes two arguments, aList and aSet, and registers the view. This can be called by the addListObservers function, with no need to check if the global observers are registered.

The global observers should instead be registered when the first of the two lists is retrieved, and not re-registered the second time.

Comment 12

[Josh Matthews [:jdm]]

(In reply to Paolo Amadini [:paolo] from comment #9)
> (In reply to Andres Hernandez [:andreshm] from comment #8)
> > I'm getting this error: 
> > WARNING: Leaking the RDF Service.: file /..
> > ../Mozilla/Central/rdf/build/nsRDFModule.cpp, line 165
> > Assertion failure: !JSRuntime::hasLiveRuntimes() (forgot to destroy a
> > runtime before shutting down), at /.. ../Mozilla/Central/js/src/jsapi.cpp:710
> 
> I don't know what this error means. Pulling the latest revision, doing a
> rebuild
> from scratch ("mach clobber"), or using a new profile might all help.

This is a symptom of one of the bugs that blocks bug 896124.

Comment 13

[Andres Hernandez [:andreshm]]

Attachment: Patch v2

Applied suggested changes.
The run time error was fixed on latest revision.

Comment 14

[:Paolo Amadini]

Comment on attachment 786164 [details] [diff] [review]
Patch v2

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

Looks good, there are mostly style changes in this iteration and a few implementation notes.

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +483,5 @@
> +
> +  /**
> +   * Register the downloads interruption observers.
> +   * @param aList the public or private downloads list.
> +   * @param aIsPrivate true if the list is private, false otherwise.

nit: use indentation and blank line conventions of other existing code, throughout the file.

Specify that this returns a promise.

@@ +494,5 @@
> +      Services.obs.addObserver(DownloadObserver, "quit-application-requested", true);
> +      Services.obs.addObserver(DownloadObserver, "offline-requested", true);
> +      Services.obs.addObserver(DownloadObserver, "last-pb-context-exiting", true);
> +    }
> +    return promise.resolve();

This is returning undefined. Should be "return Promise.resolve();", no need to call "Promise.defer()".

You may also catch exceptions and "return Promise.reject(ex);" in that case.

@@ +504,5 @@
> +  observersAdded: false,
> +  // Public downlods state.
> +  _publicDownloads: new Set(),
> +  // Private downlods state.
> +  _privateDownloads: new Set(),

_publicInProgressDownloads, _privateInProgressDownloads.

Please add JavaDoc-style comment explaining what are the contents of the Set at any given time, and specify what observersAdded means.

@@ +507,5 @@
> +  // Private downlods state.
> +  _privateDownloads: new Set(),
> +
> +  /**
> +   * Registers a view that updates the downlods state counters for a set.

Please expand this comment a bit, explaining what the view does.

@@ +513,5 @@
> +   * @param aIsPrivate true if the list is private, false otherwise.
> +   */
> +  registerView: function DO_registerView(aList, aIsPrivate) {
> +    let downloadsSet =
> +      (aIsPrivate ? this._privateDownloads : this._publicDownloads);

indentation nit:

let downloadsSet = aIsPrivate ? this._privateDownloads
                              : this._publicDownloads;

@@ +516,5 @@
> +    let downloadsSet =
> +      (aIsPrivate ? this._privateDownloads : this._publicDownloads);
> +    let downloadsView = {
> +      onDownloadAdded: function DO_V_onDownloadAdded(aDownload) {
> +        downloadsSet.add(aDownload);

Downloads may be added in the stopped state, this should check if (!aDownload.stopped).

@@ +538,5 @@
> +   * @param aSubject the subject object.
> +   * @param aTopic the notification name.
> +   * @param aData aditional data.
> +   */
> +  observe: function DO_observe(aSubject, aTopic, aData) {

nit: You may use a "//////" section with the nsIObserver interface name, then you don't need a comment on "observe" since it's an interface method.

@@ +542,5 @@
> +  observe: function DO_observe(aSubject, aTopic, aData) {
> +    let title, messageSingle, messageMultiple, dontCancelButton;
> +    switch (aTopic) {
> +      case "quit-application-requested":
> +        if (this._publicDownloads.size > 0) {

This should include the count of private downloads.

You may also move the aDownloadsCount check inside the helper function.

@@ +555,5 @@
> +          dontCancelButton = "dontQuitButtonMac";
> +#endif
> +          this._confirmCancelDownload(aSubject, this._publicDownloads.size,
> +                                      title, messageSingle, messageMultiple,
> +                                      dontCancelButton);

I think this is clearer if inlined, without helper variables:

#ifndef XP_MACOSX
  this._confirmCancelDownload(...);
#else
  this._confirmCancelDownload(aSubject, downloadsCount,
                              "quitCancelDownloadsAlertTitle",
                              "quitCancelDownloadsAlertMsgMac",
                              "quitCancelDownloadsAlertMsgMacMultiple",
                              "dontQuitButtonMac");
#endif

And so on.

@@ +559,5 @@
> +                                      dontCancelButton);
> +        }
> +        break;
> +      case "offline-requested":
> +        if (this._publicDownloads.size > 0) {

Also this should include private downloads.

@@ +592,5 @@
> +   * @param aMessageSingle the dialog message for single download.
> +   * @param aMessageMultiple the dialog message for multiple downloads.
> +   * @param aDontCancelButton the dialog don't cancel button text.
> +   */
> +  _confirmCancelDownload: function DO_confirmCancelDownload(

nit: _confirmCancelDownloads (plural) seems more correct.

Also, the parameter description should be more precise, for example the strings are actually the IDs in the bundle and not the actual messages (optionally, the arguments may also be renamed to aIdTitle or aStringNameTitle and so on to make this clearer).

@@ +596,5 @@
> +  _confirmCancelDownload: function DO_confirmCancelDownload(
> +    aCancel, aDownloadsCount, aTitle, aMessageSingle, aMessageMultiple, aDontCancelButton) {
> +    // If user has already dismissed the request, then do nothing
> +    if ((aCancel instanceof Ci.nsISupportsPRBool) && aCancel.data)
> +      return;

nit: brace single line blocks

@@ +619,5 @@
> +                                       quitButton, dontQuitButton, null, null, {});
> +    aCancel.data = (rv == 1);
> +  },
> +
> +  QueryInterface: XPCOMUtils.generateQI([

nit: "//////" section for nsISupports (see other modules).

::: toolkit/components/jsdownloads/src/Downloads.jsm
@@ +193,5 @@
>    },
> +
> +  /**
> +   * This promise is resolved with a reference to a DownloadList object that
> +   * represents persistent downloads.  This property is null before the list of

"represents private downloads"

Comment 15

[Andres Hernandez [:andreshm]]

Attachment: Patch v3

Applied suggested style changes.

Comment 16

[:Paolo Amadini]

Comment on attachment 787311 [details] [diff] [review]
Patch v3

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

Thank you for the patience!

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +484,5 @@
> +  /**
> +   * Register the downloads interruption observers.
> +   *
> +   * @param aList the public or private downloads list.
> +   * @param aIsPrivate true if the list is private, false otherwise.

indentation nit:

  @param aList
         The public or private DownloadList object.

Blank line before @return. Same for the other comments.

@@ +511,5 @@
> +   * Set that contains the active publics downloads.
> +   * It's keep updated when a public download is added, removed or change its
> +   * properties.
> +   */
> +  _publicDownloads: new Set(),

Rename to _publicInProgressDownloads, _privateInProgressDownloads

Comment 17

[Andres Hernandez [:andreshm]]

Attachment: Patch v4

Updated comments. 

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=cd0c0c0597af

Comment 18

[:Paolo Amadini]

Try is green, pushed to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/75c9ebb2ae80

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/75c9ebb2ae80