Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2094333002:
Implementation for chrome.certificateProvider.requestPin/stopPinRequest (Closed)
|
Created:
4 years, 5 months ago by igorcov Modified:
4 years ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplementation for chrome.certificateProvider.requestPin/stopPinRequest
The IDL for proposed API has been reviewed: https://codereview.chromium.org/2174423002/
This is the actual implementation of the API. The dialog to request PIN is defined in request_pin_view.h
The CertificateProviderApi is receiving the requests from extensions and uses the service object to manage the view. CertificateProviderService object keeps the state of the view and forwards updates to it. And the RequestPinView is notifying the CertificateProviderApi about the user input through callback.
BUG=612886
Committed: https://crrev.com/bb898fb7cb786880d780d53c9d0d98ddeb37d491
Cr-Commit-Position: refs/heads/master@{#435620}
Patch Set 1 #Patch Set 2 : Full implementation #Patch Set 3 : Few changes for clarity #Patch Set 4 : Improved the design #Patch Set 5 : Tests added #Patch Set 6 : Removed unused code #Patch Set 7 : Small fix #
Total comments: 29
Patch Set 8 : Fixed small issues and review comments #Patch Set 9 : Improved design and fixed review comments #Patch Set 10 : Fixed the look of the textfield #Patch Set 11 : Fixed review comments #Patch Set 12 : Implementation after redesign done #Patch Set 13 : Fixed the browser tests #Patch Set 14 : Set requestPin view non-modal #Patch Set 15 : Non-modal dialog #Patch Set 16 : Small comment improvements #Patch Set 17 : Fixed flaky test #Patch Set 18 : Implemented the stopPinRequest functionality #
Total comments: 33
Patch Set 19 : Fixed review comments #Patch Set 20 : Small changes in view layout and messages #Patch Set 21 : Comment fixed #
Total comments: 43
Patch Set 22 : Fixed review comments #Patch Set 23 : Externalized back the constant #
Total comments: 58
Patch Set 24 : Implemented review comments #
Total comments: 2
Patch Set 25 : Implemented review comments #
Total comments: 12
Patch Set 26 : Implemented review suggestions #Patch Set 27 : Converted locked_callback into a delegate to make the code easier to understand #Patch Set 28 : Fixed the double close notifications #Patch Set 29 : Fixed the comments #Patch Set 30 : Fixed attemptsLeft with no error case #
Total comments: 8
Patch Set 31 : Fixed the destructor #Patch Set 32 : Fixed review comments #Patch Set 33 : Changed Delegate class name #Patch Set 34 : Fixed review comments #
Total comments: 35
Patch Set 35 : Fixed review comments #Patch Set 36 : Fixed constant name #
Total comments: 21
Patch Set 37 : Fixed review comments and merged #
Total comments: 66
Patch Set 38 : Fixed review comments #
Total comments: 2
Patch Set 39 : small nits #Patch Set 40 : small nits #Patch Set 41 : Compile error fixed #Patch Set 42 : Fixed compile error #Patch Set 43 : Fixed review comments #
Total comments: 2
Patch Set 44 : Fixed compile error and removed duplicate method #
Total comments: 16
Patch Set 45 : Fixed review comments #
Total comments: 59
Patch Set 46 : Fixed review comments #
Total comments: 35
Patch Set 47 : Fixed review comments #Patch Set 48 : Fixed review comments #Patch Set 49 : Fixed review comments #Patch Set 50 : Improved the browser tests #
Total comments: 12
Patch Set 51 : Fixed review comments #Patch Set 52 : Merged the sources #Messages
Total messages: 88 (44 generated)
igorcov@chromium.org changed reviewers: + emaxx@chromium.org
Please review the changes
I've reviewed only a part of the CL yet, but published the comments for it for not slowing down the iterations. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/options/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.cc:37: : callback_(callback), weak_ptr_factory_(this) { It's a bit unsafe to keep all data members uninitialized (pointers and booleans). Even though they are probably all initialized by the methods called below, it's too easy to make a mistake. The simplest way would be use member initializer lists inside the class declaration. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.cc:45: RequestPinView::~RequestPinView() {} Looks like this can be omitted. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.cc:66: if (callback_.is_null()) { Shouldn't this be a DCHECK? https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.cc:67: LOG(ERROR) << "Something broke the flow, the dialog will be closed"; The message looks quite informal. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.cc:166: callback_.Reset(); In which scenario is it fine to just drop the callback without calling it? https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/options/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. The location under the options/ directory doesn't seem right for this file. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:31: using UserInputCallback = base::Callback<void(const base::string16&)>; nit: include "base/callback.h" https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:31: using UserInputCallback = base::Callback<void(const base::string16&)>; The name is not exactly correct, as the same callback is used in other scenarios, like just closing the dialog. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:32: explicit RequestPinView(const std::string& extension_name, nit: "explicit" is not necessary https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:35: const bool accept_input, nit: const-ness is not usually used with arguments of primitive types https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:51: void SetExtensionName(const std::string& extension_name); Maybe merge all these methods into a single method (e.g. Update/Restart/Reinit), that would accept and set all fields (just like constructor)? Because anyway the dialog needs to be updated fully on each repetitive request. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:66: views::Textfield* passphrase_textfield_; This name is a bit misleading: there is no passphrase in the PIN dialog. I think it's fine just to name it "textfield_" for now. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:68: views::Label* dialog_type_label_; "Dialog type" is a bit vague definition; I think it's clearer to point to the fact that it's the textfield label. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:70: bool accept_input_; Have I understood it correctly that when waiting_for_request_==true, that the user input won't be accepted regardless of the accept_input_ value? Then the name is misleading. Maybe "show_input_field_"? https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:71: // Flag to know when Chrome submited input to extension and is waiting for new Better to rephrase this comment: first, "Chrome" gives no actual context here, and, second, mentioning "extension" here breaks the code isolation a bit. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:73: bool waiting_for_request_; I'm also thinking about what could be a more telling name here... As it's not clear which "request" is meant here. Maybe "disabled_" or "is_blocked_"?
https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/options/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.cc:45: RequestPinView::~RequestPinView() {} On 2016/07/08 02:00:14, emaxx wrote: > Looks like this can be omitted. Compiler says it can't. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.cc:66: if (callback_.is_null()) { On 2016/07/08 02:00:14, emaxx wrote: > Shouldn't this be a DCHECK? Done. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.cc:166: callback_.Reset(); On 2016/07/08 02:00:14, emaxx wrote: > In which scenario is it fine to just drop the callback without calling it? It's the case when user submitted some input and the view is waiting for next request. At this point the old callback is pointing to the service::OnFlowInterrupted and is supposed to be used only in case user closed the dialog while request was processing. In this particular case it wasn't used and can be safely dropped. Will add a comment there. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/options/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/07/08 02:00:15, emaxx wrote: > The location under the options/ directory doesn't seem right for this file. Moved to ui folder. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:31: using UserInputCallback = base::Callback<void(const base::string16&)>; On 2016/07/08 02:00:15, emaxx wrote: > nit: include "base/callback.h" Done. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:32: explicit RequestPinView(const std::string& extension_name, On 2016/07/08 02:00:15, emaxx wrote: > nit: "explicit" is not necessary Done. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:35: const bool accept_input, Done
Implemented also the functionality when a dialog is open already and extension makes another request (assuming the open dialog belongs to extension). The process is: If extension asks to close the dialog, it will be closed and callback used. If extension asks to open another dialog, the request will be rejected. When extension wants to update the existing dialog, it has to request to close it and to open the new dialog after that. The closing of dialog by extension also counts towards quota limiting imposed per minute. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/options/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:31: using UserInputCallback = base::Callback<void(const base::string16&)>; On 2016/07/08 02:00:15, emaxx wrote: > nit: include "base/callback.h" Done. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:51: void SetExtensionName(const std::string& extension_name); On 2016/07/08 02:00:15, emaxx wrote: > Maybe merge all these methods into a single method (e.g. Update/Restart/Reinit), > that would accept and set all fields (just like constructor)? Because anyway the > dialog needs to be updated fully on each repetitive request. Done. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:66: views::Textfield* passphrase_textfield_; On 2016/07/08 02:00:14, emaxx wrote: > This name is a bit misleading: there is no passphrase in the PIN dialog. > I think it's fine just to name it "textfield_" for now. Done. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:68: views::Label* dialog_type_label_; On 2016/07/08 02:00:15, emaxx wrote: > "Dialog type" is a bit vague definition; I think it's clearer to point to the > fact that it's the textfield label. Done. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:70: bool accept_input_; On 2016/07/08 02:00:14, emaxx wrote: > Have I understood it correctly that when waiting_for_request_==true, that the > user input won't be accepted regardless of the accept_input_ value? Then the > name is misleading. > Maybe "show_input_field_"? Done. https://codereview.chromium.org/2094333002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/options/request_pin_view.h:73: bool waiting_for_request_; On 2016/07/08 02:00:15, emaxx wrote: > I'm also thinking about what could be a more telling name here... As it's not > clear which "request" is meant here. > Maybe "disabled_" or "is_blocked_"? Done.
Description was changed from ========== Implementation for chrome.certificateProvider.showPinDialog BUG=612886 ========== to ========== Implementation for chrome.certificateProvider.requestPin/stopPinRequest The IDL for proposed API has been reviewed: https://codereview.chromium.org/2174423002/ This is the actual implementation of the API. The dialog to request PIN is defined in request_pin_view.h The CertificateProviderApi is receiving the requests from extensions and uses the service object to manage the view. CertificateProviderService object keeps the state of the view and forwards updates to it. And the RequestPinView is notifying the CertificateProviderApi about the user input through callback. BUG=612886 ==========
igorcov@google.com changed reviewers: + stevenjb@chromium.org
igorcov@google.com changed reviewers: + igorcov@google.com
stevenjb - PTAL at chrome/browser/chromeos/*
https://codereview.chromium.org/2094333002/diff/340001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2094333002/diff/340001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6901: "$1" is requesting the code This needs an example (<ex>). Also, "requesting the code" is not very meaningful, we should use a more specific message (or messages). I'm also not sure if we want "" around $1 (I assume that is the extension name?). Are there UX mocks for this? I don't see anything linked to the issue. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc (right): https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:315: AddSignRequestId(123); What is 123? This should be a constant. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:428: active_pin_dialog_ = nullptr; return and elim else https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:523: } Is this still successful if there is no browser? https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service.h (right): https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service.h:181: // the specific error is returned. Describe each of the input parameters, many are unclear. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service.h:202: views::Widget* active_window_for_testing() { return active_window_; } This is a lot of UI specific code added to a class that currently doesn't have any real src/chrome dependencies and should probably get moved to src/extensions. We should create a separate class for managing pin dialog requests. That way in the future we can set up a delegate interface between this class and the pin dialog implementation. (No need to create the delegate for now since we don't currently need it). This will also keep the classes smaller and more maintainable. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc (right): https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:121: } Why use random numbers here instead of an increasing static int which would have no chance of repeating? https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:145: textfield_label_->SetText(base::ASCIIToUTF16(dialog_type)); Where does |dialog_type| come from? It would be better to use an enum and have the localization happen here. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:225: error_label_->SetVisible(false); return; no else https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:32: using RequestPinCallback = base::Callback<void(const base::string16&)>; Document the callback arg https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:37: const RequestPinCallback& callback); Document the input params https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:52: bool IsLocked(); Document new public methods https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:58: bool accept_input); Avoid multiple methods with the same name, it is confusing and error prone. |dialog_type| (which needs to be documented) can always be passed as an empty string or "". https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:174: } This is UI code, it shouldn't be in the API. We should pass api_cp::PinRequestErrorType to the UI (or convert it to an enum defined in the UI code), and that should do the localization. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:202: } else { no else https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:211: } else { invert, no else https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc (right): https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:46: #include "ui/views/widget/widget.h" The API test shouldn't also test the UI, we should mock that out instead. The best way to do that is to create a delegate interface that the UI can implement, that way the extension code doesn't need to know anything about the UI code.
stevenjb: Thanks for your feedback. I've changed the code according to your observations. I still have to decide on how to change the browser tests, but for the rest of the code please feel free to provide more input. https://codereview.chromium.org/2094333002/diff/340001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2094333002/diff/340001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6901: "$1" is requesting the code On 2016/08/09 21:04:38, stevenjb wrote: > This needs an example (<ex>). Also, "requesting the code" is not very > meaningful, we should use a more specific message (or messages). I'm also not > sure if we want "" around $1 (I assume that is the extension name?). Are there > UX mocks for this? I don't see anything linked to the issue. > The "" around came from UI team, and $1 is the name of extension. The text is now changed according to what dskaram agreed with UI team. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc (right): https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:315: AddSignRequestId(123); On 2016/08/09 21:04:38, stevenjb wrote: > What is 123? This should be a constant. Sorry, I forgot to remove it. It was added to be able to test manually, but shouldn't be present in the code. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:428: active_pin_dialog_ = nullptr; On 2016/08/09 21:04:39, stevenjb wrote: > return and elim else Done. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:523: } On 2016/08/09 21:04:38, stevenjb wrote: > Is this still successful if there is no browser? Didn't test it yet, but should be. It's the code taken from WifiConfigView usage that is present on login screen. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service.h (right): https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service.h:181: // the specific error is returned. On 2016/08/09 21:04:39, stevenjb wrote: > Describe each of the input parameters, many are unclear. Done. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service.h:202: views::Widget* active_window_for_testing() { return active_window_; } On 2016/08/09 21:04:39, stevenjb wrote: > This is a lot of UI specific code added to a class that currently doesn't have > any real src/chrome dependencies and should probably get moved to > src/extensions. > > We should create a separate class for managing pin dialog requests. That way in > the future we can set up a delegate interface between this class and the pin > dialog implementation. (No need to create the delegate for now since we don't > currently need it). This will also keep the classes smaller and more > maintainable. Created the PinDialogManager class to manage the dialog implementation. CertificateProviderService thus acts as a proxy providing the PinDialogManager to the parties that need it (currently CertificateProviderApi and CertificateProviderServiceFactory) https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc (right): https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:121: } On 2016/08/09 21:04:39, stevenjb wrote: > Why use random numbers here instead of an increasing static int which would have > no chance of repeating? > Security reasons. If an extension would know what's the next ID that will be given it could spam requests with that ID. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:145: textfield_label_->SetText(base::ASCIIToUTF16(dialog_type)); On 2016/08/09 21:04:39, stevenjb wrote: > Where does |dialog_type| come from? It would be better to use an enum and have > the localization happen here. Done. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:225: error_label_->SetVisible(false); On 2016/08/09 21:04:39, stevenjb wrote: > return; no else Done. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:32: using RequestPinCallback = base::Callback<void(const base::string16&)>; On 2016/08/09 21:04:39, stevenjb wrote: > Document the callback arg Done. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:37: const RequestPinCallback& callback); On 2016/08/09 21:04:39, stevenjb wrote: > Document the input params Done. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:52: bool IsLocked(); On 2016/08/09 21:04:39, stevenjb wrote: > Document new public methods Done. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:58: bool accept_input); On 2016/08/09 21:04:39, stevenjb wrote: > Avoid multiple methods with the same name, it is confusing and error prone. > |dialog_type| (which needs to be documented) can always be passed as an empty > string or "". Done. I've changed the dialog_type to an enum, as I don't like passing strings around. Hopefully it's more clear now. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:174: } On 2016/08/09 21:04:39, stevenjb wrote: > This is UI code, it shouldn't be in the API. We should pass > api_cp::PinRequestErrorType to the UI (or convert it to an enum defined in the > UI code), and that should do the localization. Done. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:202: } else { On 2016/08/09 21:04:39, stevenjb wrote: > no else Done. https://codereview.chromium.org/2094333002/diff/340001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:211: } else { On 2016/08/09 21:04:39, stevenjb wrote: > invert, no else Done.
I didn't get a chance to look too closely at the api, view code, or tests, I will try to do that tomorrow. https://codereview.chromium.org/2094333002/diff/400001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2094333002/diff/400001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6901: "$1" is requesting your $2 We still need examples: "$1<ex>My Extension</ex>" is requesting your $2<ex>An example of whatever this is?</ex> Examples are critical to good translation. Knowing $1 is an extension name might affect how the translation is structured (e.g. 'is' vs. 'are'). https://codereview.chromium.org/2094333002/diff/400001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6919: Attempts left: $1 Note: This is fine without an example since it is clear that it is just a number. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service.h (right): https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service.h:199: PinDialogManager pin_dialog_manager_; unique_ptr<> is better for embedding classes, e.g. it makes the code easier to change if we switch to a delegate later on. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc (right): https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:115: api_cp::SignRequest request; We should comment why we use a random number here, e.g. // Generate a random request id so that a malicious extension can not guess a valid id and spam the user. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc (right): https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:32: active_pin_dialog_ = nullptr; Leaky? https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:48: return last_rejected_[extension_id]; We should probably actually name this last_response_closed_. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:83: // the signRequestId check as it's possibly expired. This is confusing and I'm not sure I understand it. Can you clarify the logic below and place smaller comments appropriately, e.g: if (active_pin_dialog_ != nullptr) { DCHECK(!active_dialog_extension_id_.empty()); // That should never happen, right? if (extension_id != active_dialog_extension_id_) return RequestPinResponse::OTHER_FLOW_IN_PROGRESS; if (active_pin_dialog_->IsLocked()) { // Don't create a new dialog, instead update the callback and dialog parameters ... return RequestPinResponse::SUCCESS; } else { ??? If the dialog is not locked do we actually want to return OTHER_FLOW_IN_PROGRESS? Regardless we should add a comment to explain this state } } https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:105: int result = gettimeofday(&tv, NULL); nullptr throughout https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:127: } We should test and comment what happens if there is no browser here. As near as I can tell we always call window->Show() for WifiConfigView. Where exactly is the example you were following? https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:145: active_pin_dialog_ = nullptr; If active_pin_dialog_ is owned by acitve_window_, we need to comment that clearly where active_pin_dialog_ is declared and add a similar comment above where active_pin_dialog_ is set to null. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h (right): https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:28: // This callback function is called by the view when user closes the PIN s/This callback function is called/Called/ https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:61: // Returns SUCCESS if the dialog is displayed and extension owns it. Otherwise Can you break this up to make it more readable, e.g.: // |extension_id| - The ID of the extension requesting the dialog. // |extension_name| - The name of the extension. // |sign_request_id| - The ID given by Chrome when the extension was asked // to sign the datat. Should be a valid, not expired ID at the time the // extension was asked to sign the data... etc. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:72: // This function is called when extension calls the stopPinRequest method. s/This function is called when/Called when/ https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:93: std::map<std::string, bool> last_rejected_; last_response_rejected_ https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:97: std::map<uint64_t, uint64_t> sign_request_ids_; 'sign_request_ids_' implies this is a set of ids. This should be named something like sign_request_times_. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:101: chromeos::RequestPinView* active_pin_dialog_ = nullptr; Can we use a unique_ptr here? The code as written appears to be leaky based on the comment. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:107: nit: The extra blank lines inside a switch are a little confusing, I would remove them. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:52: // |callback| is used to send the value of the PIN/PUK the user entered. Also here, separate the input parameter descriptions to make this more readable. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:85: // to provide input. Here too. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:270: chromeos::RequestPinResponse success = s/success/result or response/ https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:281: case chromeos::RequestPinResponse::SUCCESS: return RespondLater(); Two lines (unless clang format did this, in which case, grr, but oh well). https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h (right): https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h:50: extern const int MAX_CLOSED_DIALOGS_PER_10_MINUTES; Does this need to be exposed publicly?
https://codereview.chromium.org/2094333002/diff/400001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2094333002/diff/400001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6901: "$1" is requesting your $2 On 2016/08/11 01:58:54, stevenjb wrote: > We still need examples: > "$1<ex>My Extension</ex>" is requesting your $2<ex>An example of whatever this > is?</ex> > > Examples are critical to good translation. Knowing $1 is an extension name might > affect how the translation is structured (e.g. 'is' vs. 'are'). Done. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service.h (right): https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service.h:199: PinDialogManager pin_dialog_manager_; On 2016/08/11 01:58:54, stevenjb wrote: > unique_ptr<> is better for embedding classes, e.g. it makes the code easier to > change if we switch to a delegate later on. Done. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc (right): https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:115: api_cp::SignRequest request; On 2016/08/11 01:58:54, stevenjb wrote: > We should comment why we use a random number here, e.g. > // Generate a random request id so that a malicious extension can not guess a > valid id and spam the user. > Done. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc (right): https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:32: active_pin_dialog_ = nullptr; On 2016/08/11 01:58:54, stevenjb wrote: > Leaky? No. The active_window_ manages that object, sorry for confusion from the bad comment in header file. Added a comment here too. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:48: return last_rejected_[extension_id]; On 2016/08/11 01:58:54, stevenjb wrote: > We should probably actually name this last_response_closed_. Done. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:83: // the signRequestId check as it's possibly expired. On 2016/08/11 01:58:54, stevenjb wrote: > This is confusing and I'm not sure I understand it. Can you clarify the logic > below and place smaller comments appropriately, e.g: > > if (active_pin_dialog_ != nullptr) { > DCHECK(!active_dialog_extension_id_.empty()); // That should never happen, > right? > if (extension_id != active_dialog_extension_id_) > return RequestPinResponse::OTHER_FLOW_IN_PROGRESS; > if (active_pin_dialog_->IsLocked()) { > // Don't create a new dialog, instead update the callback and dialog > parameters > ... > return RequestPinResponse::SUCCESS; > } else { > ??? If the dialog is not locked do we actually want to return > OTHER_FLOW_IN_PROGRESS? Regardless we should add a comment to explain this state > } > } Added another state in the enum, for the case when dialog is not locked. This should help the extension developers debug if a problem occurs. Also changed the code structure and comments. Thanks. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:105: int result = gettimeofday(&tv, NULL); On 2016/08/11 01:58:54, stevenjb wrote: > nullptr throughout Done. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:127: } On 2016/08/11 01:58:55, stevenjb wrote: > We should test and comment what happens if there is no browser here. As near as > I can tell we always call window->Show() for WifiConfigView. Where exactly is > the example you were following? I took the code from here: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/options/network_... But looking where it is called, https://cs.chromium.org/chromium/src/chrome/browser/chromeos/options/network_... you are right that the window is always showing. I've changed the code to have the same behavior. Thanks. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:145: active_pin_dialog_ = nullptr; On 2016/08/11 01:58:54, stevenjb wrote: > If active_pin_dialog_ is owned by acitve_window_, we need to comment that > clearly where active_pin_dialog_ is declared and add a similar comment above > where active_pin_dialog_ is set to null. Done. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h (right): https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:28: // This callback function is called by the view when user closes the PIN On 2016/08/11 01:58:55, stevenjb wrote: > s/This callback function is called/Called/ Done. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:61: // Returns SUCCESS if the dialog is displayed and extension owns it. Otherwise On 2016/08/11 01:58:55, stevenjb wrote: > Can you break this up to make it more readable, e.g.: > > // |extension_id| - The ID of the extension requesting the dialog. > // |extension_name| - The name of the extension. > // |sign_request_id| - The ID given by Chrome when the extension was asked > // to sign the datat. Should be a valid, not expired ID at the time the > // extension was asked to sign the data... > > etc. Done. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:72: // This function is called when extension calls the stopPinRequest method. On 2016/08/11 01:58:55, stevenjb wrote: > s/This function is called when/Called when/ Done. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:93: std::map<std::string, bool> last_rejected_; On 2016/08/11 01:58:55, stevenjb wrote: > last_response_rejected_ Done. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:97: std::map<uint64_t, uint64_t> sign_request_ids_; On 2016/08/11 01:58:55, stevenjb wrote: > 'sign_request_ids_' implies this is a set of ids. This should be named something > like sign_request_times_. Done. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:101: chromeos::RequestPinView* active_pin_dialog_ = nullptr; On 2016/08/11 01:58:55, stevenjb wrote: > Can we use a unique_ptr here? The code as written appears to be leaky based on > the comment. > I'm sorry, the comment is wrong here. It is managed by |active_window_| and destroyed there when the window is closed. Specifically it is destroyed at https://cs.chromium.org/chromium/src/ui/views/window/dialog_delegate.cc?sq=pa... We have to keep it as a raw pointer. I've fixed the comment. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:107: On 2016/08/11 01:58:55, stevenjb wrote: > nit: The extra blank lines inside a switch are a little confusing, I would > remove them. Done. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:52: // |callback| is used to send the value of the PIN/PUK the user entered. On 2016/08/11 01:58:55, stevenjb wrote: > Also here, separate the input parameter descriptions to make this more readable. Done. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:85: // to provide input. On 2016/08/11 01:58:55, stevenjb wrote: > Here too. Done. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:270: chromeos::RequestPinResponse success = On 2016/08/11 01:58:55, stevenjb wrote: > s/success/result or response/ Done. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:281: case chromeos::RequestPinResponse::SUCCESS: return RespondLater(); On 2016/08/11 01:58:55, stevenjb wrote: > Two lines (unless clang format did this, in which case, grr, but oh well). It was me. Fixed. https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h (right): https://codereview.chromium.org/2094333002/diff/400001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h:50: extern const int MAX_CLOSED_DIALOGS_PER_10_MINUTES; On 2016/08/11 01:58:55, stevenjb wrote: > Does this need to be exposed publicly? It's used in tests.
I still didn't look too closely at the tests, it would be great if emaxx@ or another team member could also take a look at this. I still have a couple of concerns, but generally this is looking pretty good. Thanks for your patience. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:42: base::Bind(&PinDialogManager::OnFlowInterrupted, base::Unretained(this))); base::Unretained is unsafe here. The view (dialog) is not owned by this class, so on shutdown this could be destroyed while a callback is queued, creating an unlikely but subtle shutdown crash. Also see my other comment, the notion of a 'temporary callback' is confusing. What happens to the original callback and why do we no longer care about it? https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:116: return RequestPinResponse::INVALID_ID; {} https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:23: // Manages the state of the dialog that requests the PIN from user. We should probably also describe the feature a bit more here, since this is the primary class managing it. Nothing too long but just mention that this is used by extensions that need to request a pin. Just enough to answer "why does this class/dialog exist?" Also maybe reference a tracking issue, e.g. crbug.com/1234. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:76: std::unique_ptr<int> attempts_left, Digging through the code, I don't see any reason for this to be a pointer instead of just an integer. We can use -1 to indicate an unspecified value, i.e. infinite attempts left. Using a pointer suggests that we might be referencing it somewhere else, which should not be the case. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:56: if (!callback_.is_null()) { nit: I am pretty sure that you can just use !callback_ instead of is_null throughout. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:80: SetAcceptInput(false); The dialog can still be closed with SetAcceptInput(false), correct? We should document that in the header comment for 'locked_' and maybe here also. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:128: DCHECK(locked_ || callback_.is_null()); This is confusing and seems fragile. While it may be safe, I can not quickly convince myself that it is so. I think we would be better served with two callbacks rather than re-purposing a single callback but only under certain conditions. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:143: code_type_ = base::ASCIIToUTF16("PIN"); These need to be localized strings. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:38: // A dialog box for requesting PIN code. Mention that instances of this class are managed by PinDialogManager. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:58: std::unique_ptr<int> attempts_left, See note in PinDialogManager https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:93: std::unique_ptr<int> attempts_left, Ditto https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:167: } NOTREACHED(); return chromeos::RequestPinErrorType::NONE; (I'm surprised this compiles). https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:255: params->details.request_type : Shouldn't this be *params->details.request_type ??
I've created a separate callback for locked view and implemented the code so that it is always used. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:42: base::Bind(&PinDialogManager::OnFlowInterrupted, base::Unretained(this))); On 2016/08/11 18:12:02, stevenjb wrote: > base::Unretained is unsafe here. The view (dialog) is not owned by this class, > so on shutdown this could be destroyed while a callback is queued, creating an > unlikely but subtle shutdown crash. > > Also see my other comment, the notion of a 'temporary callback' is confusing. > What happens to the original callback and why do we no longer care about it? Included in destructor of the class to close the active dialog if present. I've also implemented your suggestion to have two separate callbacks, and changed code so that temporary callback is always called. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:116: return RequestPinResponse::INVALID_ID; On 2016/08/11 18:12:02, stevenjb wrote: > {} Done. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:23: // Manages the state of the dialog that requests the PIN from user. On 2016/08/11 18:12:02, stevenjb wrote: > We should probably also describe the feature a bit more here, since this is the > primary class managing it. Nothing too long but just mention that this is used > by extensions that need to request a pin. Just enough to answer "why does this > class/dialog exist?" Also maybe reference a tracking issue, e.g. crbug.com/1234. Done. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:76: std::unique_ptr<int> attempts_left, On 2016/08/11 18:12:02, stevenjb wrote: > Digging through the code, I don't see any reason for this to be a pointer > instead of just an integer. We can use -1 to indicate an unspecified value, i.e. > infinite attempts left. Using a pointer suggests that we might be referencing it > somewhere else, which should not be the case. Yes, the possibility for the parameter to be missing is the only reason I've used unique_ptr. Changed to int with comment that -1 means the value is missing. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:56: if (!callback_.is_null()) { On 2016/08/11 18:12:02, stevenjb wrote: > nit: I am pretty sure that you can just use !callback_ instead of is_null > throughout. I can't because the callback_ object is always there. It's not a pointer. Methods is_null() and Reset() have to be used to change the internal state of the object. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:38: // A dialog box for requesting PIN code. On 2016/08/11 18:12:02, stevenjb wrote: > Mention that instances of this class are managed by PinDialogManager. Done. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:58: std::unique_ptr<int> attempts_left, On 2016/08/11 18:12:02, stevenjb wrote: > See note in PinDialogManager Done. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:93: std::unique_ptr<int> attempts_left, On 2016/08/11 18:12:02, stevenjb wrote: > Ditto Done. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:167: } On 2016/08/11 18:12:02, stevenjb wrote: > NOTREACHED(); > return chromeos::RequestPinErrorType::NONE; > > (I'm surprised this compiles). It compiles because all the possible values from enum (api_cp::PinRequestErrorType) are checked in the switch and for every branch there's a return statement. So it knows there's no way to get out of that block. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:255: params->details.request_type : On 2016/08/11 18:12:02, stevenjb wrote: > Shouldn't this be *params->details.request_type ?? params is a unique_ptr and request_type from IDL is optional parameter of type PinRequestType which is an enum. No reason for *.
Left the comments. I didn't look into the tests yet though. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:118: uint64_t sign_request_id = base::RandUint64(); There is already another "sign_request_id" in the existing code that is generated by CertificateProviderService and passed to this function - here it is named as "request_id". Can we reuse it instead of introducing a new one identifier? There is one difference between them: the existing ids are generated simply by increasing a counter instead of the random used here (see the generation code here: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/certificate_prov... ). But I think it should be fine to modify the SignRequests class implementation accordingly. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:118: uint64_t sign_request_id = base::RandUint64(); I'm afraid we can put the client extensions into a bad situation by generating the number that cannot be stored as a JavaScript number precisely. My understanding is that the JavaScript number can store precisely 53-bit integers, but I may be wrong here (and in any way hardcoding this here would be quite strange). So maybe, for simplicity reasons, we should generate the numbers that fit into the usual 4-byte int range. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:20: // The limit of the sign_request_id map size, after which the sanitizing process This looks a bit hacky, and may be not needed at all if the code would be changed to use the storage in the SignRequests class (which always has the up-to-date information regarding the currently running requests). https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:112: int result = gettimeofday(&tv, nullptr); The same question as in the header - would it be possible to use the convenient utils provided at base/time/? https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:16: enum RequestPinResponse { It's better to either make this "enum class" or move it inside the PinDialogManager class (probably, the latter is better). The current definition effectively puts the symbols into the chromeos namespace with too generic names (e.g. chromeos::SUCCESS). https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:27: virtual ~PinDialogManager(); There are no child classes of this class currently, am I correct? Then instead of this precautious move (I mean adding an empty virtual destructor) you could just mark the class as "final" now. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:31: void OnPinDialogInput(const std::string& extension_id, const bool closed); nit: regarding "const bool", "const long long" etc. - I believe usually in Chromium code base the primitive types are passed just by value. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:36: void OnFlowInterrupted(const base::string16& value); nit: #include "base/strings/string16.h" https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:36: void OnFlowInterrupted(const base::string16& value); Would it be easy to pass the extension_id here too? Currently, the API looks a bit surprising as all other functions accept the extension_id, but here it's omitted (and is obtained implicitly). https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:50: // Creates a new RequestPinView object and displays it in a dialog or reuses nit: Maybe it would make sense to reorder the method declarations, so that they follow the typical work flow (more or less). E.g. first AddSignRequestId, then ShowPingDialog, etc. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:93: views::Widget* active_window_for_testing() { return active_window_; } nit: #include "ui/views/widget/widget.h". https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:100: std::map<std::string, bool> last_response_closed_; nit: #include <map> (BTW, maybe use std::unordered_map? Not insisting, but I think that using std::map may make somebody think there's some dependency on the order of the elements.) https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:104: std::map<uint64_t, uint64_t> sign_request_times_; Is there a reason to use uint64_t for storing the time instead of base::Time? https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:112: base::WeakPtrFactory<PinDialogManager> weak_factory_; nit: #include "base/memory/weak_ptr.h". https://codereview.chromium.org/2094333002/diff/460001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service_unittest.cc (right): https://codereview.chromium.org/2094333002/diff/460001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service_unittest.cc:490: ASSERT_FALSE(closed); nit: It's usually preferable to choose the EXPECT_* version unless there is a reason to do the ASSERT (which happens, for example, when the following lines may crash otherwise). https://codereview.chromium.org/2094333002/diff/460001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/460001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:126: // CertificateProviderService::OnFlowInterrupted. It is suposed to be used nit: s/suposes/supposed/ https://codereview.chromium.org/2094333002/diff/480001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2094333002/diff/480001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6901: "<ph name="EXT_NAME">$1<ex>My Extension</ex></ph>" is requesting your <ph name="CODE_TYPE">$2<ex>PIN</ex></ph> Shouldn't a colon character be added into the end of the message? https://codereview.chromium.org/2094333002/diff/480001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/480001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: The current style guide says not to use "(c)" in the header. https://codereview.chromium.org/2094333002/diff/480001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:193: // Infomation label. nit: If you have this comment, then please also add comments for the other blocks adding UI elements in this function. https://codereview.chromium.org/2094333002/diff/480001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/480001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:153: chromeos::RequestPinErrorType GetErrorTypeForView( nit: Put this function into an anonymous namespace. https://codereview.chromium.org/2094333002/diff/480001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:170: CertificateProviderStopPinRequestFunction:: nit: Please reorder the method definitions so that they match the order of declarations in the .h file. https://codereview.chromium.org/2094333002/diff/480001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h (right): https://codereview.chromium.org/2094333002/diff/480001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h:50: extern const int MAX_CLOSED_DIALOGS_PER_10_MINUTES; The constant name is too generic - it has no attribution to certificateProvider or PIN requests. Maybe make it member of the PinDialogManager class? https://codereview.chromium.org/2094333002/diff/480001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/certificate_provider/request_pin/OWNERS (right): https://codereview.chromium.org/2094333002/diff/480001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/OWNERS:1: igorcov@chromium.org Do you think a special OWNERS file is necessary? There's already one in the parent directory.
Implemented suggestions from emaxx. Important changes: sign_request_id - changed to int and is reused from SignRequests class. Changed its generation method at source to be random. locked_callback_ in RequestPinView is given in view constructor and its lifetime is the same as the view. It is never reset to another pointer. Might be called once, many times or not called at all. Used only when the view is locked (waiting for validation response from extension) and communicates internally. The normal callback handles the external communication (with extension). https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:118: uint64_t sign_request_id = base::RandUint64(); On 2016/09/06 15:02:10, emaxx wrote: > There is already another "sign_request_id" in the existing code that is > generated by CertificateProviderService and passed to this function - here it is > named as "request_id". Can we reuse it instead of introducing a new one > identifier? > > There is one difference between them: the existing ids are generated simply by > increasing a counter instead of the random used here (see the generation code > here: > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/certificate_prov... > ). But I think it should be fine to modify the SignRequests class implementation > accordingly. Done. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:118: uint64_t sign_request_id = base::RandUint64(); On 2016/09/06 15:02:10, emaxx wrote: > I'm afraid we can put the client extensions into a bad situation by generating > the number that cannot be stored as a JavaScript number precisely. > > My understanding is that the JavaScript number can store precisely 53-bit > integers, but I may be wrong here (and in any way hardcoding this here would be > quite strange). > > So maybe, for simplicity reasons, we should generate the numbers that fit into > the usual 4-byte int range. Done. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:20: // The limit of the sign_request_id map size, after which the sanitizing process On 2016/09/06 15:02:10, emaxx wrote: > This looks a bit hacky, and may be not needed at all if the code would be > changed to use the storage in the SignRequests class (which always has the > up-to-date information regarding the currently running requests). Implemented the same way the storage is in SignRequests class. Now besides the ID, the extension_id it was generated for is also kept in the map. And the IDs are disposed from the map only when the extension is unloaded. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:112: int result = gettimeofday(&tv, nullptr); On 2016/09/06 15:02:10, emaxx wrote: > The same question as in the header - would it be possible to use the convenient > utils provided at base/time/? Done. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:16: enum RequestPinResponse { On 2016/09/06 15:02:10, emaxx wrote: > It's better to either make this "enum class" or move it inside the > PinDialogManager class (probably, the latter is better). > > The current definition effectively puts the symbols into the chromeos namespace > with too generic names (e.g. chromeos::SUCCESS). Done. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:27: virtual ~PinDialogManager(); On 2016/09/06 15:02:10, emaxx wrote: > There are no child classes of this class currently, am I correct? > Then instead of this precautious move (I mean adding an empty virtual > destructor) you could just mark the class as "final" now. Done. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:31: void OnPinDialogInput(const std::string& extension_id, const bool closed); On 2016/09/06 15:02:10, emaxx wrote: > nit: regarding "const bool", "const long long" etc. - I believe usually in > Chromium code base the primitive types are passed just by value. Done. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:36: void OnFlowInterrupted(const base::string16& value); On 2016/09/06 15:02:10, emaxx wrote: > nit: #include "base/strings/string16.h" Done. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:36: void OnFlowInterrupted(const base::string16& value); On 2016/09/06 15:02:10, emaxx wrote: > Would it be easy to pass the extension_id here too? > Currently, the API looks a bit surprising as all other functions accept the > extension_id, but here it's omitted (and is obtained implicitly). I've cleaned the extension_id from functions that are called internally as it's not needed, doesn't accomplish anything and passing a string around is bad overall. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:50: // Creates a new RequestPinView object and displays it in a dialog or reuses On 2016/09/06 15:02:10, emaxx wrote: > nit: Maybe it would make sense to reorder the method declarations, so that they > follow the typical work flow (more or less). E.g. first AddSignRequestId, then > ShowPingDialog, etc. Done. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:93: views::Widget* active_window_for_testing() { return active_window_; } On 2016/09/06 15:02:10, emaxx wrote: > nit: #include "ui/views/widget/widget.h". Done. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:100: std::map<std::string, bool> last_response_closed_; On 2016/09/06 15:02:10, emaxx wrote: > nit: #include <map> > (BTW, maybe use std::unordered_map? Not insisting, but I think that using > std::map may make somebody think there's some dependency on the order of the > elements.) Done. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:104: std::map<uint64_t, uint64_t> sign_request_times_; On 2016/09/06 15:02:10, emaxx wrote: > Is there a reason to use uint64_t for storing the time instead of base::Time? I've used timeval as that was one example I found in the code base. Changed to use base::Time now. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:112: base::WeakPtrFactory<PinDialogManager> weak_factory_; On 2016/09/06 15:02:10, emaxx wrote: > nit: #include "base/memory/weak_ptr.h". Done. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:128: DCHECK(locked_ || callback_.is_null()); On 2016/08/11 18:12:02, stevenjb wrote: > This is confusing and seems fragile. While it may be safe, I can not quickly > convince myself that it is so. I think we would be better served with two > callbacks rather than re-purposing a single callback but only under certain > conditions. Done. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:143: code_type_ = base::ASCIIToUTF16("PIN"); On 2016/08/11 18:12:02, stevenjb wrote: > These need to be localized strings. Done. https://codereview.chromium.org/2094333002/diff/480001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/480001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/09/06 15:02:11, emaxx wrote: > nit: The current style guide says not to use "(c)" in the header. Done. https://codereview.chromium.org/2094333002/diff/480001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:193: // Infomation label. On 2016/09/06 15:02:11, emaxx wrote: > nit: If you have this comment, then please also add comments for the other > blocks adding UI elements in this function. Done.
I've changed the locked_callback into a delegate. I think it makes the code easier to understand.
I'll do a more thorough review when I am back in the office on Monday, but here are a few comments from a quick review pass. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:56: if (!callback_.is_null()) { On 2016/09/06 13:22:02, igorcov wrote: > On 2016/08/11 18:12:02, stevenjb wrote: > > nit: I am pretty sure that you can just use !callback_ instead of is_null > > throughout. > > I can't because the callback_ object is always there. It's not a pointer. > Methods is_null() and Reset() have to be used to change the internal state of > the object. https://cs.chromium.org/chromium/src/base/callback_internal.h?sq=package:chro... https://codereview.chromium.org/2094333002/diff/580001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/580001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:50: } else { Why else? Wouldn't we want to always call OnPinClosed() when we destroy this? https://codereview.chromium.org/2094333002/diff/580001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:67: delegate_->OnPinDialogClosed(); Calling OnPinClosed from here and from the destructor is confusing and could be dangerous. We should either always call it from the destructor or, if necessary, have multiple delegate methods (e.g. Closed and Destroyed). https://codereview.chromium.org/2094333002/diff/580001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/580001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:42: }; This should be either defined within RequestPinView, e.g. RequestPinView::Delegate, or in a separate header as RequestPinViewDelegate. chromeos::CloseViewDelegate is much too generic of a name. https://codereview.chromium.org/2094333002/diff/580001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:126: CloseViewDelegate* delegate_; nit: blank line separating commented member from other members.
https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:56: if (!callback_.is_null()) { On 2016/09/08 16:29:31, stevenjb wrote: > On 2016/09/06 13:22:02, igorcov wrote: > > On 2016/08/11 18:12:02, stevenjb wrote: > > > nit: I am pretty sure that you can just use !callback_ instead of is_null > > > throughout. > > > > I can't because the callback_ object is always there. It's not a pointer. > > Methods is_null() and Reset() have to be used to change the internal state of > > the object. > > https://cs.chromium.org/chromium/src/base/callback_internal.h?sq=package:chro... I'm not sure what do you mean here. In my code callback_ is not a pointer. Anyway, to test it I tried "if (callback_)" and it always returns true while a statement like "callback_ = nullptr" doesn't compile. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:80: SetAcceptInput(false); On 2016/08/11 18:12:02, stevenjb wrote: > The dialog can still be closed with SetAcceptInput(false), correct? We should > document that in the header comment for 'locked_' and maybe here also. Done. https://codereview.chromium.org/2094333002/diff/480001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/480001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:153: chromeos::RequestPinErrorType GetErrorTypeForView( On 2016/09/06 15:02:11, emaxx wrote: > nit: Put this function into an anonymous namespace. Done. https://codereview.chromium.org/2094333002/diff/480001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:170: CertificateProviderStopPinRequestFunction:: On 2016/09/06 15:02:11, emaxx wrote: > nit: Please reorder the method definitions so that they match the order of > declarations in the .h file. Done. https://codereview.chromium.org/2094333002/diff/480001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/certificate_provider/request_pin/OWNERS (right): https://codereview.chromium.org/2094333002/diff/480001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/OWNERS:1: igorcov@chromium.org On 2016/09/06 15:02:11, emaxx wrote: > Do you think a special OWNERS file is necessary? There's already one in the > parent directory. Done. https://codereview.chromium.org/2094333002/diff/580001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/580001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:50: } else { On 2016/09/08 16:29:31, stevenjb wrote: > Why else? Wouldn't we want to always call OnPinClosed() when we destroy this? > The delegate was notified through callback code when present. But you are right, it's not a good design. I've removed the notification from callback code. https://codereview.chromium.org/2094333002/diff/580001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:67: delegate_->OnPinDialogClosed(); On 2016/09/08 16:29:31, stevenjb wrote: > Calling OnPinClosed from here and from the destructor is confusing and could be > dangerous. We should either always call it from the destructor or, if necessary, > have multiple delegate methods (e.g. Closed and Destroyed). You are right, it was making reaching the quota faster than it should. I've removed the code from Accept/Cancel that leads to destruction of the view. https://codereview.chromium.org/2094333002/diff/580001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/580001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:42: }; On 2016/09/08 16:29:31, stevenjb wrote: > This should be either defined within RequestPinView, e.g. > RequestPinView::Delegate, or in a separate header as RequestPinViewDelegate. > chromeos::CloseViewDelegate is much too generic of a name. Good point. Fixed. https://codereview.chromium.org/2094333002/diff/580001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:126: CloseViewDelegate* delegate_; On 2016/09/08 16:29:31, stevenjb wrote: > nit: blank line separating commented member from other members. Done.
OK, I did a complete review. Mostly nits at this point. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:56: if (!callback_.is_null()) { On 2016/09/09 15:53:42, igorcov wrote: > On 2016/09/08 16:29:31, stevenjb wrote: > > On 2016/09/06 13:22:02, igorcov wrote: > > > On 2016/08/11 18:12:02, stevenjb wrote: > > > > nit: I am pretty sure that you can just use !callback_ instead of is_null > > > > throughout. > > > > > > I can't because the callback_ object is always there. It's not a pointer. > > > Methods is_null() and Reset() have to be used to change the internal state > of > > > the object. > > > > > https://cs.chromium.org/chromium/src/base/callback_internal.h?sq=package:chro... > > I'm not sure what do you mean here. In my code callback_ is not a pointer. > Anyway, to test it I tried "if (callback_)" and it always returns true while a > statement like > "callback_ = nullptr" doesn't compile. TL;DR: is_null() seems to be much more common so nevermind, this is fine. We do however specifically define operator bool() for base::Callback: https://cs.chromium.org/chromium/src/base/callback_internal.h?sq=package:chro... So that if (!callback_) is the same as if (!callback_.is_null()) There aren't actually a lot of examples of this pattern in the code, here is one I found: https://cs.chromium.org/chromium/src/blimp/client/core/session/identity_sourc... https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service.h (right): https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service.h:22: #include "chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h" You can forward declare PinDialogManager in this header and move the #include to the C++ file. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc (right): https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:18: const int SIGN_REQUEST_ID_TIMEOUT = 10; // 10 minutes nit: TIMEOUT_MINS instead of comment (makes it clear where it is used). https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:98: } nit: Wrap the above in an anonymous namespaced helper function: gfx::NativeWindow parent = GetBrowserParentWindow(); https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:132: } nit: combine early exits https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:148: LOG(ERROR) << "StopPinRequest called by wrong extension"; nit: "StopPinRequest called by unexpected extension: " << extension_id; https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h (right): https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:103: std::map<std::pair<std::string, int>, base::Time> sign_request_times_; We should typedef the key and use the typedef in the C++ code. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/sign_requests.cc (right): https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/sign_requests.cc:26: RequestsState& state = extension_to_requests_[extension_id]; Add comment, e.g.: // Generate a random request id so that extensions using chrome.certificateProvider can not guess another extension's request id. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:64: // Destructor is called after this to notify the delegate. nit: s/is/will be/, s/to notify/which notifies/ https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:36: }; These enums need to be inside RequestPinView. Currently this defines chromeos::NONE! https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:63: // |delegate| - used to notify that dialog was closed. Can delegate be null? Document. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:88: // callback should be null at this point. s/should/must/ (maybe briefly explain how) https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:103: const bool accept_input); nit: we rarely use const for int or bool input args. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:191: return RespondNow(Error("No active dialog from extension.")); This string should be defined as a const in this file and used here and below. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:244: "MAX_SHOW_DIALOGS_PER_MINUTE")); We should name this more specifically. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:291: return RespondNow(Error("Previous request not finished")); nit: These errors should also be defined as consts above, even though they are only used once. It's helpful to have all of the responses grouped together. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h (right): https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h:50: extern const int MAX_CLOSED_DIALOGS_PER_10_MINUTES; This is much to generic a name for the extensions:: namespace. Also, it should use camelCase, e.g. kCertificateProviderMaxClosedDialogsPer10Mins. Also, where else is this used? Do the values actually need to match? https://codereview.chromium.org/2094333002/diff/650001/chrome/common/extensio... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2094333002/diff/650001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:56: // The ID given by Chrome when in SignRequest. s/when in/in/ https://codereview.chromium.org/2094333002/diff/650001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:75: // The ID given by Chrome when in SignRequest. s/when in/in/
Thanks for review comments. https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:56: if (!callback_.is_null()) { On 2016/09/12 21:16:39, stevenjb wrote: > On 2016/09/09 15:53:42, igorcov wrote: > > On 2016/09/08 16:29:31, stevenjb wrote: > > > On 2016/09/06 13:22:02, igorcov wrote: > > > > On 2016/08/11 18:12:02, stevenjb wrote: > > > > > nit: I am pretty sure that you can just use !callback_ instead of > is_null > > > > > throughout. > > > > > > > > I can't because the callback_ object is always there. It's not a pointer. > > > > Methods is_null() and Reset() have to be used to change the internal state > > of > > > > the object. > > > > > > > > > https://cs.chromium.org/chromium/src/base/callback_internal.h?sq=package:chro... > > > > I'm not sure what do you mean here. In my code callback_ is not a pointer. > > Anyway, to test it I tried "if (callback_)" and it always returns true while a > > statement like > > "callback_ = nullptr" doesn't compile. > > TL;DR: is_null() seems to be much more common so nevermind, this is fine. > > We do however specifically define operator bool() for base::Callback: > https://cs.chromium.org/chromium/src/base/callback_internal.h?sq=package:chro... > > So that > if (!callback_) > is the same as > if (!callback_.is_null()) > > There aren't actually a lot of examples of this pattern in the code, here is one > I found: > https://cs.chromium.org/chromium/src/blimp/client/core/session/identity_sourc... > Acknowledged. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service.h (right): https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service.h:22: #include "chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h" On 2016/09/12 21:16:39, stevenjb wrote: > You can forward declare PinDialogManager in this header and move the #include to > the C++ file. It doesn't compile because there also a getter implementation here. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc (right): https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:98: } On 2016/09/12 21:16:39, stevenjb wrote: > nit: Wrap the above in an anonymous namespaced helper function: > gfx::NativeWindow parent = GetBrowserParentWindow(); Done. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:132: } On 2016/09/12 21:16:39, stevenjb wrote: > nit: combine early exits Done. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:148: LOG(ERROR) << "StopPinRequest called by wrong extension"; On 2016/09/12 21:16:39, stevenjb wrote: > nit: "StopPinRequest called by unexpected extension: " << extension_id; Done. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h (right): https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:103: std::map<std::pair<std::string, int>, base::Time> sign_request_times_; On 2016/09/12 21:16:39, stevenjb wrote: > We should typedef the key and use the typedef in the C++ code. Done. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/sign_requests.cc (right): https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/sign_requests.cc:26: RequestsState& state = extension_to_requests_[extension_id]; On 2016/09/12 21:16:39, stevenjb wrote: > Add comment, e.g.: > // Generate a random request id so that extensions using > chrome.certificateProvider can not guess another extension's request id. Done. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:64: // Destructor is called after this to notify the delegate. On 2016/09/12 21:16:39, stevenjb wrote: > nit: s/is/will be/, s/to notify/which notifies/ Done. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:36: }; On 2016/09/12 21:16:39, stevenjb wrote: > These enums need to be inside RequestPinView. Currently this defines > chromeos::NONE! Done. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:63: // |delegate| - used to notify that dialog was closed. On 2016/09/12 21:16:39, stevenjb wrote: > Can delegate be null? Document. Done. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:88: // callback should be null at this point. On 2016/09/12 21:16:39, stevenjb wrote: > s/should/must/ (maybe briefly explain how) Done. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:103: const bool accept_input); On 2016/09/12 21:16:39, stevenjb wrote: > nit: we rarely use const for int or bool input args. Done. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:191: return RespondNow(Error("No active dialog from extension.")); On 2016/09/12 21:16:40, stevenjb wrote: > This string should be defined as a const in this file and used here and below. Done. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:244: "MAX_SHOW_DIALOGS_PER_MINUTE")); On 2016/09/12 21:16:40, stevenjb wrote: > We should name this more specifically. Done. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:291: return RespondNow(Error("Previous request not finished")); On 2016/09/12 21:16:40, stevenjb wrote: > nit: These errors should also be defined as consts above, even though they are > only used once. It's helpful to have all of the responses grouped together. Done. https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h (right): https://codereview.chromium.org/2094333002/diff/650001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h:50: extern const int MAX_CLOSED_DIALOGS_PER_10_MINUTES; On 2016/09/12 21:16:40, stevenjb wrote: > This is much to generic a name for the extensions:: namespace. > > Also, it should use camelCase, e.g. > kCertificateProviderMaxClosedDialogsPer10Mins. > > Also, where else is this used? Do the values actually need to match? Included the constant in certificate_provider namespace and renamed it to kMaxClosedDialogsPer10Mins. It is used in browser tests from certificate_provider_apitest. https://codereview.chromium.org/2094333002/diff/650001/chrome/common/extensio... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2094333002/diff/650001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:56: // The ID given by Chrome when in SignRequest. On 2016/09/12 21:16:40, stevenjb wrote: > s/when in/in/ Done. https://codereview.chromium.org/2094333002/diff/650001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:75: // The ID given by Chrome when in SignRequest. On 2016/09/12 21:16:40, stevenjb wrote: > s/when in/in/ Done.
lgtm. Please wait for an lg from emaxx@ also. https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc (right): https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:16: gfx::NativeWindow parent = nullptr; elim. https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:18: parent = chromeos::LoginDisplayHost::default_host()->GetNativeWindow(); just return here, no else. https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:23: parent = browser->window()->GetNativeWindow(); return https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:27: return parent; return nullptr;
stevenjb@chromium.org changed required reviewers: + emaxx@chromium.org
https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc (right): https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:17: #include "base/rand_util.h" nit: not needed anymore. https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h (right): https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:109: chromeos::RequestPinView* active_pin_dialog_ = nullptr; nit: "chromeos::" is not needed. https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/sign_requests.cc (right): https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/sign_requests.cc:27: // Generate a random request id so that extensions using Is this comment accurate? My understanding of the code is that, even if some extension B knew the request_ids received by extension A, it wouldn't be allowed to use them to interact with the dialog. If this is not true, then this would sound like a security issue. https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/sign_requests.cc:30: while (state.pending_requests.find(request_id) != nit: Do-while loop would be a bit more natural (you won't have to duplicate the RandInt call in the source code). https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:10: #include "base/macros.h" nit: not required. https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:12: #include "base/compiler_specific.h" nit: Is this include necessary? https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:25: chromeos::RequestPinView::RequestPinErrorType GetErrorTypeForView( nit: This function has to be put into an anonymous namespace. https://codereview.chromium.org/2094333002/diff/710001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2094333002/diff/710001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6326: <message name="IDS_REQUEST_PIN_DIALOG_HEADER" desc="The text displayed in the dialog to request the PIN/PUK by extension."> nit: Please make all the descriptions that refer to the PIN dialog look uniform. Currently, the first description includes the text "the dialog to request the PIN/PUK by extension", some other descriptions include the text "requestPin dialog", and other descriptions don't mention the attribution at all. I'd suggest to reformat all descriptions so that they all have some clear distinguishing "marker" - e.g.: "the certificate provider PIN request dialog". https://codereview.chromium.org/2094333002/diff/710001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6327: "<ph name="EXT_NAME">$1<ex>My Extension</ex></ph>" is requesting your <ph name="CODE_TYPE">$2<ex>PIN</ex></ph> nit: s/EXT_/EXTENSION_/ (it's always better to not shorten words in the names) https://codereview.chromium.org/2094333002/diff/710001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6341: <message name="IDS_REQUEST_PIN_DIALOG_UNKNOWN_ERROR" desc="The error message displayed in requestPin dialog when unexpected error occurred in extension code."> nit: s/unexpected/unknown/ (because the error itself may be well expected in this or that case, but it's just that API doesn't provide a corresponding standard message for it) https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc (right): https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:116: service_->pin_dialog_manager()->AddSignRequestId(extension_id, request_id); The AddSignRequestId method returns boolean value that indicates whether the operation was successful, but this is ignored here. Will the subsequent code work correctly when adding finished unsuccessfully? https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc (right): https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:15: gfx::NativeWindow GetBrowserParentWindow() { nit: Move the function into an anonymous namespace. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:15: gfx::NativeWindow GetBrowserParentWindow() { nit: #include "ui/gfx/native_widget_types.h" https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:18: } else { nit: Reformat the code to remove the "else" block. This is according to the style guide requirement: "Don't use else after return." https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:19: Browser* browser = chrome::FindTabbedBrowser( nit: #include "chrome/browser/ui/browser.h" https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:70: return RequestPinResponse::OTHER_FLOW_IN_PROGRESS; nit: I don't known whether you use this "RequestPinResponse::" attribution intentionally, but just in case: it's not required to be written explicitly anywhere. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:100: active_pin_dialog_ = new chromeos::RequestPinView( nit: "chromeos::" is not required here. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:105: parent ? nullptr : ash::Shell::GetPrimaryRootWindow(); nit: #include "ui/aura/window.h" https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:106: active_window_ = views::DialogDelegate::CreateDialogWidget(active_pin_dialog_, nit: #include "ui/views/window/dialog_delegate.h" https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h (right): https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:25: typedef std::pair<std::string, int> ExtensionNameRequestIdPair; nit: #include <utility> https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:25: typedef std::pair<std::string, int> ExtensionNameRequestIdPair; nit: Move this type definition into the private section: it's not used by any of the public definitions. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:86: bool LastPinDialogClosed(const std::string& extension_id); nit: Make the method const. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:100: // State about the last response from user to the requestPin from extension. nit: Please use more specific words here than just "state about the last response". https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:105: std::map<ExtensionNameRequestIdPair, base::Time> sign_request_times_; nit: #include "base/time/time.h" https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:107: // There can be only one active dialog to request PIN from this extension. nit: The current comment may be understood as there's only one dialog allowed _per extension_, which isn't true. Please edit the text to be more precise. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:109: chromeos::RequestPinView* active_pin_dialog_ = nullptr; nit: "chromeos::" is not required here. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/sign_requests.cc (right): https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/sign_requests.cc:29: int request_id = base::RandInt(0, INT_MAX); nit: #include <climits> https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:10: #include "base/macros.h" nit: This is already included in the header file. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:12: #include "base/strings/stringprintf.h" nit: Is this include needed? https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:14: #include "chrome/browser/chromeos/net/onc_utils.h" nit: Is this include needed? https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:18: #include "chromeos/login/login_state.h" nit: Is this include needed? https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:19: #include "components/onc/onc_constants.h" nit: Is this include needed? https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:79: ui::ModalType GetModalType() const override; nit: #include "ui/base/ui_base_types.h" https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:100: void SetDialogParameters(RequestPinView::RequestPinCodeType code_type, nit: "RequestPinView::" is not needed here or anywhere else in the class definition. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:109: views::Textfield* textfield_for_testing() { return textfield_; } nit: #include "ui/views/controls/textfield/textfield.h" https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:110: views::Label* error_label_for_testing() { return error_label_; } nit: #include "ui/views/controls/label.h" https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:25: chromeos::RequestPinView::RequestPinErrorType GetErrorTypeForView( nit: Move this function into an anonymous namespace. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc (right): https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:133: base::string16 code) { nit: Pass the strings by const-reference (here and in other places in this file). https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:201: chromeos::CertificateProviderService* LoadExtensionFromFolder( Please change the existing tests in this file to use this helper function too. Currently, it seems that there's some duplication of logic between the old code and the new code. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:213: service->pin_dialog_manager()->AddSignRequestId(extension->id(), 123); nit: The test uses the same constant "123" for different purposes: first, as the sign request id, and, second, as the entered PIN. Please change this to make these two cases using different constants. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:336: // User closes the dialog three times, and the extension should be blocked from nit: Replace the "three times" text with a more generic text, because the exact number is actually determined by the externally defined kMaxClosedDialogsPer10Mins constant. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:349: while (window == service->pin_dialog_manager()->active_window_for_testing()) Not sure whether I understand the code correctly. Are you assuming that after destroying the old Widget* window, the newly constructed window will have a different address in the memory? This may be true only by occasion, but you shouldn't rely on this in the test code. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:362: IN_PROC_BROWSER_TEST_F(CertificateProviderApiTest, ShowPinDialogAccept) { nit: Please move this test to be the first one among the PIN dialog tests - because this is actually the most basic scenario. https://codereview.chromium.org/2094333002/diff/710001/chrome/common/extensio... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2094333002/diff/710001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:56: // The ID given by Chrome in SignRequest. Please add a comment that this is only guaranteed to be unique among the simultaneously running requests (i.e. _not_ guaranteed to be unique for the whole extension's lifetime). https://codereview.chromium.org/2094333002/diff/710001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:68: // instead StopPinRequestDetails will be called with nit: s/StopPinRequestDetails/stopPinRequest/
Improved the code according to emaxx suggestions. Thanks. https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc (right): https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:17: #include "base/rand_util.h" On 2016/09/19 14:01:42, emaxx wrote: > nit: not needed anymore. Done. https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc (right): https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:16: gfx::NativeWindow parent = nullptr; On 2016/09/13 15:18:33, stevenjb wrote: > elim. Done. https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:18: parent = chromeos::LoginDisplayHost::default_host()->GetNativeWindow(); On 2016/09/13 15:18:33, stevenjb wrote: > just return here, no else. Done. https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:23: parent = browser->window()->GetNativeWindow(); On 2016/09/13 15:18:33, stevenjb wrote: > return Done. https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:27: return parent; On 2016/09/13 15:18:33, stevenjb wrote: > return nullptr; Done. https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h (right): https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:109: chromeos::RequestPinView* active_pin_dialog_ = nullptr; On 2016/09/19 14:01:42, emaxx wrote: > nit: "chromeos::" is not needed. Done. https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/sign_requests.cc (right): https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/sign_requests.cc:27: // Generate a random request id so that extensions using On 2016/09/19 14:01:42, emaxx wrote: > Is this comment accurate? > My understanding of the code is that, even if some extension B knew the > request_ids received by extension A, it wouldn't be allowed to use them to > interact with the dialog. If this is not true, then this would sound like a > security issue. I've changed this to original code because now as we have both, extension_id and request_id in the key of the map, we don't need the id to be randomized. https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/sign_requests.cc:30: while (state.pending_requests.find(request_id) != On 2016/09/19 14:01:42, emaxx wrote: > nit: Do-while loop would be a bit more natural (you won't have to duplicate the > RandInt call in the source code). Acknowledged. https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:10: #include "base/macros.h" On 2016/09/19 14:01:42, emaxx wrote: > nit: not required. Done. https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/690001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:12: #include "base/compiler_specific.h" On 2016/09/19 14:01:42, emaxx wrote: > nit: Is this include necessary? Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2094333002/diff/710001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6326: <message name="IDS_REQUEST_PIN_DIALOG_HEADER" desc="The text displayed in the dialog to request the PIN/PUK by extension."> On 2016/09/19 14:01:43, emaxx wrote: > nit: Please make all the descriptions that refer to the PIN dialog look uniform. > Currently, the first description includes the text "the dialog to request the > PIN/PUK by extension", some other descriptions include the text "requestPin > dialog", and other descriptions don't mention the attribution at all. > I'd suggest to reformat all descriptions so that they all have some clear > distinguishing "marker" - e.g.: > "the certificate provider PIN request dialog". Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6327: "<ph name="EXT_NAME">$1<ex>My Extension</ex></ph>" is requesting your <ph name="CODE_TYPE">$2<ex>PIN</ex></ph> On 2016/09/19 14:01:43, emaxx wrote: > nit: s/EXT_/EXTENSION_/ > (it's always better to not shorten words in the names) Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6341: <message name="IDS_REQUEST_PIN_DIALOG_UNKNOWN_ERROR" desc="The error message displayed in requestPin dialog when unexpected error occurred in extension code."> On 2016/09/19 14:01:43, emaxx wrote: > nit: s/unexpected/unknown/ > (because the error itself may be well expected in this or that case, but it's > just that API doesn't provide a corresponding standard message for it) Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc (right): https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:116: service_->pin_dialog_manager()->AddSignRequestId(extension_id, request_id); On 2016/09/19 14:01:43, emaxx wrote: > The AddSignRequestId method returns boolean value that indicates whether the > operation was successful, but this is ignored here. > Will the subsequent code work correctly when adding finished unsuccessfully? In the new implementation it's impossible to have the id already in the map. Changed the function signature to not return anything. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc (right): https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:15: gfx::NativeWindow GetBrowserParentWindow() { On 2016/09/19 14:01:43, emaxx wrote: > nit: #include "ui/gfx/native_widget_types.h" Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:15: gfx::NativeWindow GetBrowserParentWindow() { On 2016/09/19 14:01:43, emaxx wrote: > nit: Move the function into an anonymous namespace. Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:18: } else { On 2016/09/19 14:01:43, emaxx wrote: > nit: Reformat the code to remove the "else" block. This is according to the > style guide requirement: > "Don't use else after return." > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:19: Browser* browser = chrome::FindTabbedBrowser( On 2016/09/19 14:01:43, emaxx wrote: > nit: #include "chrome/browser/ui/browser.h" Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:70: return RequestPinResponse::OTHER_FLOW_IN_PROGRESS; On 2016/09/19 14:01:43, emaxx wrote: > nit: I don't known whether you use this "RequestPinResponse::" attribution > intentionally, but just in case: it's not required to be written explicitly > anywhere. Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:100: active_pin_dialog_ = new chromeos::RequestPinView( On 2016/09/19 14:01:43, emaxx wrote: > nit: "chromeos::" is not required here. Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:105: parent ? nullptr : ash::Shell::GetPrimaryRootWindow(); On 2016/09/19 14:01:43, emaxx wrote: > nit: #include "ui/aura/window.h" Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:106: active_window_ = views::DialogDelegate::CreateDialogWidget(active_pin_dialog_, On 2016/09/19 14:01:43, emaxx wrote: > nit: #include "ui/views/window/dialog_delegate.h" Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:79: ui::ModalType GetModalType() const override; On 2016/09/19 14:01:44, emaxx wrote: > nit: #include "ui/base/ui_base_types.h" Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:100: void SetDialogParameters(RequestPinView::RequestPinCodeType code_type, On 2016/09/19 14:01:44, emaxx wrote: > nit: "RequestPinView::" is not needed here or anywhere else in the class > definition. Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:109: views::Textfield* textfield_for_testing() { return textfield_; } On 2016/09/19 14:01:44, emaxx wrote: > nit: #include "ui/views/controls/textfield/textfield.h" Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:110: views::Label* error_label_for_testing() { return error_label_; } On 2016/09/19 14:01:44, emaxx wrote: > nit: #include "ui/views/controls/label.h" Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:25: chromeos::RequestPinView::RequestPinErrorType GetErrorTypeForView( On 2016/09/19 14:01:44, emaxx wrote: > nit: Move this function into an anonymous namespace. Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc (right): https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:133: base::string16 code) { On 2016/09/19 14:01:44, emaxx wrote: > nit: Pass the strings by const-reference (here and in other places in this > file). Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:213: service->pin_dialog_manager()->AddSignRequestId(extension->id(), 123); On 2016/09/19 14:01:44, emaxx wrote: > nit: The test uses the same constant "123" for different purposes: first, as the > sign request id, and, second, as the entered PIN. > Please change this to make these two cases using different constants. Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:349: while (window == service->pin_dialog_manager()->active_window_for_testing()) On 2016/09/19 14:01:44, emaxx wrote: > Not sure whether I understand the code correctly. Are you assuming that after > destroying the old Widget* window, the newly constructed window will have a > different address in the memory? This may be true only by occasion, but you > shouldn't rely on this in the test code. You are right. I've changed the code to wait for the view to update. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:362: IN_PROC_BROWSER_TEST_F(CertificateProviderApiTest, ShowPinDialogAccept) { On 2016/09/19 14:01:44, emaxx wrote: > nit: Please move this test to be the first one among the PIN dialog tests - > because this is actually the most basic scenario. Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/common/extensio... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2094333002/diff/710001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:68: // instead StopPinRequestDetails will be called with On 2016/09/19 14:01:44, emaxx wrote: > nit: s/StopPinRequestDetails/stopPinRequest/ Done.
The CQ bit was checked by igorcov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2094333002/diff/730001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc (right): https://codereview.chromium.org/2094333002/diff/730001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:202: std::string folder, nit: s/std::string/const std::string&/ https://codereview.chromium.org/2094333002/diff/730001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:213: service->pin_dialog_manager()->AddSignRequestId(extension->id(), 123); Please move this out into a separate method. It doesn't fill well into the function that is named just "LoadExtensionFromFolder". Generally, I think it would make sense to make a separate subclass for serving all PIN dialog tests. So the common test class would provide LoadExtensionFromFolder for using in all tests, and the PIN-related test class will provide some other method that is both calling LoadExtensionFromFolder and doing some other magic that is needed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by igorcov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by igorcov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by igorcov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by igorcov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Improved the testing code. Thanks. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h (right): https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:25: typedef std::pair<std::string, int> ExtensionNameRequestIdPair; On 2016/09/19 14:01:43, emaxx wrote: > nit: Move this type definition into the private section: it's not used by any of > the public definitions. Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:25: typedef std::pair<std::string, int> ExtensionNameRequestIdPair; On 2016/09/19 14:01:43, emaxx wrote: > nit: Move this type definition into the private section: it's not used by any of > the public definitions. Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:86: bool LastPinDialogClosed(const std::string& extension_id); On 2016/09/19 14:01:43, emaxx wrote: > nit: Make the method const. Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:100: // State about the last response from user to the requestPin from extension. On 2016/09/19 14:01:43, emaxx wrote: > nit: Please use more specific words here than just "state about the last > response". Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:107: // There can be only one active dialog to request PIN from this extension. On 2016/09/19 14:01:43, emaxx wrote: > nit: The current comment may be understood as there's only one dialog allowed > _per extension_, which isn't true. > Please edit the text to be more precise. Done. https://codereview.chromium.org/2094333002/diff/710001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:109: chromeos::RequestPinView* active_pin_dialog_ = nullptr; On 2016/09/19 14:01:43, emaxx wrote: > nit: "chromeos::" is not required here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
lgtm with a nit https://codereview.chromium.org/2094333002/diff/830001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc (right): https://codereview.chromium.org/2094333002/diff/830001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:199: // Loads certificate_provider extension from folder (the page basic.html). nit: You probably duplicated the helper method by mistake.
The CQ bit was checked by igorcov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks. The CQ dry run now passed. https://codereview.chromium.org/2094333002/diff/830001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc (right): https://codereview.chromium.org/2094333002/diff/830001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:199: // Loads certificate_provider extension from folder (the page basic.html). On 2016/09/19 23:15:02, emaxx wrote: > nit: You probably duplicated the helper method by mistake. Done.
igorcov@chromium.org changed reviewers: + rdevlin.cronin@chromium.org - igorcov@google.com
Devlin - PTAL at chrome/common/extensions/api/certificate_provider.idl and extensions/browser/extension_function_histogram_value.h. Thanks.
https://codereview.chromium.org/2094333002/diff/850001/chrome/common/extensio... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2094333002/diff/850001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:59: // The type of code requested, PIN or PUK. Default is PIN. remove ', PIN or PUK' - this is implied by the enum values. https://codereview.chromium.org/2094333002/diff/850001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:62: // The error template displayed for user. This should be set if the previous display for -> displayed to the Also, what is an error template? https://codereview.chromium.org/2094333002/diff/850001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:68: // instead stopPinRequest will be called with what will call stopPinRequest? https://codereview.chromium.org/2094333002/diff/850001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:86: // other error occurred. If some error occurred, it will be provided using We typically don't explicitly document that runtime.lastError will be set, since that's the assumption. Also, this seems like a list that's doomed to get out of date with the rest of the code. I'd probably just remove this. https://codereview.chromium.org/2094333002/diff/850001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:94: // A callback called when the dialog gets resolved with the user input, or Callback types don't need documentation; their use in the function is documentation itself. https://codereview.chromium.org/2094333002/diff/850001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:146: // allowed. The requests issued while other flow is ongoing are rejected. while other -> while another https://codereview.chromium.org/2094333002/diff/850001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:147: // It's extension's responsibility to try again later if other flow is in It's extension's -> It is the extension's if other -> if another https://codereview.chromium.org/2094333002/diff/850001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:152: // Stops the pin request started by $(ref:requestPin) function. by $ -> by the $
Devlin, PTAL. Thanks https://codereview.chromium.org/2094333002/diff/850001/chrome/common/extensio... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2094333002/diff/850001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:59: // The type of code requested, PIN or PUK. Default is PIN. On 2016/09/21 17:31:20, Devlin wrote: > remove ', PIN or PUK' - this is implied by the enum values. Done. https://codereview.chromium.org/2094333002/diff/850001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:62: // The error template displayed for user. This should be set if the previous On 2016/09/21 17:31:20, Devlin wrote: > display for -> displayed to the > > Also, what is an error template? The error template is the text displayed in the dialog as an error with some possible variables to be filled like attempts_left. We defined the templates because don't want to allow the extensions to show any text they want as an error (for security reasons). https://codereview.chromium.org/2094333002/diff/850001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:68: // instead stopPinRequest will be called with On 2016/09/21 17:31:20, Devlin wrote: > what will call stopPinRequest? Extension is expected to make that call. Included this info in the comment. https://codereview.chromium.org/2094333002/diff/850001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:86: // other error occurred. If some error occurred, it will be provided using On 2016/09/21 17:31:20, Devlin wrote: > We typically don't explicitly document that runtime.lastError will be set, since > that's the assumption. Also, this seems like a list that's doomed to get out of > date with the rest of the code. I'd probably just remove this. Done. https://codereview.chromium.org/2094333002/diff/850001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:94: // A callback called when the dialog gets resolved with the user input, or On 2016/09/21 17:31:20, Devlin wrote: > Callback types don't need documentation; their use in the function is > documentation itself. Done. https://codereview.chromium.org/2094333002/diff/850001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:146: // allowed. The requests issued while other flow is ongoing are rejected. On 2016/09/21 17:31:20, Devlin wrote: > while other -> while another Done. https://codereview.chromium.org/2094333002/diff/850001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:147: // It's extension's responsibility to try again later if other flow is in On 2016/09/21 17:31:20, Devlin wrote: > It's extension's -> It is the extension's > > if other -> if another Done. https://codereview.chromium.org/2094333002/diff/850001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:152: // Stops the pin request started by $(ref:requestPin) function. On 2016/09/21 17:31:20, Devlin wrote: > by $ -> by the $ Done.
first run through https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:299: CertificateProviderService::CertificateProviderService() : weak_factory_(this) { why not initialize this in the initializer list? https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service.h (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service.h:199: std::unique_ptr<PinDialogManager> pin_dialog_manager_; This seems to never be null, nor destroyed. Why not just own the object directly? https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:27: chrome::FindTabbedBrowser(ProfileManager::GetPrimaryUserProfile(), true); GetPrimaryUserProfile sounds like it might not be the profile that triggered this flow, which would mean we use the wrong browser here. Is that possible? Should we be using something else? https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:98: typedef std::pair<std::string, int> ExtensionNameRequestIdPair; prefer 'using' syntax in new code. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:47: callback_.Run(base::string16()); base::ResetAndReturn(&callback_).Run(base::string16()); https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:129: Delegate* delegate_; = nullptr; https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:30: case api_cp::PinRequestErrorType::PIN_REQUEST_ERROR_TYPE_INVALID_PIN: Any reason to not use these enums in the RequestPinView class? https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:204: LOG(ERROR) << "Wrong extension requesting to close the dialog"; When is this error going to be used/seen? Is it a chrome error? https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:209: return RespondNow(ArgumentList(std::move(create_results))); RespondNow(NoArguments()) https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:234: Respond(ArgumentList(std::move(create_results))); Respond(NoArguments()) https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:285: params->details.attempts_left ? *params->details.attempts_left.get() : -1; *...get() -> *... https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:321: Respond(ArgumentList(std::move(create_results))); Use the OneArgument() macro here. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:150: base::RunLoop().RunUntilIdle(); Busy loops are generally bad, and timeouts over failures are harder to debug. Can we make this more deterministic? https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:210: content::WebContents* extension_contents = How is that the extension_contents when the extension isn't loaded? Also, if we just use this for the browser context, let's just use the test class accessor. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:222: base::RunLoop().RunUntilIdle(); What's this for? https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:365: base::RunLoop().RunUntilIdle(); ditto re busy loops https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:372: base::RunLoop().RunUntilIdle(); If the "while" on line 364 was needed, then this wouldn't be sufficient. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:387: EXPECT_NE(service->pin_dialog_manager()->active_view_for_testing(), nullptr); If this were null, your expectation above would seg fault. :) https://codereview.chromium.org/2094333002/diff/870001/chrome/common/extensio... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:18: PUK nit: add a trailing comma here and below https://codereview.chromium.org/2094333002/diff/870001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:136: // Requests the PIN from user. Only one ongoing request at a time is "from the user" https://codereview.chromium.org/2094333002/diff/870001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:139: // in progress. <code>callback</code> is called when the dialog gets s/gets/is https://codereview.chromium.org/2094333002/diff/870001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:139: // in progress. <code>callback</code> is called when the dialog gets Instead of referencing variables in the main description, describe each like so: // <main description> // |details|: The details of the request (placeholder text) // |callback|: The callback of the request (placeholder text) Same for below. https://codereview.chromium.org/2094333002/diff/870001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js:9: if(chrome.runtime.lastError != null) { need a space after if (everywhere) https://codereview.chromium.org/2094333002/diff/870001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js:10: console.error("Error: " + chrome.runtime.lastError.message); prefer single quotes in js https://codereview.chromium.org/2094333002/diff/870001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js:10: console.error("Error: " + chrome.runtime.lastError.message); It seems like we should be using chrome.test methods here in order to catch these. https://codereview.chromium.org/2094333002/diff/870001/extensions/browser/ext... File extensions/browser/extension_function_histogram_value.h (right): https://codereview.chromium.org/2094333002/diff/870001/extensions/browser/ext... extensions/browser/extension_function_histogram_value.h:1210: CERTIFICATEPROVIDER_STOPPINREQUEST, need to update the xml file.
Thanks for your comments Devlin, your input is appreciated. I've tried to address the review comments. Please take a look again. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:299: CertificateProviderService::CertificateProviderService() : weak_factory_(this) { On 2016/10/20 21:20:43, Devlin wrote: > why not initialize this in the initializer list? Done. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider_service.h (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider_service.h:199: std::unique_ptr<PinDialogManager> pin_dialog_manager_; On 2016/10/20 21:20:43, Devlin wrote: > This seems to never be null, nor destroyed. Why not just own the object > directly? Done. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc:27: chrome::FindTabbedBrowser(ProfileManager::GetPrimaryUserProfile(), true); On 2016/10/20 21:20:44, Devlin wrote: > GetPrimaryUserProfile sounds like it might not be the profile that triggered > this flow, which would mean we use the wrong browser here. Is that possible? > Should we be using something else? The code is the same as the one used in wifi dialog (see wifi_config_view for details) which is available in the same places as we need for our dialog. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h:98: typedef std::pair<std::string, int> ExtensionNameRequestIdPair; On 2016/10/20 21:20:44, Devlin wrote: > prefer 'using' syntax in new code. Done. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.cc (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.cc:47: callback_.Run(base::string16()); On 2016/10/20 21:20:44, Devlin wrote: > base::ResetAndReturn(&callback_).Run(base::string16()); Done. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... File chrome/browser/chromeos/ui/request_pin_view.h (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/chromeo... chrome/browser/chromeos/ui/request_pin_view.h:129: Delegate* delegate_; On 2016/10/20 21:20:44, Devlin wrote: > = nullptr; Done. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:30: case api_cp::PinRequestErrorType::PIN_REQUEST_ERROR_TYPE_INVALID_PIN: On 2016/10/20 21:20:44, Devlin wrote: > Any reason to not use these enums in the RequestPinView class? RequestPinView class is from ui folder and I tried to make it disconnected from extensions context. I don't like the duplicated enum here either, but I feel like it's better than having UI code mixed with extensions dependency. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:204: LOG(ERROR) << "Wrong extension requesting to close the dialog"; On 2016/10/20 21:20:44, Devlin wrote: > When is this error going to be used/seen? Is it a chrome error? It happens in case extension is calling stopPinRequest without having an actual active dialog. There are two cases mainly: 1 - extension didn't request PIN but is calling stopPinRequest. In this case it is helpful for extension developer to know that there's a bug in the code. 2 - extension requested the PIN, but either user closed the dialog or the request was denied, which means a stopPinRequest is not expected by Chrome. The user will not see anything (because there's no dialog, or the active dialog is owned by other extension), but the extension code will have chrome.runtime.lastError set. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:209: return RespondNow(ArgumentList(std::move(create_results))); On 2016/10/20 21:20:44, Devlin wrote: > RespondNow(NoArguments()) Done. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:234: Respond(ArgumentList(std::move(create_results))); On 2016/10/20 21:20:44, Devlin wrote: > Respond(NoArguments()) Done. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:285: params->details.attempts_left ? *params->details.attempts_left.get() : -1; On 2016/10/20 21:20:44, Devlin wrote: > *...get() -> *... Done. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:321: Respond(ArgumentList(std::move(create_results))); On 2016/10/20 21:20:44, Devlin wrote: > Use the OneArgument() macro here. It can be also NoArgument (if the input |value| is empty) here. Don't think it makes sense to respond in two places just to use the macro. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:150: base::RunLoop().RunUntilIdle(); On 2016/10/20 21:20:44, Devlin wrote: > Busy loops are generally bad, and timeouts over failures are harder to debug. > Can we make this more deterministic? I could probably make an WidgetObserver and involve a couple of callbacks in the code to wait for closing event from window. But I feel like it will complicate the code too much. And there are other places where I need to wait for some other type of events (line 164 too). https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:210: content::WebContents* extension_contents = On 2016/10/20 21:20:44, Devlin wrote: > How is that the extension_contents when the extension isn't loaded? Also, if we > just use this for the browser context, let's just use the test class accessor. You're right, we need this just for browser context. Don't understand though what do you mean by class accessor? https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:222: base::RunLoop().RunUntilIdle(); On 2016/10/20 21:20:44, Devlin wrote: > What's this for? I expected the browser would need some time to navigate to URL. Turns out it doesn't:) https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:372: base::RunLoop().RunUntilIdle(); On 2016/10/20 21:20:44, Devlin wrote: > If the "while" on line 364 was needed, then this wouldn't be sufficient. It's different, because we don't have to wait for another window to pop-up here (like in the case above). I agree that it's not a safe bet here, as other window could pop-up after the EXPECT check was done, but don't really see a better way. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:387: EXPECT_NE(service->pin_dialog_manager()->active_view_for_testing(), nullptr); On 2016/10/20 21:20:44, Devlin wrote: > If this were null, your expectation above would seg fault. :) I think you misread, one is active_window_for_testing and the other is active_view_for_testing. https://codereview.chromium.org/2094333002/diff/870001/chrome/common/extensio... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:18: PUK On 2016/10/20 21:20:44, Devlin wrote: > nit: add a trailing comma here and below The compiler doesn't like it: [3/37] ACTION //chrome/common/extensions/api:api_schema_generator(//build/toolchain/linux:clang_x64) ../../chrome/common/extensions/api/certificate_provider.idl(19) : Error : Trailing comma in block. https://codereview.chromium.org/2094333002/diff/870001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:136: // Requests the PIN from user. Only one ongoing request at a time is On 2016/10/20 21:20:44, Devlin wrote: > "from the user" Done. https://codereview.chromium.org/2094333002/diff/870001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:139: // in progress. <code>callback</code> is called when the dialog gets On 2016/10/20 21:20:44, Devlin wrote: > Instead of referencing variables in the main description, describe each like so: > // <main description> > // |details|: The details of the request (placeholder text) > // |callback|: The callback of the request (placeholder text) > > Same for below. Done. https://codereview.chromium.org/2094333002/diff/870001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:139: // in progress. <code>callback</code> is called when the dialog gets On 2016/10/20 21:20:44, Devlin wrote: > s/gets/is Done. https://codereview.chromium.org/2094333002/diff/870001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js:9: if(chrome.runtime.lastError != null) { On 2016/10/20 21:20:45, Devlin wrote: > need a space after if (everywhere) Done. https://codereview.chromium.org/2094333002/diff/870001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js:10: console.error("Error: " + chrome.runtime.lastError.message); On 2016/10/20 21:20:45, Devlin wrote: > prefer single quotes in js Done. https://codereview.chromium.org/2094333002/diff/870001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js:10: console.error("Error: " + chrome.runtime.lastError.message); On 2016/10/20 21:20:45, Devlin wrote: > It seems like we should be using chrome.test methods here in order to catch > these. Done. https://codereview.chromium.org/2094333002/diff/870001/extensions/browser/ext... File extensions/browser/extension_function_histogram_value.h (right): https://codereview.chromium.org/2094333002/diff/870001/extensions/browser/ext... extensions/browser/extension_function_histogram_value.h:1210: CERTIFICATEPROVIDER_STOPPINREQUEST, On 2016/10/20 21:20:45, Devlin wrote: > need to update the xml file. Done.
(Just responding, will take another full look soon) https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:204: LOG(ERROR) << "Wrong extension requesting to close the dialog"; On 2016/10/25 16:38:35, igorcov wrote: > On 2016/10/20 21:20:44, Devlin wrote: > > When is this error going to be used/seen? Is it a chrome error? > > It happens in case extension is calling stopPinRequest without having an actual > active dialog. There are two cases mainly: > 1 - extension didn't request PIN but is calling stopPinRequest. In this case it > is helpful for extension developer to know that there's a bug in the code. > 2 - extension requested the PIN, but either user closed the dialog or the > request was denied, which means a stopPinRequest is not expected by Chrome. > > The user will not see anything (because there's no dialog, or the active dialog > is owned by other extension), but the extension code will have > chrome.runtime.lastError set. Errors in extension logic should result in an error being sent to the extension, not a chrome error log. Developers may not read the commandline output from the browser (and shouldn't have to), and we shouldn't add logspam for an extension that has a bug. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:150: base::RunLoop().RunUntilIdle(); On 2016/10/25 16:38:35, igorcov wrote: > On 2016/10/20 21:20:44, Devlin wrote: > > Busy loops are generally bad, and timeouts over failures are harder to debug. > > Can we make this more deterministic? > > I could probably make an WidgetObserver and involve a couple of callbacks in the > code to wait for closing event from window. But I feel like it will complicate > the code too much. And there are other places where I need to wait for some > other type of events (line 164 too). What about waiting for a message from the extension instead? Then at least we know the extension tried to close the window and can verify whether it happened. Alternatively, writing a "WaitUntilWidgetClosed" is pretty simple: class WidgetClosedWaiter : public WidgetObserver { public: WidgetClosedWaiter(Widget* widget) : WidgetObserver(widget); void OnWidgetDestroying(Widget* widget) override { closed_ = true; run_loop_.Quit(); } void Wait() { if (closed_) return; run_loop_.Run(); } private: bool closed_ = false; base::RunLoop run_loop_; }; WidgetClosedWaiter closed_waiter(widget); // Do something closed_waiter.Wait(); https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:210: content::WebContents* extension_contents = On 2016/10/25 16:38:35, igorcov wrote: > On 2016/10/20 21:20:44, Devlin wrote: > > How is that the extension_contents when the extension isn't loaded? Also, if > we > > just use this for the browser context, let's just use the test class accessor. > > You're right, we need this just for browser context. Don't understand though > what do you mean by class accessor? Browser tests have a profile() accessor. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:222: base::RunLoop().RunUntilIdle(); On 2016/10/25 16:38:35, igorcov wrote: > On 2016/10/20 21:20:44, Devlin wrote: > > What's this for? > > I expected the browser would need some time to navigate to URL. Turns out it > doesn't:) Yep, NavigateToURL() blocks until navigation is complete, by design. (And if it didn't, RunUntilIdle() would be insufficient, since run loops only apply to one thread.)
It also doesn't look like we ever test the responses we send to the extension api - that would be useful. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:372: base::RunLoop().RunUntilIdle(); On 2016/10/25 16:38:35, igorcov wrote: > On 2016/10/20 21:20:44, Devlin wrote: > > If the "while" on line 364 was needed, then this wouldn't be sufficient. > > It's different, because we don't have to wait for another window to pop-up here > (like in the case above). I agree that it's not a safe bet here, as other window > could pop-up after the EXPECT check was done, but don't really see a better way. It's not really different, though. If the while () is necessary on line 364, then that means that it takes more than one spin to open a new window. If it takes more than one spin to open a new window, then checking that a window isn't open after one spin doesn't actually test anything. Right? https://codereview.chromium.org/2094333002/diff/890001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2094333002/diff/890001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6492: <message name="IDS_REQUEST_PIN_DIALOG_HEADER" desc="The text displayed certificate provider PIN request dialog."> The text displayed *in* the certificate....? https://codereview.chromium.org/2094333002/diff/890001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6495: <message name="IDS_REQUEST_PIN_DIALOG_PROCESSING" desc="The text displayed while the certificate provider API is waiting for notification request from extension."> what is a notification request? Wouldn't this be waiting for confirmation or something? https://codereview.chromium.org/2094333002/diff/890001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6498: <message name="IDS_REQUEST_PIN_DIALOG_INVALID_PIN_ERROR" desc="The error message displayed in certificate provider PIN request dialog when invalid PIN was entered."> in *the* certificate provider...? when *an* invalid PIN...? https://codereview.chromium.org/2094333002/diff/890001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6501: <message name="IDS_REQUEST_PIN_DIALOG_INVALID_PUK_ERROR" desc="The error message displayed in certificate provider PIN request dialog when invalid PUK was entered."> in *the* certificate when *an* invalid https://codereview.chromium.org/2094333002/diff/890001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6504: <message name="IDS_REQUEST_PIN_DIALOG_MAX_ATTEMPTS_EXCEEDED_ERROR" desc="The error message displayed in certificate provider PIN request dialog when maximum allowed attempts exceeded."> ditto https://codereview.chromium.org/2094333002/diff/890001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/890001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:191: EXTENSION_FUNCTION_VALIDATE(params.get()); this .get() is redundant (here and below) https://codereview.chromium.org/2094333002/diff/890001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:204: LOG(ERROR) << "Wrong extension requesting to close the dialog"; As mentioned, chrome shouldn't be logging errors for something that can conceivably happen. https://codereview.chromium.org/2094333002/diff/890001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:219: return RespondNow(Error(kNoActiveDialog)); Can't this also happen if the dialog isn't locked? https://codereview.chromium.org/2094333002/diff/890001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:265: api_cp::PinRequestType::PIN_REQUEST_TYPE_NONE) parens not needed https://codereview.chromium.org/2094333002/diff/890001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h (right): https://codereview.chromium.org/2094333002/diff/890001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h:20: // The maximum number of times per minute, extension is allowed to show PIN Is this the maximum number of times per minute, or per 10 minutes? https://codereview.chromium.org/2094333002/diff/890001/chrome/common/extensio... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2094333002/diff/890001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:16: enum PinRequestType { Document these enums https://codereview.chromium.org/2094333002/diff/890001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:64: PinRequestErrorType? errorType; Why don't we allow the extension to set the error directly? https://codereview.chromium.org/2094333002/diff/890001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:67: // this information to the user. Chrome is not expected to enforce this, This still strikes me as very odd - why doesn't Chrome enforce this? https://codereview.chromium.org/2094333002/diff/890001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js (right): https://codereview.chromium.org/2094333002/diff/890001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js:10: chrome.test.fail('Error: ' + chrome.runtime.lastError.message); We never actually check for test failures anywhere, do we? https://codereview.chromium.org/2094333002/diff/890001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js:27: var success = (codeValue.userInput == '1234'); // validatePIN(codeValue); parens unnecessary https://codereview.chromium.org/2094333002/diff/890001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js:27: var success = (codeValue.userInput == '1234'); // validatePIN(codeValue); what is this comment for?
https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:204: LOG(ERROR) << "Wrong extension requesting to close the dialog"; On 2016/10/26 16:59:15, Devlin wrote: > On 2016/10/25 16:38:35, igorcov wrote: > > On 2016/10/20 21:20:44, Devlin wrote: > > > When is this error going to be used/seen? Is it a chrome error? > > > > It happens in case extension is calling stopPinRequest without having an > actual > > active dialog. There are two cases mainly: > > 1 - extension didn't request PIN but is calling stopPinRequest. In this case > it > > is helpful for extension developer to know that there's a bug in the code. > > 2 - extension requested the PIN, but either user closed the dialog or the > > request was denied, which means a stopPinRequest is not expected by Chrome. > > > > The user will not see anything (because there's no dialog, or the active > dialog > > is owned by other extension), but the extension code will have > > chrome.runtime.lastError set. > > Errors in extension logic should result in an error being sent to the extension, > not a chrome error log. Developers may not read the commandline output from the > browser (and shouldn't have to), and we shouldn't add logspam for an extension > that has a bug. Agree, good point. https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc (right): https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:150: base::RunLoop().RunUntilIdle(); On 2016/10/26 16:59:15, Devlin (slow) wrote: > On 2016/10/25 16:38:35, igorcov wrote: > > On 2016/10/20 21:20:44, Devlin wrote: > > > Busy loops are generally bad, and timeouts over failures are harder to > debug. > > > Can we make this more deterministic? > > > > I could probably make an WidgetObserver and involve a couple of callbacks in > the > > code to wait for closing event from window. But I feel like it will complicate > > the code too much. And there are other places where I need to wait for some > > other type of events (line 164 too). > > What about waiting for a message from the extension instead? Then at least we > know the extension tried to close the window and can verify whether it happened. > Alternatively, writing a "WaitUntilWidgetClosed" is pretty simple: > > class WidgetClosedWaiter : public WidgetObserver { > public: > WidgetClosedWaiter(Widget* widget) : WidgetObserver(widget); > void OnWidgetDestroying(Widget* widget) override { > closed_ = true; > run_loop_.Quit(); > } > > void Wait() { > if (closed_) > return; > run_loop_.Run(); > } > > private: > bool closed_ = false; > base::RunLoop run_loop_; > }; > > WidgetClosedWaiter closed_waiter(widget); > // Do something > closed_waiter.Wait(); Implemented, thanks! https://codereview.chromium.org/2094333002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:210: content::WebContents* extension_contents = On 2016/10/26 16:59:15, Devlin (slow) wrote: > On 2016/10/25 16:38:35, igorcov wrote: > > On 2016/10/20 21:20:44, Devlin wrote: > > > How is that the extension_contents when the extension isn't loaded? Also, > if > > we > > > just use this for the browser context, let's just use the test class > accessor. > > > > You're right, we need this just for browser context. Don't understand though > > what do you mean by class accessor? > > Browser tests have a profile() accessor. Fixed, Thanks. https://codereview.chromium.org/2094333002/diff/890001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2094333002/diff/890001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6492: <message name="IDS_REQUEST_PIN_DIALOG_HEADER" desc="The text displayed certificate provider PIN request dialog."> On 2016/11/01 16:24:30, Devlin (slow) wrote: > The text displayed *in* the certificate....? Done. https://codereview.chromium.org/2094333002/diff/890001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6495: <message name="IDS_REQUEST_PIN_DIALOG_PROCESSING" desc="The text displayed while the certificate provider API is waiting for notification request from extension."> On 2016/11/01 16:24:30, Devlin (slow) wrote: > what is a notification request? Wouldn't this be waiting for confirmation or > something? Done. https://codereview.chromium.org/2094333002/diff/890001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6501: <message name="IDS_REQUEST_PIN_DIALOG_INVALID_PUK_ERROR" desc="The error message displayed in certificate provider PIN request dialog when invalid PUK was entered."> On 2016/11/01 16:24:30, Devlin (slow) wrote: > in *the* certificate > when *an* invalid Done. https://codereview.chromium.org/2094333002/diff/890001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6504: <message name="IDS_REQUEST_PIN_DIALOG_MAX_ATTEMPTS_EXCEEDED_ERROR" desc="The error message displayed in certificate provider PIN request dialog when maximum allowed attempts exceeded."> On 2016/11/01 16:24:30, Devlin (slow) wrote: > ditto Done. https://codereview.chromium.org/2094333002/diff/890001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/890001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:191: EXTENSION_FUNCTION_VALIDATE(params.get()); On 2016/11/01 16:24:30, Devlin (slow) wrote: > this .get() is redundant (here and below) Done. https://codereview.chromium.org/2094333002/diff/890001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:204: LOG(ERROR) << "Wrong extension requesting to close the dialog"; On 2016/11/01 16:24:30, Devlin (slow) wrote: > As mentioned, chrome shouldn't be logging errors for something that can > conceivably happen. Done. https://codereview.chromium.org/2094333002/diff/890001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:219: return RespondNow(Error(kNoActiveDialog)); On 2016/11/01 16:24:30, Devlin (slow) wrote: > Can't this also happen if the dialog isn't locked? Yes, the extension can close the dialog even if there was no input from the user (e.g. after some timeout) https://codereview.chromium.org/2094333002/diff/890001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:265: api_cp::PinRequestType::PIN_REQUEST_TYPE_NONE) On 2016/11/01 16:24:30, Devlin (slow) wrote: > parens not needed Done. https://codereview.chromium.org/2094333002/diff/890001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h (right): https://codereview.chromium.org/2094333002/diff/890001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h:20: // The maximum number of times per minute, extension is allowed to show PIN On 2016/11/01 16:24:30, Devlin (slow) wrote: > Is this the maximum number of times per minute, or per 10 minutes? Per 10 minutes, sorry. https://codereview.chromium.org/2094333002/diff/890001/chrome/common/extensio... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2094333002/diff/890001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:16: enum PinRequestType { On 2016/11/01 16:24:30, Devlin (slow) wrote: > Document these enums Done. https://codereview.chromium.org/2094333002/diff/890001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:64: PinRequestErrorType? errorType; On 2016/11/01 16:24:30, Devlin (slow) wrote: > Why don't we allow the extension to set the error directly? This proposal didn't pass security review. Basically it's a high risk of spoofing. https://codereview.chromium.org/2094333002/diff/890001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:67: // this information to the user. Chrome is not expected to enforce this, On 2016/11/01 16:24:30, Devlin (slow) wrote: > This still strikes me as very odd - why doesn't Chrome enforce this? We don't want to limit the behavior of the extension if possible. And if we decide to implement this enforcement, then we need to decide also what to do if the next request from the extension contradicts the previous one. https://codereview.chromium.org/2094333002/diff/890001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js (right): https://codereview.chromium.org/2094333002/diff/890001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js:10: chrome.test.fail('Error: ' + chrome.runtime.lastError.message); On 2016/11/01 16:24:30, Devlin (slow) wrote: > We never actually check for test failures anywhere, do we? Right, we don't have any test for that. https://codereview.chromium.org/2094333002/diff/890001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js:27: var success = (codeValue.userInput == '1234'); // validatePIN(codeValue); On 2016/11/01 16:24:30, Devlin (slow) wrote: > what is this comment for? It's just what that code is supposed to do. Fixed the comment. https://codereview.chromium.org/2094333002/diff/890001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js:27: var success = (codeValue.userInput == '1234'); // validatePIN(codeValue); On 2016/11/01 16:24:30, Devlin (slow) wrote: > parens unnecessary Done.
(just responding) https://codereview.chromium.org/2094333002/diff/890001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/890001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:219: return RespondNow(Error(kNoActiveDialog)); On 2016/11/04 15:51:42, igorcov wrote: > On 2016/11/01 16:24:30, Devlin (slow) wrote: > > Can't this also happen if the dialog isn't locked? > > Yes, the extension can close the dialog even if there was no input from the user > (e.g. after some timeout) What I mean is, can't UpdatePinDialog also return false if the dialog is in a locked state - in which case responding with "no active dialog" seems incorrect to me. https://codereview.chromium.org/2094333002/diff/890001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js (right): https://codereview.chromium.org/2094333002/diff/890001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js:10: chrome.test.fail('Error: ' + chrome.runtime.lastError.message); On 2016/11/04 15:51:42, igorcov wrote: > On 2016/11/01 16:24:30, Devlin (slow) wrote: > > We never actually check for test failures anywhere, do we? > > Right, we don't have any test for that. We should - having chrome.test.fail/chrome.test.succeed is pretty unhelpful if we never check the result. :)
Please take a look again, Devlin. https://codereview.chromium.org/2094333002/diff/890001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/890001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:219: return RespondNow(Error(kNoActiveDialog)); On 2016/11/05 06:10:59, Devlin wrote: > On 2016/11/04 15:51:42, igorcov wrote: > > On 2016/11/01 16:24:30, Devlin (slow) wrote: > > > Can't this also happen if the dialog isn't locked? > > > > Yes, the extension can close the dialog even if there was no input from the > user > > (e.g. after some timeout) > > What I mean is, can't UpdatePinDialog also return false if the dialog is in a > locked state - in which case responding with "no active dialog" seems incorrect > to me. You're right, I didn't notice that. Thank you. https://codereview.chromium.org/2094333002/diff/890001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js (right): https://codereview.chromium.org/2094333002/diff/890001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js:10: chrome.test.fail('Error: ' + chrome.runtime.lastError.message); On 2016/11/05 06:10:59, Devlin wrote: > On 2016/11/04 15:51:42, igorcov wrote: > > On 2016/11/01 16:24:30, Devlin (slow) wrote: > > > We never actually check for test failures anywhere, do we? > > > > Right, we don't have any test for that. > > We should - having chrome.test.fail/chrome.test.succeed is pretty unhelpful if > we never check the result. :) I've changed the code to use the chrome.test.sendMessage to test that we reach the expected branches in this JS code when simulating different scenarios in CC code.
A few last nits, but otherwise lgtm. https://codereview.chromium.org/2094333002/diff/970001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/970001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:223: return RespondNow(Error(kNoUserInput)); Why is this an error? Can the extension not cancel a flow without user input? https://codereview.chromium.org/2094333002/diff/970001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:328: service->pin_dialog_manager()->OnPinDialogInput(); Why is the API doing this instead of the dialog or the manager? https://codereview.chromium.org/2094333002/diff/970001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc (right): https://codereview.chromium.org/2094333002/diff/970001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:363: EXPECT_EQ("User closed the dialog", listener.message()); Here and below, you can pass an expected message to the listener. https://codereview.chromium.org/2094333002/diff/970001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:373: listener.reset(new ExtensionTestMessageListener(false)); nit: I'd just use two different listeners (stack-allocated) here. Otherwise there's technically a race condition if the extension responded before we reset it to listen to the next message (though it would never trigger since it's across a process boundary). https://codereview.chromium.org/2094333002/diff/970001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js (right): https://codereview.chromium.org/2094333002/diff/970001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js:12: if (chrome.runtime.lastError != null) { prefer just if (chrome.runtime.lastError) https://codereview.chromium.org/2094333002/diff/970001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic_lock.js (right): https://codereview.chromium.org/2094333002/diff/970001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic_lock.js:8: if(chrome.runtime.lastError != null) { nit: missing space between 'if' and '('
https://codereview.chromium.org/2094333002/diff/970001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc (right): https://codereview.chromium.org/2094333002/diff/970001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:223: return RespondNow(Error(kNoUserInput)); On 2016/11/29 21:09:03, Devlin wrote: > Why is this an error? Can the extension not cancel a flow without user input? The extension can cancel the flow without user input - covered by the "if" from above. This is the case when extension says the flow has to be stopped and this is the error. We want to display the error and not allow any more input from the user. I felt like it would be disturbing to allow the change of the error in an active dialog. Added a comment in the code. https://codereview.chromium.org/2094333002/diff/970001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:328: service->pin_dialog_manager()->OnPinDialogInput(); On 2016/11/29 21:09:03, Devlin wrote: > Why is the API doing this instead of the dialog or the manager? Moved to be notified by the dialog. https://codereview.chromium.org/2094333002/diff/970001/chrome/browser/extensi... File chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc (right): https://codereview.chromium.org/2094333002/diff/970001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:363: EXPECT_EQ("User closed the dialog", listener.message()); On 2016/11/29 21:09:04, Devlin wrote: > Here and below, you can pass an expected message to the listener. Done. https://codereview.chromium.org/2094333002/diff/970001/chrome/browser/extensi... chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:373: listener.reset(new ExtensionTestMessageListener(false)); On 2016/11/29 21:09:04, Devlin wrote: > nit: I'd just use two different listeners (stack-allocated) here. Otherwise > there's technically a race condition if the extension responded before we reset > it to listen to the next message (though it would never trigger since it's > across a process boundary). Done. https://codereview.chromium.org/2094333002/diff/970001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js (right): https://codereview.chromium.org/2094333002/diff/970001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic.js:12: if (chrome.runtime.lastError != null) { On 2016/11/29 21:09:04, Devlin wrote: > prefer just > if (chrome.runtime.lastError) Done. https://codereview.chromium.org/2094333002/diff/970001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic_lock.js (right): https://codereview.chromium.org/2094333002/diff/970001/chrome/test/data/exten... chrome/test/data/extensions/api_test/certificate_provider/request_pin/basic_lock.js:8: if(chrome.runtime.lastError != null) { On 2016/11/29 21:09:04, Devlin wrote: > nit: missing space between 'if' and '(' Done.
The CQ bit was checked by igorcov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by igorcov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by igorcov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, emaxx@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2094333002/#ps1010001 (title: "Merged the sources")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
igorcov@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine@ PTAL extensions/browser/extension_function_histogram_value.h and tools/metrics/histograms/histograms.xml Thank you
lgtm
The CQ bit was checked by igorcov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1010001, "attempt_start_ts": 1480608192564960,
"parent_rev": "13bc67c4a3a62e77925987faca7ea41246882aaa", "commit_rev":
"0e0b55043b7f1878c062e979b41e2c38da6291b7"}
Message was sent while issue was closed.
Description was changed from ========== Implementation for chrome.certificateProvider.requestPin/stopPinRequest The IDL for proposed API has been reviewed: https://codereview.chromium.org/2174423002/ This is the actual implementation of the API. The dialog to request PIN is defined in request_pin_view.h The CertificateProviderApi is receiving the requests from extensions and uses the service object to manage the view. CertificateProviderService object keeps the state of the view and forwards updates to it. And the RequestPinView is notifying the CertificateProviderApi about the user input through callback. BUG=612886 ========== to ========== Implementation for chrome.certificateProvider.requestPin/stopPinRequest The IDL for proposed API has been reviewed: https://codereview.chromium.org/2174423002/ This is the actual implementation of the API. The dialog to request PIN is defined in request_pin_view.h The CertificateProviderApi is receiving the requests from extensions and uses the service object to manage the view. CertificateProviderService object keeps the state of the view and forwards updates to it. And the RequestPinView is notifying the CertificateProviderApi about the user input through callback. BUG=612886 ==========
Message was sent while issue was closed.
Committed patchset #52 (id:1010001)
Message was sent while issue was closed.
Description was changed from ========== Implementation for chrome.certificateProvider.requestPin/stopPinRequest The IDL for proposed API has been reviewed: https://codereview.chromium.org/2174423002/ This is the actual implementation of the API. The dialog to request PIN is defined in request_pin_view.h The CertificateProviderApi is receiving the requests from extensions and uses the service object to manage the view. CertificateProviderService object keeps the state of the view and forwards updates to it. And the RequestPinView is notifying the CertificateProviderApi about the user input through callback. BUG=612886 ========== to ========== Implementation for chrome.certificateProvider.requestPin/stopPinRequest The IDL for proposed API has been reviewed: https://codereview.chromium.org/2174423002/ This is the actual implementation of the API. The dialog to request PIN is defined in request_pin_view.h The CertificateProviderApi is receiving the requests from extensions and uses the service object to manage the view. CertificateProviderService object keeps the state of the view and forwards updates to it. And the RequestPinView is notifying the CertificateProviderApi about the user input through callback. BUG=612886 Committed: https://crrev.com/bb898fb7cb786880d780d53c9d0d98ddeb37d491 Cr-Commit-Position: refs/heads/master@{#435620} ==========
Message was sent while issue was closed.
Patchset 52 (id:??) landed as https://crrev.com/bb898fb7cb786880d780d53c9d0d98ddeb37d491 Cr-Commit-Position: refs/heads/master@{#435620} |
