Remove passwordsPrivate.canPasswordAccountBeManaged

Remove passwordsPrivate.canPasswordAccountBeManaged

After https://codereview.chromium.org/1396403004/, that function will always do the same thing (call the callback with 'true').

R=khorimoto@chromium.org
BUG=480552
Note: the presubmit check fails only because of the expected (https://codereview.chromium.org/1405643004/#msg14) warning about the histograms. There are no errors or other warnings. I filed http://crbug.com/546633 for that. Including the line below to get this landed:
NOPRESUBMIT=true

Committed: https://crrev.com/4377b57f8786cef2baaf0c84487beaaa4c57fcf8
Cr-Commit-Position: refs/heads/master@{#355560}

[vabr (Chromium), 2015-10-15 13:31:22]

vabr@chromium.org changed reviewers:
+ dbeam@chromium.org, finnur@chromium.org

[vabr (Chromium), 2015-10-15 13:56:06]

Kyle,
I checked by grepping in the codebase, that
passwordsPrivate.canPasswordAccountBeManaged is not used. Looking in
_api_features.json, I understood that the API is only to be used by WebUI code
(which it apparently is not), or blessed_extensions. I'm not sure how to check
the latter, is there a way to check which extensions can access this API?

Finnur,
Could you please review chrome/common/extensions/api/passwords_private.idl?

Dan,
Could you please review
third_party/closure_compiler/externs/passwords_private.js? It feels weird to
change third_party, but looking at other CLs like
https://codereview.chromium.org/1142693003 indicates that this particular
directory is OK to change. Is that correct?

Cheers,
Vaclav

[Finnur, 2015-10-15 14:58:54]

LGTM

[Kyle Horimoto, 2015-10-15 16:40:44]

On 2015/10/15 13:56:06, vabr (Chromium) wrote:
> Kyle,
> I checked by grepping in the codebase, that
> passwordsPrivate.canPasswordAccountBeManaged is not used. Looking in
> _api_features.json, I understood that the API is only to be used by WebUI code
> (which it apparently is not), or blessed_extensions. I'm not sure how to check
> the latter, is there a way to check which extensions can access this API?
I believe that only chrome://md-settings has access at the moment.

> 
> Finnur,
> Could you please review chrome/common/extensions/api/passwords_private.idl?
> 
> Dan,
> Could you please review
> third_party/closure_compiler/externs/passwords_private.js? It feels weird to
> change third_party, but looking at other CLs like
> https://codereview.chromium.org/1142693003 indicates that this particular
> directory is OK to change. Is that correct?
> 
> Cheers,
> Vaclav

[Kyle Horimoto, 2015-10-15 16:43:03]

I don't think this is good enough - you likely need to remove some of the
bookkeeping code originally added in https://codereview.chromium.org/1128203005/
(e.g., in histograms.xml).

Please consult with someone on the extensions API team regarding how to remove
an extension API function cleanly as I'm not sure of the process.

[vabr (Chromium), 2015-10-16 10:12:28]

vabr@chromium.org changed reviewers:
+ isherman@chromium.org

[vabr (Chromium), 2015-10-16 10:12:29]

Finnur,
Could I ask you to also review if this looks like a clean removal of the API
function, following Kyle's concerns in
https://codereview.chromium.org/1405643004/#msg5 ?

Kyle,
Thanks for the additional pointers. Looking at the linked CL I found no other
place to adjust than the histograms (done in p.s. 3).
May I assume your approval for removing the API function on the condition that
finnur@ or somebody else from Extensions team approves the way I did it?

Dan,
Could you please review
third_party/closure_compiler/externs/passwords_private.js? It feels weird to
change third_party, but looking at other CLs like
https://codereview.chromium.org/1142693003 indicates that this particular
directory is OK to change. Is that correct?

Ilya,
Please review my change to
extensions/browser/extension_function_histogram_value.h and
tools/metrics/histograms/histograms.xml. Please note that when I ran
update_extension_histograms.py it also suggested adding entries for
FILEMANAGERPRIVATEINTERNAL_GETENTRYACTIONS and
FILEMANAGERPRIVATEINTERNAL_EXECUTEENTRYACTION to the XML. But that would shift
the values behind these, such a change would not belong to this CL, and I'm not
sure if that omission is intentional or not. So I only left the change generated
for DELETED_PASSWORDSPRIVATE_CANPASSWORDACCOUNTBEMANAGED.

Thanks to all of you!
Vaclav

[Finnur, 2015-10-16 11:48:12]

It's been months since I touched any extensions code, so my world-view of it has
probably gotten stale. If you want someone fresh I'd suggest Devlin. But I can't
think of anything else to remove at the moment, so you are probably fine.

[vabr (Chromium), 2015-10-16 11:52:50]

vabr@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[vabr (Chromium), 2015-10-16 11:52:51]

Thanks, Finnur!
To be on the safe side, I'll ask Devlin.

Hi Devlin,
Could you please check if this looks like a clean removal of the API function,
following Kyle's concerns in
https://codereview.chromium.org/1405643004/#msg5 ?

[Kyle Horimoto, 2015-10-16 16:44:04]

LGTM pending an LGTM from someone on the extensions API team.

[Ilya Sherman, 2015-10-16 19:51:12]

histograms lgtm

On 2015/10/16 10:12:29, vabr (Chromium) wrote:
> Ilya,
> Please review my change to
> extensions/browser/extension_function_histogram_value.h and
> tools/metrics/histograms/histograms.xml. Please note that when I ran
> update_extension_histograms.py it also suggested adding entries for
> FILEMANAGERPRIVATEINTERNAL_GETENTRYACTIONS and
> FILEMANAGERPRIVATEINTERNAL_EXECUTEENTRYACTION to the XML. But that would shift
> the values behind these, such a change would not belong to this CL, and I'm
not
> sure if that omission is intentional or not. So I only left the change
generated
> for DELETED_PASSWORDSPRIVATE_CANPASSWORDACCOUNTBEMANAGED.

Oof!  I missed that error when originally reviewing the code that added those
entries.  That's no good!  Thanks for pointing it out, and I'll try to follow up
on it today.

[Devlin, 2015-10-20 16:35:40]

Looks like you got them all (assuming there's not an extension somewhere to be
updated).

FYI, presubmit may or may not yell at you for modifying the histogram.  It's a
bug that it does.  Ignore it.

[Devlin, 2015-10-20 16:35:56]

That was all meant to end in "LGTM".

[vabr (Chromium), 2015-10-20 16:42:44]

Thanks, Devlin!

On 2015/10/20 16:35:40, Devlin wrote:
> Looks like you got them all (assuming there's not an extension somewhere to be
> updated).
Hopefully not. Kyle seems to be of the same opinion in #4.

> 
> FYI, presubmit may or may not yell at you for modifying the histogram.  It's a
> bug that it does.  Ignore it.

[vabr (Chromium), 2015-10-20 16:43:09]

Dan,
Could you please review
third_party/closure_compiler/externs/passwords_private.js? It feels weird to
change third_party, but looking at other CLs like
https://codereview.chromium.org/1142693003 indicates that this particular
directory is OK to change. Is that correct?

[Dan Beam, 2015-10-22 16:00:53]

lgtm

[vabr (Chromium), 2015-10-22 16:13:13]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2015-10-22 16:13:13]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org
Link to the patchset: https://codereview.chromium.org/1405643004/#ps40001
(title: "Fixed histograms")

[commit-bot: I haz the power, 2015-10-22 16:13:48]

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

[commit-bot: I haz the power, 2015-10-22 16:21:51]

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

[commit-bot: I haz the power, 2015-10-22 16:21:52]

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

[vabr (Chromium), 2015-10-22 17:14:06]

Description was changed from

==========
Remove passwordsPrivate.canPasswordAccountBeManaged

After https://codereview.chromium.org/1396403004/, that function will always do
the same thing (call the callback with 'true').

R=khorimoto@chromium.org
BUG=480552
==========

to

==========
Remove passwordsPrivate.canPasswordAccountBeManaged

After https://codereview.chromium.org/1396403004/, that function will always do
the same thing (call the callback with 'true').

R=khorimoto@chromium.org
BUG=480552

Note: the presubmit check fails only because of the expected
(https://codereview.chromium.org/1405643004/#msg14) warning about the
histograms. There are no errors or other warnings. I filed
http://crbug.com/546633 for that. Including the line below to get this landed:
NOPRESUBMIT=true
==========

[vabr (Chromium), 2015-10-22 17:14:56]

As Devlin predicted, the presubmit check is giving me troubles. I did my due
diligence, including describing the details in the CL description, and will by
landing by bypassing the test.

Cheers,
Vaclav

[vabr (Chromium), 2015-10-22 17:15:07]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2015-10-22 17:16:11]

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

[commit-bot: I haz the power, 2015-10-22 17:17:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-22 17:18:22]

Patchset 3 (id:??) landed as
https://crrev.com/4377b57f8786cef2baaf0c84487beaaa4c57fcf8
Cr-Commit-Position: refs/heads/master@{#355560}