Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2698103002:
Allow embedder to use custom cryptography in Autofill table. (Closed)
|
Created:
3 years, 10 months ago by devarajn Modified:
3 years, 10 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow embedder to use custom cryptography in Autofill table.
As of today, Autofill table makes static calls to OSCrypt
component for cyptography needs and does not allow embedders
to inject custom crytography. This change fixes it by
allowing Autofill table to take encryptor as dependency.
BUG=692826
TEST=components_unittests
Review-Url: https://codereview.chromium.org/2698103002
Cr-Commit-Position: refs/heads/master@{#452757}
Committed: https://chromium.googlesource.com/chromium/src/+/c0483e6b832ff9e9117c86fdf2641b389d9c2992
Patch Set 1 #
Total comments: 24
Patch Set 2 : Allow embedder to use custom cryptography in Autofill table. #
Total comments: 6
Patch Set 3 : Allow embedder to use custom cryptography in Autofill table. #
Total comments: 31
Patch Set 4 : Allow embedder to use custom cryptography in Autofill table. #
Total comments: 8
Patch Set 5 : Allow embedder to use custom cryptography in Autofill table. #
Total comments: 2
Patch Set 6 : Allow embedder to use custom cryptography in Autofill table. #
Total comments: 2
Patch Set 7 : Changed the ifdef name space #Messages
Total messages: 65 (25 generated)
Description was changed from ========== Allow embedder to use custom cryptography in Autofill table. As of today, Autofill table makes static calls to OSCrypt component for cyptography needs and does not allow embedders to inject custom crytography. This change fixes it by allowing Autofill table to take encryptor as dependency. BUG=692826 TEST=components_unittests ========== to ========== Allow embedder to use custom cryptography in Autofill table. As of today, Autofill table makes static calls to OSCrypt component for cyptography needs and does not allow embedders to inject custom crytography. This change fixes it by allowing Autofill table to take encryptor as dependency. BUG=692826 TEST=components_unittests ==========
devarajn@amazon.com changed reviewers: + sebsg@chromium.org
Hi sebsg@, I am trying to make Autofill table embedder friendly so that I can inject my own encryptor. Do you see any issues with this approach? PTAL, Thanks!
rogerm@chromium.org changed reviewers: + rogerm@chromium.org
Nice! https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... File components/autofill/core/browser/webdata/autofill_table.cc (right): https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table.cc:410: std::move(AutofillTableEncryptorFactory::GetInstance()->Create())) {} nit: you don't need to std::move() a temporary. std::move() turns its argument into a temporary. i.e., // Result bound to a name is no longer temporary, requiring // std::move()... auto foo = GetMoveOnlyObject(); transfer(std::move(foo)); // Is equivalent to... transfer(GetMoveOnlyObject()); You _do_ need it in the other constructor though. :) https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... File components/autofill/core/browser/webdata/autofill_table_encryptor.h (right): https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor.h:14: class AutofillTableEncryptor { this requires a virtual destructor be declared and defined; otherwise, the derived's destructor is never called. https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.cc (right): https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.cc:18: if (delegate_) { nit: no need for braces on single line if with no else https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.cc:19: return std::move(delegate_->Create()); nit: no need for std::move on return values https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.cc:23: return std::move(encryptor); #inlcude "base/memory/ptr_util.h" return base::MakeUnique<SystemEncryptor>(); // or... return std::unique_ptr<AutofillTableEncryptor>(new SystemEncryptor()); // or... return std::unique_ptr<SystemEncryptor>(new SystemEncryptor()); https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.cc:27: std::unique_ptr<autofill::AutofillTableEncryptorFactory::Delegate> nit: no need for autofill:: nit: no need for AutofillTableEncryptorFactory:: either, but the clarity is nice, so meh... https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:11: #include "components/autofill/core/browser/webdata/autofill_table_encryptor.h" nit: forward declare AutofillTaableEncryptor in namespace autofill instead of including its header. https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:19: class AutofillTableEncryptorFactory { What are the expected threading constraints on the factory as well as on the encryptor? Maybe add a base::SequenceChecker to the factory to assert that the factory and encryptor are always called from the same sequence. https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:25: virtual std::unique_ptr<autofill::AutofillTableEncryptor> Create(); nit: no need for autofill:: https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:33: std::unique_ptr<autofill::AutofillTableEncryptorFactory::Delegate> nit: no need for autofill::AutofillEncryptorFactory:: here and below
https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... File components/autofill/core/browser/webdata/autofill_table.cc (right): https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table.cc:410: std::move(AutofillTableEncryptorFactory::GetInstance()->Create())) {} On 2017/02/17 21:04:08, Roger McFarlane (Chromium) wrote: > nit: > > you don't need to std::move() a temporary. std::move() turns its argument into a > temporary. > > i.e., > > // Result bound to a name is no longer temporary, requiring > // std::move()... > auto foo = GetMoveOnlyObject(); > transfer(std::move(foo)); > > // Is equivalent to... > transfer(GetMoveOnlyObject()); > > You _do_ need it in the other constructor though. :) Done. https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... File components/autofill/core/browser/webdata/autofill_table_encryptor.h (right): https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor.h:14: class AutofillTableEncryptor { On 2017/02/17 21:04:08, Roger McFarlane (Chromium) wrote: > this requires a virtual destructor be declared and defined; otherwise, the > derived's destructor is never called. Done. https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.cc (right): https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.cc:19: return std::move(delegate_->Create()); On 2017/02/17 21:04:08, Roger McFarlane (Chromium) wrote: > nit: no need for std::move on return values Done. https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.cc:23: return std::move(encryptor); On 2017/02/17 21:04:08, Roger McFarlane (Chromium) wrote: > #inlcude "base/memory/ptr_util.h" > return base::MakeUnique<SystemEncryptor>(); > > // or... > return std::unique_ptr<AutofillTableEncryptor>(new SystemEncryptor()); > > // or... > return std::unique_ptr<SystemEncryptor>(new SystemEncryptor()); Done. https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.cc:27: std::unique_ptr<autofill::AutofillTableEncryptorFactory::Delegate> On 2017/02/17 21:04:08, Roger McFarlane (Chromium) wrote: > nit: no need for autofill:: > nit: no need for AutofillTableEncryptorFactory:: either, but the clarity is > nice, so meh... > Done. https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:11: #include "components/autofill/core/browser/webdata/autofill_table_encryptor.h" On 2017/02/17 21:04:08, Roger McFarlane (Chromium) wrote: > nit: forward declare AutofillTaableEncryptor in namespace autofill instead of > including its header. Done. https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:19: class AutofillTableEncryptorFactory { On 2017/02/17 21:04:09, Roger McFarlane (Chromium) wrote: > What are the expected threading constraints on the factory as well as on the > encryptor? Maybe add a base::SequenceChecker to the factory to assert that the > factory and encryptor are always called from the same sequence. Make sense! done. https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:25: virtual std::unique_ptr<autofill::AutofillTableEncryptor> Create(); On 2017/02/17 21:04:08, Roger McFarlane (Chromium) wrote: > nit: no need for autofill:: Done. https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:33: std::unique_ptr<autofill::AutofillTableEncryptorFactory::Delegate> On 2017/02/17 21:04:08, Roger McFarlane (Chromium) wrote: > nit: no need for autofill::AutofillEncryptorFactory:: > > here and below Done.
git cl upload again to add your latest patchset to the review
Pending git cl upload after addressing one doubt https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:19: class AutofillTableEncryptorFactory { On 2017/02/17 22:28:54, devarajn wrote: > On 2017/02/17 21:04:09, Roger McFarlane (Chromium) wrote: > > What are the expected threading constraints on the factory as well as on the > > encryptor? Maybe add a base::SequenceChecker to the factory to assert that the > > factory and encryptor are always called from the same sequence. > > Make sense! done. On second thought, can you explain little bit more here. My take on this is that checking whether calling factory or encryptor on current thread, shouldn't this be in consumer's contract? Please help me understand. Thank you
https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:19: class AutofillTableEncryptorFactory { On 2017/02/17 23:02:50, devarajn wrote: > On 2017/02/17 22:28:54, devarajn wrote: > > On 2017/02/17 21:04:09, Roger McFarlane (Chromium) wrote: > > > What are the expected threading constraints on the factory as well as on the > > > encryptor? Maybe add a base::SequenceChecker to the factory to assert that > the > > > factory and encryptor are always called from the same sequence. > > > > Make sense! done. > > On second thought, can you explain little bit more here. > My take on this is that checking whether calling factory or encryptor on current > thread, shouldn't this be in consumer's contract? > Please help me understand. > > Thank you Create() and SetDelegate() are not threadsafe. This is fine if they are always called from the same sequence/thread, which can be checked with a SequenceChecker. The factor is a singleton, so all consumers must adhere to the same instance of this contract.
https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:19: class AutofillTableEncryptorFactory { On 2017/02/21 20:57:05, Roger McFarlane (Chromium) wrote: > On 2017/02/17 23:02:50, devarajn wrote: > > On 2017/02/17 22:28:54, devarajn wrote: > > > On 2017/02/17 21:04:09, Roger McFarlane (Chromium) wrote: > > > > What are the expected threading constraints on the factory as well as on > the > > > > encryptor? Maybe add a base::SequenceChecker to the factory to assert that > > the > > > > factory and encryptor are always called from the same sequence. > > > > > > Make sense! done. > > > > On second thought, can you explain little bit more here. > > My take on this is that checking whether calling factory or encryptor on > current > > thread, shouldn't this be in consumer's contract? > > Please help me understand. > > > > Thank you > > Create() and SetDelegate() are not threadsafe. This is fine if they are always > called from the same sequence/thread, which can be checked with a > SequenceChecker. The factor is a singleton, so all consumers must adhere to the > same instance of this contract. Acknowledged.
Hello Reviewers, I have uploaded the second revision for more feedbacks. Quick question on code review protocol: Should I still get "lgtm" from both of you guys or should I remove @sebsg and get "lgtm" from Roger because I don't want to randomize you guys into crs unless its absolutely necessary :) Please guide me accordingly. Thank you
Hi, Could you please take another look. Thanks
The CQ bit was checked by rogerm@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2698103002/diff/20001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table.cc (right): https://codereview.chromium.org/2698103002/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.cc:413: : autofill_table_encryptor_(std::move(autofill_table_encryptor)) {} DCHECK(autofill_table_encrypter_ != nullptr); https://codereview.chromium.org/2698103002/diff/20001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:36: AutofillTableEncryptorFactory(); you need an explicitly public out-of-line destructor (per linter)
Hello, Could you please take another look Thank you https://codereview.chromium.org/2698103002/diff/20001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table.cc (right): https://codereview.chromium.org/2698103002/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.cc:413: : autofill_table_encryptor_(std::move(autofill_table_encryptor)) {} On 2017/02/22 18:01:27, Roger McFarlane (Chromium) wrote: > DCHECK(autofill_table_encrypter_ != nullptr); Added DCHECK(autofill_table_encrypter_); in constructor. Let me know if I am wrong https://codereview.chromium.org/2698103002/diff/20001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:36: AutofillTableEncryptorFactory(); On 2017/02/22 18:01:27, Roger McFarlane (Chromium) wrote: > you need an explicitly public out-of-line destructor > > (per linter) Sorry, I could not understand properly. Do you mean a destructor in private ~AutofillTableEncryptorFactory() {} Thank you
Hello, Could you please take another look. Thank you
https://codereview.chromium.org/2698103002/diff/20001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:36: AutofillTableEncryptorFactory(); On 2017/02/22 18:13:56, devarajn wrote: > On 2017/02/22 18:01:27, Roger McFarlane (Chromium) wrote: > > you need an explicitly public out-of-line destructor > > > > (per linter) > > Sorry, I could not understand properly. > Do you mean a destructor in private > ~AutofillTableEncryptorFactory() {} > > Thank you Check the compilation/lint failures of the try bots for patchset 2 https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:37: ~AutofillTableEncryptorFactory() {} move the implementation to the cc file.
rogerm@chromium.org changed reviewers: + rouslan@chromium.org
+rouslan Are you comforable with this the CC/payments info?
https://codereview.chromium.org/2698103002/diff/20001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:36: AutofillTableEncryptorFactory(); On 2017/02/22 18:32:56, Roger McFarlane (Chromium) wrote: > On 2017/02/22 18:13:56, devarajn wrote: > > On 2017/02/22 18:01:27, Roger McFarlane (Chromium) wrote: > > > you need an explicitly public out-of-line destructor > > > > > > (per linter) > > > > Sorry, I could not understand properly. > > Do you mean a destructor in private > > ~AutofillTableEncryptorFactory() {} > > > > Thank you > > Check the compilation/lint failures of the try bots for patchset 2 Acknowledged. https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:37: ~AutofillTableEncryptorFactory() {} On 2017/02/22 18:32:56, Roger McFarlane (Chromium) wrote: > move the implementation to the cc file. Yup I am on it, just saw below link https://www.chromium.org/developers/coding-style/chromium-style-checker-errors. I am building linux-build locally for sanity check and its really really slow :( Is it possible for me to use try servers directly?
Overall design is OK https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table.h (right): https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.h:17: #include "components/autofill/core/browser/webdata/autofill_table_encryptor.h" Can you forward-declare AutofillTableEncryptor instead? https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.h:255: AutofillTable( Please mark this constructor explicit to avoid accidental auto-conversion of AutofillTableEncryptor into AutofillTable. https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table_encryptor.h (right): https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no "(c)" necessary https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.cc (right): https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no "(c)" necessary https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.cc:21: if (delegate_) Single line: return delegate_ ? delgate_->Create() : base::MakeUnique<SystemEncryptor>(); https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no "(c)" necessary https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:15: // Factory for creating Autofill table encryptor. The comment should be on AutofillTableEncryptorFactory, not on DefailtSingletonTraits. https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:20: class AutofillTableEncryptorFactory { Very strange to have autofill::DefaultSingletonTraits and base::DefaultSingletonTraits in the same file. Should this forward declaration be removed? https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:24: class Delegate { Need a virtual empty destructor. https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:26: virtual std::unique_ptr<AutofillTableEncryptor> Create(); = 0; https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/system_encryptor.cc (right): https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/system_encryptor.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no "(c)" https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/system_encryptor.h (right): https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/system_encryptor.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no "(c)" necessary https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/system_encryptor.h:13: public: Override the parent's destructor to ensure proper cleanup. https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/system_encryptor.h:19: }; private: DISALLOW_COPY_AND_ASSIGN(SystemEncryptor);
https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table.h (right): https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.h:17: #include "components/autofill/core/browser/webdata/autofill_table_encryptor.h" On 2017/02/22 19:05:42, rouslan wrote: > Can you forward-declare AutofillTableEncryptor instead? Done. https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.h:255: AutofillTable( On 2017/02/22 19:05:42, rouslan wrote: > Please mark this constructor explicit to avoid accidental auto-conversion of > AutofillTableEncryptor into AutofillTable. Done. https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table_encryptor.h (right): https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/02/22 19:05:42, rouslan wrote: > no "(c)" necessary Done. https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.cc (right): https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.cc:21: if (delegate_) On 2017/02/22 19:05:42, rouslan wrote: > Single line: > > return delegate_ ? delgate_->Create() : base::MakeUnique<SystemEncryptor>(); Done. https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/02/22 19:05:42, rouslan wrote: > no "(c)" necessary Done. https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:15: // Factory for creating Autofill table encryptor. On 2017/02/22 19:05:42, rouslan wrote: > The comment should be on AutofillTableEncryptorFactory, not on > DefailtSingletonTraits. Done. https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:20: class AutofillTableEncryptorFactory { On 2017/02/22 19:05:42, rouslan wrote: > Very strange to have autofill::DefaultSingletonTraits and > base::DefaultSingletonTraits in the same file. Should this forward declaration > be removed? removed base:: one. Done https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:24: class Delegate { On 2017/02/22 19:05:42, rouslan wrote: > Need a virtual empty destructor. I am not sure how I missed this. Done https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/system_encryptor.cc (right): https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/system_encryptor.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/02/22 19:05:42, rouslan wrote: > no "(c)" Done. https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/system_encryptor.h (right): https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/system_encryptor.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/02/22 19:05:42, rouslan wrote: > no "(c)" necessary Done. https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/system_encryptor.h:13: public: On 2017/02/22 19:05:42, rouslan wrote: > Override the parent's destructor to ensure proper cleanup. Done. https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/system_encryptor.h:19: }; On 2017/02/22 19:05:42, rouslan wrote: > private: > DISALLOW_COPY_AND_ASSIGN(SystemEncryptor); Done.
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
The CQ bit was unchecked by rouslan@chromium.org
Will wait for the updated patch to pass the try bots.
https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:11: #include "components/autofill/core/browser/webdata/autofill_table_encryptor.h" On 2017/02/17 22:28:54, devarajn wrote: > On 2017/02/17 21:04:08, Roger McFarlane (Chromium) wrote: > > nit: forward declare AutofillTaableEncryptor in namespace autofill instead of > > including its header. > > Done. I think you forgot this one. It's not done. :) https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:19: struct DefaultSingletonTraits; This isn't doing what you want. It's declaring DefaultSingletonTraits<T> in the autofill namespace. But, you're already bringing it in via base/memory/singleton.h. If you want to forward declare this... // This goes outside of namespace autofill and remove the singleton include. namespace base { template typename<T> struct DefaultSingletonTraits; } // namespace
https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:37: ~AutofillTableEncryptorFactory() {} On 2017/02/22 18:56:58, devarajn wrote: > On 2017/02/22 18:32:56, Roger McFarlane (Chromium) wrote: > > move the implementation to the cc file. > > Yup I am on it, just saw below link > https://www.chromium.org/developers/coding-style/chromium-style-checker-errors. > > I am building linux-build locally for sanity check and its really really slow :( > Is it possible for me to use try servers directly? > # after git cl upload git cl try I think you need to have a chromium.org account, though.
The CQ bit was checked by devarajn@amazon.com 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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:11: #include "components/autofill/core/browser/webdata/autofill_table_encryptor.h" On 2017/02/22 19:29:50, Roger McFarlane (Chromium) wrote: > On 2017/02/17 22:28:54, devarajn wrote: > > On 2017/02/17 21:04:08, Roger McFarlane (Chromium) wrote: > > > nit: forward declare AutofillTaableEncryptor in namespace autofill instead > of > > > including its header. > > > > Done. > > I think you forgot this one. It's not done. :) Sorry missed it. I will ensure new revision has it. https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:19: struct DefaultSingletonTraits; On 2017/02/22 19:29:50, Roger McFarlane (Chromium) wrote: > This isn't doing what you want. It's declaring DefaultSingletonTraits<T> in the > autofill namespace. But, you're already bringing it in via > base/memory/singleton.h. > > If you want to forward declare this... > > // This goes outside of namespace autofill and remove the singleton include. > namespace base { > template typename<T> struct DefaultSingletonTraits; > } // namespace Yeah, I realized it. This was a good catch!
lgtm % comment: https://codereview.chromium.org/2698103002/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table.h (right): https://codereview.chromium.org/2698103002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.h:473: std::unique_ptr<AutofillTableEncryptor> autofill_table_encryptor_; Please move the variable below all methods. (Line 553.)
Nice! https://codereview.chromium.org/2698103002/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table.h (right): https://codereview.chromium.org/2698103002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.h:256: std::unique_ptr<AutofillTableEncryptor> autofill_table_encryptor); This additional public constructor seems redundant with the factory. If the encryptor is exposed to the embedder via the factory delegate, then there's not need for this constructor. the embedder just sets the delegate the this class will do the right thing in getting the encryptor from the factory. If the encryptor is taken as a constructor param, then the factory is redundant, as the autofill construction call-sites could be updated by the embedder to instantiate a different encryptor. Is there a use case where this constructor is called, outside of constructor delegation? If not, then just collapse this with the default constructor and always use the factory. https://codereview.chromium.org/2698103002/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:30: virtual std::unique_ptr<AutofillTableEncryptor> Create(); make this pure virtual
https://codereview.chromium.org/2698103002/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table.h (right): https://codereview.chromium.org/2698103002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.h:256: std::unique_ptr<AutofillTableEncryptor> autofill_table_encryptor); On 2017/02/22 22:50:52, Roger McFarlane (Chromium) wrote: > This additional public constructor seems redundant with the factory. > > If the encryptor is exposed to the embedder via the factory delegate, then > there's not need for this constructor. the embedder just sets the delegate the > this class will do the right thing in getting the encryptor from the factory. > > If the encryptor is taken as a constructor param, then the factory is redundant, > as the autofill construction call-sites could be updated by the embedder to > instantiate a different encryptor. > > Is there a use case where this constructor is called, outside of constructor > delegation? If not, then just collapse this with the default constructor and > always use the factory. We can use this constructor to inject mocks in unit tests. Let me know what you think? https://codereview.chromium.org/2698103002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.h:473: std::unique_ptr<AutofillTableEncryptor> autofill_table_encryptor_; On 2017/02/22 22:34:58, rouslan wrote: > Please move the variable below all methods. (Line 553.) Acknowledged. https://codereview.chromium.org/2698103002/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:30: virtual std::unique_ptr<AutofillTableEncryptor> Create(); On 2017/02/22 22:50:52, Roger McFarlane (Chromium) wrote: > make this pure virtual Acknowledged.
https://codereview.chromium.org/2698103002/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table.h (right): https://codereview.chromium.org/2698103002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.h:256: std::unique_ptr<AutofillTableEncryptor> autofill_table_encryptor); On 2017/02/22 23:10:54, devarajn wrote: > On 2017/02/22 22:50:52, Roger McFarlane (Chromium) wrote: > > This additional public constructor seems redundant with the factory. > > > > If the encryptor is exposed to the embedder via the factory delegate, then > > there's not need for this constructor. the embedder just sets the delegate the > > this class will do the right thing in getting the encryptor from the factory. > > > > If the encryptor is taken as a constructor param, then the factory is > redundant, > > as the autofill construction call-sites could be updated by the embedder to > > instantiate a different encryptor. > > > > Is there a use case where this constructor is called, outside of constructor > > delegation? If not, then just collapse this with the default constructor and > > always use the factory. > > We can use this constructor to inject mocks in unit tests. Let me know what you > think? > The factory can also be used to inject mocks into the unittests, which also serves to test the factory. Let's get rid of this constructor.
https://codereview.chromium.org/2698103002/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table.h (right): https://codereview.chromium.org/2698103002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.h:256: std::unique_ptr<AutofillTableEncryptor> autofill_table_encryptor); On 2017/02/22 23:20:34, Roger McFarlane (Chromium) wrote: > On 2017/02/22 23:10:54, devarajn wrote: > > On 2017/02/22 22:50:52, Roger McFarlane (Chromium) wrote: > > > This additional public constructor seems redundant with the factory. > > > > > > If the encryptor is exposed to the embedder via the factory delegate, then > > > there's not need for this constructor. the embedder just sets the delegate > the > > > this class will do the right thing in getting the encryptor from the > factory. > > > > > > If the encryptor is taken as a constructor param, then the factory is > > redundant, > > > as the autofill construction call-sites could be updated by the embedder to > > > instantiate a different encryptor. > > > > > > Is there a use case where this constructor is called, outside of constructor > > > delegation? If not, then just collapse this with the default constructor and > > > always use the factory. > > > > We can use this constructor to inject mocks in unit tests. Let me know what > you > > think? > > > > The factory can also be used to inject mocks into the unittests, which also > serves to test the factory. > > Let's get rid of this constructor. Alright. I will remove this constructor.
devarajn@amazon.com changed reviewers: - sebsg@chromium.org
Hi, Please take another look. Thank you
The CQ bit was checked by devarajn@amazon.com 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/2698103002/diff/80001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table.h (right): https://codereview.chromium.org/2698103002/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.h:256: std::unique_ptr<AutofillTableEncryptor> autofill_table_encryptor); delete?
when you upload... git cl upload -t "description" It will help us tell your patchsets apart.
https://codereview.chromium.org/2698103002/diff/80001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table.h (right): https://codereview.chromium.org/2698103002/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.h:256: std::unique_ptr<AutofillTableEncryptor> autofill_table_encryptor); On 2017/02/22 23:46:26, Roger McFarlane (Chromium) wrote: > delete? Damn me! Rushed to get "lgtm" from you :)
The CQ bit was checked by devarajn@amazon.com to run a CQ dry run
Hi, Please take another look. Thanks
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/22 23:50:45, Roger McFarlane (Chromium) wrote: > when you upload... > > git cl upload -t "description" > > It will help us tell your patchsets apart. Sure. did not know about it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi, Could you please take another look. Thank you
Looks good except for one last nit https://codereview.chromium.org/2698103002/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:6: #define COMPONENTS_WEBDATA_SERVICES_AUTOFILL_TABLE_ENCRYPTOR_FACTORY_H_ Sorry I didn't see this before (and I'm surprised presumbit on upload didn't catch this!) COMPONENTS_AUTOFILL_CORE_BROWSER_WEBDATA_AUTOFILL_TABLE_ENCRYPTOR_FACTORY_H_
https://codereview.chromium.org/2698103002/diff/100001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h (right): https://codereview.chromium.org/2698103002/diff/100001/components/autofill/co... components/autofill/core/browser/webdata/autofill_table_encryptor_factory.h:6: #define COMPONENTS_WEBDATA_SERVICES_AUTOFILL_TABLE_ENCRYPTOR_FACTORY_H_ On 2017/02/24 03:00:13, Roger McFarlane (Chromium) wrote: > Sorry I didn't see this before (and I'm surprised presumbit on upload didn't > catch this!) > > COMPONENTS_AUTOFILL_CORE_BROWSER_WEBDATA_AUTOFILL_TABLE_ENCRYPTOR_FACTORY_H_ Good catch!. Done. Will post a new revision in a minute
Addressed @rogerm's feedback. Could you please take another look. Thank you
lgtm
On 2017/02/24 03:49:52, Roger McFarlane (Chromium) wrote: > lgtm Thanks for reviewing code at late hours :)
The CQ bit was checked by devarajn@amazon.com
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2698103002/#ps120001 (title: "Changed the ifdef name space")
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": 120001, "attempt_start_ts": 1487908417307770,
"parent_rev": "629db08eb5b5b7823c8fcdd243900bb302392bc7", "commit_rev":
"c0483e6b832ff9e9117c86fdf2641b389d9c2992"}
Message was sent while issue was closed.
Description was changed from ========== Allow embedder to use custom cryptography in Autofill table. As of today, Autofill table makes static calls to OSCrypt component for cyptography needs and does not allow embedders to inject custom crytography. This change fixes it by allowing Autofill table to take encryptor as dependency. BUG=692826 TEST=components_unittests ========== to ========== Allow embedder to use custom cryptography in Autofill table. As of today, Autofill table makes static calls to OSCrypt component for cyptography needs and does not allow embedders to inject custom crytography. This change fixes it by allowing Autofill table to take encryptor as dependency. BUG=692826 TEST=components_unittests Review-Url: https://codereview.chromium.org/2698103002 Cr-Commit-Position: refs/heads/master@{#452757} Committed: https://chromium.googlesource.com/chromium/src/+/c0483e6b832ff9e9117c86fdf264... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/c0483e6b832ff9e9117c86fdf264... |
