Closed Bug 777989 Opened 8 years ago Closed 8 years ago

Move add-on related helper functions out add-ons engine

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: gps, Assigned: gps)

Details

Attachments

(3 files)

The add-ons Sync engine contains a bunch of helper functions which are essentially convenient wrappers bridging AddonsRepository, AddonsManager, etc.

As part of the repository refactor, these functions should live outside the engine/repository. As a first step, I plan to move these into a standalone "addonutils.js" or similar.

If there is any interest from the add-ons team, we could even uplift these to Toolkit. Unless Blair or Dave say anything, I'll assume they don't want us to create more work for them and will just keep the refactoring limited to services/sync/. If you would like these functions, chime in and maybe we'll only do this refactor once!
Moved some core utility functions out of addons.js (engine file) and into new addonutils.js. I moved relevant tests from test_addons_store.js to a new test file as well. Not all functions are directly tested. If we uplift this to Toolkit, we obviously want more direct test coverage.

The only major refactor was to change the handling of requiring "secure" URIs. Before, the code just looked directly at prefs. Now, it is a proper flag and thus it is no longer tightly coupled with Sync. The add-ons engine simply sets a flag and this is passed through everything.

I also fixed some minor style nits.

xpcshell tests pass locally. I haven't tried TPS yet.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #646438 - Flags: review?(rnewman)
(In reply to Gregory Szorc [:gps] from comment #0)
> If there is any interest from the add-ons team, we could even uplift these
> to Toolkit. Unless Blair or Dave say anything, I'll assume they don't want
> us to create more work for them and will just keep the refactoring limited
> to services/sync/. If you would like these functions, chime in and maybe
> we'll only do this refactor once!

I do intend on refactoring AddonRepository sometime in the future, but that needs to be part of a bigger partial rewrite to fix some fundamental design issues. So lets keep this bug constrained in scope :)
Comment on attachment 646438 [details] [diff] [review]
Move functions out of engine, v1

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

::: services/sync/modules/addonutils.js
@@ +359,5 @@
> +      },
> +    });
> +  },
> +
> +  updateUserDisabled: function updateUserDisabled(addon, value, cb) {

New method: Javadoc, please.

::: services/sync/tests/unit/test_addon_utils.js
@@ +15,5 @@
> +
> +loadAddonTestFunctions();
> +startupManager();
> +
> +function createAndStartHTTPServer(port) {

Default arg here?

@@ +131,5 @@
> +  let installCalled = false;
> +  AddonUtils.__proto__.installAddonFromSearchResult =
> +    function testInstallAddon(addon, metadata, cb) {
> +
> +    do_check_eq("http://127.0.0.1:" + HTTP_PORT + "/require.xpi?src=sync",

N.B., you use localhost on line 14. Should these agree?

And perhaps const-lift `"http://127.0.0.1:" + HTTP_PORT`?
Attachment #646438 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/services/services-central/rev/cc797e43345c
Whiteboard: [fixed in services]
Target Milestone: --- → mozilla17
This will require simple add-on sync sanity testing for QA. TPS should be sufficient. But, a little extra can't hurt.
Switch to new APIs in TPS. I have *not* tested this because I'm having trouble with TPS on my machine :/
Attachment #647706 - Flags: review?(rnewman)
Comment on attachment 647706 [details] [diff] [review]
Use new APIs in TPS, v1

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

This looks fine to me. Land it and see if it works.
Attachment #647706 - Flags: review?(rnewman) → review+
This fixes the TPS tests. Verified locally. I also added the ability to control the AddonUtils logger via a pref, just like everything else in Sync.
Attachment #648020 - Flags: review?(rnewman)
Attachment #648020 - Flags: review?(rnewman) → review+
Add-on Sync continues to work with this fix in s-c build of 20120802
Whiteboard: [fixed in services] → [verified in services]
https://hg.mozilla.org/mozilla-central/rev/cc797e43345c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [verified in services]
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.