Sync: Add encryption customization radio.

Sync: Add encryption customization radio.

BUG=82175
TEST=none

R=csilv@chromium.org,zea@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86441

[csilv, 2011-05-20 22:14:03]

LGTM

[Nicolas Zea, 2011-05-20 23:07:32]

Would it be possible to get this behind a chrome switch somehow? It will help in
case we need to punt.

[James Hawkins, 2011-05-21 00:48:30]

On 2011/05/20 23:07:32, nzea wrote:
> Would it be possible to get this behind a chrome switch somehow? It will help
in
> case we need to punt.

This is too fine-grained to put behind a switch.  It is trivial to hide this UI
on any particular branch.

[Nicolas Zea, 2011-05-22 00:10:32]

One comment.

Also, when I try to patch this in locally I get a check failure in l10n_util.cc
FATAL:l10n_util.cc(610)] Check failed: std::string::npos != pos
(18446744073709551615 vs. 18446744073709551615) Didn't find a $1 placeholder in
Google Account

Not sure if that's because I need to rebuilt some target or something else.

Lastly, since it looks like this may not make it into m13 (due to backend
issues), and you say it's easy to hide the UI, could we go ahead and have it
hidden by default?

http://codereview.chromium.org/7056021/diff/1/chrome/browser/sync/sync_setup_...
File chrome/browser/sync/sync_setup_flow.cc (right):

http://codereview.chromium.org/7056021/diff/1/chrome/browser/sync/sync_setup_...
chrome/browser/sync/sync_setup_flow.cc:149: encrypted_types.begin(),
encrypted_types.end(), registered_types.begin());
Thinking about this further, I'm hesitant about this comparison. If a new
version comes out with a new datatype that hasn't been encrypted yet, it will be
part of registered_type but not part of encrypted_types, and encrypt_all will be
false even though it should be true. 

Could we just, at least for now, see if any types more than passwords are
enabled, and if so set encrypt_all to true? Later if "sensitive types" is
defined in the PSS we can query that directly.

[James Hawkins, 2011-05-23 18:18:31]

On 2011/05/22 00:10:32, nzea wrote:
> One comment.
> 
> Also, when I try to patch this in locally I get a check failure in
l10n_util.cc
> FATAL:l10n_util.cc(610)] Check failed: std::string::npos != pos
> (18446744073709551615 vs. 18446744073709551615) Didn't find a $1 placeholder
in
> Google Account
> 
> Not sure if that's because I need to rebuilt some target or something else.
> 
> Lastly, since it looks like this may not make it into m13 (due to backend
> issues), and you say it's easy to hide the UI, could we go ahead and have it
> hidden by default?
> 

Then it's essentially dead code; I'd rather just not commit this CL.

What is the status on this?

[Nicolas Zea, 2011-05-23 19:41:01]

On 2011/05/23 18:18:31, James Hawkins wrote:
> On 2011/05/22 00:10:32, nzea wrote:
> > One comment.
> > 
> > Also, when I try to patch this in locally I get a check failure in
> l10n_util.cc
> > FATAL:l10n_util.cc(610)] Check failed: std::string::npos != pos
> > (18446744073709551615 vs. 18446744073709551615) Didn't find a $1 placeholder
> in
> > Google Account
> > 
> > Not sure if that's because I need to rebuilt some target or something else.
> > 
> > Lastly, since it looks like this may not make it into m13 (due to backend
> > issues), and you say it's easy to hide the UI, could we go ahead and have it
> > hidden by default?
> > 
> 
> Then it's essentially dead code; I'd rather just not commit this CL.
> 
> What is the status on this?

We're pushing this back to m14 to give this time to test on the dev channel.
Since we need the UI for proper testing, but it's not ready to go live just yet,
what do you think of merging the changes in (or something similar to)
http://codereview.chromium.org/7066005/ into here.

[James Hawkins, 2011-05-23 21:25:44]

http://codereview.chromium.org/7056021/diff/1/chrome/browser/sync/sync_setup_...
File chrome/browser/sync/sync_setup_flow.cc (right):

http://codereview.chromium.org/7056021/diff/1/chrome/browser/sync/sync_setup_...
chrome/browser/sync/sync_setup_flow.cc:149: encrypted_types.begin(),
encrypted_types.end(), registered_types.begin());
On 2011/05/22 00:10:32, nzea wrote:
> Thinking about this further, I'm hesitant about this comparison. If a new
> version comes out with a new datatype that hasn't been encrypted yet, it will
be
> part of registered_type but not part of encrypted_types, and encrypt_all will
be
> false even though it should be true. 
> 
> Could we just, at least for now, see if any types more than passwords are
> enabled, and if so set encrypt_all to true? Later if "sensitive types" is
> defined in the PSS we can query that directly.

Done.

[Nicolas Zea, 2011-05-23 22:40:26]

LGTM