show notification on locale change

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/lazy_instance.h"
+#include "base/utf_string_conversions.h"
+#include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/chromeos/notifications/system_notification.h"
+#include "chrome/browser/metrics/user_metrics.h"
+#include "chrome/browser/prefs/pref_service.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/tab_contents/tab_contents.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/common/pref_names.h"
+#include "grit/app_resources.h"
+#include "grit/generated_resources.h"
+
+class ListValue;
+
+namespace {
+
+class LocaleChangeGuardImpl {
+  LocaleChangeGuardImpl()

[whywhat, 2011/01/12 16:33:16]

Could you put explicit private: modifier here to avoid any confusion?

[Denis Lagno, 2011/01/12 20:27:34]

not applicable anymore

+      : profile_id_(Profile::InvalidProfileId),
+        tab_contents_(NULL),
+        note_(NULL),
+        reverted_(false) {}
+
+  void RevertLocaleChange(const ListValue* list) {
+    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();
+    }
+    Browser* browser = Browser::GetBrowserForController(
+        &tab_contents_->controller(), NULL);
+    if (browser)
+      browser->ExecuteCommand(IDC_EXIT);

[whywhat, 2011/01/12 16:33:16]

Do we say in the message we show to user that we'll close the browser? Can user
loose his data due to that?

[Denis Lagno, 2011/01/12 20:27:34]

It happens if user presses on the link.
Link is currently labelled with following text:

"If you are unhappy about it: click here to close browser, sign out and revert."

+  }
+
+  void Check(TabContents* tab_contents) {
+    // We want notification to be shown no more than once per session.

[whywhat, 2011/01/12 16:33:16]

nit: no more than -> only?

[Denis Lagno, 2011/01/12 20:27:34]

Not sure..  "Only once" sounds to me as exactly once.
But in common case this notification is shown zero times, because it is not
needed in common case.

+    if (note_ != NULL)
+      return;
+    // We check profile Id because on ChromeOS this guard
+    // first time may be invoked for a dummy profile.

[whywhat, 2011/01/12 16:33:16]

nit: move "first time" to the end of the sentence?

[Denis Lagno, 2011/01/12 20:27:34]

Done.

+    ProfileId cur_profile_id = tab_contents->profile()->GetRuntimeId();
+    if (cur_profile_id == profile_id_)
+      return;
+    profile_id_ = cur_profile_id;
+    std::string cur_locale = g_browser_process->GetApplicationLocale();
+    if (cur_locale.empty())
+      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);
+      return;

[whywhat, 2011/01/12 16:33:16]

We return in multiple places, leaving the object partially initialized (i.e.
profile id and to_locale_ are set, but from_locale_ is empty). Is it as
designed?

[Denis Lagno, 2011/01/12 20:27:34]

if it exited early in this function then it means that no notification was shown
so those values will not be used in any way so we do not care if they are empty.

+    }
+    to_locale_ = prefs->GetString(prefs::kApplicationLocale);
+    if (to_locale_ != cur_locale)
+      return;
+    from_locale_ = prefs->GetString(prefs::kApplicationLocaleBackup);
+    if (from_locale_.empty() || from_locale_ == to_locale_)
+      return;
+    note_.reset(new chromeos::SystemNotification(
+        tab_contents->profile(),
+        "8c386938-1e3f-11e0-ac7b-18a90520e2e5" /* arbitrary unique id */,

[whywhat, 2011/01/12 16:33:16]

Could you make it a named constant?

[Denis Lagno, 2011/01/12 20:27:34]

I moved it into dedicated named method.

+        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, &LocaleChangeGuardImpl::RevertLocaleChange),
+        true /* urgent */, false /* non-sticky */);

[whywhat, 2011/01/12 16:33:16]

Could you write it like this:
true,    // urgent
false);  // non-sticky
to avoid violating style guide's rule about two spaces between code and comments
when on the same line?

[Denis Lagno, 2011/01/12 20:27:34]

Done.

+  }
+
+  ~LocaleChangeGuardImpl() {

[whywhat, 2011/01/12 16:33:16]

There was recent discussion about dtor and ctor being put together if possible
so it's easier to find them. Could you move this dtor after ctor definition?

[whywhat, 2011/01/12 16:33:16]

When the dtor will be executed? As I understand, only when the process will
exit. Is it desirable?

[Denis Lagno, 2011/01/12 20:27:34]

dtor removed.  not applicable anymore.

[Denis Lagno, 2011/01/12 20:27:34]

You're rignt.  That is bad.  Reworked that part.

+    // 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_)
+      return;
+    prefs->SetString(prefs::kApplicationLocaleBackup, to_locale_);
+    prefs->SetString(prefs::kApplicationLocaleAccepted, to_locale_);
+    prefs->ScheduleSavePersistentPrefs();
+  }
+
+  std::string from_locale_, to_locale_;

[whywhat, 2011/01/12 16:33:16]

Declaration of two variables on a single line is not allowed by style guide.

[Denis Lagno, 2011/01/12 20:27:34]

Done.

+  ProfileId profile_id_;
+  TabContents* tab_contents_;
+  scoped_ptr<chromeos::SystemNotification> note_;
+  bool reverted_;
+
+  friend struct base::DefaultLazyInstanceTraits<LocaleChangeGuardImpl>;
+  friend struct chromeos::LocaleChangeGuard;
+};
+
+base::LazyInstance<LocaleChangeGuardImpl> g_locale_change_guard(
+    base::LINKER_INITIALIZED);
+
+}  // namespace
+
+namespace chromeos {
+
+// static
+void LocaleChangeGuard::Check(TabContents* tab_contents) {

[whywhat, 2011/01/12 16:33:16]

I think using impl class here hurts readability: the header now has one method,
but would have only 4 if merge both classes into one. And cc file is harder to
read since there's one additional level of complexity to keep in mind.

[Denis Lagno, 2011/01/12 20:27:34]

ok, I moved it into single class.
But I cannot say that I like this: it adds more includes into headers, so it
increases compile time and dependencies.

(there was zero includes in original version of locale_change_guard.h; now there
are 5 complex inlcudes).

[whywhat, 2011/01/13 10:09:43]

Still I don't believe this will create a bottleneck in Chrome's compilation
time. And until this is true and proved I'd prefer readability and simplicity to
artificial reduction of include dependencies.
Thanks for bearing with me! :)

+  g_locale_change_guard.Get().Check(tab_contents);
+}
+
+}  // namespace chromeos