Handle review comments form previous change. Also, use enum types and not int in IPC messages to make things clearer.

Review URL: http://codereview.chromium.org/20140

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@9351 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 166 insertions, 58 deletions

diff --git a/chrome/test/automation/automation_constants.h b/chrome/test/automation/automation_constants.h
index 40c906f..b31139e 100644
--- a/chrome/test/automation/automation_constants.h
+++ b/chrome/test/automation/automation_constants.h
@@ -10,4 +10,10 @@ namespace automation {
   static const int kSleepTime = 250;
 }
 
+enum AutomationMsg_NavigationResponseValues {
+  AUTOMATION_MSG_NAVIGATION_ERROR = 0,
+  AUTOMATION_MSG_NAVIGATION_SUCCESS,
+  AUTOMATION_MSG_NAVIGATION_AUTH_NEEDED,
+};
+
 #endif  // CHROME_TEST_AUTOMATION_AUTOMATION_CONSTANTS_H__
diff --git a/chrome/test/automation/automation_messages.h b/chrome/test/automation/automation_messages.h
index d4e916c..289cf5e5 100644
--- a/chrome/test/automation/automation_messages.h
+++ b/chrome/test/automation/automation_messages.h
@@ -8,14 +8,120 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "chrome/browser/tab_contents/navigation_entry.h"
+#include "chrome/browser/tab_contents/security_style.h"
 #include "chrome/common/ipc_message_utils.h"
+#include "chrome/test/automation/automation_constants.h"
 
-enum AutomationMsg_NavigationResponseValues {
-  AUTOMATION_MSG_NAVIGATION_ERROR = 0,
-  AUTOMATION_MSG_NAVIGATION_SUCCESS,
-  AUTOMATION_MSG_NAVIGATION_AUTH_NEEDED,
+namespace IPC {
+
+template <>
+struct ParamTraits<AutomationMsg_NavigationResponseValues> {
+  typedef AutomationMsg_NavigationResponseValues param_type;
+  static void Write(Message* m, const param_type& p) {
+    m->WriteInt(p);
+  }
+  static bool Read(const Message* m, void** iter, param_type* p) {
+    int type;
+    if (!m->ReadInt(iter, &type))
+      return false;
+    *p = static_cast<AutomationMsg_NavigationResponseValues>(type);
+    return true;
+  }
+  static void Log(const param_type& p, std::wstring* l) {
+    std::wstring control;
+    switch (p) {
+     case AUTOMATION_MSG_NAVIGATION_ERROR:
+      control = L"AUTOMATION_MSG_NAVIGATION_ERROR";
+      break;
+     case AUTOMATION_MSG_NAVIGATION_SUCCESS:
+      control = L"AUTOMATION_MSG_NAVIGATION_SUCCESS";
+      break;
+     case AUTOMATION_MSG_NAVIGATION_AUTH_NEEDED:
+      control = L"AUTOMATION_MSG_NAVIGATION_AUTH_NEEDED";
+      break;
+     default:
+      control = L"UNKNOWN";
+      break;
+    }
+
+    LogParam(control, l);
+  }
+};
+
+template <>
+struct ParamTraits<SecurityStyle> {
+  typedef SecurityStyle param_type;
+  static void Write(Message* m, const param_type& p) {
+    m->WriteInt(p);
+  }
+  static bool Read(const Message* m, void** iter, param_type* p) {
+    int type;
+    if (!m->ReadInt(iter, &type))
+      return false;
+    *p = static_cast<SecurityStyle>(type);
+    return true;
+  }
+  static void Log(const param_type& p, std::wstring* l) {
+    std::wstring control;
+    switch (p) {
+     case SECURITY_STYLE_UNKNOWN:
+      control = L"SECURITY_STYLE_UNKNOWN";
+      break;
+     case SECURITY_STYLE_UNAUTHENTICATED:
+      control = L"SECURITY_STYLE_UNAUTHENTICATED";
+      break;
+     case SECURITY_STYLE_AUTHENTICATION_BROKEN:
+      control = L"SECURITY_STYLE_AUTHENTICATION_BROKEN";
+      break;
+     case SECURITY_STYLE_AUTHENTICATED:
+      control = L"SECURITY_STYLE_AUTHENTICATED";
+      break;
+     default:
+      control = L"UNKNOWN";
+      break;
+    }
+
+    LogParam(control, l);
+  }
 };
 
+template <>
+struct ParamTraits<NavigationEntry::PageType> {
+  typedef NavigationEntry::PageType param_type;
+  static void Write(Message* m, const param_type& p) {
+    m->WriteInt(p);
+  }
+  static bool Read(const Message* m, void** iter, param_type* p) {
+    int type;
+    if (!m->ReadInt(iter, &type))
+      return false;
+    *p = static_cast<NavigationEntry::PageType>(type);
+    return true;
+  }
+  static void Log(const param_type& p, std::wstring* l) {
+    std::wstring control;
+    switch (p) {
+     case NavigationEntry::NORMAL_PAGE:
+      control = L"NORMAL_PAGE";
+      break;
+     case NavigationEntry::ERROR_PAGE:
+      control = L"ERROR_PAGE";
+      break;
+     case NavigationEntry::INTERSTITIAL_PAGE:
+      control = L"INTERSTITIAL_PAGE";
+      break;
+     default:
+      control = L"UNKNOWN";
+      break;
+    }
+
+    LogParam(control, l);
+  }
+};
+
+}  // namespace IPC
+
 #define MESSAGES_INTERNAL_FILE \
     "chrome/test/automation/automation_messages_internal.h"
 #include "chrome/common/ipc_message_macros.h"
diff --git a/chrome/test/automation/automation_messages_internal.h b/chrome/test/automation/automation_messages_internal.h
index 05290d0d..09c8390 100644
--- a/chrome/test/automation/automation_messages_internal.h
+++ b/chrome/test/automation/automation_messages_internal.h
@@ -80,7 +80,7 @@ IPC_BEGIN_MESSAGES(Automation)
   // status code which is nonnegative on success.
   IPC_MESSAGE_ROUTED2(AutomationMsg_NavigateToURLRequest, int, GURL)
   IPC_MESSAGE_ROUTED1(AutomationMsg_NavigateToURLResponse,
-                      int)  // see AutomationMsg_NavigationResponseValues
+                      AutomationMsg_NavigationResponseValues)
 
   // This message is used to implement the asynchronous version of
   // NavigateToURL.
