740758 - dexpcom nsAccessible::GetValue

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

similar to bug 652378. Define method as
void Value(nsAString& aValue);

Comment 1

[Mark Capella [:capella]]

In bug 652378 for changing GetDescription(), the final change involved:
void Description(nsString &aDescription) ...

I'm assuming we'll do this here:
void Value(nsString& aValue);

Instead of the nsAString& in the above description.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #1)

> I'm assuming we'll do this here:
> void Value(nsString& aValue);
> 
> Instead of the nsAString& in the above description.

yes, this makes sense because some GetValue implementation would benefit of having nsString& (for example, nsHTMLComboboxAccessible::GetValue which can do option->Name(aValue) instead option->GetName(aValue) when GetName() is dexpcomed).

Comment 3

[Mark Capella [:capella]]

Attachment: Patch (v1) - very rough

I'm going to ask for feedback here, though I know this is a very rough first draft.

After all the changes I made, and got a clean build, the test suite fails with two tests, both involving Textboxs with passwords:

failed | Wrong return value for getValue on password_textbox! - got undefined, expected 2147500037

In addition to that, I'm looking at several files where I couldn't change references from GetValue() to Value().

The common thread in these files seems to be the use of do_QueryInterface():

nsHTMLFormControlAccessible.cpp, nsAccessibleWrap.cpp, nsXFormsAccessible.cpp, nsXFormsFormControlsAccessible.cpp, and nsXULFormControlAccessible.cpp.

These I don't understand yet but am looking at:

nsBaseWidgetAccessible.cpp function nsLinkableAccessible::Value()
nsHTMLImageMapAccessible.cpp function nsHTMLAreaAccessible::GetNameInternal()

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 612792 [details] [diff] [review]
Patch (v1) - very rough

Review of attachment 612792 [details] [diff] [review]:
-----------------------------------------------------------------

please list test failures

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +767,5 @@
>          rv = propElem->GetKey(name);
>          NS_ENSURE_SUCCESS(rv, objAttributeSet);
>  
>          nsAutoString value;
> +        propElem->Value(value);

propElem is not nsAccessible object

I'm curious why you don't have other changes on atk part.

::: accessible/src/base/nsAccessible.cpp
@@ +1673,5 @@
> +
> +  return NS_OK;
> +}
> +
> +/* DOMString getValue (); */

get rid this comment pls

@@ +1704,4 @@
>      }
>    }
>  
> +  return;

no need to return here

