Separate the listeners and getters for the Autofill Private API.

Separate the listeners and getters for the Autofill Private API.

OnAddressListChanged and OnCreditCardListChanged used to trigger an
update when a listener was added. This is now de-coupled so that a list
can be retrieved separately from listening for changes.

BUG=607348, 607347
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/f900222890dd7e4d6ca6ac872520b6ee9c518f04
Cr-Commit-Position: refs/heads/master@{#395462}

[hcarmona, 2016-05-07 01:30:29]

Description was changed from

==========
Separate the listeners and getters for the Autofill Private API.

OnAddressListChanged and OnCreditCardListChanged used to trigger an
update when a listener was added. This is now de-coupled so that a list
can be retrieved separately from listening for changes.

BUG=607348,607347
==========

to

==========
Separate the listeners and getters for the Autofill Private API.

OnAddressListChanged and OnCreditCardListChanged used to trigger an
update when a listener was added. This is now de-coupled so that a list
can be retrieved separately from listening for changes.

BUG=607348,607347
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[hcarmona, 2016-05-07 01:34:19]

Description was changed from

==========
Separate the listeners and getters for the Autofill Private API.

OnAddressListChanged and OnCreditCardListChanged used to trigger an
update when a listener was added. This is now de-coupled so that a list
can be retrieved separately from listening for changes.

BUG=607348,607347
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Separate the listeners and getters for the Autofill Private API.

OnAddressListChanged and OnCreditCardListChanged used to trigger an
update when a listener was added. This is now de-coupled so that a list
can be retrieved separately from listening for changes.

BUG=607348,607347
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[hcarmona, 2016-05-07 01:34:19]

hcarmona@chromium.org changed reviewers:
+ isherman@chromium.org, rdevlin.cronin@chromium.org, stevenjb@chromium.org

[hcarmona, 2016-05-07 01:36:52]

hcarmona@chromium.org changed reviewers:
- isherman@chromium.org, rdevlin.cronin@chromium.org

[hcarmona, 2016-05-07 01:36:53]

PTAL

Thanks!

[stevenjb, 2016-05-09 16:46:51]

https://codereview.chromium.org/1957043002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js
(right):

https://codereview.chromium.org/1957043002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:131:
};
I think this wrapper confuses things. Looking at this by itself I would be
concerned that multiple calls to 'setAddressesCallback' would set multiple
callbacks without tracking or removing them.

I would rather see us call these explicitly in ready() with a bound callback
assigned to a private member (and call autofillPrivate.*.removeListener on
detach).

[hcarmona, 2016-05-12 20:53:53]

https://codereview.chromium.org/1957043002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js
(right):

https://codereview.chromium.org/1957043002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:131:
};
On 2016/05/09 16:46:51, stevenjb wrote:
> I think this wrapper confuses things. Looking at this by itself I would be
> concerned that multiple calls to 'setAddressesCallback' would set multiple
> callbacks without tracking or removing them.
> 
> I would rather see us call these explicitly in ready() with a bound callback
> assigned to a private member (and call autofillPrivate.*.removeListener on
> detach).

How's this look? I've also updated the calls to the password API for
consistency.

[stevenjb, 2016-05-12 21:19:54]

https://codereview.chromium.org/1957043002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js
(right):

https://codereview.chromium.org/1957043002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:198:
this.passwordManager_ = PasswordManagerImpl.getInstance();
We might want to keep this and set it to chrome.passwordsPrivate for now, so
that we can replace it with a fake implementation in test code later.

https://codereview.chromium.org/1957043002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js
(right):

https://codereview.chromium.org/1957043002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:97:
this.setSavedPasswords_.bind(this));
I don't know JS well enough to know whether or not this will magically work, but
the pattern I have been using is:

savedPasswordsListChangedListener_: function() {},

ready:
  this.savedPasswordsListChangedListener_ = this.setSavedPasswords_.bind(this);
 
chrome.passwordsPrivate.getSavedPasswordList(this.savedPasswordsListChangedListener_);
 
chrome.passwordsPrivate.onSavedPasswordsListChanged.addListener(this.savedPasswordsListChangedListener_);

detached:
 
chrome.passwordsPrivate.onSavedPasswordsListChanged.removeListener(this.savedPasswordsListChangedListener_);

