Landing Chris Guillory CL.

See original review at:
http://codereview.chromium.org/626014/show

BUG=None
TEST=None

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

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

5 files changed, 71 insertions, 60 deletions

diff --git a/chrome/browser/browser_accessibility.cc b/chrome/browser/browser_accessibility.cc
index 2bae8f4..f6cd32f 100644
--- a/chrome/browser/browser_accessibility.cc
+++ b/chrome/browser/browser_accessibility.cc
@@ -29,7 +29,7 @@ HRESULT BrowserAccessibility::Initialize(int iaccessible_id, int routing_id,
 HRESULT BrowserAccessibility::accDoDefaultAction(VARIANT var_id) {
   if (!instance_active()) {
     // Instance no longer active, fail gracefully.
-    // TODO(klink): Once we have MSAA events, change these fails to having
+    // TODO(ctguil): Once we have MSAA events, change these fails to having
     // BrowserAccessibilityManager firing the right event.
     return E_FAIL;
   }
@@ -42,7 +42,7 @@ HRESULT BrowserAccessibility::accDoDefaultAction(VARIANT var_id) {
     return E_FAIL;
   }
 
-  if (!response().return_code)
+  if (response().return_code == WebAccessibility::RETURNCODE_FALSE)
     return S_FALSE;
 
   return S_OK;
@@ -69,11 +69,11 @@ STDMETHODIMP BrowserAccessibility::accHitTest(LONG x_left, LONG y_top,
   ::ScreenToClient(parent_hwnd_, &p);
 
   if (!RequestAccessibilityInfo(WebAccessibility::FUNCTION_HITTEST,
-                                EmptyVariant(), p.x, p.y)) {
+                                ChildSelfVariant(), p.x, p.y)) {
     return E_FAIL;
   }
 
-  if (!response().return_code) {
+  if (response().return_code == WebAccessibility::RETURNCODE_FALSE) {
     // The point is outside of the object's boundaries.
     child->vt = VT_EMPTY;
     return S_FALSE;
@@ -156,7 +156,7 @@ STDMETHODIMP BrowserAccessibility::accNavigate(LONG nav_dir, VARIANT start,
     return E_FAIL;
   }
 
-  if (!response().return_code) {
+  if (response().return_code == WebAccessibility::RETURNCODE_FALSE) {
     // No screen element was found in the specified direction.
     end->vt = VT_EMPTY;
     return S_FALSE;
@@ -198,7 +198,8 @@ STDMETHODIMP BrowserAccessibility::get_accChild(VARIANT var_child,
     return E_FAIL;
   }
 
-  if (!response().return_code) {
+  // TODO(ctguil): Figure out when the return code would be false
+  if (response().return_code == WebAccessibility::RETURNCODE_FALSE) {
     // When at a leaf, children are handled by the parent object.
     *disp_child = NULL;
     return S_FALSE;
@@ -226,7 +227,7 @@ STDMETHODIMP BrowserAccessibility::get_accChildCount(LONG* child_count) {
     return E_INVALIDARG;
 
   if (!RequestAccessibilityInfo(WebAccessibility::FUNCTION_CHILDCOUNT,
-                                EmptyVariant(), NULL, NULL)) {
+                                ChildSelfVariant(), NULL, NULL)) {
     return E_FAIL;
   }
 
@@ -249,7 +250,7 @@ STDMETHODIMP BrowserAccessibility::get_accDefaultAction(VARIANT var_id,
     return E_FAIL;
   }
 
-  if (!response().return_code) {
+  if (response().return_code == WebAccessibility::RETURNCODE_FALSE) {
     // No string found.
     return S_FALSE;
   }
@@ -275,7 +276,7 @@ STDMETHODIMP BrowserAccessibility::get_accDescription(VARIANT var_id,
     return E_FAIL;
   }
 
-  if (!response().return_code) {
+  if (response().return_code == WebAccessibility::RETURNCODE_FALSE) {
     // No string found.
     return S_FALSE;
   }
@@ -296,11 +297,11 @@ STDMETHODIMP BrowserAccessibility::get_accFocus(VARIANT* focus_child) {
     return E_INVALIDARG;
 
   if (!RequestAccessibilityInfo(WebAccessibility::FUNCTION_GETFOCUSEDCHILD,
-                                EmptyVariant(), NULL, NULL)) {
+                                ChildSelfVariant(), NULL, NULL)) {
     return E_FAIL;
   }
 
-  if (!response().return_code) {
+  if (response().return_code == WebAccessibility::RETURNCODE_FALSE) {
     // The window that contains this object is not the active window.
     focus_child->vt = VT_EMPTY;
     return S_FALSE;
@@ -337,7 +338,8 @@ STDMETHODIMP BrowserAccessibility::get_accHelp(VARIANT var_id, BSTR* help) {
     return E_FAIL;
   }
 
-  if (!response().return_code || response().output_string.empty()) {
+  if (response().return_code == WebAccessibility::RETURNCODE_FALSE ||
+      response().output_string.empty()) {
     // No string found.
     return S_FALSE;
   }
@@ -363,7 +365,7 @@ STDMETHODIMP BrowserAccessibility::get_accKeyboardShortcut(VARIANT var_id,
     return E_FAIL;
   }
 
-  if (!response().return_code) {
+  if (response().return_code == WebAccessibility::RETURNCODE_FALSE) {
     // No string found.
     return S_FALSE;
   }
@@ -388,7 +390,7 @@ STDMETHODIMP BrowserAccessibility::get_accName(VARIANT var_id, BSTR* name) {
     return E_FAIL;
   }
 
-  if (!response().return_code) {
+  if (response().return_code == WebAccessibility::RETURNCODE_FALSE) {
     // No string found.
     return S_FALSE;
   }
@@ -425,11 +427,11 @@ STDMETHODIMP BrowserAccessibility::get_accParent(IDispatch** disp_parent) {
   }
 
   if (!RequestAccessibilityInfo(WebAccessibility::FUNCTION_GETPARENT,
-                                EmptyVariant(), NULL, NULL)) {
+                                ChildSelfVariant(), NULL, NULL)) {
     return E_FAIL;
   }
 
-  if (!response().return_code) {
+  if (response().return_code == WebAccessibility::RETURNCODE_FALSE) {
     // No parent exists for this object.
     return S_FALSE;
   }
@@ -499,7 +501,8 @@ STDMETHODIMP BrowserAccessibility::get_accValue(VARIANT var_id, BSTR* value) {
     return E_FAIL;
   }
 
-  if (!response().return_code || response().output_string.empty()) {
+  if (response().return_code == WebAccessibility::RETURNCODE_FALSE ||
+      response().output_string.empty()) {
     // No string found.
     return S_FALSE;
   }
@@ -540,17 +543,20 @@ STDMETHODIMP BrowserAccessibility::CreateInstance(REFIID iid,
 bool BrowserAccessibility::RequestAccessibilityInfo(int iaccessible_func_id,
                                                     VARIANT var_id, LONG input1,
                                                     LONG input2) {
+  DCHECK(V_VT(&var_id) == VT_I4);
+
   // Create and populate IPC message structure, for retrieval of accessibility
   // information from the renderer.
   WebAccessibility::InParams in_params;
   in_params.object_id = iaccessible_id_;
   in_params.function_id = iaccessible_func_id;
-  in_params.child_id = var_id.lVal;
+  in_params.child_id = V_I4(&var_id);
   in_params.input_long1 = input1;
   in_params.input_long2 = input2;
 
   return BrowserAccessibilityManager::GetInstance()->
-      RequestAccessibilityInfo(&in_params, routing_id_, process_id_);
+           RequestAccessibilityInfo(&in_params, routing_id_, process_id_) &&
+         response().return_code != WebAccessibility::RETURNCODE_FAIL;
 }
 
 const WebAccessibility::OutParams& BrowserAccessibility::response() {
diff --git a/chrome/browser/browser_accessibility.h b/chrome/browser/browser_accessibility.h
index 8833d88..ecf746e 100644
--- a/chrome/browser/browser_accessibility.h
+++ b/chrome/browser/browser_accessibility.h
@@ -126,12 +126,13 @@ class ATL_NO_VTABLE BrowserAccessibility
   int routing_id() const { return routing_id_; }
 
  private:
-  // Creates an empty VARIANT. Used as the equivalent of a NULL (unused) input
-  // parameter.
-  VARIANT EmptyVariant() const {
-    VARIANT empty;
-    empty.vt = VT_EMPTY;
-    return empty;
+  // Creates a VARIANT that will reference the current accessibility
+  // object, not a child accessibility object.
+  VARIANT ChildSelfVariant() const {
+    VARIANT var;
+    V_VT(&var) = VT_I4;
+    V_I4(&var) = CHILDID_SELF;
+    return var;
   }
 
   // Wrapper functions, calling through to singleton instance of
diff --git a/chrome/renderer/render_view.cc b/chrome/renderer/render_view.cc
index 7d68158..7240570 100644
--- a/chrome/renderer/render_view.cc
+++ b/chrome/renderer/render_view.cc
@@ -1285,8 +1285,8 @@ void RenderView::UpdateURL(WebFrame* frame) {
 
 #if defined(OS_WIN)
   if (accessibility_.get()) {
-    // Clear accessibility info cache.
-    accessibility_->clear();
+    // Remove accessibility info cache.
+    accessibility_.reset();
   }
 #else
   // TODO(port): accessibility not yet implemented. See http://crbug.com/8288.
@@ -3660,9 +3660,10 @@ void RenderView::OnGetAccessibilityInfo(
     accessibility_->initialize(webview());
   }
 
-  webkit_glue::WebAccessibility::GetAccObjInfo(accessibility_.get(),
-                                               in_params,
-                                               out_params);
+  out_params->return_code =
+      webkit_glue::WebAccessibility::GetAccObjInfo(accessibility_.get(),
+                                                  in_params,
+                                                  out_params);
 
 #else  // defined(OS_WIN)
   // TODO(port): accessibility not yet implemented
diff --git a/webkit/glue/webaccessibility.cc b/webkit/glue/webaccessibility.cc
index 9624467..1af91d6 100644
--- a/webkit/glue/webaccessibility.cc
+++ b/webkit/glue/webaccessibility.cc
@@ -145,11 +145,9 @@ long ConvertState(const WebAccessibilityObject& o) {
   return state;
 }
 
-
-bool WebAccessibility::GetAccObjInfo(WebAccessibilityCache* cache,
+int32 WebAccessibility::GetAccObjInfo(WebAccessibilityCache* cache,
   const WebAccessibility::InParams& in_params,
   WebAccessibility::OutParams* out_params) {
-
   // Find object requested by |object_id|.
   WebAccessibilityObject active_acc_obj;
 
@@ -167,20 +165,20 @@ bool WebAccessibility::GetAccObjInfo(WebAccessibilityCache* cache,
 
     active_acc_obj = cache->getObjectById(in_params.object_id);
     if (active_acc_obj.isNull())
-      return false;
+      return RETURNCODE_FAIL;
 
     // child_id == 0 means self. Otherwise, it's a local child - 1.
     if (in_params.child_id > 0) {
       unsigned index = in_params.child_id - 1;
       if (index >= active_acc_obj.childCount())
-        return false;
+        return RETURNCODE_FAIL;
 
       active_acc_obj = active_acc_obj.childAt(index);
     }
   }
 
   if (active_acc_obj.isNull())
-    return false;
+    return RETURNCODE_FAIL;
 
   // Temp paramters for holding output information.
   WebAccessibilityObject out_acc_obj;
@@ -189,14 +187,14 @@ bool WebAccessibility::GetAccObjInfo(WebAccessibilityCache* cache,
   switch (in_params.function_id) {
     case WebAccessibility::FUNCTION_DODEFAULTACTION: {
       if (!active_acc_obj.performDefaultAction())
-        return false;
+        return RETURNCODE_FALSE;
       break;
     }
     case WebAccessibility::FUNCTION_HITTEST: {
       WebPoint point(in_params.input_long1, in_params.input_long2);
       out_acc_obj = active_acc_obj.hitTest(point);
       if (out_acc_obj.isNull())
-        return false;
+        return RETURNCODE_FALSE;
       break;
     }
     case WebAccessibility::FUNCTION_LOCATION: {
@@ -216,13 +214,13 @@ bool WebAccessibility::GetAccObjInfo(WebAccessibilityCache* cache,
         case WebAccessibility::DIRECTION_LEFT:
         case WebAccessibility::DIRECTION_RIGHT:
           // These directions are not implemented, matching Mozilla and IE.
-          return false;
+          return RETURNCODE_FALSE;
         case WebAccessibility::DIRECTION_LASTCHILD:
         case WebAccessibility::DIRECTION_FIRSTCHILD:
           // MSDN states that navigating to first/last child can only be from
           // self.
           if (!local_child)
-            return false;
+            return RETURNCODE_FALSE;
 
           if (dir == WebAccessibility::DIRECTION_FIRSTCHILD) {
             out_acc_obj = active_acc_obj.firstChild();
@@ -240,10 +238,12 @@ bool WebAccessibility::GetAccObjInfo(WebAccessibilityCache* cache,
           break;
         }
         default:
-          return false;
+          return RETURNCODE_FALSE;
       }
+
       if (out_acc_obj.isNull())
-        return false;
+        return RETURNCODE_FALSE;
+
       break;
     }
     case WebAccessibility::FUNCTION_GETCHILD: {
@@ -258,13 +258,13 @@ bool WebAccessibility::GetAccObjInfo(WebAccessibilityCache* cache,
     case WebAccessibility::FUNCTION_DEFAULTACTION: {
       out_string = active_acc_obj.actionVerb();
       if (out_string.empty())
-        return false;
+        return RETURNCODE_FALSE;
       break;
     }
     case WebAccessibility::FUNCTION_DESCRIPTION: {
       out_string = active_acc_obj.accessibilityDescription();
       if (out_string.empty())
-        return false;
+        return RETURNCODE_FALSE;
       // From the Mozilla MSAA implementation:
       // "Signal to screen readers that this description is speakable and is not
       // a formatted positional information description. Don't localize the
@@ -276,31 +276,31 @@ bool WebAccessibility::GetAccObjInfo(WebAccessibilityCache* cache,
     case WebAccessibility::FUNCTION_GETFOCUSEDCHILD: {
       out_acc_obj = active_acc_obj.focusedChild();
       if (out_acc_obj.isNull())
-        return false;
+        return RETURNCODE_FALSE;
       break;
     }
     case WebAccessibility::FUNCTION_HELPTEXT: {
       out_string = active_acc_obj.helpText();
       if (out_string.empty())
-        return false;
+        return RETURNCODE_FALSE;
       break;
     }
     case WebAccessibility::FUNCTION_KEYBOARDSHORTCUT: {
       out_string = active_acc_obj.keyboardShortcut();
       if (out_string.empty())
-        return false;
+        return RETURNCODE_FALSE;
       break;
     }
     case WebAccessibility::FUNCTION_NAME: {
       out_string = active_acc_obj.title();
       if (out_string.empty())
-        return false;
+        return RETURNCODE_FALSE;
       break;
     }
     case WebAccessibility::FUNCTION_GETPARENT: {
       out_acc_obj = active_acc_obj.parentObject();
       if (out_acc_obj.isNull())
-        return false;
+        return RETURNCODE_FALSE;
       break;
     }
     case WebAccessibility::FUNCTION_ROLE: {
@@ -314,12 +314,12 @@ bool WebAccessibility::GetAccObjInfo(WebAccessibilityCache* cache,
     case WebAccessibility::FUNCTION_VALUE: {
       out_string = active_acc_obj.stringValue();
       if (out_string.empty())
-        return false;
+        return RETURNCODE_FALSE;
       break;
     }
     default:
       // Non-supported function id.
-      return false;
+      return RETURNCODE_FAIL;
   }
 
   // Output and hashmap assignments, as appropriate.
@@ -327,14 +327,14 @@ bool WebAccessibility::GetAccObjInfo(WebAccessibilityCache* cache,
     out_params->output_string = out_string;
 
   if (out_acc_obj.isNull())
-    return true;
+    return RETURNCODE_TRUE;
 
   int id = cache->addOrGetId(out_acc_obj);
   out_params->object_id = id;
   out_params->output_long1 = -1;
 
-  // TODO(klink): Handle simple objects returned.
-  return true;
+  // TODO(ctguil): Handle simple objects returned.
+  return RETURNCODE_TRUE;
 }
 
 }  // namespace webkit_glue
diff --git a/webkit/glue/webaccessibility.h b/webkit/glue/webaccessibility.h
index b1be0d1..4bfd7743 100644
--- a/webkit/glue/webaccessibility.h
+++ b/webkit/glue/webaccessibility.h
@@ -123,6 +123,12 @@ class WebAccessibility {
     STATE_UNAVAILABLE
   };
 
+  enum ReturnCode {
+    RETURNCODE_TRUE,    // MSAA S_OK
+    RETURNCODE_FALSE,   // MSAA S_FALSE
+    RETURNCODE_FAIL     // E_FAIL
+  };
+
   // Parameters structure to hold a union of the possible accessibility function
   // INPUT variables, with the unused fields always set to default value. Used
   // in ViewMsg_GetAccessibilityInfo, as only parameter.
@@ -163,14 +169,11 @@ class WebAccessibility {
     // String output parameter.
     string16 output_string;
 
-    // Return code, either true (MSAA S_OK) or false (MSAA S_FALSE).
-    // Interface-specific error return codes (e.g. MSAA's E_POINTER,
-    // E_INVALIDARG, E_FAIL, E_NOTIMPL) must be handled on the browser side by
-    // input validation.
-    bool return_code;
+    // Return code of the accessibility function call.
+    int32 return_code;
   };
 
-  static bool GetAccObjInfo(WebKit::WebAccessibilityCache* cache,
+  static int32 GetAccObjInfo(WebKit::WebAccessibilityCache* cache,
       const InParams& in_params, OutParams* out_params);
 };