1023484 - Improvements and test for the initial requestAutocomplete stub

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[:Paolo Amadini]

I worked on improvements upon bug 1020440 and the testing framework in bug 1021060.

I've renamed what used to be called the "controller" to "content service", because I think it makes its purpose clearer. While satchel uses the word "controller", it usually denotes an object that can be instantiated and works together with a Model and related View instances. This is instead a service that is called by content (usually in the content process as well).

I also consistently used the full "FormAutofill" prefix everywhere.

I also stripped down the implementation to the bare minimum required to make the associated test pass (we'll still expand it in other bugs together with tests).

Comment 1

[:Paolo Amadini]

Attachment: The patch

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Sounds like this needs adding to this iteration.

Comment 3

[Marco Mucci [:MarcoM]]

Added to Iteration 33.1

Comment 4

[Matthew N. [:MattN]]

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

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

Some quick nits:

::: toolkit/components/formautofill/FormAutofillContentService.js
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

The mode line should be for JS, not C++. Do you use an editor which uses this? If not, I'd rather not have it.

::: toolkit/components/formautofill/nsIFormAutofillContentService.idl
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Wrong mode line here too. "IDL" seems to be a mode

::: toolkit/components/formautofill/test/chrome/chrome.ini
@@ +1,5 @@
>  [DEFAULT]
>  support-files = head.js
>  
>  [test_infrastructure.html]
> +[test_requestAutocomplete_disabled.html]

Why not mochitest-plain?

Comment 5

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #4)
> > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> 
> The mode line should be for JS, not C++. Do you use an editor which uses
> this? If not, I'd rather not have it.

Well, from past discussion it seemed C++ gave better results, while the JavaScript mode required unspecified tweaks to the rest of the mode line to work effectively.

But I don't use an editor that uses mode lines, and these assumptions are only based on past feedback, as you suggested I can just go ahead and remove all the mode lines.

> ::: toolkit/components/formautofill/nsIFormAutofillContentService.idl
> @@ +1,1 @@
> > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> 
> Wrong mode line here too. "IDL" seems to be a mode

Again, I've noticed that some IDL files use it while others use C++. I went for C++ because I was not sure editors really supported the IDL mode without some special configuration.

> ::: toolkit/components/formautofill/test/chrome/chrome.ini
> @@ +1,5 @@
> >  [DEFAULT]
> >  support-files = head.js
> >  
> >  [test_infrastructure.html]
> > +[test_requestAutocomplete_disabled.html]
> 
> Why not mochitest-plain?

If I remember correctly, when talking about this we decided that the difference between mochitest-plain and mochitest-chrome is in practice so small that we don't need the former.

In fact, while this test seems trivial, it will soon need to do special things like setting up mock objects and changing preferences. All our tests will need to do that, and part of that will be done in "head_common.js" before the test starts.

The fact that we can take a mochitest-plain and open it in any browser window, and see the results there, is just an illusion - it only works for the example test :-) As part of the research on the rAc testing framework, I sampled three mochitest-plain from our tree and none of them could run externally (including one very simple DOM mochitest).

So, mochitest-plain is unneeded unless you are specifically testing the inaccessibility of privileged functions, and even then you can do the same with an "http:" iframe in a mochitest-chrome file.

Comment 6

[Brian Nicholson (:bnicholson)]

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

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

Thanks for adding comments! Looks good to me, just some minor comments.