::: accessible/src/base/nsAccessible.h
@@ +135,5 @@
>     */
>    virtual void Description(nsString& aDescription);
>  
>    /**
> +   * get the value of this accessible

get -> Get, dot in the end

::: accessible/src/base/nsApplicationAccessible.h
@@ +106,1 @@
>    virtual void Description(nsString& aDescription);

keep it after Description

::: accessible/src/base/nsBaseWidgetAccessible.cpp
@@ +135,3 @@
>  
> +  if (mIsLink)
> +    mActionAcc->GetValue(aValue);

I'd prefer
if (aValue.IsEmpty() && mIsLink)
  mActionAcc->Value(aValue);

::: accessible/src/base/nsBaseWidgetAccessible.h
@@ +105,5 @@
>    virtual already_AddRefed<nsIURI> AnchorURIAt(PRUint32 aAnchorIndex);
>  
>  protected:
>    // nsAccessible
> +  virtual void Value(nsString& _retval);

_retval -> aValue

::: accessible/src/base/nsFormControlAccessible.cpp
@@ +113,5 @@
>  
>    nsAutoString value;
>    value.AppendFloat(percentValue); // AppendFloat isn't available on nsAString
>    value.AppendLiteral("%");
>    aValue = value;

change aValue directly

::: accessible/src/html/nsHTMLFormControlAccessible.cpp
@@ +406,5 @@
>    return NS_OK;
>  }
>  
> +void
> +nsHTMLTextFieldAccessible::Value(nsString& _retval)

_retval -> aValue

@@ +420,5 @@
>    
>    nsCOMPtr<nsIDOMHTMLInputElement> inputElement(do_QueryInterface(mContent));
>    if (inputElement) {
> +    inputElement->GetValue(_retval);
> +    return;

no return is need since end of the function

::: accessible/src/html/nsHTMLFormControlAccessible.h
@@ +140,5 @@
>    // nsHyperTextAccessible
>    virtual already_AddRefed<nsIEditor> GetEditor() const;
>  
>    // nsAccessible
> +  virtual void Value(nsString& _retval);

aValue

::: accessible/src/html/nsHTMLSelectAccessible.h
@@ +193,5 @@
>    // nsAccessNode
>    virtual void Shutdown();
>  
>    // nsAccessible
> +  virtual void Value(nsString& _retval);

aValue

::: accessible/src/mac/mozHTMLAccessible.mm
@@ +116,5 @@
>  
>  - (NSURL*)url
>  {
>    if (!mGeckoAccessible)
>      return nil;

pls add IsDefunct check

::: accessible/src/xforms/nsXFormsAccessible.h
@@ +73,5 @@
>    nsXFormsAccessible(nsIContent* aContent, nsDocAccessible* aDoc);
>  
>    // nsIAccessible
>  
>    // Returns value of instance node that xforms element is bound to.

remove these comments

@@ +81,2 @@
>    // Returns value of child xforms 'hint' element.
>    virtual void Description(nsString& aDescription);

put after dseciption please

@@ +192,3 @@
>    NS_IMETHOD DoAction(PRUint8 aIndex);
>  
> +  virtual void Value(nsString& aValue);

put // nsAccessible comment before the prototype

::: accessible/src/xforms/nsXFormsFormControlsAccessible.h
@@ +154,5 @@
>  {
>  public:
>    nsXFormsSecretAccessible(nsIContent* aContent, nsDocAccessible* aDoc);
>  
>    // nsIAccessible

remove // nsIAccessible comment

::: accessible/src/xforms/nsXFormsWidgetsAccessible.h
@@ +90,5 @@
>  public:
>    nsXFormsComboboxPopupWidgetAccessible(nsIContent* aContent,
>                                          nsDocAccessible* aDoc);
>  
>    // nsIAccessible

same

::: accessible/src/xul/nsXULColorPickerAccessible.h
@@ +49,5 @@
>  public:
>    nsXULColorPickerTileAccessible(nsIContent* aContent,
>                                   nsDocAccessible* aDoc);
>  
>    // nsIAccessible

remove comment

@@ +54,3 @@
>  
>    // nsAccessible
> +  virtual void Value(nsString& _retval);

aValue

::: accessible/src/xul/nsXULFormControlAccessible.cpp
@@ +743,5 @@
>    }
>    nsCOMPtr<nsIDOMXULMenuListElement> menuList(do_QueryInterface(mContent));
>    if (menuList) {
> +    menuList->GetLabel(aValue);
> +    return;

no need to return

::: accessible/src/xul/nsXULListboxAccessible.cpp
@@ +210,5 @@
>  /**
>    * Our value is the label of our ( first ) selected child.
>    */
> +void
> +nsXULListboxAccessible::Value(nsString& _retval)

aValue

@@ +219,5 @@
>      nsCOMPtr<nsIDOMXULSelectControlItemElement> selectedItem;
>      select->GetSelectedItem(getter_AddRefs(selectedItem));
> +    if (selectedItem) {
> +      selectedItem->GetLabel(_retval);
> +      return;

no return is needed

::: accessible/src/xul/nsXULListboxAccessible.h
@@ +103,5 @@
>  
>    // nsIAccessibleTable
>    NS_DECL_OR_FORWARD_NSIACCESSIBLETABLE_WITH_XPCACCESSIBLETABLE
>  
>    // nsIAccessible

remove comment

::: accessible/src/xul/nsXULTabAccessible.cpp
@@ +186,5 @@
>  {
>    return 0;
>  }
>  
>  /** no value */

remove comment

@@ +188,5 @@
>  }
>  
>  /** no value */
> +void
> +nsXULTabsAccessible::Value(nsString& _retval)

aValue