@@ -96,7 +96,7 @@ IPC_BEGIN_MESSAGES(Automation)
   // nonnegative on success.
   IPC_MESSAGE_ROUTED1(AutomationMsg_GoBackRequest, int)
   IPC_MESSAGE_ROUTED1(AutomationMsg_GoBackResponse,
-                      int)  // see AutomationMsg_NavigationResponseValues
+                      AutomationMsg_NavigationResponseValues)
 
   // This message notifies the AutomationProvider to navigate forward in session
   // history in the tab with given handle. The first parameter is the handle
@@ -104,7 +104,7 @@ IPC_BEGIN_MESSAGES(Automation)
   // nonnegative on success.
   IPC_MESSAGE_ROUTED1(AutomationMsg_GoForwardRequest, int)
   IPC_MESSAGE_ROUTED1(AutomationMsg_GoForwardResponse,
-                      int)  // see AutomationMsg_NavigationResponseValues
+                      AutomationMsg_NavigationResponseValues)
 
   // This message requests the number of browser windows that the app currently
   // has open.  The parameter in the response is the number of windows.
@@ -462,7 +462,7 @@ IPC_BEGIN_MESSAGES(Automation)
   // The response contains a status code which is nonnegative on success.
   IPC_MESSAGE_ROUTED2(AutomationMsg_NavigateInExternalTabRequest, int, GURL)
   IPC_MESSAGE_ROUTED1(AutomationMsg_NavigateInExternalTabResponse,
-                      int)  // see AutomationMsg_NavigationResponseValues
+                      AutomationMsg_NavigationResponseValues)
 
   // This message is an outgoing message from Chrome to an external host.
   // It is a notification that the NavigationState was changed
@@ -593,15 +593,14 @@ IPC_BEGIN_MESSAGES(Automation)
   //   - int: handle of the tab
   // Response:
   //  - bool: whether the operation was successful.
-  //  - int: the security style of the tab (enum SecurityStyle see
-  //         security_style.h)).
+  //  - SecurityStyle: the security style of the tab.
   //  - int: the status of the server's ssl cert (0 means no errors or no ssl
   //         was used).
   //  - int: the mixed content state, 0 means no mixed/unsafe contents.
   IPC_MESSAGE_ROUTED1(AutomationMsg_GetSecurityState, int)
   IPC_MESSAGE_ROUTED4(AutomationMsg_GetSecurityStateResponse,
                       bool,
-                      int,
+                      SecurityStyle,
                       int,
                       int)
 
@@ -611,10 +610,11 @@ IPC_BEGIN_MESSAGES(Automation)
   //   - int: handle of the tab
   // Response:
   //  - bool: whether the operation was successful.
