Closed Bug 899013 Opened 7 years ago Closed 4 years ago

Interface for customizing the DownloadIntegration module

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
Points:
8

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 2 open bugs)

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(1 file)

The JavaScript API for downloads should provide a way to plug additional
logic in the DownloadIntegration module, for example to change the way the
configured download directories are obtained.

This would make it possible for unit tests to provide a new implementation of
some of the DownloadIntegration functions instead of relying on test code
mixed with production code using the testMode variable.
Depends on: 899571
Blocks: 918188
Blocks: 941009
Whiteboard: p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: p=0 → p=8
Depends on: 1094201
Points: --- → 8
Whiteboard: p=8
There's some magic going on here thanks to AsyncStartup.defineLazyModuleGetter from bug 1094201, which makes it possible to register and unregister overrides dynamically for all the methods defined by the DownloadIntegration module. The first use will be to remove from the production code all the special checks used for isolating the unit tests. For example:

AsyncStartup.downloads.registerIntegration(base => ({

  shouldBlockForReputationCheck: Task.async(function* () {
    if (dontCheckApplicationReputation) {
      return {
        shouldBlock: shouldBlockInTestForApplicationReputation,
        verdict: verdictInTestForApplicationReputation,
      };
    }
    return yield base.shouldBlockForReputationCheck.apply(this, arguments);
  }),

}));

We may even register and unregister integration overrides dynamically so we can apply special logic or simply avoid checking flags like dontCheckApplicationReputation.

Given that most of the work is now done in bug 1094201, I'll use this bug to update the unit tests.
Comment on attachment 8732998 [details]
MozReview Request: Bug 899013 - Interface for customizing the DownloadIntegration module. r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41503/diff/1-2/
Comment on attachment 8732998 [details]
MozReview Request: Bug 899013 - Interface for customizing the DownloadIntegration module. r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41503/diff/2-3/
Comment on attachment 8732998 [details]
MozReview Request: Bug 899013 - Interface for customizing the DownloadIntegration module. r=mak

Work in progress. This already removes some test code from the production code, but I think the most interesting improvements will be seen later, for example we could register an override scoped to a single test so that global things like _deferTestOpenFile can be removed.
Attachment #8732998 - Flags: feedback?(mak77)
This will make the add-ons integration story better, but some add-ons will miss integration points if they currently import DownloadIntegration.jsm directly. Note that anything that uses a lazy module getter will fail to use a dynamically registered integration, so changing the current import code is required anyways.
(In reply to :Paolo Amadini from comment #6)
> This will make the add-ons integration story better, but some add-ons will
> miss integration points if they currently import DownloadIntegration.jsm
> directly. Note that anything that uses a lazy module getter will fail to use
> a dynamically registered integration, so changing the current import code is
> required anyways.

We sold defineLazyModuleGetter for years as the-thing-to-do, it seems unfortunate it's going to break... did you consider expanding the XPCOMUtils method to wrap the new asyncStartup one?
Why not XPCOMUtils.defineLazyModuleGetter(this, "name", "url", optionalIntegration)?
Also, could you please check how many add-ons we are breaking here?
https://reviewboard.mozilla.org/r/41503/#review38711

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm:160
(Diff revision 3)
>  this.DownloadIntegration = {
>    // For testing only
>    _testMode: false,
>    testPromptDownloads: 0,
> -  dontLoadList: false,
>    dontLoadObservers: false,

shouldn't dontLoadObservers be removed?

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm:303
(Diff revision 3)
> -                                    "downloads.rdf"));
> +                                    "downloads.rdf")).catch(() => {});
>  
>        }
> +    } catch (ex) {
> +      Cu.reportError(ex);
> +    }

what's the reason to enlarge this try/catch so much compared to the de-facto situation?
Attachment #8732998 - Flags: feedback?(mak77)
So, it's clearly cleaner and more robust than before.
Though, I'd like to understand the reach (how many we are breaking) of it, and if there isn't anything additional we could do to reduce the brokeness in case it's affecting many consumers.

