[cros] update screen re-factoring pt.1

chrome/browser/chromeos/login/update_screen.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/login/update_screen.h"
 
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/threading/thread_restrictions.h"
 #include "chrome/browser/chromeos/cros/cros_library.h"
 #include "chrome/browser/chromeos/login/screen_observer.h"
-#include "chrome/browser/chromeos/login/update_view.h"
+#include "chrome/browser/chromeos/login/update_screen_actor.h"
+#include "chrome/browser/chromeos/login/update_screen_views_actor.h"

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

Shouldn't be views_update_screen_actor.h to have consistency with existing
classes: we use Views or DOMUI as prefix

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

Done.

[Nikita (slow), 2011/05/20 12:09:46]

DOMUI is now named as WebUI.

 #include "chrome/browser/chromeos/login/wizard_controller.h"
 #include "content/browser/browser_thread.h"
 
+namespace chromeos {
+
 namespace {
 
 // Progress bar stages. Each represents progress bar value
 // at the beginning of each stage.
 // TODO(nkostylev): Base stage progress values on approximate time.
 // TODO(nkostylev): Animate progress during each state.
 const int kBeforeUpdateCheckProgress = 7;
 const int kBeforeDownloadProgress = 14;
 const int kBeforeVerifyingProgress = 74;
 const int kBeforeFinalizingProgress = 81;
 const int kProgressComplete = 100;
 
 // Defines what part of update progress does download part takes.
 const int kDownloadProgressIncrement = 60;
 
 // Considering 10px shadow from each side.
 const int kUpdateScreenWidth = 580;
 const int kUpdateScreenHeight = 305;
 
 const char kUpdateDeadlineFile[] = "/tmp/update-check-response-deadline";
 
+// Invoked from call to RequestUpdateCheck upon completion of the DBus call.
+void StartUpdateCallback(void* user_data,
+                         UpdateResult result,
+                         const char* msg) {
+  if (result != chromeos::UPDATE_RESULT_SUCCESS) {
+    DCHECK(user_data);
+    UpdateScreen* screen = static_cast<UpdateScreen*>(user_data);
+    if (UpdateScreen::HasInstance(screen))
+      screen->ExitUpdate(UpdateScreen::REASON_UPDATE_INIT_FAILED);
+  }
+}
+
 }  // anonymous namespace
 
-namespace chromeos {
-
-
 // static
 UpdateScreen::InstanceSet& UpdateScreen::GetInstanceSet() {
   static std::set<UpdateScreen*> instance_set;
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));  // not threadsafe.
   return instance_set;
 }
 
 // static
 bool UpdateScreen::HasInstance(UpdateScreen* inst) {
   InstanceSet& instance_set = GetInstanceSet();
   InstanceSet::iterator found = instance_set.find(inst);
   return (found != instance_set.end());
 }
 
+
 UpdateScreen::UpdateScreen(WizardScreenDelegate* delegate)
-    : DefaultViewScreen<chromeos::UpdateView>(delegate,
-                                              kUpdateScreenWidth,
-                                              kUpdateScreenHeight),
-      checking_for_update_(true),
+    : WizardScreen(delegate),
       reboot_check_delay_(0),
+      is_all_updates_critical_(true),  // See http://crosbug.com/10068

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

Comments in the header state it's false by default - could you update the
comment too (with bug number too).

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

Done.

+      is_checking_for_update_(true),
       is_downloading_update_(false),
