Add chrome.screenlockPrivate API functions to handle alternate auth types.

Add chrome.screenlockPrivate API functions to handle alternate auth types.

This CL adds the hideButton, setAuthType and getAuthType functions and the
onAuthAttempted event to the chrome.screenlockPrivate namespace. The
implementation of this API for ChromeOS is done in a separate CL.

BUG=344179
TEST=new api test

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253447

[Tim Song, 2014-02-16 07:02:15]

The implementation for the ChromeOS lock screen is split into this CL:
https://codereview.chromium.org/168623003/

[Tim Song, 2014-02-16 07:03:07]

On 2014/02/16 07:02:15, Tim Song wrote:
> The implementation for the ChromeOS lock screen is split into this CL:
> https://codereview.chromium.org/168623003/

Wrong link. It should be https://codereview.chromium.org/168813002

[Tim Song, 2014-02-20 00:58:21]

Please take a look.

[not at google - send to devlin, 2014-02-20 01:16:19]

IDL file lgtm, I didn't look at the implementation, could you find somebody
familiar with the implementation to do that?

https://codereview.chromium.org/168623003/diff/160001/chrome/common/extension...
File chrome/common/extensions/api/screenlock_private.idl (right):

https://codereview.chromium.org/168623003/diff/160001/chrome/common/extension...
chrome/common/extensions/api/screenlock_private.idl:50: static void
acceptAuthAttempt(boolean accept);
it would be a nicer API if this were a method on the event, but given this is a
private API - if it works for you, sure.

[Tim Song, 2014-02-20 01:37:46]

Thank you for the review.

+xiyuan@ for reviewing implementation details.

https://codereview.chromium.org/168623003/diff/160001/chrome/common/extension...
File chrome/common/extensions/api/screenlock_private.idl (right):

https://codereview.chromium.org/168623003/diff/160001/chrome/common/extension...
chrome/common/extensions/api/screenlock_private.idl:50: static void
acceptAuthAttempt(boolean accept);
On 2014/02/20 01:16:20, kalman wrote:
> it would be a nicer API if this were a method on the event, but given this is
a
> private API - if it works for you, sure.

That would be nice. Can you point me to another API that does this for
reference?

[not at google - send to devlin, 2014-02-20 01:49:51]

On 2014/02/20 01:37:46, Tim Song wrote:
> Thank you for the review.
> 
> +xiyuan@ for reviewing implementation details.
> 
>
https://codereview.chromium.org/168623003/diff/160001/chrome/common/extension...
> File chrome/common/extensions/api/screenlock_private.idl (right):
> 
>
https://codereview.chromium.org/168623003/diff/160001/chrome/common/extension...
> chrome/common/extensions/api/screenlock_private.idl:50: static void
> acceptAuthAttempt(boolean accept);
> On 2014/02/20 01:16:20, kalman wrote:
> > it would be a nicer API if this were a method on the event, but given this
is
> a
> > private API - if it works for you, sure.
> 
> That would be nice. Can you point me to another API that does this for
> reference?

You'd need to write some javascript for it unfortuately. I think that the
webRequest API does it, in a pretty complex way (the API is extremely complex):

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...

look for "blocking" within the webRequest API documentation
http://developer.chrome.com/extensions/webRequest

you might be able to do something simpler.

Up to you whether you think it's worth the effort.

[xiyuan, 2014-02-20 17:44:43]

LGTM

https://codereview.chromium.org/168623003/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/screenlock_private_api.cc (right):

https://codereview.chromium.org/168623003/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/screenlock_private_api.cc:237:
locker->Hide();
Isn't Hide() a static method?

https://codereview.chromium.org/168623003/diff/160001/chrome/common/extension...
File chrome/common/extensions/api/screenlock_private.idl (right):

https://codereview.chromium.org/168623003/diff/160001/chrome/common/extension...
chrome/common/extensions/api/screenlock_private.idl:17: enum AuthType
{systemPassword, numericPin, userClick};
nit: systemPassword -> offlinePassword to be consistent with LoginDisplay enum.

[Tim Song, 2014-02-20 21:06:07]

It doesn't seem like it's worth the effort and complexity to add the function to
the event listener.

https://codereview.chromium.org/168623003/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/screenlock_private_api.cc (right):

https://codereview.chromium.org/168623003/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/screenlock_private_api.cc:237:
locker->Hide();
On 2014/02/20 17:44:44, xiyuan OOO wrote:
> Isn't Hide() a static method?

Done.

https://codereview.chromium.org/168623003/diff/160001/chrome/common/extension...
File chrome/common/extensions/api/screenlock_private.idl (right):