::: accessible/src/xul/nsXULTabAccessible.h
@@ +75,5 @@
>  {
>  public:
>    nsXULTabsAccessible(nsIContent* aContent, nsDocAccessible* aDoc);
>  
>    // nsIAccessible

rm comment

::: accessible/src/xul/nsXULTreeAccessible.cpp
@@ +164,5 @@
>      if (cols)
>        cols->GetKeyColumn(getter_AddRefs(keyCol));
>  
> +    mTreeView->GetCellText(currentIndex, keyCol, aValue);
> +    return;

no return

::: accessible/src/xul/nsXULTreeAccessible.h
@@ +73,5 @@
>    NS_DECL_ISUPPORTS_INHERITED
>    NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(nsXULTreeAccessible,
>                                             nsAccessible)
>  
>    // nsIAccessible

a comment

Comment 5

[Mark Capella [:capella]]

I've incorporated the above changes.

The problems with the two test-suite failures in test_textboxes.html and test_textboxes.xul failing is due to the fact that we no longer return NS_ERROR_FAILURE as a possible result in nsHTMLTextFieldAccessible::Value nsXULTextFieldAccessible::Value.

I can fix this by a change to testtextboxes.js 
http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/testTextboxes.js#3
to handle/not expect the condition. This would fix our test-suite, but I wonder if this would this cause some regression to another external system?

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to Mark Capella [:capella] from comment #5)
> I've incorporated the above changes.
> 
> The problems with the two test-suite failures in test_textboxes.html and
> test_textboxes.xul failing is due to the fact that we no longer return
> NS_ERROR_FAILURE as a possible result in nsHTMLTextFieldAccessible::Value
> nsXULTextFieldAccessible::Value.
> 
> I can fix this by a change to testtextboxes.js 
> http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/
> testTextboxes.js#3
> to handle/not expect the condition. This would fix our test-suite, but I
> wonder if this would this cause some regression to another external system?

yeah, change the test to make sure we return no value, but don't throw.  That behavior is weird, and if someone actually depends on it they should be fix, but I really suspect nobody does.

Comment 7

[Mark Capella [:capella]]

Attachment: Patch (v2)

Latest patch ... a lot less rough than the first one :)

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #6)

> yeah, change the test to make sure we return no value, but don't throw. 
> That behavior is weird, and if someone actually depends on it they should be
> fix, but I really suspect nobody does.

you should be cautious with this change, we might depend on it internally (nsHyperTextAccessible logic) and AT could depend on it (the best way to see what IE does I think).

Comment 9

[alexander :surkov (:asurkov)]

Here's what MSAA expects (http://msdn.microsoft.com/en-us/library/windows/desktop/dd318062%28v=vs.85%29.aspx):
The Value property is a single string that contains the text in the edit control. However, if the control is password protected, the Value property returns E_ACCESSDENIED. For multiline edit controls, the string contains a carriage return and a newline character at the end of each line.

So returning a failure is a correct way. But I think we should be more specific by returning accessdenied. Perhaps we are not required to do that on internal or xpcom level (if we don't depend on this code of course) but it makes sense to do from code path uniqueness point of view. Anyway at MSAA level at least we should make sure we return correct value.

Comment 10

[Mark Capella [:capella]]

Before you lose me, since nsHTMLTextFieldAccessible::Value(nsString&) returns a void, do you plan to return the error condition in the nsString proper?

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to Mark Capella [:capella] from comment #10)
> Before you lose me, since nsHTMLTextFieldAccessible::Value(nsString&)
> returns a void, do you plan to return the error condition in the nsString
> proper?

I don't think we've decided anything yet.  I can think of a couple options.

* Value() doesn't indicate permission error just returns the same empty string, and people that care can check the role themself (it's light weight, and we can probably make it lighter).

* nsString has two sorts of empty strings one is "void" and should probably be though of similar to NULL being an empty C string, and not void but empty, so one option would be to use this to say if there just is no value or if we won't give out the value because of permissions.

* Value() could return a bool / enum to indicate failure.

I think I lean toward the first, but not really sure.

Comment 12

[Trevor Saunders (:tbsaunde)]

Comment on attachment 612988 [details] [diff] [review]
Patch (v2)

