Add SetUserPreference function to VirtualKeyboardSelector

chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc

Index: chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc
diff --git a/chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc b/chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc
index 1e427ad3acd4c2c14468f1fc9a22bd713846bf5d..0241aded90dc17c86f20c723145dc9729286e98e 100644
--- a/chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc
+++ b/chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc
@@ -8,8 +8,17 @@
 #include "base/stl_util.h"
 
 namespace {
+
 const char kDefaultURLPath[] = "index.html";
 const size_t kDefaultURLPathLen = arraysize(kDefaultURLPath) - 1;
+
+bool SameOrNull(const GURL* rhs, const GURL* lhs) {
+  if (!rhs || !lhs) {
+    return true;
+  }
+  return *rhs == *lhs;
+}
+
 }  // namespace
 
 namespace chromeos {
@@ -38,6 +47,10 @@ GURL VirtualKeyboard::GetURLForLayout(const std::string& layout) const {
   return url_.ReplaceComponents(replacements);
 }
 
+bool VirtualKeyboard::LayoutIsSupported(const std::string& layout) const {
+  return supported_layouts_.count(layout) > 0;
+}
+
 VirtualKeyboardSelector::VirtualKeyboardSelector()
     : current_(NULL) {
 }
@@ -68,12 +81,19 @@ const VirtualKeyboard* VirtualKeyboardSelector::SelectVirtualKeyboard(
     return NULL;
   }
 
-  // First, check whether the current keyboard supports the layout.
-  if (current_ && current_->supported_layouts().count(layout) > 0) {
+  // First, check the user pref.
+  std::map<std::string, const VirtualKeyboard*>::const_iterator iter =
+      user_preference_.find(layout);
+  if (iter != user_preference_.end()) {
+    current_ = iter->second;

[bryeung, 2011/07/28 18:31:48]

Before throwing away the value of current_, maybe we should check if the layout
is still supported?

I'm thinking of the case where the user has saved a preference, but then the
extension is updated and no longer supports a layout that has the preference
saved for it.  In that case, we should not lose current_ I think.

[Yusuke Sato, 2011/07/29 05:15:35]

Done.

+  }
+
+  // Second, check whether the current keyboard supports the layout.
+  if (current_ && current_->LayoutIsSupported(layout)) {
     return current_;
   }
 
-  const VirtualKeyboard* keyboard = SelectVirtualKeyboardInternal(layout);
+  const VirtualKeyboard* keyboard = SelectVirtualKeyboardByLayout(layout);

[bryeung, 2011/07/28 18:31:48]

Maybe SelectVirtualKeyboardWithoutPreferences would be more clear?

ByLayout doesn't really tell me that this function is skipping preferences.  If
you have an idea for a shorter name that conveys this that would be even better.

[Yusuke Sato, 2011/07/29 05:15:35]

Done.

   if (!keyboard) {
     VLOG(1) << "No virtual keyboard for " << layout << " is found";
     return NULL;
@@ -83,18 +103,46 @@ const VirtualKeyboard* VirtualKeyboardSelector::SelectVirtualKeyboard(
   return keyboard;
 }
 
-const VirtualKeyboard* VirtualKeyboardSelector::SelectVirtualKeyboardInternal(
+bool VirtualKeyboardSelector::SetUserPreference(
+    const std::string& layout, const GURL& url) {
+  const VirtualKeyboard* keyboard =
+      SelectVirtualKeyboardByUrl(keyboards_, layout, &url);
+  if (!keyboard)
+    keyboard = SelectVirtualKeyboardByUrl(system_keyboards_, layout, &url);
+  if (!keyboard) {
+    VLOG(1) << "Can't set user preference.";
+    return false;
+  }
+
+  RemoveUserPreference(layout);
+  user_preference_.insert(std::make_pair(layout, keyboard));
+  return true;
+}
+
+void VirtualKeyboardSelector::RemoveUserPreference(const std::string& layout) {
+  user_preference_.erase(layout);
+}
+
+const VirtualKeyboard* VirtualKeyboardSelector::SelectVirtualKeyboardByLayout(
     const std::string& layout) {
+  const VirtualKeyboard* keyboard =
+      SelectVirtualKeyboardByUrl(keyboards_, layout, NULL);  // don't check URL
+  if (!keyboard)
+    keyboard = SelectVirtualKeyboardByUrl(system_keyboards_, layout, NULL);
+  return keyboard;
+}
+
+// static
+const VirtualKeyboard* VirtualKeyboardSelector::SelectVirtualKeyboardByUrl(
+    const std::list<const VirtualKeyboard*>& keyboards,
+    const std::string& layout,
+    const GURL* url) {
   std::list<const VirtualKeyboard*>::const_iterator iter;

[mazda, 2011/07/28 07:08:16]

How about moving this to the initializer expression of the following for-loop?

[Yusuke Sato, 2011/07/29 05:15:35]

Done.

-  for (iter = keyboards_.begin(); iter != keyboards_.end(); ++iter) {
-    if ((*iter)->supported_layouts().count(layout) > 0) {
-      return *iter;
-    }
-  }
-  for (iter = system_keyboards_.begin();
-       iter != system_keyboards_.end(); ++iter) {
-    if ((*iter)->supported_layouts().count(layout) > 0) {
-      return *iter;
+  for (iter = keyboards.begin(); iter != keyboards.end(); ++iter) {
+    const VirtualKeyboard* keyboard = *iter;
+    if (SameOrNull(url, &(keyboard->url())) &&

[bryeung, 2011/07/28 18:31:48]

This could give bad results if somehow there is a keyboard with a NULL url.

How about:

if ((url == NULL || *url == *(keyboard->url()) &&
    keyboard->LayoutIsSupported(layout))

I think that makes it more clear what the intent is as well.

[Yusuke Sato, 2011/07/29 05:15:35]

Done.

+        keyboard->LayoutIsSupported(layout)) {
+      return keyboard;
     }
   }
   return NULL;