Cleanup of r71320 commit.

chrome/browser/chromeos/locale_change_guard.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/locale_change_guard.h"
 
 #include "app/l10n_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/browser_process.h"
@@ skipping 15 matching lines @@
 namespace chromeos {
 
 LocaleChangeGuard::LocaleChangeGuard()
     : profile_id_(Profile::InvalidProfileId),
       tab_contents_(NULL),
       note_(NULL),
       reverted_(false) {
 }
 
 void LocaleChangeGuard::RevertLocaleChange(const ListValue* list) {
+  if (note_ == NULL
+      || tab_contents_ == NULL

[brettw, 2011/01/17 16:40:30]

In Chrome we'll normally put the || at the end of the previous line.

[Denis Lagno, 2011/01/17 17:44:13]

Done.

+      || from_locale_.empty()
+      || to_locale_.empty()) {
+    NOTREACHED();
+    return;
+  }
+  if (reverted_)
+    return;
+
+  PrefService* prefs = tab_contents_->profile()->GetPrefs();
+  if (prefs == NULL)
+    return;
+
   reverted_ = true;
   UserMetrics::RecordAction(UserMetricsAction("LanguageChange_Revert"));
   tab_contents_->profile()->ChangeApplicationLocale(from_locale_, true);
-  PrefService* prefs = tab_contents_->profile()->GetPrefs();
-  if (prefs) {
-    prefs->SetString(prefs::kApplicationLocaleBackup, from_locale_);
-    prefs->ClearPref(prefs::kApplicationLocaleAccepted);
-    prefs->ScheduleSavePersistentPrefs();
-  }
+  prefs->SetString(prefs::kApplicationLocaleBackup, from_locale_);
+  prefs->ClearPref(prefs::kApplicationLocaleAccepted);
+  prefs->ScheduleSavePersistentPrefs();
+
   Browser* browser = Browser::GetBrowserForController(
       &tab_contents_->controller(), NULL);
   if (browser)
     browser->ExecuteCommand(IDC_EXIT);
 }
 
 void LocaleChangeGuard::CheckLocaleChange(TabContents* tab_contents) {
   // We want notification to be shown no more than once per session.
   if (note_ != NULL)
     return;
+  DCHECK(note_ == NULL && tab_contents_ == NULL);
+  DCHECK(from_locale_.empty() && to_locale_.empty());
+
   // We check profile Id because:
   // (1) we want to exit fast in common case when nothing should be done.
   // (2) on ChromeOS this guard may be invoked for a dummy profile first time.
   ProfileId cur_profile_id = tab_contents->profile()->GetRuntimeId();
-  if (cur_profile_id == profile_id_)
+  if (cur_profile_id == profile_id_) {
+    // We have already checked this profile, exiting fast.
     return;
+  }
   profile_id_ = cur_profile_id;
+
   std::string cur_locale = g_browser_process->GetApplicationLocale();
-  if (cur_locale.empty())
+  if (cur_locale.empty()) {
+    NOTREACHED();
     return;
+  }
+
   PrefService* prefs = tab_contents->profile()->GetPrefs();
   if (prefs == NULL)
     return;
-  to_locale_ = prefs->GetString(prefs::kApplicationLocaleOverride);
-  if (!to_locale_.empty()) {
-    DCHECK(to_locale_ == cur_locale);
+
+  std::string to_locale = prefs->GetString(prefs::kApplicationLocaleOverride);
+  if (!to_locale.empty()) {
+    DCHECK(to_locale == cur_locale);
     return;
   }
-  to_locale_ = prefs->GetString(prefs::kApplicationLocale);
-  if (to_locale_ != cur_locale)
+
+  to_locale = prefs->GetString(prefs::kApplicationLocale);
+  if (to_locale != cur_locale) {
+    // This conditional branch can occur in case kApplicationLocale
+    // preference was modified by synchronization.
     return;
-  from_locale_ = prefs->GetString(prefs::kApplicationLocaleBackup);
-  if (from_locale_.empty() || from_locale_ == to_locale_)
-    return;
-  note_.reset(new chromeos::SystemNotification(
-      tab_contents->profile(),
-      new Delegate(this),
-      IDR_DEFAULT_FAVICON,
-      l10n_util::GetStringUTF16(
-          IDS_OPTIONS_SETTINGS_SECTION_TITLE_LANGUAGE)));
-  tab_contents_ = tab_contents;
-  note_->Show(
-      l10n_util::GetStringFUTF16(
-          IDS_LOCALE_CHANGE_MESSAGE,
-          l10n_util::GetDisplayNameForLocale(from_locale_, to_locale_, true),
-          l10n_util::GetDisplayNameForLocale(to_locale_, to_locale_, true)),
-      l10n_util::GetStringUTF16(IDS_LOCALE_CHANGE_REVERT_MESSAGE),
-      NewCallback(this, &LocaleChangeGuard::RevertLocaleChange),
-      true,  // urgent
-      false);  // non-sticky
+  }
+
+  DCHECK(note_ == NULL && tab_contents_ == NULL);

[brettw, 2011/01/17 16:40:30]

You already did these two DCHECKs at the top of the function, and I don't think
either of them can change. I'd just delete them if this is the case.

[Denis Lagno, 2011/01/17 17:44:13]

Done.

+  DCHECK(from_locale_.empty() && to_locale_.empty());
+  std::string from_locale = prefs->GetString(prefs::kApplicationLocaleBackup);
+  if (!from_locale.empty() && from_locale != to_locale) {

[brettw, 2011/01/17 16:40:30]

I actually liked the old way of returning early in this case. The reason is that
you return early a bunch above, and somehow it's easier for me to follow if a
function is either consistently return early or not.

[Denis Lagno, 2011/01/17 17:44:13]

Done.

+    // Locale change detected, showing notification.
+    from_locale_ = from_locale;
+    to_locale_ = to_locale;
+    tab_contents_ = tab_contents;
+    note_.reset(new chromeos::SystemNotification(
+        tab_contents->profile(),
+        new Delegate(this),
+        IDR_DEFAULT_FAVICON,
+        l10n_util::GetStringUTF16(
+            IDS_OPTIONS_SETTINGS_SECTION_TITLE_LANGUAGE)));
+    note_->Show(
+        l10n_util::GetStringFUTF16(
+            IDS_LOCALE_CHANGE_MESSAGE,
+            l10n_util::GetDisplayNameForLocale(from_locale_, to_locale_, true),
+            l10n_util::GetDisplayNameForLocale(to_locale_, to_locale_, true)),
+        l10n_util::GetStringUTF16(IDS_LOCALE_CHANGE_REVERT_MESSAGE),
+        NewCallback(this, &LocaleChangeGuard::RevertLocaleChange),
+        true,  // urgent
+        false);  // non-sticky
+  }
 }
 
 void LocaleChangeGuard::AcceptLocaleChange() {
+  if (note_ == NULL
+      || tab_contents_ == NULL

[brettw, 2011/01/17 16:40:30]

Ditto with || at the end.

[Denis Lagno, 2011/01/17 17:44:13]

Done.

+      || from_locale_.empty()
+      || to_locale_.empty()) {
+    NOTREACHED();
+    return;
+  }
+
   // Check whether locale has been reverted or changed.
   // If not: mark current locale as accepted.
-  if (tab_contents_ == NULL)
-    return;
   if (reverted_)
     return;
   PrefService* prefs = tab_contents_->profile()->GetPrefs();
   if (prefs == NULL)
     return;
   std::string override_locale =
       prefs->GetString(prefs::kApplicationLocaleOverride);
   if (!override_locale.empty())
     return;
   if (prefs->GetString(prefs::kApplicationLocale) != to_locale_)
@@ skipping 13 matching lines @@
   if (by_user)
     master_->AcceptLocaleChange();
 }
 
 std::string LocaleChangeGuard::Delegate::id() const {
   // Arbitrary unique Id.
   return "8c386938-1e3f-11e0-ac7b-18a90520e2e5";
 }
 
 }  // namespace chromeos