https://codereview.chromium.org/168623003/diff/160001/chrome/common/extension...
chrome/common/extensions/api/screenlock_private.idl:17: enum AuthType
{systemPassword, numericPin, userClick};
On 2014/02/20 17:44:44, xiyuan OOO wrote:
> nit: systemPassword -> offlinePassword to be consistent with LoginDisplay
enum.

Done.

[Tim Song, 2014-02-21 01:52:09]

The CQ bit was checked by tengs@chromium.org

[Tim Song, 2014-02-21 01:52:12]

The CQ bit was unchecked by tengs@chromium.org

[commit-bot: I haz the power, 2014-02-21 01:52:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/168623003/260001

[Tim Song, 2014-02-22 23:49:37]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-02-22 23:49:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/168623003/360001

[commit-bot: I haz the power, 2014-02-22 23:53:08]

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

[commit-bot: I haz the power, 2014-02-22 23:53:08]

Try jobs failed on following builders: mac_chromium_rel

[Tim Song, 2014-02-22 23:54:34]

+asvitkine@ for histograms.xml

[Alexei Svitkine (slow), 2014-02-24 15:54:33]

https://codereview.chromium.org/168623003/diff/360001/chrome/browser/extensio...
File chrome/browser/extensions/extension_function_histogram_value.h (right):

https://codereview.chromium.org/168623003/diff/360001/chrome/browser/extensio...
chrome/browser/extensions/extension_function_histogram_value.h:755:
ENUM_BOUNDARY  // Last entry: Add new entries above.
Nit: I'd change this comment to:

// Last entry: Add new entries above and ensure to update histograms.xml.

(And maybe put it above the enum, since it probably won't fit at the end of the
line anymore.)

https://codereview.chromium.org/168623003/diff/360001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/168623003/diff/360001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:26146: +  <int value="686"
label="SCREENLOCKPRIVATE_HIDEBUTTON"/>
I think this is missing entries for:

ACTIVITYLOGPRIVATE_DELETEACTIVITIES,
NETWORKINGPRIVATE_SETWIFITDLSENABLEDSTATE,
NETWORKINGPRIVATE_GETWIFITDLSSTATUS,

After you add these, can you manually verify that
SCREENLOCKPRIVATE_ACCEPTAUTHATTEMPT indeed has value 689?

[Tim Song, 2014-02-24 19:10:57]

https://codereview.chromium.org/168623003/diff/360001/chrome/browser/extensio...
File chrome/browser/extensions/extension_function_histogram_value.h (right):

https://codereview.chromium.org/168623003/diff/360001/chrome/browser/extensio...
chrome/browser/extensions/extension_function_histogram_value.h:755:
ENUM_BOUNDARY  // Last entry: Add new entries above.
On 2014/02/24 15:54:33, Alexei Svitkine wrote:
> Nit: I'd change this comment to:
> 
> // Last entry: Add new entries above and ensure to update histograms.xml.
> 
> (And maybe put it above the enum, since it probably won't fit at the end of
the
> line anymore.)

This should probably be done in a separate CL as the PRESUBMIT and PRESUBMIT
tests need to be updated to handle a different ENUM_END_MARKER.

https://codereview.chromium.org/168623003/diff/360001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/168623003/diff/360001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:26146: +  <int value="686"
label="SCREENLOCKPRIVATE_HIDEBUTTON"/>
On 2014/02/24 15:54:33, Alexei Svitkine wrote:
> I think this is missing entries for:
> 
> ACTIVITYLOGPRIVATE_DELETEACTIVITIES,
> NETWORKINGPRIVATE_SETWIFITDLSENABLEDSTATE,
> NETWORKINGPRIVATE_GETWIFITDLSSTATUS,
> 
> After you add these, can you manually verify that
> SCREENLOCKPRIVATE_ACCEPTAUTHATTEMPT indeed has value 689?

Done. SCREENLOCKPRIVATE_ACCEPTAUTHATTEMPT is not at 692 after adding the other
enums.

[Alexei Svitkine (slow), 2014-02-24 19:16:10]

https://codereview.chromium.org/168623003/diff/360001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/168623003/diff/360001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:26146: +  <int value="686"
label="SCREENLOCKPRIVATE_HIDEBUTTON"/>
On 2014/02/24 19:10:58, Tim Song wrote:
> On 2014/02/24 15:54:33, Alexei Svitkine wrote:
> > I think this is missing entries for:
> > 
> > ACTIVITYLOGPRIVATE_DELETEACTIVITIES,
> > NETWORKINGPRIVATE_SETWIFITDLSENABLEDSTATE,
> > NETWORKINGPRIVATE_GETWIFITDLSSTATUS,
> > 
> > After you add these, can you manually verify that
> > SCREENLOCKPRIVATE_ACCEPTAUTHATTEMPT indeed has value 689?
> 
> Done. SCREENLOCKPRIVATE_ACCEPTAUTHATTEMPT is not at 692 after adding the other
> enums.

Do you mean "is now"?

[Tim Song, 2014-02-24 19:22:53]

https://codereview.chromium.org/168623003/diff/360001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/168623003/diff/360001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:26146: +  <int value="686"
label="SCREENLOCKPRIVATE_HIDEBUTTON"/>
On 2014/02/24 19:16:10, Alexei Svitkine wrote:
> On 2014/02/24 19:10:58, Tim Song wrote:
> > On 2014/02/24 15:54:33, Alexei Svitkine wrote:
> > > I think this is missing entries for:
> > > 
> > > ACTIVITYLOGPRIVATE_DELETEACTIVITIES,
> > > NETWORKINGPRIVATE_SETWIFITDLSENABLEDSTATE,
> > > NETWORKINGPRIVATE_GETWIFITDLSSTATUS,
> > > 
> > > After you add these, can you manually verify that
> > > SCREENLOCKPRIVATE_ACCEPTAUTHATTEMPT indeed has value 689?
> > 
> > Done. SCREENLOCKPRIVATE_ACCEPTAUTHATTEMPT is not at 692 after adding the
other
> > enums.
> 
> Do you mean "is now"?

That was an unfortunate typo. Yes, I did mean "is now".

[Alexei Svitkine (slow), 2014-02-24 19:25:18]

LGTM, thanks

https://codereview.chromium.org/168623003/diff/360001/chrome/browser/extensio...
File chrome/browser/extensions/extension_function_histogram_value.h (right):

https://codereview.chromium.org/168623003/diff/360001/chrome/browser/extensio...
chrome/browser/extensions/extension_function_histogram_value.h:755:
ENUM_BOUNDARY  // Last entry: Add new entries above.
On 2014/02/24 19:10:58, Tim Song wrote:
> On 2014/02/24 15:54:33, Alexei Svitkine wrote:
> > Nit: I'd change this comment to:
> > 
> > // Last entry: Add new entries above and ensure to update histograms.xml.
> > 
> > (And maybe put it above the enum, since it probably won't fit at the end of
> the
> > line anymore.)
> 
> This should probably be done in a separate CL as the PRESUBMIT and PRESUBMIT
> tests need to be updated to handle a different ENUM_END_MARKER.

Really? Changing the comment would break the presubmits? (I guess if they don't
expect a comment on the start of the line...)

I'm okay punting this to a separate CL if its indeed this much hassle. (Though
please do follow-up with this, as I think having the comment here (at the
bottom) will indeed prevent this sort of omission in the future.)