-      is_all_updates_critical_(true) { // See http://crosbug.com/10068
+      is_shown_(false),
+      actor_(new UpdateScreenViewsActor(delegate,
+                                        kUpdateScreenWidth,
+                                        kUpdateScreenHeight)) {
   GetInstanceSet().insert(this);
 }
 
 UpdateScreen::~UpdateScreen() {
-  // Remove pointer to this object from view.
-  if (view())
-    view()->set_controller(NULL);
   CrosLibrary::Get()->GetUpdateLibrary()->RemoveObserver(this);
   GetInstanceSet().erase(this);
 }
 
 void UpdateScreen::UpdateStatusChanged(UpdateLibrary* library) {
   UpdateStatusOperation status = library->status().status;
-  if (checking_for_update_ && status > UPDATE_STATUS_CHECKING_FOR_UPDATE) {
-    checking_for_update_ = false;
+  if (is_checking_for_update_ && status > UPDATE_STATUS_CHECKING_FOR_UPDATE) {
+    is_checking_for_update_ = false;
   }
 
   switch (status) {
     case UPDATE_STATUS_CHECKING_FOR_UPDATE:
       // Do nothing in these cases, we don't want to notify the user of the
       // check unless there is an update.
       break;
     case UPDATE_STATUS_UPDATE_AVAILABLE:
       MakeSureScreenIsShown();
-      view()->SetProgress(kBeforeDownloadProgress);
+      actor_->SetProgress(kBeforeDownloadProgress);
       if (!HasCriticalUpdate()) {
         LOG(INFO) << "Noncritical update available: "
                   << library->status().new_version;
         ExitUpdate(REASON_UPDATE_NON_CRITICAL);
       } else {
         LOG(INFO) << "Critical update available: "
                   << library->status().new_version;
       }
       break;
     case UPDATE_STATUS_DOWNLOADING:
       {
         MakeSureScreenIsShown();
         if (!is_downloading_update_) {
           // Because update engine doesn't send UPDATE_STATUS_UPDATE_AVAILABLE
           // we need to is update critical on first downloading notification.
           is_downloading_update_ = true;
           if (!HasCriticalUpdate()) {
             LOG(INFO) << "Non-critical update available: "
                       << library->status().new_version;
             ExitUpdate(REASON_UPDATE_NON_CRITICAL);
           } else {
             LOG(INFO) << "Critical update available: "
                       << library->status().new_version;
           }
         }
-        view()->ShowCurtain(false);
+        actor_->ShowCurtain(false);
         int download_progress = static_cast<int>(
             library->status().download_progress * kDownloadProgressIncrement);
-        view()->SetProgress(kBeforeDownloadProgress + download_progress);
+        actor_->SetProgress(kBeforeDownloadProgress + download_progress);
       }
       break;
     case UPDATE_STATUS_VERIFYING:
       MakeSureScreenIsShown();
-      view()->SetProgress(kBeforeVerifyingProgress);
+      actor_->SetProgress(kBeforeVerifyingProgress);
       break;
     case UPDATE_STATUS_FINALIZING:
       MakeSureScreenIsShown();
-      view()->SetProgress(kBeforeFinalizingProgress);
+      actor_->SetProgress(kBeforeFinalizingProgress);
       break;
     case UPDATE_STATUS_UPDATED_NEED_REBOOT:
       MakeSureScreenIsShown();
       // Make sure that first OOBE stage won't be shown after reboot.
       WizardController::MarkOobeCompleted();
-      view()->SetProgress(kProgressComplete);
+      actor_->SetProgress(kProgressComplete);
       if (HasCriticalUpdate()) {
-        view()->ShowCurtain(false);
+        actor_->ShowCurtain(false);
         VLOG(1) << "Initiate reboot after update";
         CrosLibrary::Get()->GetUpdateLibrary()->RebootAfterUpdate();
         reboot_timer_.Start(base::TimeDelta::FromSeconds(reboot_check_delay_),
                             this,
                             &UpdateScreen::OnWaitForRebootTimeElapsed);
       } else {
         ExitUpdate(REASON_UPDATE_NON_CRITICAL);
       }
       break;
     case UPDATE_STATUS_IDLE:
     case UPDATE_STATUS_ERROR:
     case UPDATE_STATUS_REPORTING_ERROR_EVENT:
       ExitUpdate(REASON_UPDATE_ENDED);
       break;
     default:
       NOTREACHED();
       break;
   }
 }
 
-namespace {
-// Invoked from call to RequestUpdateCheck upon completion of the DBus call.
-void StartUpdateCallback(void* user_data,
-                         UpdateResult result,
-                         const char* msg) {
-  if (result != UPDATE_RESULT_SUCCESS) {
-    DCHECK(user_data);
-    UpdateScreen* screen = static_cast<UpdateScreen*>(user_data);
-    if (UpdateScreen::HasInstance(screen))
-      screen->ExitUpdate(UpdateScreen::REASON_UPDATE_INIT_FAILED);
-  }
-}
-}  // namespace
-
 void UpdateScreen::StartUpdate() {
-  // Reset view if view was created.
-  if (view()) {
-    view()->Reset();
-    view()->set_controller(this);
-    is_downloading_update_ = false;
-    view()->SetProgress(kBeforeUpdateCheckProgress);
-  }
-
   if (!CrosLibrary::Get()->EnsureLoaded()) {
     LOG(ERROR) << "Error loading CrosLibrary";
     ExitUpdate(REASON_UPDATE_INIT_FAILED);
   } else {
     CrosLibrary::Get()->GetUpdateLibrary()->AddObserver(this);
     VLOG(1) << "Initiate update check";
     CrosLibrary::Get()->GetUpdateLibrary()->RequestUpdateCheck(
         StartUpdateCallback, this);
   }
 }
 
 void UpdateScreen::CancelUpdate() {
-  // Screen has longer lifetime than it's view.
-  // View is deleted after wizard proceeds to the next screen.
-  if (view())
-    ExitUpdate(REASON_UPDATE_CANCELED);
+  ExitUpdate(REASON_UPDATE_CANCELED);
 }
 
 void UpdateScreen::Show() {
-  DefaultViewScreen<UpdateView>::Show();
-  view()->set_controller(this);
-  is_downloading_update_ = false;
-  view()->SetProgress(kBeforeUpdateCheckProgress);
+  is_shown_ = true;
+  actor_->Show();
+  actor_->SetProgress(kBeforeUpdateCheckProgress);
+}
+
+void UpdateScreen::Hide() {
+  actor_->Hide();
+  is_shown_ = false;
+}
+
+gfx::Size UpdateScreen::GetScreenSize() const {
+  return gfx::Size(kUpdateScreenWidth, kUpdateScreenHeight);
 }
 
 void UpdateScreen::ExitUpdate(UpdateScreen::ExitReason reason) {
   ScreenObserver* observer = delegate()->GetObserver(this);
   if (CrosLibrary::Get()->EnsureLoaded())
     CrosLibrary::Get()->GetUpdateLibrary()->RemoveObserver(this);
 
-  switch(reason) {
+  switch (reason) {
     case REASON_UPDATE_CANCELED:
       observer->OnExit(ScreenObserver::UPDATE_NOUPDATE);
       break;
     case REASON_UPDATE_INIT_FAILED:
       observer->OnExit(ScreenObserver::UPDATE_ERROR_CHECKING_FOR_UPDATE);
       break;
     case REASON_UPDATE_NON_CRITICAL:
     case REASON_UPDATE_ENDED:
       {
         UpdateLibrary* update_library = CrosLibrary::Get()->GetUpdateLibrary();
         switch (update_library->status().status) {
           case UPDATE_STATUS_UPDATE_AVAILABLE:
           case UPDATE_STATUS_UPDATED_NEED_REBOOT:
           case UPDATE_STATUS_DOWNLOADING:
           case UPDATE_STATUS_FINALIZING:
           case UPDATE_STATUS_VERIFYING:
             DCHECK(!HasCriticalUpdate());
             // Noncritical update, just exit screen as if there is no update.
             // no break
           case UPDATE_STATUS_IDLE:
             observer->OnExit(ScreenObserver::UPDATE_NOUPDATE);
             break;
           case UPDATE_STATUS_ERROR:
           case UPDATE_STATUS_REPORTING_ERROR_EVENT:
-            observer->OnExit(checking_for_update_ ?
+            observer->OnExit(is_checking_for_update_ ?
                 ScreenObserver::UPDATE_ERROR_CHECKING_FOR_UPDATE :
                 ScreenObserver::UPDATE_ERROR_UPDATING);
             break;
           default:
             NOTREACHED();
         }
       }
       break;
     default:
       NOTREACHED();
   }
 }
 
 void UpdateScreen::OnWaitForRebootTimeElapsed() {
   LOG(ERROR) << "Unable to reboot - asking user for a manual reboot.";
   MakeSureScreenIsShown();
-  view()->ShowManualRebootInfo();
+  actor_->ShowManualRebootInfo();
 }
 
 void UpdateScreen::MakeSureScreenIsShown() {
-  if (!view()) {
+  if (!is_shown_)

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

I think is_shown_ could be smth like IsVisible method of the actor. Why do we
need MakeSureScreenIsShown anyway? As we have actor now, maybe it should show
itself in SetProgress/SetCurtain if it's not shown. I doesn't look like the
screen should own that.

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

I thought about it.
First of all, currently, WizardController do some stuff before showing the
screen. Probably, after re-factoring it will not, but now it does. Also, my
desire is to make actor as simple as possible. Or using other words, move all
common logic to the controller (screen) part. What do you think?

[whywhat, 2011/05/21 19:26:58]

From code simplicity point of view it's better to perform this check in one/two
methods of actor than each time before we call one of these methods. If this is
needed for WebUIUpdateScreenActor, we can always make UpdateScreenActor a base
class with this check in it or a common wrapper. Since we'll eventually have
only one implementation, having check in two actors is not a big problem
comparing to calling MakeSureScreenIsShown in each case branch. BTW, can we call
it before the switch once?

[altimofeev, 2011/05/23 08:13:58]

To understand why MakeSureScreenIsShown can't be called before switch I refresh
the current logic: 
- WizardController initiates update checking.
- WizardController schedules delayed screen showing.
- if UpdateCheck returns with nothing to do, screen is not shown at all
(scheduled show is canceled).
- MakeSureScreenIsShown actually checks that screen has already been shown by
WizardController. Note, it speaks to WizardController to show the screen, not
Actor.

About simplicity - I don't get what methods are you talking about. Currently,
there is one method in UpdateScreen (MakeSureScreenIsShown), which interacts
with WizardController. 

[whywhat, 2011/05/23 08:46:28]

Ok, thanks for the explanation! Guess this logic distribution worth a separate
look later.

     delegate()->ShowCurrentScreen();
-  }
 }
 
 void UpdateScreen::SetRebootCheckDelay(int seconds) {
   if (seconds <= 0)
     reboot_timer_.Stop();
   DCHECK(!reboot_timer_.IsRunning());
   reboot_check_delay_ = seconds;
 }
 
 bool UpdateScreen::HasCriticalUpdate() {
@@ skipping 13 matching lines @@
   // TODO(dpolukhin): Analyze file content. Now we can just assume that
   // if the file exists and not empty, there is critical update.
   return true;
 }
 
 void UpdateScreen::SetAllUpdatesCritical(bool is_critical) {
   is_all_updates_critical_ = is_critical;
 }
 
 }  // namespace chromeos