Add SetUserPreference function to VirtualKeyboardSelector

chrome/browser/chromeos/input_method/virtual_keyboard_selector.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/input_method/virtual_keyboard_selector.h"
 
 #include "base/logging.h"
 #include "base/stl_util.h"
 
-namespace {
-const char kDefaultURLPath[] = "index.html";
-const size_t kDefaultURLPathLen = arraysize(kDefaultURLPath) - 1;
-}  // namespace
-
 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;
+
+// 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,
                                  bool is_system)
     : url_(url),
       supported_layouts_(supported_layouts),
       is_system_(is_system) {
 }
 
 VirtualKeyboard::~VirtualKeyboard() {
 }
 
 GURL VirtualKeyboard::GetURLForLayout(const std::string& layout) const {
   if (layout.empty()) {
     return url_;
   }
   url_canon::Replacements<char> replacements;
   replacements.SetPath(
       kDefaultURLPath, url_parse::Component(0, kDefaultURLPathLen));
   // TODO(yusukes): would be better to URL-encode the |layout|?
   replacements.SetRef(layout.c_str(), url_parse::Component(0, layout.length()));
   return url_.ReplaceComponents(replacements);
 }
 
+bool VirtualKeyboard::IsLayoutSupported(const std::string& layout) const {
+  return supported_layouts_.count(layout) > 0;
+}
+
 VirtualKeyboardSelector::VirtualKeyboardSelector()
     : current_(NULL) {
 }
 
 VirtualKeyboardSelector::~VirtualKeyboardSelector() {
   STLDeleteElements(&keyboards_);
   STLDeleteElements(&system_keyboards_);
 }
 
 void VirtualKeyboardSelector::AddVirtualKeyboard(
@@ skipping 10 matching lines @@
   }
 }
 
 const VirtualKeyboard* VirtualKeyboardSelector::SelectVirtualKeyboard(
     const std::string& layout) {
   if (layout.empty()) {
     LOG(ERROR) << "No layout is specified";
     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;
   }
 
   current_ = keyboard;
   return keyboard;
 }
 
-const VirtualKeyboard* VirtualKeyboardSelector::SelectVirtualKeyboardInternal(
+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;
+  }
+
+  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) {
-  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;
-    }
-  }
-  return NULL;
+  const VirtualKeyboard* keyboard =
+      SelectVirtualKeyboardInternal(keyboards_, layout, NULL);
+  if (!keyboard)
+    keyboard = SelectVirtualKeyboardInternal(system_keyboards_, layout, NULL);
+  return keyboard;
 }
 
 }  // namespace input_method
 }  // namespace chromeos