Make Encryptor a component. Used by //chrome and (soon) //components/webdata.

Move Encryptor to //components/webdata/encryptor.

Used by //components/autofill, //chrome/browser and (soon) //components/webdata itself.

Looking at [ git blame ], dhollowa@ and thestig@ seemed like the most likely candidates for the OWNERS. Let me know if you disagree.

This seems small enough not to warrant a dynamic lib, and it doesn't
have any singletons or such that would require it to be one (since it
is used by multiple other components in the components build), so I
made it a static library.

TBR=ben@chromium.org
BUG=181277
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190292

[Jói, 2013-03-21 23:38:12]

isherman@ for chrome/browser/password_manager/OWNERS.

dhollowa@ to check if new OWNERS list looks right, and because of the next
paragraph.

I realized as I wrapped this up for review that an alternative would be to make
this code part of //components/webdata.  It will be depended on by
//components/webdata, //components/autofill, and a few parts of //chrome. 
Because of the way the dependency graph between those parts flows, it would
actually be OK for the code to live in //components/webdata, e.g.
//components/webdata/common.

Let me know which you think is better.  It has no dependencies on the WebData
code, but since that is the "lowest layer" that uses it, so to speak, that's
where we could put it if you don't think it warrants a separate component.

Cheers,
Jói

[Ilya Sherman, 2013-03-22 01:34:34]

LGTM

https://codereview.chromium.org/12902030/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://codereview.chromium.org/12902030/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service_factory.cc:51: path =
path.Append(kWebDataFilename);
nit: Why is this diff part of this CL?

https://codereview.chromium.org/12902030/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/12902030/diff/1/chrome/chrome_browser.gypi#ne...
chrome/chrome_browser.gypi:2785: 'encryptor/encryptor_posix.cc',
Hmm:
  (a) is this filename actually correct if we don't want to build it on Mac?
  (b) Should this rule be moved into components.gyp:encryptor?

https://codereview.chromium.org/12902030/diff/1/components/encryptor/README
File components/encryptor/README (right):

https://codereview.chromium.org/12902030/diff/1/components/encryptor/README#n...
components/encryptor/README:4: facilities to perform the encryption.
Optional: Perhaps "system facilities" -> "system services"?

[Jói, 2013-03-22 10:49:55]

Thanks Ilya. I've uploaded a new patch that addresses your comments.

Cheers,
Jói

https://codereview.chromium.org/12902030/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_data_service_factory.cc (right):

https://codereview.chromium.org/12902030/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service_factory.cc:51: path =
path.Append(kWebDataFilename);
On 2013/03/22 01:34:35, Ilya Sherman wrote:
> nit: Why is this diff part of this CL?

Sorry, this was a bug in a local upstream branch. Gone.

https://codereview.chromium.org/12902030/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/12902030/diff/1/chrome/chrome_browser.gypi#ne...
chrome/chrome_browser.gypi:2785: 'encryptor/encryptor_posix.cc',
On 2013/03/22 01:34:35, Ilya Sherman wrote:
> Hmm:
>   (a) is this filename actually correct if we don't want to build it on Mac?
>   (b) Should this rule be moved into components.gyp:encryptor?

Whoops, I missed that one, thanks.  I've moved it to components.gyp:encryptor.

https://codereview.chromium.org/12902030/diff/1/components/encryptor/README
File components/encryptor/README (right):

https://codereview.chromium.org/12902030/diff/1/components/encryptor/README#n...
components/encryptor/README:4: facilities to perform the encryption.
On 2013/03/22 01:34:35, Ilya Sherman wrote:
> Optional: Perhaps "system facilities" -> "system services"?

Done.

[dhollowa, 2013-03-22 16:13:23]

