Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1968083004:
Implement the private API for quick unlock. (Closed)
|
Created:
4 years, 7 months ago by jdufault Modified:
4 years, 6 months ago CC:
asvitkine+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, davemoore+watch_chromium.org, extensions-reviews_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement the private API for quick unlock.
The design doc is at go/cros-quick-unlock-design.
BUG=603217
Committed: https://crrev.com/422a5b1d21c2f099e656d63050e1322245e8ed8e
Cr-Commit-Position: refs/heads/master@{#401780}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Address comments #
Total comments: 2
Patch Set 3 : Move checkPassword into setModes, add onActiveModesChange event #Patch Set 4 : Rebase #
Total comments: 37
Patch Set 5 : Rebase #Patch Set 6 : Address comments #
Total comments: 14
Patch Set 7 : Migrate browser tests to unit tests #
Total comments: 20
Patch Set 8 : Comments #
Total comments: 24
Patch Set 9 : Address comments #
Total comments: 10
Patch Set 10 : Rebase #Patch Set 11 : Move to base::Callback, adjust api_test_utils api, address comments #
Total comments: 9
Patch Set 12 : Misc fixes #
Total comments: 17
Patch Set 13 : mpearson@ comments & nits #
Total comments: 2
Patch Set 14 : Fix compile, rerun python snippet #Patch Set 15 : Fix compile #Messages
Total messages: 66 (19 generated)
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Initial implementation of private API for quick unlock. BUG= ========== to ========== Implement the private API for quick unlock. BUG=603217 ==========
jdufault@chromium.org changed reviewers: + achuith@chromium.org, rockot@chromium.org, stevenjb@chromium.org
stevenjb@/achuith@, PTAL at chrome/browser/chromeos/extensions/* and chrome/test/* rockot@: PTAL at chrome/common/extensions/* Thanks!
rockot@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
+rdevlin.cronin (extensions) -rockot (not extensions) I should probably disappear myself the extensions OWNERS files :)
Wondering if we really need QuickUnlockModes. Is there a concrete plan to implement any other modes? If not, then the over-design doesn't add value. https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:27: } // namespace drop comment https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:101: Error("|modes| and |passwords| must have the same number of elements")); Does this need to be translated? https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:116: quick_unlock_private::QuickUnlockMode mode = params->modes[i]; const https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:117: std::string password = params->passwords[i]; you don't need this local? https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:171: std::vector<quick_unlock_private::QuickUnlockMode> modes; https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-... https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h (right): https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h:15: } // namespace chromeos don't think you need the comment https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h:27: chromeos::ExtendedAuthenticator* (*)(chromeos::AuthStatusConsumer* why not base::Callback? I think that pattern is more common https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_test.cc (right): https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_test.cc:20: } // namespace drop comment
Can you link to the API review?
Description was changed from ========== Implement the private API for quick unlock. BUG=603217 ========== to ========== Implement the private API for quick unlock. The design doc is at go/cros-quick-unlock-design. BUG=603217 ==========
https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:27: } // namespace On 2016/05/20 23:50:21, achuithb wrote: > drop comment Done. https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:101: Error("|modes| and |passwords| must have the same number of elements")); On 2016/05/20 23:50:21, achuithb wrote: > Does this need to be translated? It seems most/all of the existing invocations are hard-coded to English. https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:116: quick_unlock_private::QuickUnlockMode mode = params->modes[i]; On 2016/05/20 23:50:21, achuithb wrote: > const Done. https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:117: std::string password = params->passwords[i]; On 2016/05/20 23:50:21, achuithb wrote: > you don't need this local? Used twice below, and in any future quick unlock mode. https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:171: std::vector<quick_unlock_private::QuickUnlockMode> modes; On 2016/05/20 23:50:21, achuithb wrote: > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-... Moved to initializer syntax for now, but when policy support is implemented it will have to revert to push_back construction style since we will not always add QUICK_UNLOCK_MODE_PIN. https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h (right): https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h:15: } // namespace chromeos On 2016/05/20 23:50:21, achuithb wrote: > don't think you need the comment Done. https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h:27: chromeos::ExtendedAuthenticator* (*)(chromeos::AuthStatusConsumer* On 2016/05/20 23:50:21, achuithb wrote: > why not base::Callback? I think that pattern is more common base::Callback has a dtor so it cannot be have static storage. https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_test.cc (right): https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_test.cc:20: } // namespace On 2016/05/20 23:50:21, achuithb wrote: > drop comment Done.
On 2016/05/20 23:50:22, achuithb wrote: > Wondering if we really need QuickUnlockModes. Is there a concrete plan to > implement any other modes? If not, then the over-design doesn't add value. Yes, we are planning on having more quick unlock modes - though it might be awhile before that happens. My biggest concern with the API is the support for multiple modes, since we may never actually support that. However, it doesn't cost much from an implementation standpoint since it just returns an error. On 2016/05/21 00:25:46, Devlin wrote: > Can you link to the API review? Done. go/cros-quick-unlock-design
lgtm https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:117: std::string password = params->passwords[i]; On 2016/05/23 18:06:09, jdufault wrote: > On 2016/05/20 23:50:21, achuithb wrote: > > you don't need this local? > > Used twice below, and in any future quick unlock mode. I wouldn't worry about future proofing this. Below, couldn't you do pin_password = params->passwords[i]; update_pin = !pin_password.empty();
https://codereview.chromium.org/1968083004/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:179: std::vector<quick_unlock_private::QuickUnlockMode> modes = { const?
On 2016/05/23 18:07:36, jdufault wrote: > On 2016/05/21 00:25:46, Devlin wrote: > > Can you link to the API review? > > Done. go/cros-quick-unlock-design This isn't an extension api review (it's just a general design doc). Has this gone through the API review process?
Specifically: http://www.chromium.org/developers/design-documents/extensions/proposed-chang... On Mon, May 23, 2016 at 2:17 PM, <rdevlin.cronin@chromium.org> wrote: > On 2016/05/23 18:07:36, jdufault wrote: > > On 2016/05/21 00:25:46, Devlin wrote: > > > Can you link to the API review? > > > > Done. go/cros-quick-unlock-design > <https://goto.google.com/cros-quick-unlock-design> > > This isn't an extension api review (it's just a general design doc). Has > this > gone through the API review process? > > https://codereview.chromium.org/1968083004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
(Note also: For private APIs the process tends to be fairly streamlined, but it is important and helpful ensure a well designed and secure API). On Mon, May 23, 2016 at 2:31 PM, Steven Bennetts <stevenjb@chromium.org> wrote: > Specifically: > > > http://www.chromium.org/developers/design-documents/extensions/proposed-chang... > > > On Mon, May 23, 2016 at 2:17 PM, <rdevlin.cronin@chromium.org> wrote: > >> On 2016/05/23 18:07:36, jdufault wrote: >> > On 2016/05/21 00:25:46, Devlin wrote: >> > > Can you link to the API review? >> > >> > Done. go/cros-quick-unlock-design >> <https://goto.google.com/cros-quick-unlock-design> >> >> This isn't an extension api review (it's just a general design doc). Has >> this >> gone through the API review process? >> >> https://codereview.chromium.org/1968083004/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/05/23 20:17:52, Devlin wrote: > This isn't an extension api review (it's just a general design doc). Has this > gone through the API review process? Oops - not yet. I've sent out a review.
On 2016/05/23 22:06:38, jdufault wrote: > On 2016/05/23 20:17:52, Devlin wrote: > > This isn't an extension api review (it's just a general design doc). Has this > > gone through the API review process? > > Oops - not yet. I've sent out a review. See https://groups.google.com/a/chromium.org/forum/#!topic/apps-dev/kqpnk18Qxs0.
On 2016/05/23 22:07:07, jdufault wrote: > On 2016/05/23 22:06:38, jdufault wrote: > > On 2016/05/23 20:17:52, Devlin wrote: > > > This isn't an extension api review (it's just a general design doc). Has > this > > > gone through the API review process? > > > > Oops - not yet. I've sent out a review. > > See https://groups.google.com/a/chromium.org/forum/#!topic/apps-dev/kqpnk18Qxs0. Looks like the API review's wrapping up, but the latest patch needs to be rebased.
On 2016/05/27 19:47:59, Devlin wrote: > Looks like the API review's wrapping up, but the latest patch needs to be > rebased. Yep, I was just finishing that up. https://codereview.chromium.org/1968083004/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:179: std::vector<quick_unlock_private::QuickUnlockMode> modes = { On 2016/05/23 19:10:40, achuithb wrote: > const? Done.
ping - the extension review is done.
https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:93: : chrome_details_(this) {} If this needs chrome details, it should probably just be a ChromeUIThreadExtensionFunction https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:106: /* static */ nit: // static https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:113: : chrome_details_(this) { ditto re ChromeUIThreadExtensionFunction https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:141: AddRef(); Do we need this even though we have the implicit refcount with the bind? https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:153: results_ = SetModes::Results::Create(false); Use Respond(). Also, should this be an error? https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:168: SendResponse(true); use Resond() https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:188: const std::string credential = params->credentials[i]; const &? https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h:19: // Implements the chrome.quickUnlockPrivate.getAvailableModes method. nit: I think these are self-explained by the name and the DECLARE_EXTENSION_FUNCTION macros. I'd probably remove these as not necessary or adding any extra information. https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h:63: // Override how the ExtendedAuthenticator is created. This allows tests to Can we combine this comment with the one on line 71? https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_test.cc (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_test.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_test.cc:24: IN_PROC_BROWSER_TEST_F(QuickUnlockPrivateApiTest, ApiTests) { Could these be unittests? https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... chrome/common/extensions/api/_api_features.json:704: "contexts": ["blessed_extension"] which extension is using this? https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... chrome/common/extensions/api/_permission_features.json:872: "channel": "trunk", Doesn't this mean that the api tests will fail on dev/beta/stable bots? https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:39: // the same for one of the modes, pass an empty string. add comments for each param, rather than a giant comment for the function. https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:40: static void setModes(DOMString accountPassword, We pass a plaintext password? https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... File chrome/common/extensions/permissions/chrome_api_permissions.cc (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... chrome/common/extensions/permissions/chrome_api_permissions.cc:203: APIPermissionInfo::kFlagCannotBeOptional}, Why can't it be optional? https://codereview.chromium.org/1968083004/diff/140001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/quick_unlock_private/apiTests/background.js (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/quick_unlock_private/apiTests/background.js:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. no (c)
https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:93: : chrome_details_(this) {} On 2016/05/31 22:58:38, Devlin wrote: > If this needs chrome details, it should probably just be a > ChromeUIThreadExtensionFunction There's a comment on top of that function saying it is deprecated and that ChromeExtensionFunctionDetails should be used instead. https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:106: /* static */ On 2016/05/31 22:58:38, Devlin wrote: > nit: // static Done. https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:113: : chrome_details_(this) { On 2016/05/31 22:58:38, Devlin wrote: > ditto re ChromeUIThreadExtensionFunction See above. https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:141: AddRef(); On 2016/05/31 22:58:38, Devlin wrote: > Do we need this even though we have the implicit refcount with the bind? Yes, because the implicit refcount is being done on extended_authenticator_ (which holds a pointer to SetModesFunction object). The manual ref management is keeping the SetModesFunction instance alive so that the extended_authenticator_ instance can call into it. https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:153: results_ = SetModes::Results::Create(false); On 2016/05/31 22:58:38, Devlin wrote: > Use Respond(). Done. > Also, should this be an error? The password is a user-entered value, so it's a valid scenario it fails to authenticate. I've been using Error when the API is used incorrectly or in unsupported ways. https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:168: SendResponse(true); On 2016/05/31 22:58:38, Devlin wrote: > use Resond() Done. https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:188: const std::string credential = params->credentials[i]; On 2016/05/31 22:58:38, Devlin wrote: > const &? Done. https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h:19: // Implements the chrome.quickUnlockPrivate.getAvailableModes method. On 2016/05/31 22:58:38, Devlin wrote: > nit: I think these are self-explained by the name and the > DECLARE_EXTENSION_FUNCTION macros. I'd probably remove these as not necessary > or adding any extra information. Done. https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h:63: // Override how the ExtendedAuthenticator is created. This allows tests to On 2016/05/31 22:58:38, Devlin wrote: > Can we combine this comment with the one on line 71? Done. https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_test.cc (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_test.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/05/31 22:58:38, Devlin wrote: > no (c) Done. https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_test.cc:24: IN_PROC_BROWSER_TEST_F(QuickUnlockPrivateApiTest, ApiTests) { On 2016/05/31 22:58:38, Devlin wrote: > Could these be unittests? I may be able to do something like the wallpaper private API unit tests[1], but I haven't played around with it much. If you prefer that approach, I can investigate it. 1: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... chrome/common/extensions/api/_api_features.json:704: "contexts": ["blessed_extension"] On 2016/05/31 22:58:38, Devlin wrote: > which extension is using this? The test extension, and possibly an app launched by options. https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... File chrome/common/extensions/api/_permission_features.json (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... chrome/common/extensions/api/_permission_features.json:872: "channel": "trunk", On 2016/05/31 22:58:38, Devlin wrote: > Doesn't this mean that the api tests will fail on dev/beta/stable bots? I've changed it to stable. https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:39: // the same for one of the modes, pass an empty string. On 2016/05/31 22:58:38, Devlin wrote: > add comments for each param, rather than a giant comment for the function. Done. https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:40: static void setModes(DOMString accountPassword, On 2016/05/31 22:58:38, Devlin wrote: > We pass a plaintext password? Yes; the password transformation happens on the C++ side. https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... File chrome/common/extensions/permissions/chrome_api_permissions.cc (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... chrome/common/extensions/permissions/chrome_api_permissions.cc:203: APIPermissionInfo::kFlagCannotBeOptional}, On 2016/05/31 22:58:38, Devlin wrote: > Why can't it be optional? Changed to kFlagNone. I don't have a strong opinion here, but every consumer needs the permission to perform any functionality w.r.t. quick unlock, so it doesn't make sense for it to be optional. https://codereview.chromium.org/1968083004/diff/140001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/quick_unlock_private/apiTests/background.js (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/test/data/exten... chrome/test/data/extensions/api_test/quick_unlock_private/apiTests/background.js:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/05/31 22:58:39, Devlin wrote: > no (c) Done.
Looking good. Mostly just nits, but I'd really like to see these be unittests. https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_test.cc (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_test.cc:24: IN_PROC_BROWSER_TEST_F(QuickUnlockPrivateApiTest, ApiTests) { On 2016/06/01 00:07:35, jdufault wrote: > On 2016/05/31 22:58:38, Devlin wrote: > > Could these be unittests? > > I may be able to do something like the wallpaper private API unit tests[1], but > I haven't played around with it much. If you prefer that approach, I can > investigate it. > > 1: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... Please do look into using unittests. The wallpaper ones aren't a great example, because they mock out too much of the system. Instead, use the utilities in api_test_utils.h. I think the management api and developer private api are decent examples of unittesting api functions. https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... chrome/common/extensions/api/_api_features.json:704: "contexts": ["blessed_extension"] On 2016/06/01 00:07:35, jdufault wrote: > On 2016/05/31 22:58:38, Devlin wrote: > > which extension is using this? > > The test extension, and possibly an app launched by options. If we move this to a unittest, we should be able to remove the entry, which would be nice IMO. https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... File chrome/common/extensions/permissions/chrome_api_permissions.cc (right): https://codereview.chromium.org/1968083004/diff/140001/chrome/common/extensio... chrome/common/extensions/permissions/chrome_api_permissions.cc:203: APIPermissionInfo::kFlagCannotBeOptional}, On 2016/06/01 00:07:36, jdufault wrote: > On 2016/05/31 22:58:38, Devlin wrote: > > Why can't it be optional? > > Changed to kFlagNone. I don't have a strong opinion here, but every consumer > needs the permission to perform any functionality w.r.t. quick unlock, so it > doesn't make sense for it to be optional. Well, it does in the case of an app/extension that does more than just quick unlock. CannotBeOptional should be used rarely, because optional permissions are far preferable if it's feasible. The extension can always make the choice to still have it be a required permission if it's core to its functionality. (Of course, not as big a deal here, since it's a private API.) https://codereview.chromium.org/1968083004/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:154: Release(); nit: Release(); // Balanced in Run(). (here and on line 167) https://codereview.chromium.org/1968083004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:171: std::unique_ptr<SetModes::Params> params(SetModes::Params::Create(*args_)); EXTENSION_FUNCTION_VALIDATE(params.get()); https://codereview.chromium.org/1968083004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:194: // Use the computed update_pin, pin_credential, etc, values. this comment doesn't really add much value IMO. https://codereview.chromium.org/1968083004/diff/180001/chrome/common/extensio... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/1968083004/diff/180001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:35: // |accountPassword| is the password associated with the account (e.g. the nit: preferred syntax would be |<var>|: description e.g. |accountPassword|: The password associated... (We parse this out into descriptions for each variable, and it doesn't make sense to have a description that begins with "is") https://codereview.chromium.org/1968083004/diff/180001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:42: // chrome.runtime.lastError will be set if the API was used incorrectly. This (lastError comment) is standard for all APIs. I'd remove it. https://codereview.chromium.org/1968083004/diff/180001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:49: // Called after the active set of quick unlock modes has changed. Might be worth specifying that newModes is the collection of all currentModes, rather than the delta between old and current. https://codereview.chromium.org/1968083004/diff/180001/chrome/common/extensio... File chrome/common/extensions/permissions/chrome_api_permissions.cc (left): https://codereview.chromium.org/1968083004/diff/180001/chrome/common/extensio... chrome/common/extensions/permissions/chrome_api_permissions.cc:49: APIPermissionInfo::kFlagCannotBeOptional}, We shouldn't update existing permissions in this patch.
https://codereview.chromium.org/1968083004/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:154: Release(); On 2016/06/01 17:44:12, Devlin wrote: > nit: > Release(); // Balanced in Run(). > > (here and on line 167) Done. https://codereview.chromium.org/1968083004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:171: std::unique_ptr<SetModes::Params> params(SetModes::Params::Create(*args_)); On 2016/06/01 17:44:13, Devlin wrote: > EXTENSION_FUNCTION_VALIDATE(params.get()); That macro is implemented by returning a ResponseAction object, which means it will not compile if placed inside of ApplyModeChange. I've added a comment saying that the arguments have already been validated inside of Run(). https://codereview.chromium.org/1968083004/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:194: // Use the computed update_pin, pin_credential, etc, values. On 2016/06/01 17:44:13, Devlin wrote: > this comment doesn't really add much value IMO. I've changed the comments around so it is more clear that the previous block is for computing needed changes, and that this block is for applying those changes. https://codereview.chromium.org/1968083004/diff/180001/chrome/common/extensio... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/1968083004/diff/180001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:35: // |accountPassword| is the password associated with the account (e.g. the On 2016/06/01 17:44:13, Devlin wrote: > nit: preferred syntax would be > |<var>|: description > e.g. > |accountPassword|: The password associated... > > (We parse this out into descriptions for each variable, and it doesn't make > sense to have a description that begins with "is") Done. https://codereview.chromium.org/1968083004/diff/180001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:42: // chrome.runtime.lastError will be set if the API was used incorrectly. On 2016/06/01 17:44:13, Devlin wrote: > This (lastError comment) is standard for all APIs. I'd remove it. Done. https://codereview.chromium.org/1968083004/diff/180001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:49: // Called after the active set of quick unlock modes has changed. On 2016/06/01 17:44:13, Devlin wrote: > Might be worth specifying that newModes is the collection of all currentModes, > rather than the delta between old and current. Done. https://codereview.chromium.org/1968083004/diff/180001/chrome/common/extensio... File chrome/common/extensions/permissions/chrome_api_permissions.cc (left): https://codereview.chromium.org/1968083004/diff/180001/chrome/common/extensio... chrome/common/extensions/permissions/chrome_api_permissions.cc:49: APIPermissionInfo::kFlagCannotBeOptional}, On 2016/06/01 17:44:13, Devlin wrote: > We shouldn't update existing permissions in this patch. Done, not sure how that happened.
https://codereview.chromium.org/1968083004/diff/200001/extensions/browser/api... File extensions/browser/api_test_utils.h (right): https://codereview.chromium.org/1968083004/diff/200001/extensions/browser/api... extensions/browser/api_test_utils.h:79: base::Value* RunFunctionWithDelegateAndReturnSingleResult( Adding this function avoids an unnecessary serialization round-trip. I'm fine with this round-trip, but I feel that avoiding it is a cleaner approach.
https://codereview.chromium.org/1968083004/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:45: extensions::events::QUICK_UNLOCK_PRIVATE_ON_ACTIVE_MODES_CHANGED, nit: no extensions:: prefix (applies to this whole file). https://codereview.chromium.org/1968083004/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:68: for (size_t i = 0; i < a.size(); ++i) { nitty nit: might be worth a comment: // This is a pretty inefficient comparison, but ModeLists are never going to be big enough to matter. https://codereview.chromium.org/1968083004/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:187: std::unique_ptr<SetModes::Params> params(SetModes::Params::Create(*args_)); We should store params as a member so we don't have to parse them twice (and don't need the comment). https://codereview.chromium.org/1968083004/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/1968083004/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:52: QuickUnlockPrivateSetModesFunction::SetCreateAuthenticatorForTesting( The create_authenticator_ variable is global, which means that it needs to be reset after each test. You could do this in TearDown(), or you could just expose the variable as a static member of the function so that you could use a base::AutoReset<CreateAuthenticator> as a member on this test. https://codereview.chromium.org/1968083004/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:140: auto serialized_modes = new base::ListValue(); nit: typically preferred style to enforce ownership from the start, so just put this in a unique_ptr immediately and std::move it below. https://codereview.chromium.org/1968083004/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:225: QuickUnlockPrivateSetModesFunction::SetModesChangedEventHandlerForTesting( Why do we need to do this everywhere? https://codereview.chromium.org/1968083004/diff/200001/extensions/browser/api... File extensions/browser/api_test_utils.cc (right): https://codereview.chromium.org/1968083004/diff/200001/extensions/browser/api... extensions/browser/api_test_utils.cc:162: base::Value* RunFunctionWithDelegateAndReturnSingleResult( If you want to add this, we should do a few things: - Have the other version call into this after parsing the args. - Change the return value to be a std::unique_ptr<> for this version. - Accept a scoped_refptr<> for the function in this version. https://codereview.chromium.org/1968083004/diff/200001/extensions/browser/ext... File extensions/browser/extension_function_histogram_value.h (right): https://codereview.chromium.org/1968083004/diff/200001/extensions/browser/ext... extensions/browser/extension_function_histogram_value.h:1197: QUICKUNLOCKPRIVATE_CHECKPASSWORD, Does this method exist?
https://codereview.chromium.org/1968083004/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:45: extensions::events::QUICK_UNLOCK_PRIVATE_ON_ACTIVE_MODES_CHANGED, On 2016/06/06 16:35:01, Devlin wrote: > nit: no extensions:: prefix (applies to this whole file). Done. https://codereview.chromium.org/1968083004/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:68: for (size_t i = 0; i < a.size(); ++i) { On 2016/06/06 16:35:01, Devlin wrote: > nitty nit: might be worth a comment: > // This is a pretty inefficient comparison, but ModeLists are never going to be > big enough to matter. Done. https://codereview.chromium.org/1968083004/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:187: std::unique_ptr<SetModes::Params> params(SetModes::Params::Create(*args_)); On 2016/06/06 16:35:01, Devlin wrote: > We should store params as a member so we don't have to parse them twice (and > don't need the comment). Done. https://codereview.chromium.org/1968083004/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/1968083004/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:52: QuickUnlockPrivateSetModesFunction::SetCreateAuthenticatorForTesting( On 2016/06/06 16:35:01, Devlin (slow until June 9) wrote: > The create_authenticator_ variable is global, which means that it needs to be > reset after each test. You could do this in TearDown(), or you could just > expose the variable as a static member of the function so that you could use a > base::AutoReset<CreateAuthenticator> as a member on this test. I've done this is TearDown(). https://codereview.chromium.org/1968083004/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:140: auto serialized_modes = new base::ListValue(); On 2016/06/06 16:35:01, Devlin (slow until June 9) wrote: > nit: typically preferred style to enforce ownership from the start, so just put > this in a unique_ptr immediately and std::move it below. Done. https://codereview.chromium.org/1968083004/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:225: QuickUnlockPrivateSetModesFunction::SetModesChangedEventHandlerForTesting( On 2016/06/06 16:35:01, Devlin (slow until June 9) wrote: > Why do we need to do this everywhere? You mean calling SetModesChangedEventHandlerForTesting? Because calling SetModes will trigger an event, and we want to verify the behavior of the event. I've added a helper method that makes this easier to read. https://codereview.chromium.org/1968083004/diff/200001/extensions/browser/api... File extensions/browser/api_test_utils.cc (right): https://codereview.chromium.org/1968083004/diff/200001/extensions/browser/api... extensions/browser/api_test_utils.cc:162: base::Value* RunFunctionWithDelegateAndReturnSingleResult( On 2016/06/06 16:35:01, Devlin (slow until June 9) wrote: > If you want to add this, we should do a few things: > - Have the other version call into this after parsing the args. > - Change the return value to be a std::unique_ptr<> for this version. > - Accept a scoped_refptr<> for the function in this version. I've made the other version call into this one. But why does this variant need to return a std::unique_ptr? Why does it need to accept a scoped_refptr? In particular, returning a std::unique_ptr will propagate to the other variants calling into this function, as we either need to unwrap the unique_ptr or change the interface. If the interface is updated, then the various call sites will have to be updated. At that point, the change should be done in a separate CL. https://codereview.chromium.org/1968083004/diff/200001/extensions/browser/ext... File extensions/browser/extension_function_histogram_value.h (right): https://codereview.chromium.org/1968083004/diff/200001/extensions/browser/ext... extensions/browser/extension_function_histogram_value.h:1197: QUICKUNLOCKPRIVATE_CHECKPASSWORD, On 2016/06/06 16:35:01, Devlin (slow until June 9) wrote: > Does this method exist? Oops - that was an old method. Removed.
https://codereview.chromium.org/1968083004/diff/200001/extensions/browser/api... File extensions/browser/api_test_utils.cc (right): https://codereview.chromium.org/1968083004/diff/200001/extensions/browser/api... extensions/browser/api_test_utils.cc:162: base::Value* RunFunctionWithDelegateAndReturnSingleResult( On 2016/06/08 18:39:10, jdufault wrote: > On 2016/06/06 16:35:01, Devlin (slow until June 9) wrote: > > If you want to add this, we should do a few things: > > - Have the other version call into this after parsing the args. > > - Change the return value to be a std::unique_ptr<> for this version. > > - Accept a scoped_refptr<> for the function in this version. > > I've made the other version call into this one. > > But why does this variant need to return a std::unique_ptr? Why does it need to > accept a scoped_refptr? > > In particular, returning a std::unique_ptr will propagate to the other variants > calling into this function, as we either need to unwrap the unique_ptr or change > the interface. If the interface is updated, then the various call sites will > have to be updated. At that point, the change should be done in a separate CL. The old versions of this *should* return a std::unique_ptr and accept a scoped_refptr to make everything clear. However, they don't, and I don't want you to have to update N callsites in this CL since it's wildly out of scope here. :) That said, we should stop the incorrect pattern (in which ownership is unclear) from propagating, so if we're adding something new, it should be correct. https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:30: QuickUnlockPrivateSetModesFunction::CreateAuthenticator create_authenticator_ = nit: this should be something like 'g_create_authenticator' - it's global, and not a private member of a class. https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:33: modes_changed_handler_ = nullptr; ditto https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:33: void EXPECT_MODES(const ModeList& x, const ModeList& y) { does EXPECT_EQ(x, y) not work? https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:79: ExtensionApiUnittest::TearDown(); nitty nit: typically prefer to call this after doing suite-specific tear down. It doesn't matter much in this case, but it could in the future and is generally good practice. https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:102: base::ListValue* list; nit: initialize all variables (everywhere) https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:103: EXPECT_TRUE(result->GetAsList(&list)); make this an assert, since you'll crash on line 104 if it's not. https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:104: for (base::ListValue::iterator i = list->begin(); i != list->end(); ++i) { nit: prefer 'for (const base::Value* value : list)' https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:115: auto result = RunFunction(new QuickUnlockPrivateGetActiveModesFunction(), I think auto here detracts from readability, since it isn't entirely clear what the result will be. https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:121: EXPECT_TRUE(result->GetAsList(&list)); same comments as above. https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:157: auto func_result = same comment as above re auto readability https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:212: SetModesChangedEventHandler([](const ModeList& modes) { FAIL(); }); I'm actually not sure if style allows lambdas to be used in this way. It seems to contradict the cases listed in https://chromium-cpp.appspot.com/. Do you know if we allow this type of usage?
https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:30: QuickUnlockPrivateSetModesFunction::CreateAuthenticator create_authenticator_ = On 2016/06/08 23:26:42, Devlin (slow until June 9) wrote: > nit: this should be something like 'g_create_authenticator' - it's global, and > not a private member of a class. Done. https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:33: modes_changed_handler_ = nullptr; On 2016/06/08 23:26:41, Devlin (slow until June 9) wrote: > ditto Done. https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:33: void EXPECT_MODES(const ModeList& x, const ModeList& y) { On 2016/06/08 23:26:42, Devlin (slow until June 9) wrote: > does EXPECT_EQ(x, y) not work? Done. https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:79: ExtensionApiUnittest::TearDown(); On 2016/06/08 23:26:42, Devlin (slow until June 9) wrote: > nitty nit: typically prefer to call this after doing suite-specific tear down. > It doesn't matter much in this case, but it could in the future and is generally > good practice. Done. https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:103: EXPECT_TRUE(result->GetAsList(&list)); On 2016/06/08 23:26:42, Devlin (slow until June 9) wrote: > make this an assert, since you'll crash on line 104 if it's not. Yes, ASSERT_TRUE is the right function to use here, but the ASSERT_* family causes compiler errors because the return type is not void. https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:104: for (base::ListValue::iterator i = list->begin(); i != list->end(); ++i) { On 2016/06/08 23:26:42, Devlin (slow until June 9) wrote: > nit: prefer 'for (const base::Value* value : list)' This isn't quite as nice as it would seem, because the iterated type is unique_ptr. I've converted it to a for-each loop where the unique_ptr value is consumed via a reference so that loop doesn't destroy every element in the collection. Each approach has downsides, imo. https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:115: auto result = RunFunction(new QuickUnlockPrivateGetActiveModesFunction(), On 2016/06/08 23:26:42, Devlin (slow until June 9) wrote: > I think auto here detracts from readability, since it isn't entirely clear what > the result will be. Done, I agree. https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:121: EXPECT_TRUE(result->GetAsList(&list)); On 2016/06/08 23:26:42, Devlin (slow until June 9) wrote: > same comments as above. Done. https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:157: auto func_result = On 2016/06/08 23:26:42, Devlin (slow until June 9) wrote: > same comment as above re auto readability Done. https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:212: SetModesChangedEventHandler([](const ModeList& modes) { FAIL(); }); On 2016/06/08 23:26:42, Devlin (slow until June 9) wrote: > I'm actually not sure if style allows lambdas to be used in this way. It seems > to contradict the cases listed in https://chromium-cpp.appspot.com/. Do you > know if we allow this type of usage? "Do not bind or store lambdas outside the lifetime of the stack frame they are defined in;" I think the helper method is ok because it's the call lives entirely within the stack frame. The lambda is stored in static-storage as a function pointer instead of as a base::Callback because base::Callback has a dtor. I think base::Callback is preferable here, though, if it were available. I can move the callback so it is an instance member of the function object, then I can convert the code to use base::Callback. I'm fine with either approach. Storing the callback on the function object will have a small implication on how the tests are written.
https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:212: SetModesChangedEventHandler([](const ModeList& modes) { FAIL(); }); On 2016/06/09 02:41:49, jdufault wrote: > On 2016/06/08 23:26:42, Devlin (slow until June 9) wrote: > > I'm actually not sure if style allows lambdas to be used in this way. It > seems > > to contradict the cases listed in https://chromium-cpp.appspot.com/. Do you > > know if we allow this type of usage? > > "Do not bind or store lambdas outside the lifetime of the stack frame they are > defined in;" > > I think the helper method is ok because it's the call lives entirely within the > stack frame. > > The lambda is stored in static-storage as a function pointer instead of as a > base::Callback because base::Callback has a dtor. I think base::Callback is > preferable here, though, if it were available. > > I can move the callback so it is an instance member of the function object, then > I can convert the code to use base::Callback. > > I'm fine with either approach. Storing the callback on the function object will > have a small implication on how the tests are written. base::Callback is actually overkill here, too - the alternative would be to just have an anonymous function that we take the address of, which avoids lifetime/dtor issues. It might be worth a message to cxx@ or chromium-dev@ to see if people have opinions on this type of lambda usage. The main concern with lambdas is capturing issues, which, since this deduces to a c-style function ptr, is guaranteed to be a non-issue. (That said, we don't always use that as enough reason in our style guide.)
Seems like you got some good feedback in cxx@ re the lambdas. Thanks for clearing that up! https://codereview.chromium.org/1968083004/diff/200001/extensions/browser/api... File extensions/browser/api_test_utils.cc (right): https://codereview.chromium.org/1968083004/diff/200001/extensions/browser/api... extensions/browser/api_test_utils.cc:162: base::Value* RunFunctionWithDelegateAndReturnSingleResult( On 2016/06/08 23:26:41, Devlin wrote: > On 2016/06/08 18:39:10, jdufault wrote: > > On 2016/06/06 16:35:01, Devlin (slow until June 9) wrote: > > > If you want to add this, we should do a few things: > > > - Have the other version call into this after parsing the args. > > > - Change the return value to be a std::unique_ptr<> for this version. > > > - Accept a scoped_refptr<> for the function in this version. > > > > I've made the other version call into this one. > > > > But why does this variant need to return a std::unique_ptr? Why does it need > to > > accept a scoped_refptr? > > > > In particular, returning a std::unique_ptr will propagate to the other > variants > > calling into this function, as we either need to unwrap the unique_ptr or > change > > the interface. If the interface is updated, then the various call sites will > > have to be updated. At that point, the change should be done in a separate CL. > > The old versions of this *should* return a std::unique_ptr and accept a > scoped_refptr to make everything clear. However, they don't, and I don't want > you to have to update N callsites in this CL since it's wildly out of scope > here. :) That said, we should stop the incorrect pattern (in which ownership is > unclear) from propagating, so if we're adding something new, it should be > correct. Looks like this still needs to be done? https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:103: EXPECT_TRUE(result->GetAsList(&list)); On 2016/06/09 02:41:49, jdufault wrote: > On 2016/06/08 23:26:42, Devlin (slow until June 9) wrote: > > make this an assert, since you'll crash on line 104 if it's not. > > Yes, ASSERT_TRUE is the right function to use here, but the ASSERT_* family > causes compiler errors because the return type is not void. if (!list) FAIL(); Does that work? https://codereview.chromium.org/1968083004/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:24: namespace { nitty nit: \n after this line https://codereview.chromium.org/1968083004/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:147: nit: if (params_->modes.empty()) return RespondNow(...) Everything would work, but it's nice to avoid the extra work. https://codereview.chromium.org/1968083004/diff/240001/chrome/common/extensio... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/1968083004/diff/240001/chrome/common/extensio... chrome/common/extensions/api/_api_features.json:707: ] this should be cros-only, right? (I think you specify it in the idl, but good to do so here, too. https://codereview.chromium.org/1968083004/diff/240001/chrome/common/extensio... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/1968083004/diff/240001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:17: // TODO: Add more quick unlock modes, such as a pattern unlock. nit: TODO(jdufault) https://codereview.chromium.org/1968083004/diff/240001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1968083004/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:72249: + <int value="1136" label="QUICKUNLOCKPRIVATE_CHECKPASSWORD"/> ?
https://codereview.chromium.org/1968083004/diff/200001/extensions/browser/api... File extensions/browser/api_test_utils.cc (right): https://codereview.chromium.org/1968083004/diff/200001/extensions/browser/api... extensions/browser/api_test_utils.cc:162: base::Value* RunFunctionWithDelegateAndReturnSingleResult( On 2016/06/13 14:50:23, Devlin wrote: > On 2016/06/08 23:26:41, Devlin wrote: > > On 2016/06/08 18:39:10, jdufault wrote: > > > On 2016/06/06 16:35:01, Devlin (slow until June 9) wrote: > > > > If you want to add this, we should do a few things: > > > > - Have the other version call into this after parsing the args. > > > > - Change the return value to be a std::unique_ptr<> for this version. > > > > - Accept a scoped_refptr<> for the function in this version. > > > > > > I've made the other version call into this one. > > > > > > But why does this variant need to return a std::unique_ptr? Why does it need > > to > > > accept a scoped_refptr? > > > > > > In particular, returning a std::unique_ptr will propagate to the other > > variants > > > calling into this function, as we either need to unwrap the unique_ptr or > > change > > > the interface. If the interface is updated, then the various call sites will > > > have to be updated. At that point, the change should be done in a separate > CL. > > > > The old versions of this *should* return a std::unique_ptr and accept a > > scoped_refptr to make everything clear. However, they don't, and I don't want > > you to have to update N callsites in this CL since it's wildly out of scope > > here. :) That said, we should stop the incorrect pattern (in which ownership > is > > unclear) from propagating, so if we're adding something new, it should be > > correct. > > Looks like this still needs to be done? I've changed the APIs. This propagated to a number of other calls as well, but the call sites didn't need to get changed. https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/1968083004/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:103: EXPECT_TRUE(result->GetAsList(&list)); On 2016/06/13 14:50:23, Devlin wrote: > On 2016/06/09 02:41:49, jdufault wrote: > > On 2016/06/08 23:26:42, Devlin (slow until June 9) wrote: > > > make this an assert, since you'll crash on line 104 if it's not. > > > > Yes, ASSERT_TRUE is the right function to use here, but the ASSERT_* family > > causes compiler errors because the return type is not void. > > if (!list) > FAIL(); > Does that work? Nope :( https://codereview.chromium.org/1968083004/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:24: namespace { On 2016/06/13 14:50:23, Devlin wrote: > nitty nit: \n after this line Done. https://codereview.chromium.org/1968083004/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:147: On 2016/06/13 14:50:23, Devlin wrote: > nit: > if (params_->modes.empty()) > return RespondNow(...) > > Everything would work, but it's nice to avoid the extra work. This would break disabling quick unlock. https://codereview.chromium.org/1968083004/diff/240001/chrome/common/extensio... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/1968083004/diff/240001/chrome/common/extensio... chrome/common/extensions/api/_api_features.json:707: ] On 2016/06/13 14:50:23, Devlin wrote: > this should be cros-only, right? (I think you specify it in the idl, but good > to do so here, too. Done. https://codereview.chromium.org/1968083004/diff/240001/chrome/common/extensio... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/1968083004/diff/240001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:17: // TODO: Add more quick unlock modes, such as a pattern unlock. On 2016/06/13 14:50:23, Devlin wrote: > nit: TODO(jdufault) Done. https://codereview.chromium.org/1968083004/diff/240001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1968083004/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:72249: + <int value="1136" label="QUICKUNLOCKPRIVATE_CHECKPASSWORD"/> On 2016/06/13 14:50:23, Devlin wrote: > ? Done.
lgtm with a few nits https://codereview.chromium.org/1968083004/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:213: EventRouter::Get(profile)->BroadcastEvent(std::move(event)); nit: just use browser_context() here. https://codereview.chromium.org/1968083004/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h (right): https://codereview.chromium.org/1968083004/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h:74: void SetAuthenticatorAllocatorForTesting(AuthenticatorAllocator allocator); const& on these callbacks now. https://codereview.chromium.org/1968083004/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/1968083004/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:295: } Test deactivating quick unlock? https://codereview.chromium.org/1968083004/diff/280001/extensions/browser/api... File extensions/browser/api_test_utils.cc (right): https://codereview.chromium.org/1968083004/diff/280001/extensions/browser/api... extensions/browser/api_test_utils.cc:172: return base::WrapUnique(single_result->DeepCopy()); nit: return single_result->CreateDeepCopy(); https://codereview.chromium.org/1968083004/diff/280001/extensions/browser/api... File extensions/browser/api_test_utils.h (right): https://codereview.chromium.org/1968083004/diff/280001/extensions/browser/api... extensions/browser/api_test_utils.h:68: std::unique_ptr<base::Value> RunFunctionWithDelegateAndReturnSingleResult( Nice! Thanks for doing this :)
https://codereview.chromium.org/1968083004/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:213: EventRouter::Get(profile)->BroadcastEvent(std::move(event)); On 2016/06/21 00:16:45, Devlin wrote: > nit: just use browser_context() here. Done. https://codereview.chromium.org/1968083004/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h (right): https://codereview.chromium.org/1968083004/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h:74: void SetAuthenticatorAllocatorForTesting(AuthenticatorAllocator allocator); On 2016/06/21 00:16:45, Devlin wrote: > const& on these callbacks now. Done. https://codereview.chromium.org/1968083004/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/1968083004/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:295: } On 2016/06/21 00:16:46, Devlin wrote: > Test deactivating quick unlock? Yep, that is done inside of 'SetModesAndGetActiveModes'. I've added a comment to that function and also added checks against PinStorage::IsPinSet. https://codereview.chromium.org/1968083004/diff/280001/extensions/browser/api... File extensions/browser/api_test_utils.cc (right): https://codereview.chromium.org/1968083004/diff/280001/extensions/browser/api... extensions/browser/api_test_utils.cc:172: return base::WrapUnique(single_result->DeepCopy()); On 2016/06/21 00:16:46, Devlin wrote: > nit: return single_result->CreateDeepCopy(); Done.
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/1968083004/#ps300001 (title: "Misc fixes")
The CQ bit was unchecked by jdufault@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1968083004/300001
jdufault@chromium.org changed reviewers: + mpearson@chromium.org
mpearson@: PTAL at the histogram changes. Thanks!
https://codereview.chromium.org/1968083004/diff/300001/extensions/browser/ext... File extensions/browser/extension_event_histogram_value.h (right): https://codereview.chromium.org/1968083004/diff/300001/extensions/browser/ext... extensions/browser/extension_event_histogram_value.h:425: // python tools/metrics/histograms/update_extension_histograms.py Did you forget to do this? https://codereview.chromium.org/1968083004/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1968083004/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:73723: + <int value="199" label="kQuickUnlockPrivate"/> I don't see this defined anywhere in this changelist. Did you forgot to add extensions/common/permissions/api_permission.h to this changelist?
Apologies for the late review. https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:14: namespace quick_unlock_private = extensions::api::quick_unlock_private; nit: Unnecessary if you put all of this inside namespace extensions { https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:19: using QuickUnlockMode = quick_unlock_private::QuickUnlockMode; nit: FWIW, I generally find using the full name more clear in the glue code to differentiate between the API namespace and the model namespace. https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:20: using ModeList = std::vector<QuickUnlockMode>; nit: for those of us unfamiliar with the code, UqickUnlockModeList would be more clear, even if it is more verbose. https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:29: "Currently at most one quick unlock mode can be active."; nit: Remove "Currently" https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:61: //////////// quickUnlockPrivate.getAvailableModes nit: Just '// quickUnlockPrivate.getAvailableModes' should be sufficient and is more common. https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:130: AddRef(); Why not make this an AsyncExtensionFunction and use RunAsync instead?
https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:130: AddRef(); On 2016/06/22 21:35:21, stevenjb wrote: > Why not make this an AsyncExtensionFunction and use RunAsync instead? NoNoNo. AsyncExtensionFunction uber deprecated and must die! Async/Sync extension functions are really silly idioms that don't make much sense, and don't allow for possibly returning synchronously/asynchronously (and cause tons of fun lifetime issues!). Vanilla ExtensionFunctions are much more clear what's going on.
https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:130: AddRef(); On 2016/06/22 21:47:04, Devlin wrote: > On 2016/06/22 21:35:21, stevenjb wrote: > > Why not make this an AsyncExtensionFunction and use RunAsync instead? > > NoNoNo. AsyncExtensionFunction uber deprecated and must die! > > Async/Sync extension functions are really silly idioms that don't make much > sense, and don't allow for possibly returning synchronously/asynchronously (and > cause tons of fun lifetime issues!). Vanilla ExtensionFunctions are much more > clear what's going on. I see. Thanks for correcting me. I find the explict AddRef / Release unfortunate, but listen to Devlin not to me :) We should try to aggressively eliminate AsyncExtensionFunction so that we don't continue to have bad examples in the code.
On 2016/06/22 22:02:19, stevenjb wrote: > https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... > File > chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc > (right): > > https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... > chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:130: > AddRef(); > On 2016/06/22 21:47:04, Devlin wrote: > > On 2016/06/22 21:35:21, stevenjb wrote: > > > Why not make this an AsyncExtensionFunction and use RunAsync instead? > > > > NoNoNo. AsyncExtensionFunction uber deprecated and must die! > > > > Async/Sync extension functions are really silly idioms that don't make much > > sense, and don't allow for possibly returning synchronously/asynchronously > (and > > cause tons of fun lifetime issues!). Vanilla ExtensionFunctions are much more > > clear what's going on. > > I see. Thanks for correcting me. I find the explict AddRef / Release > unfortunate, but listen to Devlin not to me :) > > We should try to aggressively eliminate AsyncExtensionFunction so that we don't > continue to have bad examples in the code. BTW, lgtm based on Devlin's comment with or without nits
https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:14: namespace quick_unlock_private = extensions::api::quick_unlock_private; On 2016/06/22 21:35:21, stevenjb wrote: > nit: Unnecessary if you put all of this inside namespace extensions { Done. https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:19: using QuickUnlockMode = quick_unlock_private::QuickUnlockMode; On 2016/06/22 21:35:20, stevenjb wrote: > nit: FWIW, I generally find using the full name more clear in the glue code to > differentiate between the API namespace and the model namespace. Keeping existing approach as discussed offline. https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:20: using ModeList = std::vector<QuickUnlockMode>; On 2016/06/22 21:35:21, stevenjb wrote: > nit: for those of us unfamiliar with the code, UqickUnlockModeList would be more > clear, even if it is more verbose. Done. https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:29: "Currently at most one quick unlock mode can be active."; On 2016/06/22 21:35:21, stevenjb wrote: > nit: Remove "Currently" Done. https://codereview.chromium.org/1968083004/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:61: //////////// quickUnlockPrivate.getAvailableModes On 2016/06/22 21:35:20, stevenjb wrote: > nit: Just '// quickUnlockPrivate.getAvailableModes' should be sufficient and is > more common. Done. https://codereview.chromium.org/1968083004/diff/300001/extensions/browser/ext... File extensions/browser/extension_event_histogram_value.h (right): https://codereview.chromium.org/1968083004/diff/300001/extensions/browser/ext... extensions/browser/extension_event_histogram_value.h:425: // python tools/metrics/histograms/update_extension_histograms.py On 2016/06/21 19:49:33, Mark P wrote: > Did you forget to do this? Done. https://codereview.chromium.org/1968083004/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1968083004/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:73723: + <int value="199" label="kQuickUnlockPrivate"/> On 2016/06/21 19:49:34, Mark P wrote: > I don't see this defined anywhere in this changelist. Did you forgot to add > extensions/common/permissions/api_permission.h > to this changelist? There used to be a permission; looks like I missed undoing this changed.
https://codereview.chromium.org/1968083004/diff/320001/extensions/browser/ext... File extensions/browser/extension_event_histogram_value.h (right): https://codereview.chromium.org/1968083004/diff/320001/extensions/browser/ext... extensions/browser/extension_event_histogram_value.h:425: // python tools/metrics/histograms/update_extension_histograms.py It still looks like you didn't do this.
https://codereview.chromium.org/1968083004/diff/320001/extensions/browser/ext... File extensions/browser/extension_event_histogram_value.h (right): https://codereview.chromium.org/1968083004/diff/320001/extensions/browser/ext... extensions/browser/extension_event_histogram_value.h:425: // python tools/metrics/histograms/update_extension_histograms.py On 2016/06/23 18:31:55, Mark P wrote: > It still looks like you didn't do this. That's really weird; I definitely ran it. I think I was in the middle of a rebase when I ran it though, so that might have caused the change to not get picked up.
histograms.xml lgtm
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org, rdevlin.cronin@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1968083004/#ps340001 (title: "Fix compile, rerun python snippet")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1968083004/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org, mpearson@chromium.org, rdevlin.cronin@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1968083004/#ps360001 (title: "Fix compile")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1968083004/360001
Message was sent while issue was closed.
Description was changed from ========== Implement the private API for quick unlock. The design doc is at go/cros-quick-unlock-design. BUG=603217 ========== to ========== Implement the private API for quick unlock. The design doc is at go/cros-quick-unlock-design. BUG=603217 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== Implement the private API for quick unlock. The design doc is at go/cros-quick-unlock-design. BUG=603217 ========== to ========== Implement the private API for quick unlock. The design doc is at go/cros-quick-unlock-design. BUG=603217 Committed: https://crrev.com/422a5b1d21c2f099e656d63050e1322245e8ed8e Cr-Commit-Position: refs/heads/master@{#401780} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/422a5b1d21c2f099e656d63050e1322245e8ed8e Cr-Commit-Position: refs/heads/master@{#401780} |
