Decompose BrowserStateMonitor into two parts, simplifying unit tests and APIs.

Decompose BrowserStateMonitor into two parts, simplifying unit tests and APIs.

Decouple InputMethodManagerImpl from content notifications by requiring the client to push said notifications. BrowserStateMonitor and InputMethodPersistence thus become part of the client (configuration layer).

BUG=164375
TBR=sky

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173015

[erikwright (departed), 2012-12-10 20:56:50]

nona: PTAL.

With this CL the decoupling from chrome/ and content/ is nearly complete.

[Seigo Nonaka, 2012-12-11 04:33:04]

https://codereview.chromium.org/11466010/diff/17001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/browser_state_monitor.cc (right):

https://codereview.chromium.org/11466010/diff/17001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/browser_state_monitor.cc:84:
observer_.Run(state_);
null check? If |observer_| become never NULL, please add comment in header file.

https://codereview.chromium.org/11466010/diff/17001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/browser_state_monitor.h (right):

https://codereview.chromium.org/11466010/diff/17001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/browser_state_monitor.h:28: const
base::Callback<void(InputMethodManager::State)>& observer);
nit: Can we pass null callback? please write comments.

https://codereview.chromium.org/11466010/diff/17001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/input_method_persistence.cc (right):

https://codereview.chromium.org/11466010/diff/17001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:26: if
(local_state) {
You check the |local_state| just above.

Plz remove this condition or just
if (!local_state)
  return;
is fine, I think.

[erikwright (departed), 2012-12-11 16:39:44]

PTAL.

https://codereview.chromium.org/11466010/diff/17001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/browser_state_monitor.cc (right):

https://codereview.chromium.org/11466010/diff/17001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/browser_state_monitor.cc:84:
observer_.Run(state_);
On 2012/12/11 04:33:04, Seigo Nonaka wrote:
> null check? If |observer_| become never NULL, please add comment in header
file.

Done.

https://codereview.chromium.org/11466010/diff/17001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/browser_state_monitor.h (right):

https://codereview.chromium.org/11466010/diff/17001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/browser_state_monitor.h:28: const
base::Callback<void(InputMethodManager::State)>& observer);
On 2012/12/11 04:33:04, Seigo Nonaka wrote:
> nit: Can we pass null callback? please write comments.

Done.

https://codereview.chromium.org/11466010/diff/17001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/input_method_persistence.cc (right):

https://codereview.chromium.org/11466010/diff/17001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/input_method_persistence.cc:26: if
(local_state) {
On 2012/12/11 04:33:04, Seigo Nonaka wrote:
> You check the |local_state| just above.
> 
> Plz remove this condition or just
> if (!local_state)
>   return;
> is fine, I think.

Done. Note that the original code did DCHECK in this condition.

See the removal in the current CL:
https://codereview.chromium.org/11466010/diff/22001/chrome/browser/chromeos/i...

And it originated from BrowserStateMonitor - see removal in a prior CL (line
70):
https://codereview.chromium.org/11415266/diff/25001/chrome/browser/chromeos/i...

But I don't see big value in DCHECKing here at this point.

[Seigo Nonaka, 2012-12-11 17:01:52]

lgtm

https://codereview.chromium.org/11466010/diff/22001/chrome/browser/chromeos/i...
File chrome/browser/chromeos/input_method/browser_state_monitor.cc (right):

https://codereview.chromium.org/11466010/diff/22001/chrome/browser/chromeos/i...
chrome/browser/chromeos/input_method/browser_state_monitor.cc:35:
observer.Run(state_);
nit: please add null check here too.

[commit-bot: I haz the power, 2012-12-12 17:57:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/11466010/32001

[commit-bot: I haz the power, 2012-12-12 17:57:25]

Failed to apply patch for
chrome/browser/chromeos/input_method/browser_state_monitor.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/input_method/browser_state_monitor.h
  Hunk #1 FAILED at 8.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/chromeos/input_method/browser_state_monitor.h.rej

Patch:       chrome/browser/chromeos/input_method/browser_state_monitor.h
Index: chrome/browser/chromeos/input_method/browser_state_monitor.h
diff --git a/chrome/browser/chromeos/input_method/browser_state_monitor.h
b/chrome/browser/chromeos/input_method/browser_state_monitor.h
index
e74f8cae4f6deaa6bbcdd4108e3f0b63b5c40386..ba07af0c144ce057f6f6f59db90357d03d1da52e
100644
--- a/chrome/browser/chromeos/input_method/browser_state_monitor.h
+++ b/chrome/browser/chromeos/input_method/browser_state_monitor.h
@@ -8,49 +8,36 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "chrome/browser/chromeos/input_method/input_method_manager.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
-#include "content/public/browser/notification_types.h"
 
 namespace chromeos {
 namespace input_method {
 
-class InputMethodDelegate;
-
-// A class which monitors a notification from the browser to keep track of the
-// browser state (not logged in, logged in, etc.) and notify the current state
-// to the input method manager. The class also updates the appropriate Chrome
-// prefs via the InputMethodDelegate, depending on the current browser state.
-class BrowserStateMonitor : public content::NotificationObserver,
-                            public InputMethodManager::Observer {
+// Translates notifications from the browser (not logged in, logged in, etc.),
+// into InputMethodManager::State transitions.
+class BrowserStateMonitor : public content::NotificationObserver {
  public:
-  BrowserStateMonitor(InputMethodManager* manager,
-                      InputMethodDelegate* delegate);
+  // Constructs a monitor that will invoke the given observer callback whenever
+  // the InputMethodManager::State changes. Assumes that the current state is
+  // STATE_LOGIN_SCREEN. |observer| may be null.
+  explicit BrowserStateMonitor(
+      const base::Callback<void(InputMethodManager::State)>& observer);
   virtual ~BrowserStateMonitor();
 
   InputMethodManager::State state() const { return state_; }
 
- protected:
-  // InputMethodManager::Observer overrides:
-  virtual void InputMethodChanged(InputMethodManager* manager,
-                                  bool show_message) OVERRIDE;
-  virtual void InputMethodPropertyChanged(InputMethodManager* manager)
OVERRIDE;
-
   // content::NotificationObserver overrides:
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
  private:
-  void SetState(InputMethodManager::State new_state);
-
-  InputMethodManager* manager_;
-  InputMethodDelegate* delegate_;
+  base::Callback<void(InputMethodManager::State)> observer_;
   InputMethodManager::State state_;
-
-  // This is used to register this object to some browser notifications.
   content::NotificationRegistrar notification_registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(BrowserStateMonitor);

[commit-bot: I haz the power, 2012-12-13 20:07:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/11466010/35001

[commit-bot: I haz the power, 2012-12-13 20:07:53]

Presubmit check for 11466010-35001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.7s to calculate.

[erikwright (departed), 2012-12-13 20:23:08]

TBR sky for c/*.gypi, as sky requested in a comment in
https://chromiumcodereview.appspot.com/11488011

[commit-bot: I haz the power, 2012-12-13 20:23:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikwright@chromium.org/11466010/38001

[commit-bot: I haz the power, 2012-12-14 00:17:10]

Change committed as 173015