-  //  - int: the type of the page currently displayed (enum PageType see
-  //         entry_navigation.h).
+  //  - NavigationEntry::PageType: the type of the page currently displayed.
   IPC_MESSAGE_ROUTED1(AutomationMsg_GetPageType, int)
-  IPC_MESSAGE_ROUTED2(AutomationMsg_GetPageTypeResponse, bool, int)
+  IPC_MESSAGE_ROUTED2(AutomationMsg_GetPageTypeResponse,
+                      bool,
+                      NavigationEntry::PageType)
 
   // This message simulates the user action on the SSL blocking page showing in
   // the specified tab.  This message is only effective if an interstitial page
@@ -659,7 +659,7 @@ IPC_BEGIN_MESSAGES(Automation)
   // success.
   IPC_MESSAGE_ROUTED1(AutomationMsg_ReloadRequest, int)
   IPC_MESSAGE_ROUTED1(AutomationMsg_ReloadResponse,
-                      int)  // see AutomationMsg_NavigationResponseValues
+                      AutomationMsg_NavigationResponseValues)
 
   // This message requests the handle (int64 app-unique identifier) of the
   // last active browser window, or the browser at index 0 if there is no last
diff --git a/chrome/test/automation/tab_proxy.cc b/chrome/test/automation/tab_proxy.cc
index afc0701..61b50cf 100644
--- a/chrome/test/automation/tab_proxy.cc
+++ b/chrome/test/automation/tab_proxy.cc
@@ -123,18 +123,18 @@ int TabProxy::FindInPage(const std::wstring& search_string,
   return matches;
 }
 