On 2013/03/21 23:38:12, Jói wrote:
> isherman@ for chrome/browser/password_manager/OWNERS.
> 
> dhollowa@ to check if new OWNERS list looks right, and because of the next
> paragraph.
> 
> I realized as I wrapped this up for review that an alternative would be to
make
> this code part of //components/webdata.  It will be depended on by
> //components/webdata, //components/autofill, and a few parts of //chrome. 
> Because of the way the dependency graph between those parts flows, it would
> actually be OK for the code to live in //components/webdata, e.g.
> //components/webdata/common.
> 
> Let me know which you think is better.  It has no dependencies on the WebData
> code, but since that is the "lowest layer" that uses it, so to speak, that's
> where we could put it if you don't think it warrants a separate component.
> 
> Cheers,
> Jói

Yes, following the "principle of narrowest scope" it sounds like
//components/webdata/common or //components/webdata/encryptor would be most
appropriate.

[Jói, 2013-03-22 17:57:41]

Moved to //components/webdata/encryptor.

Cheers,
Jói


On Fri, Mar 22, 2013 at 4:13 PM,  <dhollowa@chromium.org> wrote:
> On 2013/03/21 23:38:12, Jói wrote:
>>
>> isherman@ for chrome/browser/password_manager/OWNERS.
>
>
>> dhollowa@ to check if new OWNERS list looks right, and because of the next
>> paragraph.
>
>
>> I realized as I wrapped this up for review that an alternative would be to
>
> make
>>
>> this code part of //components/webdata.  It will be depended on by
>> //components/webdata, //components/autofill, and a few parts of //chrome.
>> Because of the way the dependency graph between those parts flows, it
>> would
>> actually be OK for the code to live in //components/webdata, e.g.
>> //components/webdata/common.
>
>
>> Let me know which you think is better.  It has no dependencies on the
>> WebData
>> code, but since that is the "lowest layer" that uses it, so to speak,
>> that's
>> where we could put it if you don't think it warrants a separate component.
>
>
>> Cheers,
>> Jói
>
>
> Yes, following the "principle of narrowest scope" it sounds like
> //components/webdata/common or //components/webdata/encryptor would be most
> appropriate.
>
> https://codereview.chromium.org/12902030/

[dhollowa, 2013-03-22 18:13:07]

LGTM.  Thanks!

[commit-bot: I haz the power, 2013-03-24 17:32:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12902030/12045

[commit-bot: I haz the power, 2013-03-24 17:32:48]

Presubmit check for 12902030-12045 failed and returned exit status 1.

INFO:root:Found 30 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/sync/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/sync/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/signin/token_service_unittest.cc
    chrome/browser/sync/test/integration/sync_test.cc
    chrome/browser/signin/signin_manager_unittest.cc
    chrome/chrome_browser.gypi
    chrome/browser/DEPS
    chrome/browser/sync/glue/chrome_encryptor_unittest.cc
    sync/util/data_encryption_win.cc
    chrome/browser/sync/glue/chrome_encryptor.cc

Presubmit checks took 2.5s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Jói, 2013-03-24 17:34:31]

TBR=ben@chromium.org for many trivial automated include path updates, and fixes
to chrome/browser/DEPS for this change.

Cheers,
Jói

[commit-bot: I haz the power, 2013-03-24 17:34:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12902030/12045

[commit-bot: I haz the power, 2013-03-24 17:44:31]

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=linux_chro...

[commit-bot: I haz the power, 2013-03-24 17:57:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12902030/12045

[commit-bot: I haz the power, 2013-03-24 18:05:25]

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=linux_chro...

[commit-bot: I haz the power, 2013-03-24 19:12:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12902030/12045

[commit-bot: I haz the power, 2013-03-24 20:30:15]

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=linux_rel&...

[Jói, 2013-03-24 20:57:52]

Committed patchset #5 manually as r190292 (presubmit successful).

[Lei Zhang, 2013-03-25 20:22:08]

On 2013/03/24 20:57:52, Jói wrote:
> Committed patchset #5 manually as r190292 (presubmit successful).

Sure. I guess I can play owner...