Add password sync for Android.

chrome/browser/sync/profile_sync_service.cc

Index: chrome/browser/sync/profile_sync_service.cc
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index aabef52ed85a745e063366d18353730bea6c5465..9f879e9bb0de6252f8af607ca12807704eab4ac1 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -74,6 +74,10 @@
 #include "sync/util/cryptographer.h"
 #include "ui/base/l10n/l10n_util.h"
 
+#if defined(OS_ANDROID)
+#include "sync/internal_api/public/read_transaction.h"
+#endif
+
 using browser_sync::ChangeProcessor;
 using browser_sync::DataTypeController;
 using browser_sync::DataTypeManager;
@@ -185,6 +189,23 @@ bool ProfileSyncService::IsSyncTokenAvailable() {
     return false;
   return token_service->HasTokenForService(GaiaConstants::kSyncService);
 }
+#if defined(OS_ANDROID)
+bool ProfileSyncService::IsPasswordSyncEnabledForAndroid() const {

[Nicolas Zea, 2013/01/18 00:05:11]

This method isn't really about whether it is enabled, but whether it should be
right? Perhaps ShouldEnablePasswordSyncForAndroid is more appropriate a name.

[shashi, 2013/01/18 02:06:31]

Correct, thanks much better.
On 2013/01/18 00:05:11, Nicolas Zea wrote:

+  const syncer::ModelTypeSet registered_types = GetRegisteredDataTypes();
+  const syncer::ModelTypeSet preferred_types =
+      sync_prefs_.GetPreferredDataTypes(registered_types);
+  if (!preferred_types.Has(syncer::PASSWORDS))
+    return false;
+  const syncer::PassphraseType passphraseType = GetPassphraseType();
+  // On Android we do not want to prompt user to enter a passphrase except in
+  // case of custom passphrase, when we have no choice.
+  // We enable passwords always for CUSTOM_PASSPHRASE but for all other
+  // passphrase types we disable them till the cryptographer is ready.
+  syncer::ReadTransaction trans(FROM_HERE, GetUserShare());
+  return (passphraseType == syncer::CUSTOM_PASSPHRASE

[Nicolas Zea, 2013/01/18 00:05:11]

So, given that we already have logic to enable passwords when the passphrase is
accepted, do we care about enabling them here for a custom pasphrase when there
are pending keys? Isn't IsCryptographerReady sufficient?

Also, move the || up onto the end of this line.

[shashi, 2013/01/18 02:06:31]

This is to deal with a corner case: User has a custom passphrase and has
selected "encrypt only passwords". 
Without this conditional we will disable the password datatype because
cryptographer is not ready. With passwords disabled there are no enabled
encrypted datatypes. 
Android only prompts user for passphrase if there is an enabled encrypted
datatype which needs to be decrypted. In this case there is no such encrypted
datatype (since we disabled it) so there is no user prompt for entering custom
passphrase. This means passwords will remain disabled permanently.

On 2013/01/18 00:05:11, Nicolas Zea wrote:

[Nicolas Zea, 2013/01/18 22:55:13]

Hmm, the IsEncryptedDatatypeEnabled method looks at the preferred types, not the
running types. As long as we're not marking passwords as not preferred (which I
don't think failed datatypes get marked as), it shouldn't get affected. Is the
Android UI logic not using IsEncryptedDatatypeEnabled?

[shashi, 2013/01/18 23:20:52]

IsEncryptedDatatypeEnabled uses PSS.GetPreferredDataTypes() which filters failed
datatypes.
Android UI uses IsPassphraseRequiredForDecryption, which is
IsEncryptedDatatypeEnabled && IsPassphraseRequired.
In this case since passwords are disabled we do not show a dialog.
Having said that, I can change the UI logic to show a dialog when
(passphrase_type == CUSTOM_PASSPHRASE && !IsCryptographerReady), this will make
this much simpler.

On 2013/01/18 22:55:13, Nicolas Zea wrote:

[Nicolas Zea, 2013/01/19 01:10:16]

That sounds good.

Also, apparently the reason for my confusion was the GetPreferredDataTypes
filters out the failed types (which I think might be wrong, but isn't something
we need to address here). 

[shashi, 2013/01/23 21:25:24]

Yes the function name is confusing :), GetEnabledPreferredDataTypes may be a
better name.
On 2013/01/19 01:10:16, Nicolas Zea wrote:

+      || IsCryptographerReady(&trans));
+}
+#endif
 
 void ProfileSyncService::Initialize() {
   DCHECK(!invalidator_registrar_.get());
@@ -989,6 +1010,17 @@ void ProfileSyncService::OnPassphraseAccepted() {
   // types are enabled, and we don't want to clobber the true passphrase error.
   passphrase_required_reason_ = syncer::REASON_PASSPHRASE_NOT_REQUIRED;
 
+#if defined(OS_ANDROID)
+  // Re-enable passwords if we have disabled them.
+  if (failed_datatypes_handler_.GetFailedTypes().Has(syncer::PASSWORDS)
+      && IsPasswordSyncEnabledForAndroid()) {

[Nicolas Zea, 2013/01/18 00:05:11]

move && operator to previous line

[shashi, 2013/01/18 02:06:31]

Done.

+    syncer::ModelTypeSet preferredTypes = GetPreferredDataTypes();
+    preferredTypes.Put(syncer::PASSWORDS);

[Nicolas Zea, 2013/01/18 00:05:11]

This shouldn't be necessary right? From what I remember, if a type is in the
failed types list, it means it was already preferred.

I think this should just call failed_datatypes_handler_.OnUserChoseDatatypes
again, which will clear the error'd types.

Similarly, why do we return here? We'll be triggering a reconfiguration at the
end of this function anyways, which should attempt to configure passwords.

[shashi, 2013/01/18 02:06:31]

PSS.GetPreferredDataTypes filters failed types, but
sync_prefs_.GetPreferredDatatTypes() will give all preferred datatypes.

Using failed_datatypes_handler_.OnUserChoseDatatypes may be sufficient here. I
was concerned that I may be clearing errors for all other datatypes without
trigerring a complete reconfig. OnUserChoseDataTypes updates some UMA stats
besides a reconfig, that was the reason for return but I have simplified to just
clear fail_datatypes and removed return.

On 2013/01/18 00:05:11, Nicolas Zea wrote:

[Nicolas Zea, 2013/01/18 22:55:13]

I see. Yeah, I think clearing failed datatypes should be okay. Presumably any
other datatypes with legitimate failures will just re-fail, which I think is
okay given that we should only do this once.

+    OnUserChoseDatatypes(sync_prefs_.HasKeepEverythingSynced(), preferredTypes);
+    return;
+  }
+#endif
+
   // Make sure the data types that depend on the passphrase are started at
   // this time.
   const syncer::ModelTypeSet types = GetPreferredDataTypes();
@@ -1451,6 +1483,13 @@ void ProfileSyncService::ConfigureDataTypeManager() {
                        base::Unretained(this))));
   }
 
+#if defined(OS_ANDROID)
+  if (GetPreferredDataTypes().Has(syncer::PASSWORDS)
+      && !IsPasswordSyncEnabledForAndroid()) {

[Nicolas Zea, 2013/01/18 00:05:11]

move && operator to previous line

[shashi, 2013/01/18 02:06:31]

Done.

+    DisableBrokenDatatype(syncer::PASSWORDS, FROM_HERE, "Not supported.");
+  }
+#endif
+
   const syncer::ModelTypeSet types = GetPreferredDataTypes();
   if (IsPassphraseRequiredForDecryption()) {
     // We need a passphrase still. We don't bother to attempt to configure