The important difference is that removeListener() uses the same function object
passed to addListener(), instead of a new function object. (i.e. I don't think
that the removeListener() call will work as written.

Using the same function object for getSavedPasswordList() and
onSavedPasswordsListChanged() is somewhat convenient but not necessary.

[hcarmona, 2016-05-13 02:01:02]

https://codereview.chromium.org/1957043002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js
(right):

https://codereview.chromium.org/1957043002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:198:
this.passwordManager_ = PasswordManagerImpl.getInstance();
On 2016/05/12 21:19:54, stevenjb wrote:
> We might want to keep this and set it to chrome.passwordsPrivate for now, so
> that we can replace it with a fake implementation in test code later. 

Done.

https://codereview.chromium.org/1957043002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js
(right):

https://codereview.chromium.org/1957043002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:97:
this.setSavedPasswords_.bind(this));
On 2016/05/12 21:19:54, stevenjb wrote:
> I don't know JS well enough to know whether or not this will magically work,
but
> the pattern I have been using is:
> 
> savedPasswordsListChangedListener_: function() {},
> 
> ready:
>   this.savedPasswordsListChangedListener_ =
this.setSavedPasswords_.bind(this);
>  
>
chrome.passwordsPrivate.getSavedPasswordList(this.savedPasswordsListChangedListener_);
>  
>
chrome.passwordsPrivate.onSavedPasswordsListChanged.addListener(this.savedPasswordsListChangedListener_);
> 
> detached:
>  
>
chrome.passwordsPrivate.onSavedPasswordsListChanged.removeListener(this.savedPasswordsListChangedListener_);
> 
> The important difference is that removeListener() uses the same function
object
> passed to addListener(), instead of a new function object. (i.e. I don't think
> that the removeListener() call will work as written.
> 
> Using the same function object for getSavedPasswordList() and
> onSavedPasswordsListChanged() is somewhat convenient but not necessary.
> 

You are correct. I didn't realize that bind() would return a new function
reference each time.

Fixed.

[stevenjb, 2016-05-13 15:58:40]

https://codereview.chromium.org/1957043002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js
(right):

https://codereview.chromium.org/1957043002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:11:
var AutofillManager = chrome.autofillPrivate;
I would avoid putting generically named variables in the global namespace like
this. We used this pattern for bletooth (from bluetooth_page.js):

var bluetoothPage = bluetoothPage || {
  /**
   * Set this to provide a fake implementation for testing.
   * @type {Bluetooth}
   */
  bluetoothApiForTest: null,

  /**
   * Set this to provide a fake implementation for testing.
   * @type {BluetoothPrivate}
   */
  bluetoothPrivateApiForTest: null,
};

This both provides a specific namespace and allows the test code to set these
properties before this JS is loaded.

https://codereview.chromium.org/1957043002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:86:
}.bind(this);
Is clang OK with this? Not declaring these as members is certainly convenient
and seems fine to me, assuming clang is OK with it, but you might want to run
this pattern by Dan or Michael also.

[hcarmona, 2016-05-18 00:13:16]

Patchset #4 (id:60001) has been deleted

[hcarmona, 2016-05-18 00:14:45]

PTAL. Also this CL is starting to get a bit large. Let me know if you want me to
split the tests into a separate CL to make it easier to review or if this is
fine.

Thanks!

https://codereview.chromium.org/1957043002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js
(right):

https://codereview.chromium.org/1957043002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:11:
var AutofillManager = chrome.autofillPrivate;
On 2016/05/13 15:58:40, stevenjb wrote:
> I would avoid putting generically named variables in the global namespace like
> this. We used this pattern for bletooth (from bluetooth_page.js):
> 
> var bluetoothPage = bluetoothPage || {
>   /**
>    * Set this to provide a fake implementation for testing.
>    * @type {Bluetooth}
>    */
>   bluetoothApiForTest: null,
> 
>   /**
>    * Set this to provide a fake implementation for testing.
>    * @type {BluetoothPrivate}
>    */
>   bluetoothPrivateApiForTest: null,
> };
> 
> This both provides a specific namespace and allows the test code to set these
> properties before this JS is loaded.

