Add logic to flip Nigori bit to instruct other clients to sync tabs. (issue 7767013)
atwi...@chromium.org
import
chrome/common from there, but LGTM once you've addressed the check_deps
problem.
http://codereview.chromium.org/7767013/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):
http://codereview.chromium.org/7767013/diff/1/chrome/common/chrome_switches.cc#newcode523
chrome/common/chrome_switches.cc:523: const char
kEnableSyncSessionsForOtherClients[] =
"enable-sync-sessions-for-other-clients";
line >80 chars
yfri...@chromium.org
Message:
Fixed the deps issues and the nit
http://codereview.chromium.org/7767013/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):
http://codereview.chromium.org/7767013/diff/1/chrome/common/chrome_switches.cc#newcode523
chrome/common/chrome_switches.cc:523: const char
kEnableSyncSessionsForOtherClients[] =
"enable-sync-sessions-for-other-clients";
On 2011/08/27 06:08:52, Andrew T Wilson wrote:
> line >80 chars
Done.
Description:
Add logic to flip Nigori bit to instruct other clients to sync tabs.
For reference, see: http://codereview.chromium.org/7669073/
BUG=
TEST=
Please review this at http://codereview.chromium.org/7767013/
SVN Base: http://git.chromium.org/git/chromium.git@trunk
Affected files:
M chrome/browser/sync/internal_api/DEPS
M chrome/browser/sync/internal_api/sync_manager.h
M chrome/browser/sync/internal_api/sync_manager.cc
M chrome/browser/sync/internal_api/syncapi_unittest.cc
M chrome/common/chrome_switches.h
M chrome/common/chrome_switches.cc
Index: chrome/browser/sync/internal_api/DEPS
diff --git a/chrome/browser/sync/internal_api/DEPS
b/chrome/browser/sync/internal_api/DEPS
index
ff4b760b9116b81daaa8122c279eb449f2e09e88..5cac978ccca18420afaa2b28e864f238cf9e6c6e
100644
--- a/chrome/browser/sync/internal_api/DEPS
+++ b/chrome/browser/sync/internal_api/DEPS
@@ -7,6 +7,9 @@ include_rules = [
"-chrome/browser/sync/api",
"-chrome/browser/sync/glue",
+ # Some functionality depends on command-line swithces
+ "+chrome/common/chrome_switches.h",
+
# unittests need this for mac osx keychain overriding
"+chrome/browser/password_manager/encryptor.h",
Index: chrome/browser/sync/internal_api/sync_manager.cc
diff --git a/chrome/browser/sync/internal_api/sync_manager.cc
b/chrome/browser/sync/internal_api/sync_manager.cc
index
01f1712e7f9e381dc3d20985ff860c59f9ab2aa9..bc7cd1697031bae71c9845ad2c09a5497f229c5e
100644
--- a/chrome/browser/sync/internal_api/sync_manager.cc
+++ b/chrome/browser/sync/internal_api/sync_manager.cc
@@ -8,6 +8,7 @@
#include <vector>
#include "base/base64.h"
+#include "base/command_line.h"
#include "base/json/json_writer.h"
#include "base/string_number_conversions.h"
#include "base/values.h"
@@ -41,6 +42,7 @@
#include "chrome/browser/sync/syncable/syncable.h"
#include "chrome/browser/sync/util/cryptographer.h"
#include "chrome/browser/sync/weak_handle.h"
+#include "chrome/common/chrome_switches.h"
#include "net/base/network_change_notifier.h"
using std::string;
@@ -235,6 +237,10 @@ class SyncManager::SyncInternal
// Called when the user disables or enables a sync type.
void UpdateEnabledTypes();
+ // Conditionally sets the flag in the Nigori node which instructs other
+ // clients to start syncing tabs.
+ void MaybeSetSyncTabsInNigoriNode(const syncable::ModelTypeSet
enabled_types);
+
// Tell the sync engine to start the syncing process.
void StartSyncingNormally();
@@ -656,6 +662,11 @@ void SyncManager::UpdateEnabledTypes() {
data_->UpdateEnabledTypes();
}
+void SyncManager::MaybeSetSyncTabsInNigoriNode(
+ const syncable::ModelTypeSet enabled_types) {
+ data_->MaybeSetSyncTabsInNigoriNode(enabled_types);
+}
+
bool SyncManager::InitialSyncEndedForAllEnabledTypes() {
return data_->InitialSyncEndedForAllEnabledTypes();
}
@@ -942,6 +953,27 @@ void SyncManager::SyncInternal::UpdateEnabledTypes() {
enabled_types.insert(it->first);
}
sync_notifier_->UpdateEnabledTypes(enabled_types);
+ if (CommandLine::ForCurrentProcess()->HasSwitch(
+ switches::kEnableSyncSessionsForOtherClients)) {
+ MaybeSetSyncTabsInNigoriNode(enabled_types);
+ }
+}
+
+void SyncManager::SyncInternal::MaybeSetSyncTabsInNigoriNode(
+ const syncable::ModelTypeSet enabled_types) {
+ if (enabled_types.count(syncable::SESSIONS) > 0) {
+ WriteTransaction trans(FROM_HERE, GetUserShare());
+ WriteNode node(&trans);
+ if (!node.InitByTagLookup(kNigoriTag)) {
+ NOTREACHED() << "Unable to set 'sync_tabs' bit because Nigori node
not "
+ << "found.";
+ return;
+ }
+
+ sync_pb::NigoriSpecifics specifics(node.GetNigoriSpecifics());
+ specifics.set_sync_tabs(true);
+ node.SetNigoriSpecifics(specifics);
+ }
}
void SyncManager::SyncInternal::RaiseAuthNeededEvent() {
Index: chrome/browser/sync/internal_api/sync_manager.h
diff --git a/chrome/browser/sync/internal_api/sync_manager.h
b/chrome/browser/sync/internal_api/sync_manager.h
index
2dfe1259170ce337e09796d94877abb854762b30..b8f43a46aba28c864fb976d50a0bd877269272eb
100644
--- a/chrome/browser/sync/internal_api/sync_manager.h
+++ b/chrome/browser/sync/internal_api/sync_manager.h
@@ -434,6 +434,10 @@ class SyncManager {
// Called when the user disables or enables a sync type.
void UpdateEnabledTypes();
+ // Conditionally sets the flag in the Nigori node which instructs other
+ // clients to start syncing tabs.
+ void MaybeSetSyncTabsInNigoriNode(const syncable::ModelTypeSet
enabled_types);
+
// Put the syncer in normal mode ready to perform nudges and polls.
void StartSyncingNormally();
Index: chrome/browser/sync/internal_api/syncapi_unittest.cc
diff --git a/chrome/browser/sync/internal_api/syncapi_unittest.cc
b/chrome/browser/sync/internal_api/syncapi_unittest.cc
index
1dcc4788db675de22e8a091ce88bf53f7d06beb8..7ca746eda586eddd64222174bd31ef51e2df7b1d
100644
--- a/chrome/browser/sync/internal_api/syncapi_unittest.cc
+++ b/chrome/browser/sync/internal_api/syncapi_unittest.cc
@@ -916,6 +916,28 @@ TEST_F(SyncManagerTest, UpdateEnabledTypes) {
EXPECT_EQ(2, update_enabled_types_call_count_);
}
+TEST_F(SyncManagerTest, DoNotSyncTabsInNigoriNode) {
+ syncable::ModelTypeSet encrypted_types;
+ encrypted_types.insert(syncable::TYPED_URLS);
+ sync_manager_.MaybeSetSyncTabsInNigoriNode(encrypted_types);
+
+ ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+ ReadNode node(&trans);
+ ASSERT_TRUE(node.InitByIdLookup(GetIdForDataType(syncable::NIGORI)));
+ EXPECT_FALSE(node.GetNigoriSpecifics().sync_tabs());
+}
+
+TEST_F(SyncManagerTest, SyncTabsInNigoriNode) {
+ syncable::ModelTypeSet encrypted_types;
+ encrypted_types.insert(syncable::SESSIONS);
+ sync_manager_.MaybeSetSyncTabsInNigoriNode(encrypted_types);
+
+ ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+ ReadNode node(&trans);
+ ASSERT_TRUE(node.InitByIdLookup(GetIdForDataType(syncable::NIGORI)));
+ EXPECT_TRUE(node.GetNigoriSpecifics().sync_tabs());
+}
+
TEST_F(SyncManagerTest, ProcessJsMessage) {
const JsArgList kNoArgs;
Index: chrome/common/chrome_switches.cc
diff --git a/chrome/common/chrome_switches.cc
b/chrome/common/chrome_switches.cc
index
06f797191de7e3ab806f4094de35c9670ef98735..ced7cf5477919f6e91f57ff0066610a660b5adac
100644
--- a/chrome/common/chrome_switches.cc
+++ b/chrome/common/chrome_switches.cc
@@ -519,6 +519,10 @@ const char kEnableSyncOAuth[]
= "enable-sync-oauth";
// Enable syncing browser sessions.
const char kEnableSyncSessions[] = "enable-sync-sessions";
+// Enable syncing browser sessions for other synced clients.
+const char kEnableSyncSessionsForOtherClients[] =
+ "enable-sync-sessions-for-other-clients";
+
// Enables context menu for selecting groups of tabs.
const char kEnableTabGroupsContextMenu[]
= "enable-tab-groups-context-menu";
Index: chrome/common/chrome_switches.h
diff --git a/chrome/common/chrome_switches.h
b/chrome/common/chrome_switches.h
index
1ebeb9cf4c53de10f5d1ee56ddbf390533cf06dc..2f730afc43b81d1f003dc39c13cfbbe89310a3cd
100644
--- a/chrome/common/chrome_switches.h
+++ b/chrome/common/chrome_switches.h
@@ -150,6 +150,7 @@ extern const char kEnableSmoothScrolling[];
extern const char kEnableSSLCachedInfo[];
extern const char kEnableSyncOAuth[];
extern const char kEnableSyncSessions[];
+extern const char kEnableSyncSessionsForOtherClients[];
extern const char kEnableSyncedBookmarksFolder[];
extern const char kEnableTabGroupsContextMenu[];
extern const char kEnableTcpFastOpen[];
yfri...@chromium.org
> Fixed the deps issues and the nit
> http://codereview.chromium.org/7767013/diff/1/chrome/common/chrome_switches.cc
> File chrome/common/chrome_switches.cc (right):
http://codereview.chromium.org/7767013/diff/1/chrome/common/chrome_switches.cc#newcode523
> chrome/common/chrome_switches.cc:523: const char
> kEnableSyncSessionsForOtherClients[] =
> "enable-sync-sessions-for-other-clients";
> On 2011/08/27 06:08:52, Andrew T Wilson wrote:
> > line >80 chars
> Done.
Actually, this doesn't quite work as expected. Starting up a previously
configured client hits a CHECK because SignIn hasn't completed before the
first
call to UpdateEnabledTypes (stack below). Should I add a conditional in
UpdateEnabledTypes to see that it's initialized and also call
UpdateEnabledTypes
at the bottom of init? I was hoping there would be a single place where the
user
has changed their preferred set of types and perform the modification there
but
I don't see it.
[16651:16685:242997249288:FATAL:sync_manager.cc(313)] Check failed:
initialized_.
Backtrace:
base::debug::StackTrace::StackTrace() [0x7f099349a312]
logging::LogMessage::~LogMessage() [0x7f0993421886]
sync_api::SyncManager::SyncInternal::GetUserShare() [0x7f09954717cc]
sync_api::SyncManager::SyncInternal::MaybeSetSyncTabsInNigoriNode()
[0x7f09954696ea]
sync_api::SyncManager::SyncInternal::UpdateEnabledTypes()
[0x7f099546965d]
sync_api::SyncManager::SyncInternal::SignIn() [0x7f09954690d6]
...
yfri...@chromium.org
Ok, I fixed the check by adding a guard in MaybeSetSyncTabsInNigoriNode.
Drew,
PTAL. Note, there was a re-base between 1 and 2
