Move certificate shimming and AddonManager restarts to shared helper module

RESOLVED FIXED in Firefox 51

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: rhelmer, Assigned: kmag)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Currently we have to test some things over in `toolkit/mozapps/extensions/tests` because that's the only place that certificate shimming works.

Things like bug 1279012 (which is mostly about the webextension API) could potentially be done in `toolkit/components/extensions` instead (we'd also need a way to restart AddonManager, incidentally.)
This has been coming up a lot lately. It should probably be done before bug 1294287, since it would make a lot of things simpler.
Assignee: nobody → kmaglione+bmo
Blocks: 1294287
Component: WebExtensions: General → Add-ons Manager
Summary: support certificate shimming in webextension tests → Move certificate shimming and AddonManager restarts to shared helper module
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 5

a year ago
mozreview-review
Comment on attachment 8780883 [details]
Bug 1294811: Move AddonManager test helpers to a shared test module.

https://reviewboard.mozilla.org/r/71452/#review69456

It's difficult to thoroughly review a patch this large that moves so much existing code, but I really like the approach and I think it'll be a huge improvement. Noticed a few things but they were in the existing code, I see you have cleaned things up as you went so included these.

I don't see any problem from visual inspection so r+, but make sure you do a full try run :)

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:101
(Diff revision 3)
> + * @param {string} str
> + *        The string to escape.
> + * @return {string} The escaped string.
> + */
> +function escapeXML(str) {
> +  let replacements = {"&": "&amp;", '"': "&quot;", "<": "&lt;", ">": "&gt;"};

Should we replace for `&apos;` too?

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:130
(Diff revision 3)
> +
> +  return result.join("");
> +}
> +
> +
> +class AddonsList {

Nice to see classes

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:183
(Diff revision 3)
> +    var xpiPath = dir.clone();
> +    xpiPath.append(id + ".xpi");
> +
> +    return this[type].some(file => {
> +      if (!file.exists())
> +        throw new Error("Non-existant path found in extensions.ini: " + file.path)

s/existant/existent/

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:328
(Diff revision 3)
> +   * @param {Promise} promise
> +   *        The promise to wait on.
> +   * @returns {*} The promise's resolution value.
> +   * @throws The promise's rejection value, if it rejects.
> +   */
> +  awaitPromise(promise) {

I really like the `await` nomenclature vs. `loopUntil`

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:776
(Diff revision 3)
> +   */
> +  createTempXPIFile(files) {
> +    var file = this.tempDir.clone();
> +    file.append("foo.xpi");
> +    do {
> +      file.leafName = Math.floor(Math.random() * 1000000) + ".xpi";

I wonder why this doesn't just use a UUID.
Attachment #8780883 - Flags: review?(rhelmer) → review+
(Assignee)

Comment 6

a year ago
mozreview-review-reply
Comment on attachment 8780883 [details]
Bug 1294811: Move AddonManager test helpers to a shared test module.

https://reviewboard.mozilla.org/r/71452/#review69456

Yeah, sorry about that.

I at least tried to keep the layout of `head_addons.js` a similar as possible, so it was obvious what changed there. I thought about `hg cp`ing `head_addons.js` to `AddonTestUtils.jsm` so it would be diff-able, but I figured that as long as we were doing this kind of huge refactor, it would be a good opportunity to untangle some of the years of accumulated cruft. So in the end, I don't think the diff would have been that helpful.

> Should we replace for `&apos;` too?

Yeah, leaving it out seems like a footgun waiting to happen.

> I wonder why this doesn't just use a UUID.

Good idea.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 9

a year ago
mozreview-review
Comment on attachment 8782122 [details]
Bug 1294811: Fix external tests that import head_addons.js.

https://reviewboard.mozilla.org/r/72362/#review70328
Attachment #8782122 - Flags: review?(rhelmer) → review+

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/23923c9bee18
https://hg.mozilla.org/mozilla-central/rev/e9313a026f95
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Duplicate of this bug: 1207735
You need to log in before you can comment on or make changes to this bug.