Hide Autofill Wallet checkbox when not syncing autofill data.

Hide Autofill Wallet checkbox when not syncing autofill data.

If you are not signed into sync or have unchecked "Autofill" then the option should be hidden.

BUG=468987
Committed: https://crrev.com/5e26b8a570bb183c6a3686555fc61e545a3868c2
Cr-Commit-Position: refs/heads/master@{#322243}

[Evan Stade, 2015-03-20 22:29:42]

estade@chromium.org changed reviewers:
+ brettw@chromium.org, zea@chromium.org

[Evan Stade, 2015-03-20 22:29:42]

we still need to disable the AUTOFILL_WALLET_DATA type when the user unchecks
the box in chrome://settings/autofill. That's crbug.com/450843

[Nicolas Zea, 2015-03-20 22:36:33]

https://codereview.chromium.org/1020303007/diff/20001/chrome/browser/autofill...
File chrome/browser/autofill/options_util.cc (right):

https://codereview.chromium.org/1020303007/diff/20001/chrome/browser/autofill...
chrome/browser/autofill/options_util.cc:13: // TODO(estade): what to do in the
IsManaged case?
IsSyncEnabledAndLoggedIn will return false if IsManaged is false.

https://codereview.chromium.org/1020303007/diff/20001/chrome/browser/autofill...
chrome/browser/autofill/options_util.cc:18:
service->GetRegisteredDataTypes().Has(syncer::AUTOFILL_WALLET_DATA);
nit: no need to check registered types. Active types already checks this.

Also note that active types means the type is up and running, which means that
at startup before sync has finished initializing this will be false. If that's
an issue you can use GetPreferredDataTypes instead.

[Evan Stade, 2015-03-21 00:51:29]

https://codereview.chromium.org/1020303007/diff/20001/chrome/browser/autofill...
File chrome/browser/autofill/options_util.cc (right):

https://codereview.chromium.org/1020303007/diff/20001/chrome/browser/autofill...
chrome/browser/autofill/options_util.cc:13: // TODO(estade): what to do in the
IsManaged case?
On 2015/03/20 22:36:33, Nicolas Zea wrote:
> IsSyncEnabledAndLoggedIn will return false if IsManaged is false.

did you mean: "IsSyncEnabledAndLoggedIn will return false if IsManaged is true"
?

https://codereview.chromium.org/1020303007/diff/20001/chrome/browser/autofill...
chrome/browser/autofill/options_util.cc:18:
service->GetRegisteredDataTypes().Has(syncer::AUTOFILL_WALLET_DATA);
On 2015/03/20 22:36:33, Nicolas Zea wrote:
> nit: no need to check registered types. Active types already checks this.

Perhaps this is a bit subtle. We check whether AUTOFILL_PROFILE is active (or
preferred I guess), because unchecking that box in sync settings should hide
this checkbox. We also check whether AUTOFILL_WALLET_DATA is registered. The
latter will be false for people who do not have the sync experiment enabled.

> 
> Also note that active types means the type is up and running, which means that
> at startup before sync has finished initializing this will be false. If that's
> an issue you can use GetPreferredDataTypes instead.

ah, I should have read the docs more closely.

[Nicolas Zea, 2015-03-23 17:00:17]

https://codereview.chromium.org/1020303007/diff/20001/chrome/browser/autofill...
File chrome/browser/autofill/options_util.cc (right):

https://codereview.chromium.org/1020303007/diff/20001/chrome/browser/autofill...
chrome/browser/autofill/options_util.cc:13: // TODO(estade): what to do in the
IsManaged case?
On 2015/03/21 00:51:29, Evan Stade wrote:
> On 2015/03/20 22:36:33, Nicolas Zea wrote:
> > IsSyncEnabledAndLoggedIn will return false if IsManaged is false.
> 
> did you mean: "IsSyncEnabledAndLoggedIn will return false if IsManaged is
true"
> ?

Whoops, yes, that's what I meant.

https://codereview.chromium.org/1020303007/diff/20001/chrome/browser/autofill...
chrome/browser/autofill/options_util.cc:18:
service->GetRegisteredDataTypes().Has(syncer::AUTOFILL_WALLET_DATA);
On 2015/03/21 00:51:29, Evan Stade wrote:
> On 2015/03/20 22:36:33, Nicolas Zea wrote:
> > nit: no need to check registered types. Active types already checks this.
> 
> Perhaps this is a bit subtle. We check whether AUTOFILL_PROFILE is active (or
> preferred I guess), because unchecking that box in sync settings should hide
> this checkbox. We also check whether AUTOFILL_WALLET_DATA is registered. The
> latter will be false for people who do not have the sync experiment enabled.

Right, but my point is that GetActiveDataTypes and GetPreferredDataTypes are
already filtering for registered types. In other words, if it's not registered,
it can't be preferred or active.

> 
> > 
> > Also note that active types means the type is up and running, which means
that
> > at startup before sync has finished initializing this will be false. If
that's
> > an issue you can use GetPreferredDataTypes instead.
> 
> ah, I should have read the docs more closely.

[Evan Stade, 2015-03-23 17:06:12]

https://codereview.chromium.org/1020303007/diff/20001/chrome/browser/autofill...
File chrome/browser/autofill/options_util.cc (right):

https://codereview.chromium.org/1020303007/diff/20001/chrome/browser/autofill...
chrome/browser/autofill/options_util.cc:18:
service->GetRegisteredDataTypes().Has(syncer::AUTOFILL_WALLET_DATA);
On 2015/03/23 17:00:17, Nicolas Zea wrote:
> On 2015/03/21 00:51:29, Evan Stade wrote:
> > On 2015/03/20 22:36:33, Nicolas Zea wrote:
> > > nit: no need to check registered types. Active types already checks this.
> > 
> > Perhaps this is a bit subtle. We check whether AUTOFILL_PROFILE is active
(or
> > preferred I guess), because unchecking that box in sync settings should hide
> > this checkbox. We also check whether AUTOFILL_WALLET_DATA is registered. The
> > latter will be false for people who do not have the sync experiment enabled.
> 
> Right, but my point is that GetActiveDataTypes and GetPreferredDataTypes are
> already filtering for registered types. In other words, if it's not
registered,
> it can't be preferred or active.

this code doesn't care if AUTOFILL_WALLET_DATA is preferred or active

> 
> > 
> > > 
> > > Also note that active types means the type is up and running, which means
> that
> > > at startup before sync has finished initializing this will be false. If
> that's
> > > an issue you can use GetPreferredDataTypes instead.
> > 
> > ah, I should have read the docs more closely.
>

[Nicolas Zea, 2015-03-23 17:37:17]

sync usage LGTM

https://codereview.chromium.org/1020303007/diff/20001/chrome/browser/autofill...
File chrome/browser/autofill/options_util.cc (right):

https://codereview.chromium.org/1020303007/diff/20001/chrome/browser/autofill...
chrome/browser/autofill/options_util.cc:18:
service->GetRegisteredDataTypes().Has(syncer::AUTOFILL_WALLET_DATA);
On 2015/03/23 17:06:12, Evan Stade wrote:
> On 2015/03/23 17:00:17, Nicolas Zea wrote:
> > On 2015/03/21 00:51:29, Evan Stade wrote:
> > > On 2015/03/20 22:36:33, Nicolas Zea wrote:
> > > > nit: no need to check registered types. Active types already checks
this.
> > > 
> > > Perhaps this is a bit subtle. We check whether AUTOFILL_PROFILE is active
> (or
> > > preferred I guess), because unchecking that box in sync settings should
hide
> > > this checkbox. We also check whether AUTOFILL_WALLET_DATA is registered.
The
> > > latter will be false for people who do not have the sync experiment
enabled.
> > 
> > Right, but my point is that GetActiveDataTypes and GetPreferredDataTypes are
> > already filtering for registered types. In other words, if it's not
> registered,
> > it can't be preferred or active.
> 
> this code doesn't care if AUTOFILL_WALLET_DATA is preferred or active
> 
> > 
> > > 
> > > > 
> > > > Also note that active types means the type is up and running, which
means
> > that
> > > > at startup before sync has finished initializing this will be false. If
> > that's
> > > > an issue you can use GetPreferredDataTypes instead.
> > > 
> > > ah, I should have read the docs more closely.
> > 

Ah crap, I just realized we're talking about too different datatypes
(AUTOFILL_PROFILE VS AUTOFILL_WALLET_DATA). My mistake.

[Evan Stade, 2015-03-24 15:28:24]

On 2015/03/23 17:37:17, Nicolas Zea wrote:
> sync usage LGTM
> 
>
https://codereview.chromium.org/1020303007/diff/20001/chrome/browser/autofill...
> File chrome/browser/autofill/options_util.cc (right):
> 
>
https://codereview.chromium.org/1020303007/diff/20001/chrome/browser/autofill...
> chrome/browser/autofill/options_util.cc:18:
> service->GetRegisteredDataTypes().Has(syncer::AUTOFILL_WALLET_DATA);
> On 2015/03/23 17:06:12, Evan Stade wrote:
> > On 2015/03/23 17:00:17, Nicolas Zea wrote:
> > > On 2015/03/21 00:51:29, Evan Stade wrote:
> > > > On 2015/03/20 22:36:33, Nicolas Zea wrote:
> > > > > nit: no need to check registered types. Active types already checks
> this.
> > > > 
> > > > Perhaps this is a bit subtle. We check whether AUTOFILL_PROFILE is
active
> > (or
> > > > preferred I guess), because unchecking that box in sync settings should
> hide
> > > > this checkbox. We also check whether AUTOFILL_WALLET_DATA is registered.
> The
> > > > latter will be false for people who do not have the sync experiment
> enabled.
> > > 
> > > Right, but my point is that GetActiveDataTypes and GetPreferredDataTypes
are
> > > already filtering for registered types. In other words, if it's not
> > registered,
> > > it can't be preferred or active.
> > 
> > this code doesn't care if AUTOFILL_WALLET_DATA is preferred or active
> > 
> > > 
> > > > 
> > > > > 
> > > > > Also note that active types means the type is up and running, which
> means
> > > that
> > > > > at startup before sync has finished initializing this will be false.
If
> > > that's
> > > > > an issue you can use GetPreferredDataTypes instead.
> > > > 
> > > > ah, I should have read the docs more closely.
> > > 
> 
> Ah crap, I just realized we're talking about too different datatypes
> (AUTOFILL_PROFILE VS AUTOFILL_WALLET_DATA). My mistake.

ping brettw

[brettw, 2015-03-25 19:47:52]

LGTM

[Evan Stade, 2015-03-25 20:43:00]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-03-25 20:43:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1020303007/40001

[commit-bot: I haz the power, 2015-03-25 22:09:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-25 22:10:06]

Patchset 3 (id:??) landed as
https://crrev.com/5e26b8a570bb183c6a3686555fc61e545a3868c2
Cr-Commit-Position: refs/heads/master@{#322243}