::: toolkit/components/formautofill/FormAutofillContentService.js
@@ +31,5 @@
> +FormAutofillContentService.prototype = {
> +  classID: Components.ID("{ed9c2c3c-3f86-4ae5-8e31-10f71b0f19e6}"),
> +
> +  //////////////////////////////////////////////////////////////////////////////
> +  //// nsISupports

I'd prefer we avoid this comment style as it's a bit distracting -- is there really value in making these the nsISupports/Globals sections stand out so much? Taking a look at most JS files in the tree, this style isn't particularly common (excluding tests and downloads, which you wrote :) ).

I guess this is mostly a judgment call, so if MattN likes this style and I'm outvoted, we can keep it.

@@ -16,4 @@
>    },
> -
> -  _dispatchDisabled: function (form, win, message) {
> -    Services.console.logStringMessage("requestAutocomplete disabled: " + message);

Any reason you're removing this? The rAc docs say that the "disabled" message should be accompanied by a log message. Granted, I'm not sure Services.console.logStringMessage was necessarily the correct API to use -- we should probably dispatch the log to the tab so it appears in the tab's web console. But we should keep some form of logging so devs know why the error events are happening.

Comment 7

[Matthew N. [:MattN]]

(In reply to :Paolo Amadini from comment #5)
> Again, I've noticed that some IDL files use it while others use C++. I went
> for C++ because I was not sure editors really supported the IDL mode without
> some special configuration.

I prefer to use the more-specific modes and developers can configure their editor to make it smarter than just plaintext editing for IDL. It seems like mode lines are commonly used for IDL (not sure of the numbers) but not as common on JS.

> > Why not mochitest-plain?
> 
> If I remember correctly, when talking about this we decided that the
> difference between mochitest-plain and mochitest-chrome is in practice so
> small that we don't need the former.

I thought we decided we don't need the latter since mochitest-chrome doesn't run on Android/B2G IIUC.

Comment 8

[Matthew N. [:MattN]]

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

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

::: content/html/content/src/HTMLFormElement.cpp
@@ -305,5 @@
>  void
>  HTMLFormElement::RequestAutocomplete()
>  {
>    bool dummy;
> -  nsCOMPtr<nsIDOMWindow> win = do_QueryInterface(OwnerDoc()->GetScriptHandlingObject(dummy));

Not sure this line needed to change. Note that you'll need DOM review.

::: toolkit/components/formautofill/FormAutofillContentService.js
@@ +22,1 @@
>  Cu.import("resource://gre/modules/Services.jsm");

Any reason to re-arrange? They were alphabetical…

@@ +26,3 @@
>  
> +function FormAutofillContentService()
> +{

Nit: opening brace on the same line as |function|

@@ +31,5 @@
> +FormAutofillContentService.prototype = {
> +  classID: Components.ID("{ed9c2c3c-3f86-4ae5-8e31-10f71b0f19e6}"),
> +
> +  //////////////////////////////////////////////////////////////////////////////
> +  //// nsISupports

I agree we don't need the ////… comment for nsISupports or Globals in this case and I think the classId/QI members could remain at the bottom of the object which I think is more common.

@@ -16,4 @@
>    },
> -
> -  _dispatchDisabled: function (form, win, message) {
> -    Services.console.logStringMessage("requestAutocomplete disabled: " + message);

I think we should keep the logging and the _dispatchDisabled helper as there are going to be a few different error events dispatched and having to duplicate the log function every time will be error-prone. There should probably be a more general _dispatchAutocompleteError method that takes the reason as an argument and dispatches the event and logs a message.

::: toolkit/components/formautofill/nsIFormAutofillContentService.idl
@@ +25,1 @@
>  [scriptable, uuid(cbf47f9d-a13d-4fad-8221-8964086b1b8a)]

Should the uuid change for a rename or is it fine since this is still on m-c?

::: toolkit/components/formautofill/test/chrome/test_requestAutocomplete_disabled.html
@@ +1,1 @@
> +<!DOCTYPE HTML><html><head><meta charset="utf-8"></head><body>

Nit: lowercase "HTML"

Comment 9

[:Paolo Amadini]

(In reply to Brian Nicholson (:bnicholson) from comment #6)
> I'd prefer we avoid this comment style as it's a bit distracting -- is there
> really value in making these the nsISupports/Globals sections stand out so
> much? Taking a look at most JS files in the tree, this style isn't
> particularly common (excluding tests and downloads, which you wrote :) ).

Actually, I agree that the nsISupports part can be handled more discreetly.

> Any reason you're removing this? The rAc docs say that the "disabled"
> message should be accompanied by a log message. Granted, I'm not sure
> Services.console.logStringMessage was necessarily the correct API to use --
> we should probably dispatch the log to the tab so it appears in the tab's
> web console. But we should keep some form of logging so devs know why the
> error events are happening.

This is true, but I think this is not needed in this initial version, so keeping or removing it would be the same for me. I filed bug 1023813 so that we can implement the logging properly and have tests for that.

Comment 10

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #7)
> > > Why not mochitest-plain?
> > 
> > If I remember correctly, when talking about this we decided that the
> > difference between mochitest-plain and mochitest-chrome is in practice so
> > small that we don't need the former.
> 
> I thought we decided we don't need the latter since mochitest-chrome doesn't
> run on Android/B2G IIUC.

Ah, so I definitely misunderstood our conversation, I believed mochitest-browser-chrome didn't run and other mochitests did, while only mochitest-plain currently runs.

Any idea why mochitest-chrome doesn't run on Android? It isn't conceptually very different from mochitest-plain.

Comment 11

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #8)
> >  Cu.import("resource://gre/modules/Services.jsm");
> 
> Any reason to re-arrange? They were alphabetical…

Hm, I guess alphabetical makes more sense. There was an historical reason for placing XPCOMUtils first in code I've worked on, in that it was the only module that couldn't be imported lazily, and we used to import everything else lazily including Services.jsm.

> I think the classId/QI members could remain at the bottom of the
> object which I think is more common.

Placing them at the bottom makes me want to add back the separator to close the nsIFormAutofillContentService section :-) Also, I think these declarations come logically before the actual implementations of the interfaces they declare.

> I think we should keep the logging and the _dispatchDisabled helper as there
> are going to be a few different error events dispatched and having to
> duplicate the log function every time will be error-prone. There should
> probably be a more general _dispatchAutocompleteError method that takes the
> reason as an argument and dispatches the event and logs a message.

Or we can keep all the interfacing code (which is minimal) inside the requestAutocomplete method, and construct the event based on the response from the instance object created there. This avoids passing around the "form" and "window" arguments.

> ::: toolkit/components/formautofill/nsIFormAutofillContentService.idl
> @@ +25,1 @@
> >  [scriptable, uuid(cbf47f9d-a13d-4fad-8221-8964086b1b8a)]
> 
> Should the uuid change for a rename or is it fine since this is still on m-c?

Not sure about the case of renames on m-c, but I can change it to be on the safe side. Thanks for noticing that!

Comment 12

[:Paolo Amadini]

Attachment: Updated proposal

Here is the updated style proposal. To clarify the previous comment, I think the dispatchDisabled function can be added back or handled in a different way when we add the tests that check its behavior.

The test will probably become a mochitest-plain when we figure out how to do that in bug 1021060.

Olli, there is a change in the DOM code that is pretty straightforward, we only changed the component name on our side.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8438394 [details] [diff] [review]
Updated proposal

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

(In reply to :Paolo Amadini from comment #11)
> (In reply to Matthew N. [:MattN] from comment #8)
> > I think the classId/QI members could remain at the bottom of the
> > object which I think is more common.
> 
> Placing them at the bottom makes me want to add back the separator to close
> the nsIFormAutofillContentService section :-) Also, I think these
> declarations come logically before the actual implementations of the
> interfaces they declare.

I think the main argument for putting them at the bottom is that you shouldn't need to look at them very much and they're more out of the way there.  I don't really care though but I was wondering why you changed it.

> > I think we should keep the logging and the _dispatchDisabled helper as there
> > are going to be a few different error events dispatched and having to
> > duplicate the log function every time will be error-prone. There should
> > probably be a more general _dispatchAutocompleteError method that takes the
> > reason as an argument and dispatches the event and logs a message.
> 
> Or we can keep all the interfacing code (which is minimal) inside the
> requestAutocomplete method, and construct the event based on the response
> from the instance object created there. This avoids passing around the
> "form" and "window" arguments.

I think we will want a helper as there are at least 3 different "reason"s to be dispatched and some of those reasons will be dispatched for many different reasons which each need their own log message.

::: toolkit/components/formautofill/FormAutofillContentService.js
@@ +12,4 @@
>  "use strict";
>  
> +////////////////////////////////////////////////////////////////////////////////
> +//// Globals

Nit: Let's remove the //// headings for now until there is something different to separate the 3 sections from.

@@ +32,5 @@
> +  //////////////////////////////////////////////////////////////////////////////
> +  //// nsIFormAutofillContentService
> +
> +  requestAutocomplete: function (aForm, aWindow)
> +  {

Nit: brace on the line above

::: toolkit/components/formautofill/test/chrome/chrome.ini
@@ +3,5 @@
>    ../head_common.js
>    head.js
>  
>  [test_infrastructure.html]
> +[test_requestAutocomplete_disabled.html]

I would prefer this was a mochitest-plain so it runs on android and e10s from the beginning. The test is easy to write without the chrome privileged helpers.

Comment 14

[:Paolo Amadini]

Attachment: Fixed nits

https://tbpl.mozilla.org/?tree=Try&rev=829626fa881d

Comment 15

[:Paolo Amadini]

Attachment: Updated manifests

The previous version missed the changes in the installation manifests.

Landed this one on fx-team:

https://hg.mozilla.org/integration/fx-team/rev/8ca431594c4a

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8ca431594c4a