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;
+
+bool SameOrNull(const GURL* rhs, const GURL* lhs) {
+  if (!rhs || !lhs) {
+    return true;
+  }
+  return *rhs == *lhs;
+}
+
 }  // namespace
 
 namespace chromeos {
 namespace input_method {
 
 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::LayoutIsSupported(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()) {
+    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;
   }
 
   current_ = keyboard;
   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;
 }
 
 }  // namespace input_method
 }  // namespace chromeos