After looking at that pattern, it looks like a lack of interface
prevents that for autofill and passwords (based on my existing changes).
I've created an interface for autofill to match the passwords one and
added tests. WDYT?

https://codereview.chromium.org/1957043002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:86:
}.bind(this);
On 2016/05/13 15:58:40, stevenjb wrote:
> Is clang OK with this? Not declaring these as members is certainly convenient
> and seems fine to me, assuming clang is OK with it, but you might want to run
> this pattern by Dan or Michael also.

The closure compiler didn't complain, but this was weird. I've updated
it so the listener functions are now data members that can be set.
Closure will verify the function signatures and they can be assigned
per instance of this element and cleaned up on |detached|. I've added
tests to verify this.

[stevenjb, 2016-05-23 17:40:01]

I think that there is an extra layer in passwords_and_forms_page.js that could
be removed, but I also think that would be better done in a separate pass.

Thanks for adding the extra tests.

LGTM

https://codereview.chromium.org/1957043002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js
(right):

https://codereview.chromium.org/1957043002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:334:
setCreditCards_: null,
nit: I would append 'Listener' to all four of these (at which point only the
@type part of the comment is really needed).

[hcarmona, 2016-05-23 18:59:40]

hcarmona@chromium.org changed reviewers:
+ isherman@chromium.org, rdevlin.cronin@chromium.org

[hcarmona, 2016-05-23 19:02:11]

+Ilya for extension_function_histogram_value.h

+Devlin for autofill_private.idl

PTAL, Thanks!

https://codereview.chromium.org/1957043002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js
(right):

https://codereview.chromium.org/1957043002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/passwords_and_forms_page.js:334:
setCreditCards_: null,
On 2016/05/23 17:40:01, stevenjb wrote:
> nit: I would append 'Listener' to all four of these (at which point only the
> @type part of the comment is really needed).

Done.

[Devlin, 2016-05-23 20:19:09]

On 2016/05/23 19:02:11, Hector Carmona wrote:
> +Devlin for autofill_private.idl

Looks like you have some red trybots, but idl lgtm assuming it doesn't change
with any requisite fixes.

[Ilya Sherman, 2016-05-23 21:00:30]

Please run the script to update histograms.xml

[hcarmona, 2016-05-23 22:52:29]

Updated histograms.xml using script and fixed tests (still running, but
expecting green)

[Ilya Sherman, 2016-05-23 22:54:56]

histograms.xml lgtm

[hcarmona, 2016-05-23 23:16:08]

The CQ bit was checked by hcarmona@chromium.org

[hcarmona, 2016-05-23 23:16:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1957043002/#ps140001
(title: "update histogram.xml and test")

[commit-bot: I haz the power, 2016-05-23 23:17:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1957043002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1957043002/140001

[commit-bot: I haz the power, 2016-05-23 23:29:33]

Description was changed from

==========
Separate the listeners and getters for the Autofill Private API.

OnAddressListChanged and OnCreditCardListChanged used to trigger an
update when a listener was added. This is now de-coupled so that a list
can be retrieved separately from listening for changes.

BUG=607348,607347
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Separate the listeners and getters for the Autofill Private API.

OnAddressListChanged and OnCreditCardListChanged used to trigger an
update when a listener was added. This is now de-coupled so that a list
can be retrieved separately from listening for changes.

BUG=607348,607347
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-05-23 23:29:35]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-05-23 23:34:59]

Description was changed from

==========
Separate the listeners and getters for the Autofill Private API.

OnAddressListChanged and OnCreditCardListChanged used to trigger an
update when a listener was added. This is now de-coupled so that a list
can be retrieved separately from listening for changes.

BUG=607348,607347
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Separate the listeners and getters for the Autofill Private API.

OnAddressListChanged and OnCreditCardListChanged used to trigger an
update when a listener was added. This is now de-coupled so that a list
can be retrieved separately from listening for changes.

BUG=607348,607347
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/f900222890dd7e4d6ca6ac872520b6ee9c518f04
Cr-Commit-Position: refs/heads/master@{#395462}
==========

[commit-bot: I haz the power, 2016-05-23 23:35:02]

Patchset 7 (id:??) landed as
https://crrev.com/f900222890dd7e4d6ca6ac872520b6ee9c518f04
Cr-Commit-Position: refs/heads/master@{#395462}