384052 - Allow custom parameters in the update URL

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[Wladimir Palant]

The extension manager will replace some parameters in the update URL like %APP_ID% or %APP_OS%. The list of parameters is currently hardcoded, it should be extensible to allow XULRunner-based applications to specify their own parameters. In the case I am currently looking at, TomTom HOME needs to give the server information about the user's TomTom device type - only users with some device types will need an extension update while the others can continue using the old version.

My idea: allow components to implement nsIUpdateParameterSupplier interface and register in the update-parameter-suppliers category. nsIUpdateParameterSupplier will contain only the method getParameterValue(AString parameter). When the extension manager finds an unknown parameter in the update URL it should go through the category and call getParameterValue() on all components until one of them returns a non-null value.

Comment 1

[Benjamin Smedberg]

1) Sounds like a job for an existing interface like nsIPropertyBag2
2) don't get carried away. The smallest patch possible is what we want for this kind of goop.
3) You're going to need to write a unit test to get this in the tree

Comment 2

[Wladimir Palant]

1) Looks already overcomplicated. We don't need variants, it is always strings. And we don't need enumeration. nsIPropertyBag2 will make using this option more complicated especially when parameter values are generated dynamically on request.
2) The smallest patch is exactly what I want to have. Unfortunately, the one I already have does ugly things like unwrapping JavaScript objects, that's why the more complicated approach here. I should attach a patch tomorrow.
3) Sure.

Comment 3

[Ben Bucksch (:BenB)]

Note that the parameter doesn't have to be a URL query value. You could configure
http://update.example.com/myapp/%CUSTOMPARAMS%, then return for CUSTOMPARAMs either "/bla/foo" or "/foo?bla=a&bar=b", so it's quite flexible.

In fact, if you assume that possibility, and if you wanted to make the API really simple, you could just have a function
attribute ACString customParams(); // returns replacement for %CUSTOMPARAM%; already URL-escaped, if necessary.
And the component would only be instantiated and the function called, if the extension manager finds %CUSTOMPARAMS% in the URL.

Comment 4

[Dave Townsend [:mossop]]

Note that I've been doing some work on EM unit testing in bug 382752 and there is even a testcase that tests the variable substitution in update urls there already that you may want to use as a start. Of course none of that has been reviewed yet...

Comment 5

[Wladimir Palant]

Attachment: Proposed patch

Extension manager doesn't like running under xpcshell, using MochiTest for now. The testcase can be converted to xpcshell once bug 382752 lands.

Comment 6

[Wladimir Palant]

Attachment: Proposed patch (corrected)

Previous patch was missing a line break, sorry about bugspam.

Comment 7

[Dave Townsend [:mossop]]

Is there actually a need for these custom parameters to be calculated? Can you not just store them as is in the category manager, i.e. "extension-update-params", "CUSTOM1", "custom_parameter_1"?

If not then does an app really need multiple parameter providers, couldn't you just have a standard contract id that you look for and use that one rather than needing the category manager lookup? I can't imagine a particularly good situation where extensions could plug in their own providers that were used for other addons updates (but then again even the single contract id is easily circumventable).

Presumably you are limiting the parameters to being 3 or more characters long to avoid unnecessary lookups for %-encoded characters?

Can you add some extra %'s (failure cases) into your testcase, it reads to me that any sequence of characters surrounded by %'s not matched will get removed which should not happen.

I agree that the extra interface is unnecessary. As I understand it xpconnect automatically handles translation to/from nsIVariant so I don't see what you save by defining your own interface.

Comment 8

[Ben Bucksch (:BenB)]

> Is there actually a need for these custom parameters to be calculated?

Yes. (For reasons of code design of the consumers.)

> couldn't you just have a standard contract id that you look for and
> use that one rather than needing the category manager lookup?

Yes, we could, we could even get away with one parameter only, see above.
Wladimir's proposal however has a clear advantage when you have 2 independent extensions which both want to use this API (and have separate update URLs).

[url escaping]

Good point, that shouldn't be treated as param.

Comment 9

[Ben Bucksch (:BenB)]