>-      return presShell->GetLinkLocation(DOMNode, aValue);
>+      presShell->GetLinkLocation(DOMNode, aValue);
>     }
>   }
> 
>-  return NS_OK;
> }

nit, no need for the blank line then.

>   // nsAccessible
>   virtual PRUint64 NativeState();
> 
>@@ -102,16 +101,17 @@ public:
>   virtual PRUint8 ActionCount();
>   virtual KeyBinding AccessKey() const;
> 
>   // HyperLinkAccessible
>   virtual already_AddRefed<nsIURI> AnchorURIAt(PRUint32 aAnchorIndex);
> 
> protected:
>   // nsAccessible
>+  virtual void Value(nsString& aValue);

um, public / protected / private mean have an effect on the members, so you may have just declared a new virtual function instead of over riding the base classes one, other other hand maybe C++ says that if it has the same name the protectedness is only determined by the base class.  While we could ask Luke how this works every time we ownder if this is actually correct I suspect that would be anoying to all involved, so please keep this in the public section.

>-  rv = GetMaximumValue(&maxValue);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  GetMaximumValue(&maxValue);

this isn't correct since now if converting the value to a double in GetMaximumValue() fails maxValue is some undefined value and you continue on which is bad.

> 
>   double curValue = 0;
>-  rv = GetCurrentValue(&curValue);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  GetCurrentValue(&curValue);

same

>+  aValue.Truncate();
>   if (NativeState() & states::PROTECTED)    // Don't return password text!
>-    return NS_ERROR_FAILURE;
>+    return;

nit, do this I think

aValue.Truncate();

// Don't return password text.
if (IsPassword())
  return;

(pending figuring out how we want to handle this)

> - (NSURL*)url
> {
>-  if (!mGeckoAccessible)
>+  if (IsDefunct() || !mGeckoAccessible)

wrong, this isn't an accessible, so IsDefunct() isn't a defined function.

>+nsXULTextFieldAccessible::Value(nsString& aValue)
> {
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;
>-
>-  PRUint64 state = NativeState();
>-
>-  if (state & states::PROTECTED)    // Don't return password text!
>-    return NS_ERROR_FAILURE;
>+  aValue.Truncate();
>+  if (NativeState() & states::PROTECTED)    // Don't return password text!
>+    return;

same style as nsHTMLTextFieldAccessible, also I tend to think we should use Role() instead of NativeRole() incase of aria things in future.

> 
>   nsCOMPtr<nsIDOMXULTextBoxElement> textBox(do_QueryInterface(mContent));
>   if (textBox) {
>-    return textBox->GetValue(aValue);
>+    textBox->GetValue(aValue);
>+    return;
>   }
>   nsCOMPtr<nsIDOMXULMenuListElement> menuList(do_QueryInterface(mContent));
>   if (menuList) {
>-    return menuList->GetLabel(aValue);
>+    menuList->GetLabel(aValue);
>   }
>-  return NS_ERROR_FAILURE;
>+
> }

nit, no blank line

>-    if (selectedItem)
>-      return selectedItem->GetLabel(_retval);
>+    if (selectedItem) {
>+      selectedItem->GetLabel(aValue);
>+    }

nit, no braces.

>   }
>-  return NS_ERROR_FAILURE;
>+
> }

nit, no blank line.

Comment 13

[Trevor Saunders (:tbsaunde)]

I think I need to see another version mostly because we haven't decided the issue of passwords yet.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #11)
> * Value() doesn't indicate permission error just returns the same empty
> string, and people that care can check the role themself (it's light weight,
> and we can probably make it lighter).

Ok, I think I'm fine with this option. MSAA part can take care of this. But please make sure our internal code is ok with this assumption.

