Add functions to the passwords private api that update the lists.

Add functions to the passwords private api that update the lists.

Making the update explicit will allow us to decouple listening and
updating of both lists.

BUG=598826
Committed: https://crrev.com/de3ada6f945ac01d767ed68db95d17d1dec0d5f8
Cr-Commit-Position: refs/heads/master@{#388889}

[hcarmona, 2016-04-15 22:40:16]

Description was changed from

==========
Add functions to the passwords private api that update the lists.

Making the update explicit will allow us to decouple listening and
updating of both lists. This CL only adds the new functionality and does
NOT remove the implicit update when adding a listener.

BUG=598826
==========

to

==========
Add functions to the passwords private api that update the lists.

Making the update explicit will allow us to decouple listening and
updating of both lists.

BUG=598826
==========

[hcarmona, 2016-04-15 22:40:16]

hcarmona@chromium.org changed reviewers:
+ stevenjb@chromium.org

[hcarmona, 2016-04-15 22:45:47]

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

[hcarmona, 2016-04-15 22:46:26]

hcarmona@chromium.org changed reviewers:
+ isherman@chromium.org
- mpearson@chromium.org

[hcarmona, 2016-04-15 22:51:06]

Steven, PTAL at the passwords_private_api changes + test
Devlin, PTAL at the idl file
Ilya, PTAL at the histogram enum addition

[Devlin, 2016-04-16 00:41:49]

https://codereview.chromium.org/1892613006/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/passwords_private.idl (right):

https://codereview.chromium.org/1892613006/diff/20001/chrome/common/extension...
chrome/common/extensions/api/passwords_private.idl:66: static void
requestSavedPasswordList();
I thought we automatically sent these when a listener was added?

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

[Devlin, 2016-04-16 00:43:34]

https://codereview.chromium.org/1892613006/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/passwords_private.idl (right):

https://codereview.chromium.org/1892613006/diff/20001/chrome/common/extension...
chrome/common/extensions/api/passwords_private.idl:66: static void
requestSavedPasswordList();
On 2016/04/16 00:41:49, Devlin wrote:
> I thought we automatically sent these when a listener was added?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

I guess I should have looked at part 2 of this series.  My bad.

If we're going to do this, let's have a getSavedPasswordList that takes a
callback, rather than forcing an event occurrence.

[Ilya Sherman, 2016-04-16 00:44:32]

histograms.xml lgtm

[stevenjb, 2016-04-18 18:48:18]

https://codereview.chromium.org/1892613006/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/passwords_private.idl (right):

https://codereview.chromium.org/1892613006/diff/20001/chrome/common/extension...
chrome/common/extensions/api/passwords_private.idl:66: static void
requestSavedPasswordList();
On 2016/04/16 00:43:34, Devlin wrote:
> On 2016/04/16 00:41:49, Devlin wrote:
> > I thought we automatically sent these when a listener was added?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> 
> I guess I should have looked at part 2 of this series.  My bad.
> 
> If we're going to do this, let's have a getSavedPasswordList that takes a
> callback, rather than forcing an event occurrence.

I agree. The signature can match that of the event so that the same JS function
can be passed to both the listener and the explicit request. That is a common
pattern in other extension APIs.

[hcarmona, 2016-04-19 20:35:36]

Patchset #3 (id:40001) has been deleted

[hcarmona, 2016-04-19 20:42:02]

Updated requestSavedPasswordList and requestPasswordExceptionList so they use a
callback.

[stevenjb, 2016-04-19 20:59:18]

Thanks for changing this. A few suggestions.

https://codereview.chromium.org/1892613006/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/passwords_private/passwords_private_api.cc
(right):

https://codereview.chromium.org/1892613006/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/passwords_private/passwords_private_api.cc:103:
event_args->GetList(0, &entries);
I believe that if this is called with no entries argument this will crash
chrome? It would be better to call SetError("Invalid arguments") and return
false if event_args is null.

https://codereview.chromium.org/1892613006/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/passwords_private/passwords_private_api.cc:122:
event_args->GetList(0, &exceptions);
Same here.

https://codereview.chromium.org/1892613006/diff/60001/chrome/common/extension...
File chrome/common/extensions/api/passwords_private.idl (right):

https://codereview.chromium.org/1892613006/diff/60001/chrome/common/extension...
chrome/common/extensions/api/passwords_private.idl:62: // TODO(hcarmona): Make
this use a callback for consistency.
This is a bit more complicated. I don't remember the details off hand, but that
change might be tricky. Maybe s/Make this use/Investigate using/.

https://codereview.chromium.org/1892613006/diff/60001/chrome/common/extension...
chrome/common/extensions/api/passwords_private.idl:71: static void
requestSavedPasswordList(passwordListCallback callback);
I think these should now be 'get' instead of 'request'.

[hcarmona, 2016-04-19 23:07:46]

https://codereview.chromium.org/1892613006/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/passwords_private/passwords_private_api.cc
(right):

https://codereview.chromium.org/1892613006/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/passwords_private/passwords_private_api.cc:103:
event_args->GetList(0, &entries);
On 2016/04/19 20:59:17, stevenjb wrote:
> I believe that if this is called with no entries argument this will crash
> chrome? It would be better to call SetError("Invalid arguments") and return
> false if event_args is null.
> 

I fixed my issue. I was creating an event object and taking the first
param from it. I should have been using the result creation method for
my new API functions. Updated.

https://codereview.chromium.org/1892613006/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/passwords_private/passwords_private_api.cc:122:
event_args->GetList(0, &exceptions);
On 2016/04/19 20:59:17, stevenjb wrote:
> Same here.

Same changes here.

https://codereview.chromium.org/1892613006/diff/60001/chrome/common/extension...
File chrome/common/extensions/api/passwords_private.idl (right):

https://codereview.chromium.org/1892613006/diff/60001/chrome/common/extension...
chrome/common/extensions/api/passwords_private.idl:62: // TODO(hcarmona): Make
this use a callback for consistency.
On 2016/04/19 20:59:17, stevenjb wrote:
> This is a bit more complicated. I don't remember the details off hand, but
that
> change might be tricky. Maybe s/Make this use/Investigate using/.

Sounds good. I haven't investigated this yet, I noticed it was different
and added the TODO.

https://codereview.chromium.org/1892613006/diff/60001/chrome/common/extension...
chrome/common/extensions/api/passwords_private.idl:71: static void
requestSavedPasswordList(passwordListCallback callback);
On 2016/04/19 20:59:17, stevenjb wrote:
> I think these should now be 'get' instead of 'request'.

Done.

[stevenjb, 2016-04-19 23:13:38]

lgtm

https://codereview.chromium.org/1892613006/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/passwords_private/passwords_private_api.cc
(right):

https://codereview.chromium.org/1892613006/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/passwords_private/passwords_private_api.cc:103:
event_args->GetList(0, &entries);
On 2016/04/19 23:07:45, Hector Carmona wrote:
> On 2016/04/19 20:59:17, stevenjb wrote:
> > I believe that if this is called with no entries argument this will crash
> > chrome? It would be better to call SetError("Invalid arguments") and return
> > false if event_args is null.
> > 
> 
> I fixed my issue. I was creating an event object and taking the first
> param from it. I should have been using the result creation method for
> my new API functions. Updated.

Ah. Yes, this is better.

[hcarmona, 2016-04-20 20:48:29]

@Devlin, friendly ping on this CL

[Devlin, 2016-04-20 22:32:52]

idl lgtm with nits

https://codereview.chromium.org/1892613006/diff/80001/chrome/common/extension...
File chrome/common/extensions/api/passwords_private.idl (right):

https://codereview.chromium.org/1892613006/diff/80001/chrome/common/extension...
chrome/common/extensions/api/passwords_private.idl:42: callback
passwordListCallback = void(PasswordUiEntry[] entries);
nitty nit: prefer PasswordListCallback (start with capital for types like this,
lowercase for functions and properties)

https://codereview.chromium.org/1892613006/diff/80001/chrome/common/extension...
chrome/common/extensions/api/passwords_private.idl:68: // Returns the list of
saved passwords using |callback|.
nit: remove "using |callback|".  It's clear from the line below.

https://codereview.chromium.org/1892613006/diff/80001/chrome/common/extension...
chrome/common/extensions/api/passwords_private.idl:69: //
nit: remove

https://codereview.chromium.org/1892613006/diff/80001/chrome/common/extension...
chrome/common/extensions/api/passwords_private.idl:74: //
(Same nits as above)

[hcarmona, 2016-04-20 23:10:58]

Addressed nits.

https://codereview.chromium.org/1892613006/diff/80001/chrome/common/extension...
File chrome/common/extensions/api/passwords_private.idl (right):

https://codereview.chromium.org/1892613006/diff/80001/chrome/common/extension...
chrome/common/extensions/api/passwords_private.idl:42: callback
passwordListCallback = void(PasswordUiEntry[] entries);
On 2016/04/20 22:32:52, Devlin wrote:
> nitty nit: prefer PasswordListCallback (start with capital for types like
this,
> lowercase for functions and properties)

Done.

https://codereview.chromium.org/1892613006/diff/80001/chrome/common/extension...
chrome/common/extensions/api/passwords_private.idl:68: // Returns the list of
saved passwords using |callback|.
On 2016/04/20 22:32:52, Devlin wrote:
> nit: remove "using |callback|".  It's clear from the line below.

Done.

https://codereview.chromium.org/1892613006/diff/80001/chrome/common/extension...
chrome/common/extensions/api/passwords_private.idl:69: //
On 2016/04/20 22:32:52, Devlin wrote:
> nit: remove

Done.

https://codereview.chromium.org/1892613006/diff/80001/chrome/common/extension...
chrome/common/extensions/api/passwords_private.idl:74: //
On 2016/04/20 22:32:52, Devlin wrote:
> (Same nits as above)

Done.

[hcarmona, 2016-04-20 23:11:46]

The CQ bit was checked by hcarmona@chromium.org

[hcarmona, 2016-04-20 23:11:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, stevenjb@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1892613006/#ps100001
(title: "nits")

[commit-bot: I haz the power, 2016-04-20 23:12:35]

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

[commit-bot: I haz the power, 2016-04-21 00:10:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-21 00:10:19]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hcarmona, 2016-04-21 20:36:17]

The CQ bit was checked by hcarmona@chromium.org

[commit-bot: I haz the power, 2016-04-21 20:37:02]

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

[commit-bot: I haz the power, 2016-04-21 20:43:14]

Description was changed from

==========
Add functions to the passwords private api that update the lists.

Making the update explicit will allow us to decouple listening and
updating of both lists.

BUG=598826
==========

to

==========
Add functions to the passwords private api that update the lists.

Making the update explicit will allow us to decouple listening and
updating of both lists.

BUG=598826
==========

[commit-bot: I haz the power, 2016-04-21 20:43:16]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-04-22 19:39:07]

Description was changed from

==========
Add functions to the passwords private api that update the lists.

Making the update explicit will allow us to decouple listening and
updating of both lists.

BUG=598826
==========

to

==========
Add functions to the passwords private api that update the lists.

Making the update explicit will allow us to decouple listening and
updating of both lists.

BUG=598826

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

[commit-bot: I haz the power, 2016-04-22 19:39:09]

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