Mostly, it sounds a bit scary that if I import the module elsewhere in the code without using the right method, I lose all the integration, sounds like something that may cause subtle bugs when multiple add-ons use downloads (one having integration, one not). But maybe it's a so edge case it doesn't matter much in practice.
(In reply to Marco Bonardo [::mak] from comment #9)
> ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm:303
> (Diff revision 3)
> > -                                    "downloads.rdf"));
> > +                                    "downloads.rdf")).catch(() => {});
> >  
> >        }
> > +    } catch (ex) {
> > +      Cu.reportError(ex);
> > +    }
> 
> what's the reason to enlarge this try/catch so much compared to the de-facto
> situation?

It was actually incorrect to begin with, errors during migration shouldn't prevent the DownloadAutoSaveView from being registered. It's probably irrelevant anyways because this code won't run unless you're migrating from a very old profile.
(In reply to Marco Bonardo [::mak] from comment #8)
> Also, could you please check how many add-ons we are breaking here?

I think only four or five. A simple compatibility check pattern of "DownloadIntegration.jsm" would catch those as well as about a dozen false positives. This is why I think there is not a strong need of using stricter compatibility techniques.
Comment on attachment 8732998 [details]
MozReview Request: Bug 899013 - Interface for customizing the DownloadIntegration module. r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41503/diff/3-4/
Attachment #8732998 - Attachment description: MozReview Request: Bug 899013 - Interface for customizing the DownloadIntegration module. → MozReview Request: Bug 899013 - Interface for customizing the DownloadIntegration module. r=mak
Attachment #8732998 - Flags: review?(mak77)
The patch is mostly a refactoring except for a change in how the directories are handled. Previously, we used the real system downloads directory during most tests, and the fake one during the platform integration tests. I think this was meant to be the other way around.
Let's see if the change above causes any issues on other platforms:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9299ec2dc7ec
The failure was obviously because of an accidental moz.build change in the dependent bug. Retry:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b230e4edc519
Blocks: 1261344
Comment on attachment 8732998 [details]
MozReview Request: Bug 899013 - Interface for customizing the DownloadIntegration module. r=mak

https://reviewboard.mozilla.org/r/41503/#review40361

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm:94
(Diff revision 4)
>  XPCOMUtils.defineLazyServiceGetter(this, "volumeService",
>                                     "@mozilla.org/telephony/volume-service;1",
>                                     "nsIVolumeService");
>  
> +// We have to use the gDownloadIntegration identifier because, in this module
> +// only, the DownloadIntegration identifier refers to the base version.

what about a more different name to help avoiding confusion?
downloadIntegrationClient, derivedDownloadIntegration, downloadIntegrationShell or something like that...

::: toolkit/components/jsdownloads/test/unit/common_test_Download.js:134
(Diff revision 4)
>    do_check_true(downloadTarget.exists);
>    do_check_eq(downloadTarget.size, expectedContents.length);
>  });
>  
> +/**
> + * Waits for an attempt to the launch a file, and returns the nsIMIMEInfo used

typo (or maybe I don't knwo this form) "to the launch a file"

::: toolkit/components/jsdownloads/test/unit/common_test_Download.js:156
(Diff revision 4)
> +    Integration.downloads.register(waitFn);
> +  });
> +}
> +
> +/**
> + * Waits for an attempt to the show the directory where a file is located, and

typo (or maybe I don't knwo this form) "to the show the directory"
Attachment #8732998 - Flags: review?(mak77) → review+
can't wait for superPromises to become a reality ;)
Comment on attachment 8732998 [details]
MozReview Request: Bug 899013 - Interface for customizing the DownloadIntegration module. r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41503/diff/4-5/
https://hg.mozilla.org/mozilla-central/rev/ada9dd8caf58
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee: nobody → paolo.mozmail
You need to log in before you can comment on or make changes to this bug.