Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11932004:
Implemented attestation message flow for Chrome OS. (Closed)
|
Created:
7 years, 11 months ago by dkrahn Modified:
7 years, 10 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, oshima+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplemented attestation message flow for Chrome OS.
Chrome OS has an attestation service which can interact with a Privacy CA in order to generate an attestation certificate. The AttestationFlow class coordinates this interaction.
BUG=chromium-os:37806
TEST=unit
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178354
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180987
Patch Set 1 #
Total comments: 54
Patch Set 2 : #
Total comments: 23
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Messages
Total messages: 20 (0 generated)
+zelidrag - OWNER +mnissler - For general familiarity with attestation design
+satorux for DBUS part
chromeos/cryptohome LGTM with nits https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/cryptohome/mo... File chromeos/cryptohome/mock_async_method_caller.h (right): https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/cryptohome/mo... chromeos/cryptohome/mock_async_method_caller.h:19: static const char* kFakeAttestationEnrollRequest; nit: please do the following for consistency static const char kFakeAttestationEnrollRequest[];
https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... File chromeos/attestation/attestation.cc (right): https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.cc:11: nit: remove extra blank line. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.cc:15: const char* Attestation::kEnterpriseMachineKey = "attest-ent-machine"; The type of this should be const char[] https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.cc:51: void Attestation::DBusBoolRedirectCallback(const base::Closure& on_true, This function can be a static helper only visible to the implementation file. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.cc:61: base::Closure task = value ? on_true : on_false; declare as reference? https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... File chromeos/attestation/attestation.h (right): https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:15: #include "third_party/cros_system_api/dbus/service_constants.h" needed in this file? https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:17: nit: remove extra blank line https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:23: nit: remove extra blank line. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:33: public: This needs a virtual dtor. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:43: class CHROMEOS_EXPORT Attestation { nit: I think the name is a bit generic, seeing it I have no clue what this class does. Is it a data container that contains the Attestation cert? Is it the class that implements the entire Attestation flow? Better class-level comments on what this is good for and how you would use it would certainly help as well. Edit: After reading the rest of the code I think this should be named AttestationFlow or similar. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:45: typedef base::Callback<void(bool success)> StatusCallback; unused? https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:53: virtual ~Attestation(); Why all the virtualness in this class? https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:105: virtual void OnCreateEnrollRequest(const base::Closure& on_failure, Maybe rename to SendEnrollRequest? https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:138: // enrollment must success before this operation can succeed. fix "must success" https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:154: virtual void OnCreateCertificateRequest(const CertificateCallback& callback, Maybe rename to SendCertificateRequestToPCA? https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... File chromeos/attestation/attestation_unittest.cc (right): https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation_unittest.cc:13: remove extra blank line https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation_unittest.cc:24: void DbusCallbackFalse(const BoolDBusMethodCallback& callback) { nit: You spell Dbus with capital B elsewhere (i.e. DBus). Make consistent? https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation_unittest.cc:293: } // namespace attestation swap last two lines https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/m... File chromeos/attestation/mock_attestation.cc (right): https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/m... chromeos/attestation/mock_attestation.cc:9: nit: remove extra blank line https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/m... chromeos/attestation/mock_attestation.cc:36: } // namespace attestion https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/m... chromeos/attestation/mock_attestation.cc:37: } // namespace chromeos https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/m... File chromeos/attestation/mock_attestation.h (right): https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/m... chromeos/attestation/mock_attestation.h:9: remove extra blank line https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/m... chromeos/attestation/mock_attestation.h:61: class MockAttestation : public Attestation { It seems this is unused? https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/m... chromeos/attestation/mock_attestation.h:67: } // namespace attestation https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/m... chromeos/attestation/mock_attestation.h:68: } // namespace chromeos https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/chromeos.gyp#... chromeos/chromeos.gyp:33: 'attestation/attestation.h', alphabetize https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/chromeos.gyp#... chromeos/chromeos.gyp:210: 'attestation/mock_attestation.h', alphabetize
One more thing: Per https://groups.google.com/a/chromium.org/d/topic/chromium-dev/KY62p0l4Cas/dis... (and earlier discussions), please write a CL description that makes sense to people who have no background on RA.
Addressed all comments and improved CL description. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... File chromeos/attestation/attestation.cc (right): https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.cc:11: On 2013/01/16 10:39:26, Mattias Nissler wrote: > nit: remove extra blank line. Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.cc:15: const char* Attestation::kEnterpriseMachineKey = "attest-ent-machine"; On 2013/01/16 10:39:26, Mattias Nissler wrote: > The type of this should be const char[] Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.cc:51: void Attestation::DBusBoolRedirectCallback(const base::Closure& on_true, On 2013/01/16 10:39:26, Mattias Nissler wrote: > This function can be a static helper only visible to the implementation file. Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.cc:61: base::Closure task = value ? on_true : on_false; On 2013/01/16 10:39:26, Mattias Nissler wrote: > declare as reference? Was following the callback.h recommendation: "The Callback objects themselves should be passed by const-reference, and stored by copy." However, this isn't exactly 'storing' and a reference would be optimal. Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... File chromeos/attestation/attestation.h (right): https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:15: #include "third_party/cros_system_api/dbus/service_constants.h" On 2013/01/16 10:39:26, Mattias Nissler wrote: > needed in this file? Yes, for cryptohome::MountError... https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:17: On 2013/01/16 10:39:26, Mattias Nissler wrote: > nit: remove extra blank line Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:23: On 2013/01/16 10:39:26, Mattias Nissler wrote: > nit: remove extra blank line. Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:33: public: On 2013/01/16 10:39:26, Mattias Nissler wrote: > This needs a virtual dtor. Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:43: class CHROMEOS_EXPORT Attestation { On 2013/01/16 10:39:26, Mattias Nissler wrote: > nit: I think the name is a bit generic, seeing it I have no clue what this class > does. Is it a data container that contains the Attestation cert? Is it the class > that implements the entire Attestation flow? Better class-level comments on what > this is good for and how you would use it would certainly help as well. > > Edit: After reading the rest of the code I think this should be named > AttestationFlow or similar. Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:45: typedef base::Callback<void(bool success)> StatusCallback; On 2013/01/16 10:39:26, Mattias Nissler wrote: > unused? Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:53: virtual ~Attestation(); On 2013/01/16 10:39:26, Mattias Nissler wrote: > Why all the virtualness in this class? Removed. Was at one time thinking of allowing mocks to reuse / override these. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:105: virtual void OnCreateEnrollRequest(const base::Closure& on_failure, On 2013/01/16 10:39:26, Mattias Nissler wrote: > Maybe rename to SendEnrollRequest? Done. This was actually the original name of the method :). I couldn't decide which was better, your vote has sealed it. Added 'ToPCA' however as per your suggestion below; I think it's more clear. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:138: // enrollment must success before this operation can succeed. On 2013/01/16 10:39:26, Mattias Nissler wrote: > fix "must success" Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation.h:154: virtual void OnCreateCertificateRequest(const CertificateCallback& callback, On 2013/01/16 10:39:26, Mattias Nissler wrote: > Maybe rename to SendCertificateRequestToPCA? Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... File chromeos/attestation/attestation_unittest.cc (right): https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation_unittest.cc:13: On 2013/01/16 10:39:26, Mattias Nissler wrote: > remove extra blank line Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation_unittest.cc:24: void DbusCallbackFalse(const BoolDBusMethodCallback& callback) { On 2013/01/16 10:39:26, Mattias Nissler wrote: > nit: You spell Dbus with capital B elsewhere (i.e. DBus). Make consistent? Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/a... chromeos/attestation/attestation_unittest.cc:293: } // namespace attestation On 2013/01/16 10:39:26, Mattias Nissler wrote: > swap last two lines Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/m... File chromeos/attestation/mock_attestation.cc (right): https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/m... chromeos/attestation/mock_attestation.cc:9: On 2013/01/16 10:39:26, Mattias Nissler wrote: > nit: remove extra blank line Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/m... chromeos/attestation/mock_attestation.cc:36: } On 2013/01/16 10:39:26, Mattias Nissler wrote: > // namespace attestion Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/m... chromeos/attestation/mock_attestation.cc:37: } On 2013/01/16 10:39:26, Mattias Nissler wrote: > // namespace chromeos Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/m... File chromeos/attestation/mock_attestation.h (right): https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/m... chromeos/attestation/mock_attestation.h:9: On 2013/01/16 10:39:26, Mattias Nissler wrote: > remove extra blank line Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/m... chromeos/attestation/mock_attestation.h:61: class MockAttestation : public Attestation { On 2013/01/16 10:39:26, Mattias Nissler wrote: > It seems this is unused? Removed. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/m... chromeos/attestation/mock_attestation.h:67: } On 2013/01/16 10:39:26, Mattias Nissler wrote: > // namespace attestation Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/attestation/m... chromeos/attestation/mock_attestation.h:68: } On 2013/01/16 10:39:26, Mattias Nissler wrote: > // namespace chromeos Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/chromeos.gyp#... chromeos/chromeos.gyp:33: 'attestation/attestation.h', On 2013/01/16 10:39:26, Mattias Nissler wrote: > alphabetize Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/chromeos.gyp#... chromeos/chromeos.gyp:210: 'attestation/mock_attestation.h', On 2013/01/16 10:39:26, Mattias Nissler wrote: > alphabetize Done. https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/cryptohome/mo... File chromeos/cryptohome/mock_async_method_caller.h (right): https://chromiumcodereview.appspot.com/11932004/diff/1/chromeos/cryptohome/mo... chromeos/cryptohome/mock_async_method_caller.h:19: static const char* kFakeAttestationEnrollRequest; On 2013/01/15 23:04:13, satorux1 wrote: > nit: please do the following for consistency > > static const char kFakeAttestationEnrollRequest[]; Done.
Looks good for the most part. A few remaining nits, and some suggestions/questions. Also, have you considered error reporting for the flow? If something fails, we probably want to tell the user somehow. This will likely have to be added to this class, but it's fine to do so later. https://codereview.chromium.org/11932004/diff/10001/chromeos/attestation/atte... File chromeos/attestation/attestation_flow.cc (right): https://codereview.chromium.org/11932004/diff/10001/chromeos/attestation/atte... chromeos/attestation/attestation_flow.cc:32: on_fail.Run(); IIRC base::Callback implementors generally advise you to do checks on callback invocation like this: if (!on_fail.is_null()) on_fail.Run(); The benefit is that you can pass a dummy callback if you're not interested in the result of the operation. Same in the rest of the file. https://codereview.chromium.org/11932004/diff/10001/chromeos/attestation/atte... File chromeos/attestation/attestation_flow.h (right): https://codereview.chromium.org/11932004/diff/10001/chromeos/attestation/atte... chromeos/attestation/attestation_flow.h:58: virtual ~AttestationFlow(); drop virtual? https://codereview.chromium.org/11932004/diff/10001/chromeos/attestation/atte... chromeos/attestation/attestation_flow.h:72: virtual void GetCertificate(const std::string& name, drop virtual? https://codereview.chromium.org/11932004/diff/10001/chromeos/attestation/atte... chromeos/attestation/attestation_flow.h:76: static const char kEnterpriseMachineKey[]; could use a comment https://codereview.chromium.org/11932004/diff/10001/chromeos/attestation/atte... chromeos/attestation/attestation_flow.h:109: void OnEnrollResponse(const base::Closure& on_failure, Maybe rename to SendEnrollResponseToDaemon? https://codereview.chromium.org/11932004/diff/10001/chromeos/attestation/atte... chromeos/attestation/attestation_flow.h:156: void OnCertificateResponse(const CertificateCallback& callback, Maybe rename to SendCertificateResponseToDaemon? https://codereview.chromium.org/11932004/diff/10001/chromeos/attestation/atte... File chromeos/attestation/attestation_flow_unittest.cc (right): https://codereview.chromium.org/11932004/diff/10001/chromeos/attestation/atte... chromeos/attestation/attestation_flow_unittest.cc:45: // Use StrictMock when it is important that calls get triggered exactly once. This comment is not accurate: "Exactly once" you specify with Times(1) (which you do below). I guess you mean to say that some calls shouldn't get triggered at all and that is what you want to make sure gets tested? https://codereview.chromium.org/11932004/diff/10001/chromeos/attestation/atte... chromeos/attestation/attestation_flow_unittest.cc:62: .Times(1); Suggestion: It looks like that you might want to check as well that these calls happen in the right sequence, which you can do with the InSequence() class. https://codereview.chromium.org/11932004/diff/10001/chromeos/attestation/atte... chromeos/attestation/attestation_flow_unittest.cc:88: message_loop.RunUntilIdle(); according to base/message_loop.h you should use base::RunLoop instead. https://codereview.chromium.org/11932004/diff/10001/chromeos/attestation/atte... chromeos/attestation/attestation_flow_unittest.cc:268: TEST(AttestationTest, GetCertificate_FailIsEnrolled) { What's the difference between this test and GetCertificate_NoEK? They seem to be identical to me expect for the async_caller setup which you claim is not receiving any calls. I'm probably missing something. https://codereview.chromium.org/11932004/diff/10001/chromeos/attestation/mock... File chromeos/attestation/mock_attestation_flow.h (right): https://codereview.chromium.org/11932004/diff/10001/chromeos/attestation/mock... chromeos/attestation/mock_attestation_flow.h:23: callback.Run(result_, request + "_response"); #include "base/callback.h" https://codereview.chromium.org/11932004/diff/10001/chromeos/attestation/mock... chromeos/attestation/mock_attestation_flow.h:31: bool result_; DISALLOW_COPY_AND_ASSIGN(FakeServerProxy) and #include "base/basictypes.h"
My thoughts on error reporting is that it needs to happen where more context exists. A failure here doesn't mean much to a user without a broader sense of why the call was made. https://chromiumcodereview.appspot.com/11932004/diff/10001/chromeos/attestati... File chromeos/attestation/attestation_flow.cc (right): https://chromiumcodereview.appspot.com/11932004/diff/10001/chromeos/attestati... chromeos/attestation/attestation_flow.cc:32: on_fail.Run(); On 2013/01/18 13:38:46, Mattias Nissler wrote: > IIRC base::Callback implementors generally advise you to do checks on callback > invocation like this: > > if (!on_fail.is_null()) > on_fail.Run(); > > The benefit is that you can pass a dummy callback if you're not interested in > the result of the operation. > > Same in the rest of the file. Done. https://chromiumcodereview.appspot.com/11932004/diff/10001/chromeos/attestati... File chromeos/attestation/attestation_flow.h (right): https://chromiumcodereview.appspot.com/11932004/diff/10001/chromeos/attestati... chromeos/attestation/attestation_flow.h:58: virtual ~AttestationFlow(); On 2013/01/18 13:38:46, Mattias Nissler wrote: > drop virtual? I want the class to be mock-able and have plans to create a mock soon. https://chromiumcodereview.appspot.com/11932004/diff/10001/chromeos/attestati... chromeos/attestation/attestation_flow.h:76: static const char kEnterpriseMachineKey[]; On 2013/01/18 13:38:46, Mattias Nissler wrote: > could use a comment Done. https://chromiumcodereview.appspot.com/11932004/diff/10001/chromeos/attestati... chromeos/attestation/attestation_flow.h:109: void OnEnrollResponse(const base::Closure& on_failure, On 2013/01/18 13:38:46, Mattias Nissler wrote: > Maybe rename to SendEnrollResponseToDaemon? Done. https://chromiumcodereview.appspot.com/11932004/diff/10001/chromeos/attestati... chromeos/attestation/attestation_flow.h:156: void OnCertificateResponse(const CertificateCallback& callback, On 2013/01/18 13:38:46, Mattias Nissler wrote: > Maybe rename to SendCertificateResponseToDaemon? Done. https://chromiumcodereview.appspot.com/11932004/diff/10001/chromeos/attestati... File chromeos/attestation/attestation_flow_unittest.cc (right): https://chromiumcodereview.appspot.com/11932004/diff/10001/chromeos/attestati... chromeos/attestation/attestation_flow_unittest.cc:45: // Use StrictMock when it is important that calls get triggered exactly once. On 2013/01/18 13:38:46, Mattias Nissler wrote: > This comment is not accurate: "Exactly once" you specify with Times(1) (which > you do below). I guess you mean to say that some calls shouldn't get triggered > at all and that is what you want to make sure gets tested? Yes - comment corrected. https://chromiumcodereview.appspot.com/11932004/diff/10001/chromeos/attestati... chromeos/attestation/attestation_flow_unittest.cc:62: .Times(1); On 2013/01/18 13:38:46, Mattias Nissler wrote: > Suggestion: It looks like that you might want to check as well that these calls > happen in the right sequence, which you can do with the InSequence() class. Good idea. Done. https://chromiumcodereview.appspot.com/11932004/diff/10001/chromeos/attestati... chromeos/attestation/attestation_flow_unittest.cc:88: message_loop.RunUntilIdle(); On 2013/01/18 13:38:46, Mattias Nissler wrote: > according to base/message_loop.h you should use base::RunLoop instead. Done. https://chromiumcodereview.appspot.com/11932004/diff/10001/chromeos/attestati... chromeos/attestation/attestation_flow_unittest.cc:268: TEST(AttestationTest, GetCertificate_FailIsEnrolled) { On 2013/01/18 13:38:46, Mattias Nissler wrote: > What's the difference between this test and GetCertificate_NoEK? They seem to be > identical to me expect for the async_caller setup which you claim is not > receiving any calls. I'm probably missing something. The difference is the use of 'DBusCallbackFail' as the callback for TpmAttestationIsEnrolled. This test verifies that the code handles this failure properly. https://chromiumcodereview.appspot.com/11932004/diff/10001/chromeos/attestati... File chromeos/attestation/mock_attestation_flow.h (right): https://chromiumcodereview.appspot.com/11932004/diff/10001/chromeos/attestati... chromeos/attestation/mock_attestation_flow.h:23: callback.Run(result_, request + "_response"); On 2013/01/18 13:38:46, Mattias Nissler wrote: > #include "base/callback.h" Done. https://chromiumcodereview.appspot.com/11932004/diff/10001/chromeos/attestati... chromeos/attestation/mock_attestation_flow.h:31: bool result_; On 2013/01/18 13:38:46, Mattias Nissler wrote: > DISALLOW_COPY_AND_ASSIGN(FakeServerProxy) and #include "base/basictypes.h" Done.
LGTM then
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dkrahn@google.com/11932004/16001
Message was sent while issue was closed.
Change committed as 178354
This CL passed the commit queue but broke the chromeos asan build. Fixed errors and explicitly added asan / clang trybots.
still LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dkrahn@google.com/11932004/16011
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dkrahn@google.com/11932004/51001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dkrahn@google.com/11932004/51001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dkrahn@google.com/11932004/51001
Message was sent while issue was closed.
Change committed as 180987 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
