Add SetUserPreference function to VirtualKeyboardSelector

chrome/browser/chromeos/input_method/virtual_keyboard_selector.h

Index: chrome/browser/chromeos/input_method/virtual_keyboard_selector.h
diff --git a/chrome/browser/chromeos/input_method/virtual_keyboard_selector.h b/chrome/browser/chromeos/input_method/virtual_keyboard_selector.h
index 72cda9123625891333000f2e7af642f00c80201e..dc89c01d28f3df09076ab492979e202cd90cf45c 100644
--- a/chrome/browser/chromeos/input_method/virtual_keyboard_selector.h
+++ b/chrome/browser/chromeos/input_method/virtual_keyboard_selector.h
@@ -7,6 +7,7 @@
 #pragma once
 
 #include <list>
+#include <map>
 #include <set>
 #include <string>
 
@@ -32,6 +33,9 @@ class VirtualKeyboard {
   // in this case.
   GURL GetURLForLayout(const std::string& layout) const;
 
+  // Returns true if the virtual keyboard extension supports the |layout|.
+  bool LayoutIsSupported(const std::string& layout) const;

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

I prefer prefixing the function with "Is" (i.e. IsLayoutSupported or
IsSupportedLayout) because that makes it clear that this function return boolean
value.

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

+1 for IsSupportedLayout

On 2011/07/28 07:08:16, mazda wrote:

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

Done.

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

Done.

+
   const GURL& url() const { return url_; }
   const std::set<std::string>& supported_layouts() const {
     return supported_layouts_;
@@ -62,7 +66,9 @@ class VirtualKeyboardSelector {
 
   // Selects and returns the most suitable virtual keyboard extension for the
   // |layout|. Returns NULL if no virtual keyboard extension for the layout
-  // is found. If |current_|, which is the virtual keyboard extension currently
+  // is found. If a specific virtual keyboard extension for the |layout| is
+  // already set by SetUserPreference, the virtual keyboard extension is always
+  // returned. If |current_|, which is the virtual keyboard extension currently
   // in use, supports the |layout|, the current one will be returned. Otherwise
   // the function scans the list of |keyboards_| and then the list of
   // |system_keyboards_|. The most recently added keyboards to each list take
@@ -75,24 +81,43 @@ class VirtualKeyboardSelector {
   //   currently using.
   const VirtualKeyboard* SelectVirtualKeyboard(const std::string& layout);
 
-  // TODO(yusukes): Add a function something like
-  //   void SetUserPreference(const std::string& layout,
-  //                          const VirtualKeyboard& keyboard);
-  // so that users could use a specific virtual keyboard extension for the
-  // |layout|.
+  // Sets user preferences on virtual keyboard selection so that the virtual
+  // keyboard extension specified by the |url| is always selected for the
+  // |layout|. Returns false if a virtual keyboard extension whose address is
+  // |url| is not registered, or the extension specified by the |url| does not
+  // support the |layout|.
+  bool SetUserPreference(const std::string& layout, const GURL& url);
+
+  // Removes the preference for the |layout| added by SetUserPreference.
+  void RemoveUserPreference(const std::string& layout);
 
  protected:
-  // This function neither checks |current_| nor updates the variable. The
-  // function is protected for testability.
-  const VirtualKeyboard* SelectVirtualKeyboardInternal(
+  // Selects and returns the most suitable virtual keyboard extension for the
+  // |layout|. Unlike SelectVirtualKeyboard(), this function only scans
+  // |keyboards_| and |system_keyboards_| (in this order), and never updates
+  // |current_|. The function is protected for testability.
+  const VirtualKeyboard* SelectVirtualKeyboardByLayout(
       const std::string& layout);
 
  private:
+  // Selects and returns a virtual keyboard extension from |keyboards| which
+  // supports the |layout| and whose address is |url|. |url| can be NULL.
+  static const VirtualKeyboard* SelectVirtualKeyboardByUrl(

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

This name is a bit misleading, as it is actually selecting by both Layout and
Url.

Also, please add to the comment that if url is NULL, then URL checking is
ignored.

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

I've moved the function to the anonymous namespace in
virtual_keyboard_selector.cc, and renamed it.
Revised the function comment.

+      const std::list<const VirtualKeyboard*>& keyboards,
+      const std::string& layout,
+      const GURL* url);
+
   // A list of third party virtual keyboard extensions.
   std::list<const VirtualKeyboard*> keyboards_;
   // A list of system virtual keyboard extensions.
   std::list<const VirtualKeyboard*> system_keyboards_;
 
+  // A map from a layout name to a virtual keyboard extension.

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

would be easier to parse without "a", i.e. "A map from layout name to virtual
keyboard extension".

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

Done.

+  std::map<std::string, const VirtualKeyboard*> user_preference_;

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

How does this actually get saved between restarts of the browser?

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

Every time when the browser is restarted, NotifyPrefChanged function in
chrome/browser/chromeos/preferences.cc is called. I'll add SetUserPreference
calls to the function.

On 2011/07/28 18:31:48, bryeung wrote:

+
+  // TODO(yusukes): Support per-site preference. e.g. always use virtual
+  // keyboard ABC on https://mail.google.com/, XYZ on http://www.google.com/.
+
   // The virtual keyboard currently in use.
   const VirtualKeyboard* current_;