[cros] update screen re-factoring pt.1

chrome/browser/chromeos/login/update_screen.h

Index: chrome/browser/chromeos/login/update_screen.h
diff --git a/chrome/browser/chromeos/login/update_screen.h b/chrome/browser/chromeos/login/update_screen.h
index c1afb0d9409b71f21ebd7121dc863c41861d8d99..aac24feb8b87214175465152c49607723b65c1f8 100644
--- a/chrome/browser/chromeos/login/update_screen.h
+++ b/chrome/browser/chromeos/login/update_screen.h
@@ -6,37 +6,49 @@
 #define CHROME_BROWSER_CHROMEOS_LOGIN_UPDATE_SCREEN_H_
 #pragma once
 
+#include <set>
+
+#include "base/gtest_prod_util.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/timer.h"
 #include "chrome/browser/chromeos/cros/update_library.h"
-#include "chrome/browser/chromeos/login/update_view.h"
-#include "chrome/browser/chromeos/login/view_screen.h"
+#include "chrome/browser/chromeos/login/wizard_screen.h"
 
 namespace chromeos {
 
-class UpdateController {
- public:
-  // Starts update.
-  virtual void StartUpdate() = 0;
-  // Cancels pending update without error.
-  virtual void CancelUpdate() = 0;
-};
+// Forward declaration.
+class UpdateScreenActor;
 
-class UpdateScreen: public DefaultViewScreen<chromeos::UpdateView>,
-                    public UpdateLibrary::Observer,
-                    public UpdateController {
+// Controller for the update screen. It does not depends on the specific

[whywhat, 2011/05/20 09:29:50]

nit: depends -> depend

[altimofeev, 2011/05/20 12:00:32]

Done.

+// realization of the screen showing (Views of WebUI based), it delegates it to

[whywhat, 2011/05/20 09:29:50]

nit: realization -> implementation, it delegates it -> the dependency is moved

[altimofeev, 2011/05/20 12:00:32]

Done.

+// the UpdateScreenActor instead.
+class UpdateScreen: public UpdateLibrary::Observer,
+                    public WizardScreen {
  public:
   explicit UpdateScreen(WizardScreenDelegate* delegate);
   virtual ~UpdateScreen();
 
-  // UpdateLibrary::Observer implementation:
-  virtual void UpdateStatusChanged(UpdateLibrary* library);
+  // Overridden from WizardScreen.
+  virtual void Show();
+  virtual void Hide();
+  virtual gfx::Size GetScreenSize() const;
 
-  // Overridden from UpdateController:
+  // Checks for updates and updates if needed. Made virtual to simplify mocking.

[whywhat, 2011/05/20 09:29:50]

nit: second "updates" -> performs an update

[altimofeev, 2011/05/20 12:00:32]

Done.

   virtual void StartUpdate();
+
+  // Force udpate cancel. Made virtual to simplify mocking.

[whywhat, 2011/05/20 09:29:50]

nit: Force cancel update (udpate -> update and cancel is a verb)

[altimofeev, 2011/05/20 12:00:32]

Done.
nit: cancel is a noun too.

   virtual void CancelUpdate();
 
-  // Overridden from ViewScreen.
-  virtual void Show();
+  // Reboot check delay get/set, in seconds.
+  int reboot_check_delay() const { return reboot_check_delay_; }
+  void SetRebootCheckDelay(int seconds);
+
+  // Set flag to treat all updates as critical (for test purpose mainly).
+  // Default value is false.
+  void SetAllUpdatesCritical(bool is_critical);

[whywhat, 2011/05/20 09:29:50]

If it's only for tests, why not make it private and use FRIEND_TEST?

[altimofeev, 2011/05/20 12:00:32]

Then I can completely remove it - done.

+
+  // Returns true if this instance is still active (i.e. has not been deleted).
+  static bool HasInstance(UpdateScreen* inst);
 
   enum ExitReason {
      REASON_UPDATE_CANCELED,
@@ -44,26 +56,19 @@ class UpdateScreen: public DefaultViewScreen<chromeos::UpdateView>,
      REASON_UPDATE_NON_CRITICAL,
      REASON_UPDATE_ENDED
   };
-
   // Reports update results to the ScreenObserver.
   virtual void ExitUpdate(ExitReason reason);
 
-  // Reboot check delay get/set, in seconds.
-  int reboot_check_delay() const { return reboot_check_delay_; }
-  void SetRebootCheckDelay(int seconds);
+ private:
+  FRIEND_TEST_ALL_PREFIXES(UpdateScreenTest, TestBasic);
+
+  // UpdateLibrary::Observer implementation:
+  virtual void UpdateStatusChanged(UpdateLibrary* library);
 
   // Returns true if there is critical system update that requires installation
   // and immediate reboot.
   bool HasCriticalUpdate();
 
-  // Set flag to treat all updates as critical (for test purpose mainly).
-  // Default value is false.
-  void SetAllUpdatesCritical(bool is_critical);
-
-  // Returns true if this instance is still active (i.e. has not been deleted).
-  static bool HasInstance(UpdateScreen* inst);
-
- private:
   // Timer notification handlers.
   void OnWaitForRebootTimeElapsed();
 
@@ -78,19 +83,22 @@ class UpdateScreen: public DefaultViewScreen<chromeos::UpdateView>,
   typedef std::set<UpdateScreen*> InstanceSet;
   static InstanceSet& GetInstanceSet();
 
-  // True if in the process of checking for update.
-  bool checking_for_update_;
-
   // Time in seconds after which we decide that the device has not rebooted
   // automatically. If reboot didn't happen during this interval, ask user to
   // reboot device manually.
   int reboot_check_delay_;
 
+  // Is all updates critical? If true, update deadlines are ignored.

[whywhat, 2011/05/20 09:29:50]

nit: Is -> Are

[altimofeev, 2011/05/20 12:00:32]

Done.

+  bool is_all_updates_critical_;

[whywhat, 2011/05/20 09:29:50]

optional: there too. or maybe rename to ignore_update_deadlines?

[altimofeev, 2011/05/20 12:00:32]

Done.

+  // True if in the process of checking for update.
+  bool is_checking_for_update_;
   // Flag that is used to detect when update download has just started.
   bool is_downloading_update_;
+  // Whether the update screen is shown.
+  bool is_shown_;
 
-  // Is all updates critical? If true, update deadlines are ignored.
-  bool is_all_updates_critical_;
+  // Keeps actor which is delegated with all showing operations.
+  scoped_ptr<UpdateScreenActor> actor_;
 
   DISALLOW_COPY_AND_ASSIGN(UpdateScreen);
 };