[Tim Song, 2014-02-25 03:33:43]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-02-25 03:36:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/168623003/540001

[commit-bot: I haz the power, 2014-02-25 03:36:14]

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

[commit-bot: I haz the power, 2014-02-25 03:36:14]

Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_function_histogram_value.h
  Hunk #1 FAILED at 748.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej

Patch:       chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
72614ac4d4e7339ba998cae7d4632f61c0877fe5..e1c07587dba7243cda9a5a8df90cb283fc8f74d0
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -748,6 +748,10 @@ enum HistogramValue {
   ACTIVITYLOGPRIVATE_DELETEACTIVITIES,
   NETWORKINGPRIVATE_SETWIFITDLSENABLEDSTATE,
   NETWORKINGPRIVATE_GETWIFITDLSSTATUS,
+  SCREENLOCKPRIVATE_HIDEBUTTON,
+  SCREENLOCKPRIVATE_SETAUTHTYPE,
+  SCREENLOCKPRIVATE_GETAUTHTYPE,
+  SCREENLOCKPRIVATE_ACCEPTAUTHATTEMPT,
   ENUM_BOUNDARY  // Last entry: Add new entries above.
 };

[Tim Song, 2014-02-25 18:21:22]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-02-25 18:23:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/168623003/720001

[commit-bot: I haz the power, 2014-02-25 21:51:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/168623003/720001

[commit-bot: I haz the power, 2014-02-25 22:16:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/168623003/720001

[commit-bot: I haz the power, 2014-02-25 23:19:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/168623003/720001

[commit-bot: I haz the power, 2014-02-26 00:06:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/168623003/720001

[Paweł Hajdan Jr., 2014-02-26 05:33:58]

The CQ bit was unchecked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-26 05:34:55]

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

[Paweł Hajdan Jr., 2014-02-26 05:42:28]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-26 05:52:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/168623003/720001

[commit-bot: I haz the power, 2014-02-26 14:41:55]

Change committed as 253447