Add SetUserPreference function to VirtualKeyboardSelector

chrome/browser/chromeos/input_method/virtual_keyboard_selector.h

 // 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.
 
 #ifndef CHROME_BROWSER_CHROMEOS_INPUT_METHOD_VIRTUAL_KEYBOARD_SELECTOR_H_
 #define CHROME_BROWSER_CHROMEOS_INPUT_METHOD_VIRTUAL_KEYBOARD_SELECTOR_H_
 #pragma once
 
 #include <list>
+#include <map>
 #include <set>
 #include <string>
 
 #include "base/basictypes.h"
 #include "googleurl/src/gurl.h"
 
 namespace chromeos {
 namespace input_method {
 
 // A class which represents a virtual keyboard extension. One virtual keyboard
 // extension can support more than one keyboard layout.
 class VirtualKeyboard {
  public:
   VirtualKeyboard(const GURL& url,
                   const std::set<std::string>& supported_layouts,
                   bool is_system);
   ~VirtualKeyboard();
 
   // Returns URL for displaying the keyboard UI specified by |layout|.
   // For example, when |url_| is "http://adcfj..kjhil/" and |layout| is "us",
   // the function would return "http://adcfj..kjhil/index.html#us". When
   // |layout| is empty, it returns |url_| as-is, which is "http://adcfj..kjhil/"
   // 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_;
   }
   bool is_system() const { return is_system_; }
 
  private:
   const GURL url_;
   const std::set<std::string> supported_layouts_;
   const bool is_system_;
@@ skipping 10 matching lines @@
   // Adds a new virtual keyboard extension. If |keyboard.is_system_| is true,
   // the virtual keyboard extension will have lower priority than non-system
   // keyboard extensions.
   // TODO(yusukes): Add RemoveVirtualKeyboard() as well.
   void AddVirtualKeyboard(const GURL& url,
                           const std::set<std::string>& supported_layouts,
                           bool is_system);
 
   // 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
   // precedence.
   //
   // Checking the |current_| keyboard is important for the following use case:
   // - If I have installed a VK extension that provides a US and an FR layout
   //   and I switch from the US layout of the extension (+ English IME) to the
   //   French IME, then I would like to use the FR layout of the extension I am
   //   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_;
 
   DISALLOW_COPY_AND_ASSIGN(VirtualKeyboardSelector);
 };
 
 }  // namespace input_method
 }  // namespace chromeos
 
 #endif  // CHROME_BROWSER_CHROMEOS_INPUT_METHOD_VIRTUAL_KEYBOARD_SELECTOR_H_