Comment 15

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #14)
> (In reply to Trevor Saunders (:tbsaunde) from comment #11)
> > * Value() doesn't indicate permission error just returns the same empty
> > string, and people that care can check the role themself (it's light weight,
> > and we can probably make it lighter).
> 
> Ok, I think I'm fine with this option. MSAA part can take care of this. But
> please make sure our internal code is ok with this assumption.

I took a look before saying that, but looking again can't hurt.

Comment 16

[Mark Capella [:capella]]

Ok, you're not referencing files but I think the comment#12 change re:

  > - (NSURL*)url
  > {
  >-  if (!mGeckoAccessible)
  >+  if (IsDefunct() || !mGeckoAccessible)
  wrong, this isn't an accessible, so IsDefunct() isn't a defined function

Is something Alex asked to add in comment #4

  ::: accessible/src/mac/mozHTMLAccessible.mm
  @@ +116,5 @@
  >  
  >  - (NSURL*)url
  >  {
  >    if (!mGeckoAccessible)
  >      return nil;
  pls add IsDefunct check mozHTMLAccessible.mm

Maybe I added it incorrectly ... Also, I'm waiting on this since there's no clear direction yet:

  I tend to think we should use Role() instead of NativeRole() incase of aria things in future.

I'm also waiting all other final changes / nits until the decision on this is made for the the two places in nsXULTextFieldAccessible and nsHTMLTextFieldAccessible:

  nit, do this I think
  aValue.Truncate();
  // Don't return password text.
  if (IsPassword())
    return;


Let me know, mark

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to Mark Capella [:capella] from comment #16)
> Ok, you're not referencing files but I think the comment#12 change re:
> 
>   > - (NSURL*)url
>   > {
>   >-  if (!mGeckoAccessible)
>   >+  if (IsDefunct() || !mGeckoAccessible)
>   wrong, this isn't an accessible, so IsDefunct() isn't a defined function
> 
> Is something Alex asked to add in comment #4
> 
>   ::: accessible/src/mac/mozHTMLAccessible.mm
>   @@ +116,5 @@
>   >  
>   >  - (NSURL*)url
>   >  {
>   >    if (!mGeckoAccessible)
>   >      return nil;
>   pls add IsDefunct check mozHTMLAccessible.mm
> 
> Maybe I added it incorrectly ... Also, I'm waiting on this since there's no

ok, you could

if (!mGeckAccessible || mGeckoAccessible->IsDefunct())
  return nil;

However it seems like dealing with defunct accessibles on mac is just a mess currently, and I don't off hand see a reason we can't make mGeckoAccessible be null when it becomes defunct so I'm not sure adding this check would do any good, but I guess if Alex would prefer it why not it should be pretty cheap now.

> clear direction yet:
> 
>   I tend to think we should use Role() instead of NativeRole() incase of
> aria things in future.
> 
> I'm also waiting all other final changes / nits until the decision on this
> is made for the the two places in nsXULTextFieldAccessible and
> nsHTMLTextFieldAccessible:
> 
>   nit, do this I think
>   aValue.Truncate();
>   // Don't return password text.
>   if (IsPassword())
>     return;
> 

I'll try and double chekc we never depend on the handling of this internally soon.

Comment 18

[Mark Capella [:capella]]

Attachment: Patch (v3)

All nits addressed up to this point / comment / patch ... except for any issues not yet discovered: "handling of this internally".

Comment 19

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #17)

> if (!mGeckAccessible || mGeckoAccessible->IsDefunct())
>   return nil;
> 
> However it seems like dealing with defunct accessibles on mac is just a mess
> currently, and I don't off hand see a reason we can't make mGeckoAccessible
> be null when it becomes defunct so I'm not sure adding this check would do
> any good, but I guess if Alex would prefer it why not it should be pretty
> cheap now.

We should null out mGeckoAccessible when accessible shutdowns but it seems we don't do that currently.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #16)
> Ok, you're not referencing files but I think the comment#12 change re:
> 
>   > - (NSURL*)url
>   > {
>   >-  if (!mGeckoAccessible)
>   >+  if (IsDefunct() || !mGeckoAccessible)
>   wrong, this isn't an accessible, so IsDefunct() isn't a defined function
> 
> Is something Alex asked to add in comment #4

sorry I wasn't clear on this. I meant mGeckoAccessible->IsDefunct

Comment 21

[Mark Capella [:capella]]

Ok Alex :) I pushed to try to see how that goes ...

Comment 22

[alexander :surkov (:asurkov)]

Comment on attachment 613157 [details] [diff] [review]
Patch (v3)

Review of attachment 613157 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/nsFormControlAccessible.cpp
@@ +129,5 @@
>  
>    nsAutoString value;
>    value.AppendFloat(percentValue); // AppendFloat isn't available on nsAString
>    value.AppendLiteral("%");
>    aValue = value;

you should use aValue as I mentioned before

::: accessible/src/mac/mozHTMLAccessible.mm
@@ +124,1 @@
>    NS_ENSURE_SUCCESS(rv, nil);

it returns void

::: accessible/src/msaa/nsAccessibleWrap.cpp
@@ +285,5 @@
>    if (!xpAccessible || xpAccessible->IsDefunct())
>      return E_FAIL;
>  
>    nsAutoString value;
> +  xpAccessible->Value(value);

you should have a role check to return accessdenied value

::: accessible/src/xforms/nsXFormsAccessible.cpp
@@ +144,5 @@
>      AppendChild(accessible);
>    }
>  }
>  
>  // nsIAccessible

remove the comment

::: accessible/src/xforms/nsXFormsFormControlsAccessible.cpp
@@ +299,1 @@
>  {

aValue.Truncate()

::: accessible/src/xul/nsXULFormControlAccessible.cpp
@@ +733,4 @@
>  {
> +  aValue.Truncate();
> +  if (NativeState() & states::PROTECTED)    // Don't return password text!
> +    return;

it's cheaper to use NativeRole

::: accessible/src/xul/nsXULListboxAccessible.cpp
@@ +215,2 @@
>  {
> +  aValue.Truncate();

new line pls

::: accessible/src/xul/nsXULTabAccessible.cpp
@@ +192,1 @@
>  {

aValue.Truncate(); pls

::: accessible/tests/mochitest/testTextboxes.js
@@ -6,5 @@
>        value = aAcc.value;
>        do_throw("We do not want a value on " + aID + "!");
>      } catch(e) {
> -      is(e.result, Components.results.NS_ERROR_FAILURE,
> -         "Wrong return value for getValue on " + aID + "!");

you just removed the test

Comment 23

[alexander :surkov (:asurkov)]

Attachment: patch (v4)

I thought the patch had r+ already so I wanted to address my comments before landing since there's nothing serious but then I noticed no one patch has r+, so pushing my version of the patch.

Comment 24

[Trevor Saunders (:tbsaunde)]

Comment on attachment 613222 [details] [diff] [review]
patch (v4)

void
>+ProgressMeterAccessible<Max>::Value(nsString& aValue)
> {
>-  nsresult rv = nsFormControlAccessible::GetValue(aValue);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>+  nsFormControlAccessible::Value(aValue);
>   if (!aValue.IsEmpty())
>-    return NS_OK;
>+    return;
> 
>   double maxValue = 0;
>-  rv = GetMaximumValue(&maxValue);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  GetMaximumValue(&maxValue);
> 
>   double curValue = 0;
>-  rv = GetCurrentValue(&curValue);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  GetCurrentValue(&curValue);

you need to check the return value of those functions for errors.

>+nsXULColorPickerTileAccessible::Value(nsString& aValue)
> {
>   aValue.Truncate();
> 
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;
>+  mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::color, aValue);
> 
>-  mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::color, aValue);
>-  return NS_OK;
> }

unneeded blank line.

+nsXULComboboxAccessible::Value(nsString& aValue)
>+{
>+  aValue.Truncate();
>+
>+  // The value is the option or text shown entered in the combobox.
>+  nsCOMPtr<nsIDOMXULMenuListElement> menuList(do_QueryInterface(mContent));
>+  if (menuList)
>+    menuList->GetLabel(aValue);
>+
>+}

extra blank line.

>-/** no value */
>-NS_IMETHODIMP nsXULTabsAccessible::GetValue(nsAString& _retval)
>+void
>+nsXULTabsAccessible::Value(nsString& aValue)
> {
>-  return NS_OK;
> }

should probably truncate the string.

r=me with those fixed

Comment 25

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/807a41c71132

Comment 26

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 613157
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=d39686ee8be3
Try run started, revision d39686ee8be3. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=d39686ee8be3

Comment 27

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/807a41c71132