1020495 - Add registration of requestAutocomplete UI handlers on initialization

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[:Paolo Amadini]

We should allow different platforms to specify which object they want to use for the requestAutocomplete UI, before the system is initialized.

This should be done using a direct registration mechanism, calling a common JavaScript module rather than XPCOM, to get better debugger integration and stack traces.

The registration request may happen either on startup or lazily, for example through the observer service, a known XPCOM service, or the category system, whichever is simpler.

The same system may later be used for the platform-specific storage module, and to define platform-specific behaviors.

Comment 1

[:Paolo Amadini]

Also, tests should be able to override the default UI module to mock responses.

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Implement stub UI glue

Paolo is working on this, but I figured I'd just dump the UI stub that I'm currently using. Bug 1020602, bug 1020616, and bug 1020618 will be based on this patch for now, but they can of course be updated when this changes.

Comment 3

[:Paolo Amadini]

(In reply to Brian Nicholson (:bnicholson) from comment #2)
> Paolo is working on this, but I figured I'd just dump the UI stub that I'm
> currently using. Bug 1020602, bug 1020616, and bug 1020618 will be based on
> this patch for now, but they can of course be updated when this changes.

Thanks!

Comment 4

[:Paolo Amadini]

Attachment: Work in progress

OK, here is a first version of the registration based on the Downloads.jsm "integration" model. I placed the registration methods in "FormAutofill.jsm", that works as a global service that lives in the parent process.

For now I added a factory method that is specific to the requestAutocomplete UI. Consumers can register integration objects with function they're interested in overriding:

FormAutofill.registerIntegration({
  createRequestAutocompleteUI: Task.async(function* ()
  {
    // The overridden factory function is still accessible, if needed.
    let baseObject =
      yield this.prototype.createRequestAutocompleteUI.apply(this, arguments);
    return baseObject;
  }),

  // ...other overridable functions may be declared...
});

Integration objects don't need to be unregistered, but it can be done, mainly in case restartless add-ons want to provide an alternate UI.

The patch needs more code documentation, and we should decide when to register the integration on Android startup, but this is ready for a round of feedback.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8436961 [details] [diff] [review]
Work in progress

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

::: toolkit/components/formautofill/FormAutofill.jsm
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=2 et sw=2 tw=80 filetype=javascript: */

Nit: Nuke modelines in JS

@@ +66,5 @@
> +
> +  /**
> +   * Adds the specified override to the list of integration objects.
> +   */
> +  registerIntegration: function (aIntegration)

Nit: remove the spaces between |function| and |(|

@@ +94,5 @@
> +    delete this.integration;
> +
> +    let base = FormAutofillIntegration;
> +    for (let integration of this._integrations) {
> +      let derived = Heritage.extend(base, integration);

I'm not familiar with Heritage. How is it better than using Prototype inheritance in this case?

::: toolkit/components/formautofill/FormAutofillIntegration.jsm
@@ +30,5 @@
> +////////////////////////////////////////////////////////////////////////////////
> +//// FormAutofillIntegration
> +
> +/**
> + * Provides functions to integrate with the host application.

Do you expect many other functions to be added here? What did you have in mind?

::: toolkit/components/formautofill/test/xpcshell/test_integration.js
@@ +18,5 @@
> +add_task(function* test_integration_override()
> +{
> +  let newIntegration = {
> +    createRequestAutocompleteUI: Task.async(function* (aProperties)
> +    {

Nit: brace on same line as "function" throughout

@@ +19,5 @@
> +{
> +  let newIntegration = {
> +    createRequestAutocompleteUI: Task.async(function* (aProperties)
> +    {
> +      yield this.prototype.createRequestAutocompleteUI.apply(this, arguments);

Could we flip a boolean here so we can check after createRequestAutocompleteUI is called below that it was actually this method called or is that too obviously going to pass? I'm thinking of a case where we have a default integration registered (e.g. for Firefox more likely in a b-c test) and we want to make sure that the proper integration is used. Perhaps I'm missing something and this is already ensuring that.

Comment 6

[:Paolo Amadini]

Attachment: Updated work in progress

(In reply to Matthew N. [:MattN] from comment #5)
> @@ +94,5 @@
> > +    delete this.integration;
> > +
> > +    let base = FormAutofillIntegration;
> > +    for (let integration of this._integrations) {
> > +      let derived = Heritage.extend(base, integration);
> 
> I'm not familiar with Heritage. How is it better than using Prototype
> inheritance in this case?

I believe it achieves a similar effect to prototype inheritance without the object needing to be constructable with "new". Now that I look at it closer, it is actually a very simple implementation:

http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/core/heritage.js#26

I've removed the references to the Add-on SDK and reimplemented the function in place.

> ::: toolkit/components/formautofill/FormAutofillIntegration.jsm
> @@ +30,5 @@
> > +////////////////////////////////////////////////////////////////////////////////
> > +//// FormAutofillIntegration
> > +
> > +/**
> > + * Provides functions to integrate with the host application.
> 
> Do you expect many other functions to be added here? What did you have in
> mind?

I can think of the platform-specific storage module integration.

> Could we flip a boolean here so we can check after
> createRequestAutocompleteUI is called below that it was actually this method
> called or is that too obviously going to pass

Good catch, that actually makes the test much more useful :-)

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8438434 [details] [diff] [review]
Updated work in progress

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

Sorry for being so nit-picky but it seems like we're trying to lay a good foundation so we may as well sort some of the style things out before we right a lot more code.

::: toolkit/components/formautofill/FormAutofill.jsm
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * Main entry point to get references to the back-end objects.

I'm not sure this is right since my understanding is this is running in the parent process whereas the backend stuff is in the child. It seems to be more about the UI

@@ +9,5 @@
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = [
> +  "FormAutofill",
> +];

Nit: this could probably be one line for now but no big deal

@@ +12,5 @@
> +  "FormAutofill",
> +];
> +
> +////////////////////////////////////////////////////////////////////////////////
> +//// Globals

I don't think this is necessary since it's normal for globals to be at the top of the file. The only non-global is "use strict"; but that's fine.

@@ +50,5 @@
> +
> +  /**
> +   * Adds the specified override to the list of integration objects.
> +   */
> +  registerIntegration: function (aIntegration) {

Nit: there is a space after function throughout this file. Now that we don't need to name the functions for debugging, I don't think the space is necessary.

::: toolkit/components/formautofill/FormAutofillIntegration.jsm
@@ +9,5 @@
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = [
> +  "FormAutofillIntegration",
> +];

One line this?

@@ +37,5 @@
> +   *
> +   * @param aProperties
> +   *        Provides the initial properties for the newly created object.
> +   *        {
> +   *          form: Reference to the DOM Form element intiating the request.

Isn't this going to be in the parent process? If so, this won't be a real form DOM element, probably a custom object with information about the form instead.

::: toolkit/components/formautofill/test/chrome/test_requestAutocomplete_disabled.html
@@ +19,5 @@
>  
>  /**
>   * Tests the case where the feature is disabled globally.
>   */
> +add_task(function* test_disabled_globally() {

I guess this change was made in the wrong patch

::: toolkit/components/formautofill/test/xpcshell/test_integration.js
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=2 et sw=2 tw=80: */

Mode line

@@ +9,5 @@
> +
> +"use strict";
> +
> +////////////////////////////////////////////////////////////////////////////////
> +//// Tests

I don't think this sectioning is necessary. I would prefer you move the "use strict"; above the /** and then the /** serves the same purpose.

In general, I think the JavaDoc style comment is more common and preferred over ///////…

@@ +15,5 @@
> +/**
> + * Tests registering and unregistering integration overrides.
> + */
> +add_task(function* test_integration_override()
> +{

Nit: brace on line above

@@ +36,5 @@
> +  Assert.ok(overrideCalled);
> +});
> +
> +////////////////////////////////////////////////////////////////////////////////
> +//// Termination

Same for this comment. I don't really think it's necessary but if you think it is, I would prefer the a style of comment that is more common and less effort to produce.

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 8438434 [details] [diff] [review]
Updated work in progress

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

f+ for the general design, but I have some questions below regarding the inheritance implementation. Can discuss more in our meeting tomorrow!

::: toolkit/components/formautofill/FormAutofill.jsm
@@ +42,5 @@
> +   * Platform integration code and add-ons can override methods of this object,
> +   * calling the registerIntegration and unregisterIntegration methods.
> +   */
> +  get integration() {
> +    // This lazy getter is only called in case addIntegration was never called.

s/addIntegration/registerIntegration/

@@ +86,5 @@
> +      }
> +      let derived = Object.create(base, descriptors);
> +
> +      // The previous Object.create operation assigns the actual prototype
> +      // returned by Object.getProrotypeOf and used for inheritance, but we

s/getProrotypeOf/getPrototypeOf/

@@ +89,5 @@
> +      // The previous Object.create operation assigns the actual prototype
> +      // returned by Object.getProrotypeOf and used for inheritance, but we
> +      // still need to assign the separate "prototype" property that will be
> +      // used by the derived object to access methods on the base object.
> +      derived.prototype = base;

I'm confused why we'd need to do this. As you say in your comment, the parent object is accessible via Object.getPrototypeOf(derived). Are you just doing this as a convenience method? If so, maybe you could just create a getter on the FormAutofillIntegration base, e.g.:

  get super() { return Object.getPrototypeOf(this); }

as that would help ensure that the property never becomes stale, and you wouldn't have to manually update it.

Also, you're creating this new property on the object named "prototype", which could be easily confused with the JS "prototype" property on Function to a casual reader. Perhaps name this something else, such as "super" or "parent"?

@@ +90,5 @@
> +      // returned by Object.getProrotypeOf and used for inheritance, but we
> +      // still need to assign the separate "prototype" property that will be
> +      // used by the derived object to access methods on the base object.
> +      derived.prototype = base;
> +      base = derived;

I'm not sure I understand the inheritance pattern you're using here. Let's say I have a storage integration and a UI integration, registered in that order. The hierarchy would look like this:

FormAutofillIntegration -> storageIntegration -> uiIntegration

Using "is-a" OO terminology, this implies that uiIntegration "is-a" storageIntegration, which seems incorrect.

From your example at https://bugzilla.mozilla.org/show_bug.cgi?id=1020495#c4, I would have expected each integration to directly inherit from base:
FormAutofillIntegration -> storageIntegration
FormAutofillIntegration -> uiIntegration

Your inheritance pattern could still be useful in other cases (e.g., FormAutofillIntegration -> genericStorageIntegration -> androidStorageIntegration), but if that's your goal, I imagine callers would have to be explicit about which integration they're inheriting from (e.g., FormAutofill.registerIntegration(<new integration>, <super integration>) ). Or am I misunderstanding what you're trying to do?

::: toolkit/components/formautofill/FormAutofillIntegration.jsm
@@ +25,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "Task",
> +                                  "resource://gre/modules/Task.jsm");
> +
> +////////////////////////////////////////////////////////////////////////////////
> +//// FormAutofillIntegration

As I mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1023484#c6, I think we should drop this comment style (I gave Globals and nsISupports as examples, but I was suggesting you remove all of them). One reason is the small maintenance overhead of making sure these comments are exactly 80 cols wide. Another reason, as MattN pointed out, is that they are often redundant with proper JavaDocs. The prevailing style in our tree, by far, is to use normal JavaDoc-style comments for these cases.

Since style is a subjective issue, I generally wouldn't be opposed to using our own styles in certain situations if the three of us came to a consensus (or at least a majority opinion). But that doesn't seem to be the case here.

::: toolkit/components/formautofill/test/xpcshell/test_integration.js
@@ +20,5 @@
> +  let overrideCalled = false;
> +
> +  let newIntegration = {
> +    createRequestAutocompleteUI: Task.async(function* (aProperties) {
> +      yield this.prototype.createRequestAutocompleteUI.apply(this, arguments);

This looks like a bug if we register multiple integrations. Let's say I add a newIntegration2 integration which calls its super createRequestAutocompleteUI method (where super == newIntegration in this example). Since these integrations each call apply using 'this', the 'this' value would be forwarded to every function in the call chain. That is, the 'this' value in newIntegration#createRequestAutocompleteUI would actually be equal to newIntegration2, which I believe will result in infinite recursion.

Going from my other comment above, I'm thinking your intent was to have every derived object inherit from the same base class. If so, that would fix this bug.

Comment 9

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #8)
> @@ +90,5 @@
> > +      // returned by Object.getProrotypeOf and used for inheritance, but we
> > +      // still need to assign the separate "prototype" property that will be
> > +      // used by the derived object to access methods on the base object.
> > +      derived.prototype = base;
> > +      base = derived;
> 
> I'm not sure I understand the inheritance pattern you're using here. Let's
> say I have a storage integration and a UI integration, registered in that
> order. The hierarchy would look like this:
> 
> FormAutofillIntegration -> storageIntegration -> uiIntegration
> ...

OK, so after sleeping on this, I think my misconception here is that an "integration" is a module (storage module, UI module, etc.). Instead, an integration is a layer of the implementation containing calls to these modules, so you'd have maybe a full generic integration that contains the standard storage, validation, and UI methods, a desktop integration on top of that which could show the desktop UI, and then test integrations on top of that. That means an integration is not as fine-grained as I thought it was when I wrote this comment, but instead a full layer containing all of the necessary rAc methods. In that case, I think your abstraction makes sense -- is my understanding correct?

Comment 10

[:Paolo Amadini]

(In reply to Brian Nicholson (:bnicholson) from comment #9)
> Is my understanding correct?

Yes! I can probably clarify this in code comments.

Comment 11

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #8)
> Going from my other comment above, I'm thinking your intent was to have
> every derived object inherit from the same base class. If so, that would fix
> this bug.

This comment doesn't make sense then. We'll have to think a bit more about how we want to handle the 'this' forwarding problem.

Comment 12

[:Paolo Amadini]

Attachment: Fix generation of integration overrides

(In reply to Brian Nicholson (:bnicholson) from comment #11)
> This comment doesn't make sense then. We'll have to think a bit more about
> how we want to handle the 'this' forwarding problem.

This is what I came up with. It isn't optimal, as the object expression needs to be re-evaluated every time the hierarchy changes. It has the advantage of being a really simple syntax. Suggestions welcome...

Comment 13

[Brian Nicholson (:bnicholson)]

Comment on attachment 8442150 [details] [diff] [review]
Fix generation of integration overrides

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

I'll think about this a bit more to see if I an offer any improvements, but this seems OK to me.

::: toolkit/components/formautofill/FormAutofill.jsm
@@ +72,5 @@
> +    // TODO: Fix base calls.
> +    let base = FormAutofillIntegration;
> +    for (let integrationFn of this._integrations) {
> +      let currentBase = base;
> +      let integration = integrationFn.call(null, currentBase);

Can you just pass base here directly? Doesn't seem like you need currentBase.

@@ +86,5 @@
> +      // The previous Object.create operation assigns the actual prototype
> +      // returned by Object.getPrototypeOf and used for inheritance, but we
> +      // still need to assign the separate "prototype" property that will be
> +      // used by the derived object to access methods on the base object.
> +      derived.prototype = base;

We can remove this now that derived objects have base, right?

Comment 14

[:Paolo Amadini]

Attachment: The patch

Cleaned up code and comments. Tryserver build here:

https://tbpl.mozilla.org/?tree=Try&rev=8cf2b4410aab

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8445880 [details] [diff] [review]
The patch

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

::: toolkit/components/formautofill/test/xpcshell/test_integration.js
@@ +32,5 @@
> +
> +/**
> + * Registers an integration override function that throws an exception.
> + */
> +add_task(function* test_integration_override_error() {

I'm not exactly sure what this test is testing to be honest i.e. what could go wrong that isn't? Perhaps an example in the comment would help.

@@ +33,5 @@
> +/**
> + * Registers an integration override function that throws an exception.
> + */
> +add_task(function* test_integration_override_error() {
> +  let overrideCalled = false;

overrideCalled is unused.

Comment 16

[:Paolo Amadini]

Attachment: Updated patch

Updated the second test case to be more relevant.

https://hg.mozilla.org/integration/fx-team/rev/95ade88228e8

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/95ade88228e8