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..316b217a0f721ad73a1311e565812fe0b877850a 100644
--- a/chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc
+++ b/chrome/browser/chromeos/input_method/virtual_keyboard_selector.cc
@@ -7,13 +7,33 @@
 #include "base/logging.h"
 #include "base/stl_util.h"
 
+namespace chromeos {
+namespace input_method {
+
 namespace {

[bryeung, 2011/08/03 22:04:46]

I don't think the anonymous namespace should be nested inside the others.

[Yusuke Sato, 2011/08/04 06:26:56]

Done. Added a namespace alias (which is explicitly allowed in the style guide)
to follow the 80 col rule.

+
 const char kDefaultURLPath[] = "index.html";
 const size_t kDefaultURLPathLen = arraysize(kDefaultURLPath) - 1;
-}  // namespace
 
-namespace chromeos {
-namespace input_method {
+// Selects and returns a virtual keyboard extension from |keyboards| which
+// supports the |layout| and whose address is |url|. If |url| is NULL, URL
+// checking is ignored.
+const VirtualKeyboard* SelectVirtualKeyboardInternal(
+    const std::list<const VirtualKeyboard*>& keyboards,
+    const std::string& layout,
+    const GURL* url) {

[bryeung, 2011/08/03 22:04:46]

Is it worth adding a map from url to VirtualKeyboard in this class as well?  
The more I look at this, the more awkward I feel the URL check in this method
is.

It seems there are two use cases:
 1) we want to search a list for a layout
 2) we want to get the keyboard for a given URL and check if it supports a
layout

If you think that the lists are always short, and that it is not worth
maintaining the map, I could be convinced to leave it like this.  But I'd like
to hear your feedback on this idea.

[Yusuke Sato, 2011/08/04 06:26:56]

Sure, added the map to make SetUserPreference function simpler and faster.

On 2011/08/03 22:04:46, bryeung wrote:

+  for (std::list<const VirtualKeyboard*>::const_iterator iter =
+           keyboards.begin(); iter != keyboards.end(); ++iter) {
+    const VirtualKeyboard* keyboard = *iter;
+    if (((!url) || (*url) == keyboard->url()) &&

[bryeung, 2011/08/03 22:04:46]

Can you remove the extra parens?

if ((!url || *url == keyboard->url()) &&

will be easier to read I think.

[Yusuke Sato, 2011/08/04 06:26:56]

(I've removed the url parameter from the function.)

+        keyboard->IsLayoutSupported(layout)) {
+      return keyboard;
+    }
+  }
+  return NULL;
+}
+
+}  // namespace
 
 VirtualKeyboard::VirtualKeyboard(const GURL& url,
                                  const std::set<std::string>& supported_layouts,
@@ -38,6 +58,10 @@ GURL VirtualKeyboard::GetURLForLayout(const std::string& layout) const {
   return url_.ReplaceComponents(replacements);
 }
 
+bool VirtualKeyboard::IsLayoutSupported(const std::string& layout) const {
+  return supported_layouts_.count(layout) > 0;
+}
+
 VirtualKeyboardSelector::VirtualKeyboardSelector()
     : current_(NULL) {
 }
@@ -68,12 +92,21 @@ 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() &&
+      iter->second->IsLayoutSupported(layout)) {
+    current_ = iter->second;
+  }
+
+  // Second, check whether the current keyboard supports the layout.
+  if (current_ && current_->IsLayoutSupported(layout)) {

[bryeung, 2011/08/03 22:04:46]

This should probably be an else if, or you'll be doing a redundant call to
IsLayoutSupported

Either that, or you should return current_ above.

[Yusuke Sato, 2011/08/04 06:26:56]

Done.

     return current_;
   }
 
-  const VirtualKeyboard* keyboard = SelectVirtualKeyboardInternal(layout);
+  const VirtualKeyboard* keyboard =
+      SelectVirtualKeyboardWithoutPreferences(layout);
   if (!keyboard) {
     VLOG(1) << "No virtual keyboard for " << layout << " is found";
     return NULL;
@@ -83,21 +116,34 @@ const VirtualKeyboard* VirtualKeyboardSelector::SelectVirtualKeyboard(
   return keyboard;
 }
 
-const VirtualKeyboard* VirtualKeyboardSelector::SelectVirtualKeyboardInternal(
-    const std::string& layout) {
-  std::list<const VirtualKeyboard*>::const_iterator iter;
-  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;
-    }
+bool VirtualKeyboardSelector::SetUserPreference(
+    const std::string& layout, const GURL& url) {
+  const VirtualKeyboard* keyboard =
+      SelectVirtualKeyboardInternal(keyboards_, layout, &url);
+  if (!keyboard)
+    keyboard = SelectVirtualKeyboardInternal(system_keyboards_, layout, &url);
+  if (!keyboard) {
+    VLOG(1) << "Can't set user preference.";
+    return false;
   }
-  return NULL;
+
+  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::SelectVirtualKeyboardWithoutPreferences(
+    const std::string& layout) {
+  const VirtualKeyboard* keyboard =
+      SelectVirtualKeyboardInternal(keyboards_, layout, NULL);
+  if (!keyboard)
+    keyboard = SelectVirtualKeyboardInternal(system_keyboards_, layout, NULL);
+  return keyboard;
 }
 
 }  // namespace input_method