> (For reasons of code design of the consumers.)

To expand: It should be pull, not push. The values passed change a lot, and having to write a pref every time they change means that the code is scattered all over the app. If they get calculated, we have one code point which creates the parameters and pulls the information that it needs, when it needs it, and then it's always uptodate.

Comment 10

[Wladimir Palant]

(In reply to comment #7)
> If not then does an app really need multiple parameter providers, couldn't you
> just have a standard contract id that you look for and use that one rather than
> needing the category manager lookup?

That's a rather messy approach. If the application needs to supply two entirely different sets of parameters, it is logical to separate those into two components. This is especially true if the application has optional parts (extensions).

> Presumably you are limiting the parameters to being 3 or more characters long
> to avoid unnecessary lookups for %-encoded characters?

Exactly, we don't want lookups for %D0%C8.

> Can you add some extra %'s (failure cases) into your testcase, it reads to me
> that any sequence of characters surrounded by %'s not matched will get removed
> which should not happen.

Yes, I already noticed that returning an empty string on failure isn't the best choice. What we really want is:

+      catch(e) {
+        return match;
+      }

> I agree that the extra interface is unnecessary. As I understand it xpconnect
> automatically handles translation to/from nsIVariant so I don't see what you
> save by defining your own interface.

I have a cleaner interface that meets our needs - and doesn't provide useless functionality that makes implementations more difficult. Even if we take nsIPropertyBag instead of nsIPropertyBag2 and the transition to nsIVariant is handled automatically (unless we call from C++ of course), there is still the enumerator. Implementations can decide to throw NS_ERROR_NOT_IMPLEMENTED there but then it should be documented that this property isn't required - messy again. Let's see what Benjamin says...

Comment 11

[Benjamin Smedberg]

Comment on attachment 268092 [details] [diff] [review]
Proposed patch (corrected)

I do prefer existing generic interfaces (nsIPropertyBag2/nsIVariant) to a custom interface. Fix the existing nits, plus those below, and resubmit and ask rob strong for approval.

>Index: toolkit/mozapps/extensions/Makefile.in

This ifdef is unnecessary and please don't indent the variable inside.

>+ifdef MOZ_MOCHITEST
>+	DIRS += test
>+endif

>Index: toolkit/mozapps/extensions/test/Makefile.in


>+# The Initial Developer of the Original Code is
>+# Mozilla.org.

If this is true, you need to indicate where you copied it from. If not, please update the initial developer and copyright date to be correct.

>+DEPTH		= ../../../..
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@
>+VPATH		= @srcdir@
>+relativesrcdir  = toolkit/mozapps/extensions/test

please use spaces instead of tabs for new files


>+_TEST_FILES = 	test_bug384052.html \
>+		$(NULL)

please use separate lines, i.e.:

_TEST_FILES = \
  test_bug384052.html \
  $(NULL)

Comment 12

[Wladimir Palant]

Attachment: Patch v2

All nits fixed with the exception of:

>This ifdef is unnecessary and please don't indent the variable inside.
>
>>+ifdef MOZ_MOCHITEST

All makefiles that I have seen have this ifdef. The documentation (http://developer.mozilla.org/en/docs/Mochitest#Adding_new_Mochitest_tests_to_the_tree) also points to an example with this ifdef. So if this construction became unnecessary for some reason, please fix the documentation first.

Comment 13

[Robert Strong (they/them - no direct email)]

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

Wladimir, could you convert this to an xpcshell test now that bug 382752 has landed? Other than that, I'm fine with this and will + it once the tests have been converted.

Comment 14

[Wladimir Palant]

Attachment: Patch v3

Attaching new patch - now with xpcshell testcase, actual patch unchanged. Had to add one line to extension manager initialization in the framework, extension manager was throwing an exception without it. Other than that pretty straightforward.

Comment 15

[Ryan Flint [:rflint] (ping via IRC for reviews)]

mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in 	1.235 	mozilla/toolkit/mozapps/extensions/test/unit/test_bug384052.js 	1.1 	mozilla/toolkit/mozapps/extensions/test/unit/head_extensionmanager.js 	1.2