-int TabProxy::NavigateToURL(const GURL& url) {
+AutomationMsg_NavigationResponseValues TabProxy::NavigateToURL(const GURL& url) {
   return NavigateToURLWithTimeout(url, INFINITE, NULL);
 }
 
-int TabProxy::NavigateToURLWithTimeout(const GURL& url,
-                                       uint32 timeout_ms,
-                                       bool* is_timeout) {
+AutomationMsg_NavigationResponseValues TabProxy::NavigateToURLWithTimeout(
+    const GURL& url, uint32 timeout_ms, bool* is_timeout) {
   if (!is_valid())
     return AUTOMATION_MSG_NAVIGATION_ERROR;
 
   IPC::Message* response = NULL;
-  int navigate_response = AUTOMATION_MSG_NAVIGATION_ERROR;
+  AutomationMsg_NavigationResponseValues navigate_response =
+      AUTOMATION_MSG_NAVIGATION_ERROR;
   if (sender_->SendAndWaitForResponseWithTimeout(
       new AutomationMsg_NavigateToURLRequest(0, handle_, url), &response,
       AutomationMsg_NavigateToURLResponse::ID, timeout_ms, is_timeout)) {
@@ -144,13 +144,14 @@ int TabProxy::NavigateToURLWithTimeout(const GURL& url,
   return navigate_response;
 }
 
-int TabProxy::NavigateInExternalTab(const GURL& url) {
+AutomationMsg_NavigationResponseValues TabProxy::NavigateInExternalTab(
+    const GURL& url) {
   if (!is_valid())
     return AUTOMATION_MSG_NAVIGATION_ERROR;
 
   IPC::Message* response = NULL;
   bool is_timeout = false;
-  int rv = AUTOMATION_MSG_NAVIGATION_ERROR;
+  AutomationMsg_NavigationResponseValues rv = AUTOMATION_MSG_NAVIGATION_ERROR;
   if (sender_->SendAndWaitForResponseWithTimeout(
       new AutomationMsg_NavigateInExternalTabRequest(0, handle_, url),
       &response, AutomationMsg_NavigateInExternalTabResponse::ID, INFINITE,
@@ -206,12 +207,13 @@ bool TabProxy::NeedsAuth() const {
   return needs_auth;
 }
 
-int TabProxy::GoBack() {
+AutomationMsg_NavigationResponseValues TabProxy::GoBack() {
   if (!is_valid())
     return AUTOMATION_MSG_NAVIGATION_ERROR;
 
   IPC::Message* response = NULL;
-  int navigate_response = AUTOMATION_MSG_NAVIGATION_ERROR;
+  AutomationMsg_NavigationResponseValues navigate_response =
+      AUTOMATION_MSG_NAVIGATION_ERROR;
   if (sender_->SendAndWaitForResponse(
       new AutomationMsg_GoBackRequest(0, handle_), &response,
       AutomationMsg_GoBackResponse::ID)) {
@@ -221,12 +223,13 @@ int TabProxy::GoBack() {
   return navigate_response;
 }
 
-int TabProxy::GoForward() {
+AutomationMsg_NavigationResponseValues TabProxy::GoForward() {
   if (!is_valid())
     return AUTOMATION_MSG_NAVIGATION_ERROR;
 
   IPC::Message* response = NULL;
-  int navigate_response = AUTOMATION_MSG_NAVIGATION_ERROR;
+  AutomationMsg_NavigationResponseValues navigate_response =
+      AUTOMATION_MSG_NAVIGATION_ERROR;
   if (sender_->SendAndWaitForResponse(
       new AutomationMsg_GoForwardRequest(0, handle_), &response,
       AutomationMsg_GoForwardResponse::ID)) {
@@ -236,12 +239,13 @@ int TabProxy::GoForward() {
   return navigate_response;
 }
 
-int TabProxy::Reload() {
+AutomationMsg_NavigationResponseValues TabProxy::Reload() {
   if (!is_valid())
     return AUTOMATION_MSG_NAVIGATION_ERROR;
 
   IPC::Message* response = NULL;
-  int navigate_response = AUTOMATION_MSG_NAVIGATION_ERROR;
+  AutomationMsg_NavigationResponseValues navigate_response =
+      AUTOMATION_MSG_NAVIGATION_ERROR;
   if (sender_->SendAndWaitForResponse(
       new AutomationMsg_ReloadRequest(0, handle_), &response,
       AutomationMsg_ReloadResponse::ID)) {
@@ -277,11 +281,11 @@ bool TabProxy::GetCurrentURL(GURL* url) const {
   }
 
   IPC::Message* response = NULL;
-  bool succeeded;
-  succeeded = sender_->SendAndWaitForResponse(
+  bool succeeded = sender_->SendAndWaitForResponse(
       new AutomationMsg_TabURLRequest(0, handle_), &response,
       AutomationMsg_TabURLResponse::ID) &&
-      AutomationMsg_TabURLResponse::Read(response, &succeeded, url);
+      AutomationMsg_TabURLResponse::Read(response, &succeeded, url) &&
+      succeeded;
   scoped_ptr<IPC::Message> auto_deleter(response);
   return succeeded;
 }
@@ -463,11 +467,11 @@ ConstrainedWindowProxy* TabProxy::GetConstrainedWindow(
     return NULL;
 
   IPC::Message* response = NULL;
-  int handle;
   if (sender_->SendAndWaitForResponse(
       new AutomationMsg_ConstrainedWindowRequest(0, handle_, window_index),
       &response, AutomationMsg_ConstrainedWindowResponse::ID)) {
     scoped_ptr<IPC::Message> response_deleter(response);
+    int handle;
     if (AutomationMsg_ConstrainedWindowResponse::Read(response, &handle))
       return new ConstrainedWindowProxy(sender_, tracker_, handle);
   }
@@ -584,12 +588,11 @@ bool TabProxy::HideInterstitialPage() {
     return false;
 
   IPC::Message* response = NULL;
-  bool result;
   bool succeeded = sender_->SendAndWaitForResponse(
       new AutomationMsg_HideInterstitialPageRequest(0, handle_),
       &response, AutomationMsg_HideInterstitialPageResponse::ID) &&
-      AutomationMsg_HideInterstitialPageResponse::Read(response, &result) &&
-      result;
+      AutomationMsg_HideInterstitialPageResponse::Read(response, &succeeded) &&
+      succeeded;
   scoped_ptr<IPC::Message> response_deleter(response);
   return succeeded;
 }
@@ -660,38 +663,34 @@ bool TabProxy::GetSecurityState(SecurityStyle* security_style,
 
   IPC::Message* response = NULL;
   bool is_timeout = false;
-  int value;
-  bool succeeded;
-  succeeded = sender_->SendAndWaitForResponseWithTimeout(
+  bool succeeded = sender_->SendAndWaitForResponseWithTimeout(
       new AutomationMsg_GetSecurityState(0, handle_),
       &response,
       AutomationMsg_GetSecurityStateResponse::ID, INFINITE, &is_timeout) &&
       AutomationMsg_GetSecurityStateResponse::Read(
-          response, &succeeded, &value, ssl_cert_status, mixed_content_state);
-  if (succeeded)
-    *security_style = static_cast<SecurityStyle>(value);
+          response, &succeeded, security_style, ssl_cert_status,
+          mixed_content_state) &&
+      succeeded;
   scoped_ptr<IPC::Message> auto_deleter(response);
   return succeeded;
 }
 
-bool TabProxy::GetPageType(NavigationEntry::PageType* page_type) {
-  DCHECK(page_type);
+bool TabProxy::GetPageType(NavigationEntry::PageType* type) {
+  DCHECK(type);
 
   if (!is_valid())
     return false;
 
   IPC::Message* response = NULL;
   bool is_timeout = false;
-  int value;
   bool succeeded;
   succeeded = sender_->SendAndWaitForResponseWithTimeout(
       new AutomationMsg_GetPageType(0, handle_),
       &response,
       AutomationMsg_GetPageTypeResponse::ID, INFINITE, &is_timeout) &&
-      AutomationMsg_GetPageTypeResponse::Read(response, &succeeded, &value);
+      AutomationMsg_GetPageTypeResponse::Read(response, &succeeded, type) &&
+      succeeded;
   scoped_ptr<IPC::Message> auto_deleter(response);
-  if (succeeded)
-    *page_type = static_cast<NavigationEntry::PageType>(value);
   return succeeded;
 }
 
diff --git a/chrome/test/automation/tab_proxy.h b/chrome/test/automation/tab_proxy.h
index 508de11..932f5c7 100644
--- a/chrome/test/automation/tab_proxy.h
+++ b/chrome/test/automation/tab_proxy.h
@@ -12,6 +12,7 @@
 #include "chrome/browser/download/save_package.h"
 #include "chrome/browser/tab_contents/navigation_entry.h"
 #include "chrome/browser/tab_contents/security_style.h"
+#include "chrome/test/automation/automation_constants.h"
 #include "chrome/test/automation/automation_handle_tracker.h"
 
 class ConstrainedWindowProxy;
@@ -67,21 +68,19 @@ class TabProxy : public AutomationResourceProxy {
   // Navigates to a url. This method accepts the same kinds of URL input that
   // can be passed to Chrome on the command line. This is a synchronous call and
   // hence blocks until the navigation completes.
-  // Returns a status from AutomationMsg_NavigationResponseValues.
-  int NavigateToURL(const GURL& url);
+  AutomationMsg_NavigationResponseValues NavigateToURL(const GURL& url);
 
   // Navigates to a url. This is same as NavigateToURL with a timeout option.
   // The function returns until the navigation completes or timeout (in
   // milliseconds) occurs. If return after timeout, is_timeout is set to true.
-  int NavigateToURLWithTimeout(const GURL& url, uint32 timeout_ms,
-                               bool* is_timeout);
+  AutomationMsg_NavigationResponseValues NavigateToURLWithTimeout(
+      const GURL& url, uint32 timeout_ms, bool* is_timeout);
 
   // Navigates to a url in an externally hosted tab.
   // This method accepts the same kinds of URL input that
   // can be passed to Chrome on the command line. This is a synchronous call and
   // hence blocks until the navigation completes.
-  // Returns a status from AutomationMsg_NavigationResponseValues.
-  int NavigateInExternalTab(const GURL& url);
+  AutomationMsg_NavigationResponseValues NavigateInExternalTab(const GURL& url);
 
   // Navigates to a url. This is an asynchronous version of NavigateToURL.
   // The function returns immediately after sending the LoadURL notification
@@ -99,16 +98,15 @@ class TabProxy : public AutomationResourceProxy {
 
   // Equivalent to hitting the Back button. This is a synchronous call and
   // hence blocks until the navigation completes.
-  int GoBack();
+  AutomationMsg_NavigationResponseValues GoBack();
 
   // Equivalent to hitting the Forward button. This is a synchronous call and
   // hence blocks until the navigation completes.
-  // Returns a status from AutomationMsg_NavigationResponseValues.
-  int GoForward();
+  AutomationMsg_NavigationResponseValues GoForward();
 
   // Equivalent to hitting the Reload button. This is a synchronous call and
   // hence blocks until the navigation completes.
-  int Reload();
+  AutomationMsg_NavigationResponseValues Reload();
 
   // Closes the tab. This is synchronous, but does NOT block until the tab has
   // closed, rather it blocks until the browser has initiated the close. Use
@@ -131,7 +129,6 @@ class TabProxy : public AutomationResourceProxy {
 
   // Gets the HWND that corresponds to the content area of this tab.
   // Returns true if the call was successful.
-  // Returns a status from AutomationMsg_NavigationResponseValues.
   bool GetHWND(HWND* hwnd) const;
 
   // Gets the process ID that corresponds to the content area of this tab.