740747 - dexpcom nsAccessible::GetName

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

identical to bug 652378, just follow the idea of the patch.

Comment 1

[alexander :surkov (:asurkov)]

one note:
GetName used to return NS_OK_NAME_FROM_TOOLTIP and NS_OK_EMPTY_NAME besides NS_OK and failures.

So you need to introduce enum
enum ENameValueFlag {
 eName,
 eEmptyName,
 eNameFromTooltip
}

and then define nsAccessible::Name as:
ENameValueFlag Name(nsString& aName);

Comment 2

[Mark Capella [:capella]]

Attachment: Patch (v1)

Here's the first / rough draft to tear up. There's a failing test in name/test_general.html ... I've been poking at if for a couple hours ... must be too tired to see it ... gonna post an attachment next showing the errors then sleep on it ...

Comment 3

[Mark Capella [:capella]]

Attachment: Error

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 614844 [details] [diff] [review]
Patch (v1)

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

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +689,5 @@
>      }
>  
>      /* nsIAccessible is responsible for the non-NULL name */
>      nsAutoString uniName;
> +    nsresult rv = accWrap->Name(uniName);

it doesn't return nsresult anymore

@@ +1054,5 @@
>  
>      case nsIAccessibleEvent::EVENT_NAME_CHANGE:
>        {
>          nsString newName;
> +        accessible->Name(newName);

nsAutoString, right?

::: accessible/src/base/nsAccessible.cpp
@@ +208,5 @@
>        printf(" Con: %s@%p",
>               NS_ConvertUTF16toUTF8(content->NodeInfo()->QualifiedName()).get(),
>               (void *)content.get());
>        nsAutoString buf;
> +      if (NS_SUCCEEDED(Name(buf))) {

it doesn't return nsresult

@@ +293,4 @@
>    GetARIAName(aName);
> +  if (!aName.IsEmpty()) {
> +    return eName;
> +  }

don't wrap it by braces please

@@ +313,5 @@
>      tooltipAttr = nsGkAtoms::title;
>    else if (mContent->IsXUL())
>      tooltipAttr = nsGkAtoms::tooltiptext;
>    else
> +    return eEmptyName;

it's not empty name but absent name.

we have two kinds of no name state
1) empty name when the author intentionally provided empty name like atl=""
2) absent name when the author didn't provide name at all

in 2nd case AT can try to repair name, in 1nd case empty name is on demand and AT shouldn't repair it

@@ +319,5 @@
>    // XXX: if CompressWhiteSpace worked on nsAString we could avoid a copy.
>    nsAutoString name;
>    if (mContent->GetAttr(kNameSpaceID_None, tooltipAttr, name)) {
>      name.CompressWhitespace();
>      aName = name;

you should use aName to avoid copy

@@ +320,5 @@
>    nsAutoString name;
>    if (mContent->GetAttr(kNameSpaceID_None, tooltipAttr, name)) {
>      name.CompressWhitespace();
>      aName = name;
> +    return eNameFromTooltip;

if you decide to have eNoName then make sure to check it. In either case name.IsEmpty() and NS_OK_NAME_FROM_TOOLTIP result aren't different in logic from name.IsEmpty() and NS_OK.

@@ +326,5 @@
>  
>    if (rv != NS_OK_EMPTY_NAME)
>      aName.SetIsVoid(true);
>  
> +  return eName;

it can be empty name

@@ +379,1 @@
>            if (name.IsEmpty() || aDescription == name)

yeap, having eNoName you can do
nsAutoString name;
if (Name(name) == eNoName || aDescription == name)

::: accessible/src/base/nsAccessible.h
@@ +146,5 @@
> +  enum ENameValueFlag {
> +   eName,
> +   eEmptyName,
> +   eNameFromTooltip,
> +   eNameFailed

it'd be nice to have short comment for each of them

@@ +147,5 @@
> +   eName,
> +   eEmptyName,
> +   eNameFromTooltip,
> +   eNameFailed
> +  };

perhaps we need eNameFailed (though I didn't check code paths)

we can consider to have eNoName. having it we can do

if (BaseClass::Name(aName) != eNoName)
  return eNoName;

instead

BaseClass::Name(aName);
if (aName.IsEmpty())
  return;

I'd like to hear Trevor on this.

Also I'm not sure I like eName, consider alternatives: eNameOk, eNormalName, eNotEmptyName (the last works if you don't introduce eNoName).

Comment 5

[Trevor Saunders (:tbsaunde)]

> we can consider to have eNoName. having it we can do
> 
> if (BaseClass::Name(aName) != eNoName)
>   return eNoName;

did you really mean that? it seems kind of crazy to return eNoName immediately if the base class returned anything other than eNoName

> instead
> 
> BaseClass::Name(aName);
> if (aName.IsEmpty())
>   return;

how would that work with the returning an enum part?

Comment 6

[alexander :surkov (:asurkov)]

oh yes the common patter is different:

nsAutoString name;
BaseClass::GetName(name);
if (!name.IsEmpty())
  return NS_OK;

having eNoName we don't get much benefits in this case:
nsAutoString name;
ENameValueFlag nameFlag = BaseClass::Name(name);
if (nameFlag != eNoName)
  return nameFlag;

But we can have benefits in nsAccessible::Description for example:

nsAutoString name;
GetName(name);
if (name.IsEmpty() || aDescription == name)

and 
nsAutoString name;
if (Name(name) == eNoName || aDescription == name)

Comment 7

[Mark Capella [:capella]]

Attachment: Patch (v2)

Problem with patch failing "name" mochitests corrected ... nits addressed ... rebuilt and tested ...

Comment 8

[Mozilla RelEng Bot]

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

Comment 9

[Mozilla RelEng Bot]

Try run for fc63bc2ae088 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=fc63bc2ae088
Results (out of 2 total builds):
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fc63bc2ae088

Comment 10

[Mark Capella [:capella]]

Attachment: Patch (v3)

last patch burned on Linux ... bug corrected

Comment 11

[Mozilla RelEng Bot]

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

Comment 12

[Mozilla RelEng Bot]

Try run for 8cdc94191df3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8cdc94191df3
Results (out of 2 total builds):
    success: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8cdc94191df3

Comment 13

[alexander :surkov (:asurkov)]

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

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

::: accessible/src/base/nsAccessible.h
@@ +144,5 @@
> +   * Flags used to describe the name of this accessible.
> +   */
> +  enum ENameValueFlag {
> +   eNameOK, // Name either present or empty
> +   eNameFailed, // Name neither present or empty

is there any guidance when you return eNameOk or eNameFailed for empty name?

btw, later I think GetNameInternal should return ENameValueFlag too, and then you need to have a flag empty name case (eEmptyName).

btw, why you don't like to have
eNameOK // Name is present
eEmptyName // Name is empty (on the author's purpose)
eNoName // No name
eNameFromTooltip // Name is generated from tooltip

Comment 14

[Mark Capella [:capella]]

I made the changes such that eNameFailed is returned where previous code could have set a return value due to not finding a "Name" in unexpected ways ... such as in the routine nsApplicationAccessible::Name ... it used to return an error value when bundle service was not present so here I returned eNameFailed.

I suspect in reality no one is checking the return value, but I tried to preserve case. The only place where the return value in our code is checked and required is for eNameFromTooltip. Otherwise, eNameOK can probably, almost always, just be used.

GetNameInternal could be modified also. Do you want to include those changes here or close this with a followup "good first bug"?

The wording on the comments is a nit, We can change them if you prefer for clarity.

Comment 15

[alexander :surkov (:asurkov)]

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

> I suspect in reality no one is checking the return value, but I tried to
> preserve case. The only place where the return value in our code is checked
> and required is for eNameFromTooltip. Otherwise, eNameOK can probably,
> almost always, just be used.

there's NS_OK_EMPTY_NAME too. But you need it for GetNameInternal part.

> GetNameInternal could be modified also. Do you want to include those changes
> here or close this with a followup "good first bug"?

as followup.

Comment 16

[alexander :surkov (:asurkov)]

So what constants do you want to have?

Comment 17

[Mark Capella [:capella]]

I don't see a need to change them from what we have here, if that's what you mean. We could do all sorts of planning for the future, but why not let that go until the need arises?

Comment 18

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #17)
> I don't see a need to change them from what we have here, if that's what you
> mean.

well, I think we can map failures into eNameOk.

So Name(aName) returns
1) eNameOk when name is not from tooltip
2) eNameFromTooltip when name is from tooltip
3) aName.IsEmpty() when name is empty
4) aName.IsVoid() when no name

I suggested to map 3 and 4 to flags as well. But we are not required to that now. If you put this into Name() comment then it's ok. 

> We could do all sorts of planning for the future, but why not let that
> go until the need arises?

we can do that

Comment 19

[Mark Capella [:capella]]

Attachment: Patch (v4)

Changed, rebuilt and tested locally ... all looks good :)

Comment 20

[alexander :surkov (:asurkov)]

Comment on attachment 615266 [details] [diff] [review]
Patch (v4)

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

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +697,5 @@
>      NS_ConvertUTF8toUTF16 objName(aAtkObj->name);
>      if (!uniName.Equals(objName)) {
>          atk_object_set_name(aAtkObj,
>                              NS_ConvertUTF16toUTF8(uniName).get());
>      }

If I understand right then we used to return "" for empty string, now you return null

::: accessible/src/base/nsAccDocManager.h
@@ +426,5 @@
>  
>  #define NS_LOG_ACCDOCLOAD_REQUEST(aRequest)                                    \
>    if (aRequest) {                                                              \
>      nsCAutoString name;                                                        \
> +    aRequest->Name(name);                                                      \

I don't think aRequest is accessible object

::: accessible/src/base/nsAccessible.cpp
@@ +213,1 @@
>          printf(" Name:[%s]", NS_ConvertUTF16toUTF8(buf).get());

no, the name from tooltip was still ok

@@ +284,5 @@
> +  return NS_OK;
> +}
> +
> +nsAccessible::ENameValueFlag
> +nsAccessible::Name(nsAString& aName)

use nsString& please

@@ +323,1 @@
>    }

and then you can get rid this XXX comment

::: accessible/src/base/nsAccessible.h
@@ +144,5 @@
> +   * Flags used to describe the name of this accessible.
> +   */
> +  enum ENameValueFlag {
> +   eNameOK, // Name either present or aName.IsEmpty() or aName.IsVoid()
> +   eNameFromTooltip // Name found in Tooltip

tooltip was used as a name

::: accessible/src/base/nsRootAccessible.cpp
@@ +112,5 @@
>  
>  ////////////////////////////////////////////////////////////////////////////////
>  // nsIAccessible
>  
>  /* readonly attribute AString name; */

remove this comment

@@ +119,5 @@
>  {
>    aName.Truncate();
>  
> +  if (!mDocument)
> +    return eNameOK;

you don't need this check since you should operate on non-defunct accessible

::: accessible/src/base/nsTextEquivUtils.cpp
@@ +232,4 @@
>    bool isEmptyTextEquiv = true;
>  
>    // If the name is from tooltip then append it to result string in the end
>    // (see h. step of name computation guide).

move nsAutoString text here

::: accessible/src/msaa/nsAccessibleWrap.cpp
@@ +267,5 @@
>      return CO_E_OBJNOTCONNECTED;
>  
>    nsAutoString name;
> +  if (xpAccessible->Name(name) != eNameOK)
> +    return S_FALSE;

so name from tooltip results in S_FALSE, S_FALSE should be when there's no name (except empty name case)

::: accessible/src/msaa/nsXULMenuAccessibleWrap.cpp
@@ +53,5 @@
>  {
>    // XXX This should be done in get_accName() so that nsIAccessible::GetName()]
>    // provides the same results on all platforms
> +  if (nsXULMenuitemAccessible::Name(aName) != eNameOK)
> +    return eNameOK;

perhaps not a real case but the same point

::: accessible/src/xul/XULFormControlAccessible.cpp
@@ +465,5 @@
>    nsAccessible* label =
>      RelationByType(nsIAccessibleRelation::RELATION_LABELLED_BY).Next();
>    if (label)
> +    if (label->Name(aName) != nsAccessible::eNameOK)
> +      return NS_ERROR_FAILURE;

same issue

Comment 21

[Mark Capella [:capella]]

Attachment: Patch (v5)

Revised, try - try - again ...

Comment 22

[alexander :surkov (:asurkov)]

Comment on attachment 615284 [details] [diff] [review]
Patch (v5)

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

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +691,5 @@
>      /* nsIAccessible is responsible for the non-NULL name */
>      nsAutoString uniName;
> +    accWrap->Name(uniName);
> +    if (uniName.IsEmpty())
> +      return nsnull;

If I get right then it's revised patch  but this issue is not addressed

Comment 23

[Mark Capella [:capella]]

Attachment: Patch (v6)

Ah ok ... I forgot to QREFRESH before posting the last tested patch ... here it is ...

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 615318 [details] [diff] [review]
Patch (v6)

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

r=me with comments fixed but please get another review from Trevor to be on safe side

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +688,1 @@
>          return nsnull;

use two spaces here and below, feel free to indent whole logical blocks

@@ +689,4 @@
>  
>      /* nsIAccessible is responsible for the non-NULL name */
>      nsAutoString uniName;
> +    accWrap->Name(uniName);

this comment confuses me, it goes from initial times. Either we should follow the comment if it makes sense or remove it (please check it with Ginn or Trevor)

::: accessible/src/base/nsAccessible.cpp
@@ +208,5 @@
>        printf(" Con: %s@%p",
>               NS_ConvertUTF16toUTF8(content->NodeInfo()->QualifiedName()).get(),
>               (void *)content.get());
>        nsAutoString buf;
> +      if (NS_SUCCEEDED(GetName(buf)))

just Name(buf), no if no GetName

@@ +311,5 @@
>      tooltipAttr = nsGkAtoms::title;
>    else if (mContent->IsXUL())
>      tooltipAttr = nsGkAtoms::tooltiptext;
>    else
> +    return eNameOK;

not your fault but in case of not HTML and not XUL markup you should do SetIsVoid() for rv != NS_OK_EMPTY_NAME

::: accessible/src/base/nsAccessible.h
@@ +140,5 @@
>     */
>    virtual void Value(nsString& aValue);
>  
>    /**
> +   * Flags used to describe the name of this accessible.

maybe name type? 'of this accessible' sounds excess
perhaps simple 'Name type flags'?

::: accessible/src/base/nsDocAccessible.cpp
@@ -236,2 @@
>  }
> -

you don't need to remove that empty line, right?

::: accessible/src/base/nsDocAccessible.h
@@ +102,5 @@
>    // nsIDocumentObserver
>    NS_DECL_NSIDOCUMENTOBSERVER
>  
>    // nsAccessNode
> +  virtual ENameValueFlag Name(nsString& aName);

keep under // nsAccessible section please

::: accessible/src/base/nsRootAccessible.cpp
@@ +110,5 @@
>  {
>  }
>  
>  ////////////////////////////////////////////////////////////////////////////////
>  // nsIAccessible

it's time to replace it on nsAccessible

::: accessible/src/base/nsTextEquivUtils.cpp
@@ +41,5 @@
>  
>  #include "Accessible-inl.h"
>  #include "AccIterator.h"
>  #include "nsAccessibilityService.h"
> +#include "nsAccessible.h"

you don't need it since Accessible-inl is included already

::: accessible/src/msaa/nsAccessibleWrap.cpp
@@ +267,5 @@
>      return CO_E_OBJNOTCONNECTED;
>  
>    nsAutoString name;
> +  if (xpAccessible->Name(name) != eNameOK)
> +    return S_OK;

if name from tooltip then you don't return a name

::: accessible/src/msaa/nsXULMenuAccessibleWrap.cpp
@@ +53,5 @@
>  {
>    // XXX This should be done in get_accName() so that nsIAccessible::GetName()]
>    // provides the same results on all platforms
> +  if (nsXULMenuitemAccessible::Name(aName) != eNameOK)
> +    return eNameOK;

same, iirc I commented that already for previous patch

Comment 25

[Mark Capella [:capella]]

   I understand all but the last two items: Are you just saying that I should add "aName.Truncate();" before returning?

::: accessible/src/msaa/nsAccessibleWrap.cpp
@@ +267,5 @@
>      return CO_E_OBJNOTCONNECTED;
>  
>    nsAutoString name;
> +  if (xpAccessible->Name(name) != eNameOK)
> +    return S_OK;
if name from tooltip then you don't return a name

::: accessible/src/msaa/nsXULMenuAccessibleWrap.cpp
@@ +53,5 @@
>  {
>    // XXX This should be done in get_accName() so that nsIAccessible::GetName()]
>    // provides the same results on all platforms
> +  if (nsXULMenuitemAccessible::Name(aName) != eNameOK)
> +    return eNameOK;
same, iirc I commented that already for previous patch

Comment 26

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #25)
>    I understand all but the last two items: Are you just saying that I
> should add "aName.Truncate();" before returning?

no, I meant name from tooltip is a normal name, you just should return it

> ::: accessible/src/msaa/nsAccessibleWrap.cpp
> @@ +267,5 @@
> >      return CO_E_OBJNOTCONNECTED;
> >  
> >    nsAutoString name;
> > +  if (xpAccessible->Name(name) != eNameOK)
> > +    return S_OK;
> if name from tooltip then you don't return a name
> 
> ::: accessible/src/msaa/nsXULMenuAccessibleWrap.cpp
> @@ +53,5 @@
> >  {
> >    // XXX This should be done in get_accName() so that nsIAccessible::GetName()]
> >    // provides the same results on all platforms
> > +  if (nsXULMenuitemAccessible::Name(aName) != eNameOK)
> > +    return eNameOK;
> same, iirc I commented that already for previous patch

Comment 27

[Mark Capella [:capella]]

Attachment: Patch (v7)

Alex, thanks for the review+. I've gone through and addressed the last issues, and as you asked am now asking Trevor for final review.

WRT the indents in atk/nsAccessibleWrap.cpp, the entire program is using 4 spaces, so instead of re-indenting the whole program for a one line change, I left it alone for consistancy.

WRT the suggestion to remove the comment in atk/nsAccessibleWrap.cpp routine (getNameCB)... it's followed by another routine (getDescriptionCB) with a similar comment for the description field, so I've also left for consistancy.

Everything else I agreed with and made the requested changes. Please feel free to change any minor nits left if it looks like we can land. I really have no opinion as to how to word particular comments and such. As long as the code works, is reasonably clear, and passes all tests I'm happy :P

Comment 28

[Trevor Saunders (:tbsaunde)]

> WRT the indents in atk/nsAccessibleWrap.cpp, the entire program is using 4
> spaces, so instead of re-indenting the whole program for a one line change,
> I left it alone for consistancy.

we're reindenting all that code as we go along so please change that method since you touch half the lines in it anyway.

> WRT the suggestion to remove the comment in atk/nsAccessibleWrap.cpp routine
> (getNameCB)... it's followed by another routine (getDescriptionCB) with a
> similar comment for the description field, so I've also left for consistancy.

don't keep things that are crazy just to be consistant.

Comment 29

[Mark Capella [:capella]]

Attachment: Patch (v8)

Cleaned-up that method, rebuild and tested locally.

Comment 30

[Trevor Saunders (:tbsaunde)]

Comment on attachment 616032 [details] [diff] [review]
Patch (v8)

>+nsAccessible::ENameValueFlag
>+nsApplicationAccessibleWrap::Name(nsString& aName)

could we please loose this nsAccessible:: stuff?  I suspect you can just delete them and it'll still be fine, but if not you could just move the enum to the a11y namespace.

>+++ b/accessible/src/base/nsAccessible.cpp
>@@ -204,19 +204,18 @@ nsAccessible::nsAccessible(nsIContent* a
>             (void*)static_cast<nsIAccessible*>(this), (void*)aNode,
>             (void*)shell.get());
>     nsCOMPtr<nsIContent> content = do_QueryInterface(aNode);
>     if (content) {
>       printf(" Con: %s@%p",
>              NS_ConvertUTF16toUTF8(content->NodeInfo()->QualifiedName()).get(),
>              (void *)content.get());
>       nsAutoString buf;
>-      if (NS_SUCCEEDED(GetName(buf))) {
>+      if (NS_SUCCEEDED(Name(buf)))

I'm pretty sure this ifdef has been broken for a long time, but that change isn't correct.

>   /**
>+   * Name type flags.

nit, they aren't really flags

>+   */
>+  enum ENameValueFlag {
>+   eNameOK, // Name either present or aName.IsEmpty() or aName.IsVoid()

comment isn't really useful, all it tells me is the name string is in a sain state for a nsString after the method.

also please document what the difference between void and just empty means.

Comment 31

[Trevor Saunders (:tbsaunde)]

Comment on attachment 616032 [details] [diff] [review]
Patch (v8)

> nsAccessible::GetName(nsAString& aName)
> {
>   aName.Truncate();
> 
>   if (IsDefunct())
>     return NS_ERROR_FAILURE;
> 
>+  nsAutoString name;
>+  Name(name);
>+  aName.Assign(name);
>+
>+  return NS_OK;

this isn't technically equal ince you never return NS_OK_NAME_FROM_TOOLTIP, but I think that's probably fine since nobody can use that any way afaik.  However you should get rid of the macro declaring that nsresult value in nsAccessible.h

> 
>   if (mContent->IsHTML())
>     tooltipAttr = nsGkAtoms::title;
>   else if (mContent->IsXUL())
>     tooltipAttr = nsGkAtoms::tooltiptext;
>-  else
>-    return NS_OK;
>-
>-  // XXX: if CompressWhiteSpace worked on nsAString we could avoid a copy.
>-  nsAutoString name;
>-  if (mContent->GetAttr(kNameSpaceID_None, tooltipAttr, name)) {
>-    name.CompressWhitespace();
>-    aName = name;
>-    return NS_OK_NAME_FROM_TOOLTIP;
>+  else {
>+    aName.SetIsVoid(true);
>+    return eNameOK;
>+  }
>+
>+  if (mContent->GetAttr(kNameSpaceID_None, tooltipAttr, aName)) {
>+    aName.CompressWhitespace();
>+    return eNameFromTooltip;
>   }
> 
>   if (rv != NS_OK_EMPTY_NAME)
>     aName.SetIsVoid(true);

I think this logic would be a little clearer if you don't use the toolTipAttr variable and just do

if (IsHTML() && GetAttr(nsGkAtoms::tooltip))
  blah
else if (IsXUL() && GetAttr(nsGkAtoms::bar))
  baz
else
  ....

Comment 32

[alexander :surkov (:asurkov)]

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

> >+nsAccessible::ENameValueFlag
> >+nsApplicationAccessibleWrap::Name(nsString& aName)
> 
> could we please loose this nsAccessible:: stuff?  I suspect you can just
> delete them and it'll still be fine, but if not you could just move the enum
> to the a11y namespace.

simple deletion shouldn't work but keeping it in a11y namespace should be fine

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

> this isn't technically equal ince you never return NS_OK_NAME_FROM_TOOLTIP,
> but I think that's probably fine since nobody can use that any way afaik. 

right

> I think this logic would be a little clearer if you don't use the
> toolTipAttr variable and just do
> 
> if (IsHTML() && GetAttr(nsGkAtoms::tooltip))
>   blah
> else if (IsXUL() && GetAttr(nsGkAtoms::bar))
>   baz
> else
>   ....

or

if (IsHTML()) {
  if (GetAttr(nsGkAtoms::tooltip, aName)) {
    aName.Compress();
    return eNameFromTooltipl
  }
}
else if (IsXUL())
}

to not hit IsXUL and default part in case of HTML and no tooltip attribute

Comment 33

[Mark Capella [:capella]]

Attachment: Patch (v9)

Ok, I've gotten back to finishing this one.

I've re-based the patch with the latest changes (including de-ns-ify nsApplicationAccessible), moved the enum into the a11y namespace, and re-worked the IsHTML and IsXUL logic section of the main Name() routine.

Everything builds and tests out ok locally once again.

Comment 34

[alexander :surkov (:asurkov)]

Comment on attachment 618867 [details] [diff] [review]
Patch (v9)

let's get back feedback from Trevor (since he had concerns)

Comment 35

[Trevor Saunders (:tbsaunde)]

Comment on attachment 618867 [details] [diff] [review]
Patch (v9)

>diff --git a/accessible/src/atk/ApplicationAccessibleWrap.h b/accessible/src/atk/ApplicationAccessibleWrap.h
>--- a/accessible/src/atk/ApplicationAccessibleWrap.h
>+++ b/accessible/src/atk/ApplicationAccessibleWrap.h
>@@ -38,32 +38,33 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #ifndef MOZILLA_A11Y_APPLICATION_ACCESSIBLE_WRAP_H__
> #define MOZILLA_A11Y_APPLICATION_ACCESSIBLE_WRAP_H__
> 
> #include "ApplicationAccessible.h"
> 
>+using namespace mozilla::a11y;

NO! putting using namespace declartions in headers especially exported ones is almost always very wrong, since anyone who pull this header in directly or indirectly will now be using that namespace.

>--- a/accessible/src/base/nsAccessible.cpp
>+++ b/accessible/src/base/nsAccessible.cpp
>@@ -204,19 +204,18 @@ nsAccessible::nsAccessible(nsIContent* a
>             (void*)static_cast<nsIAccessible*>(this), (void*)aNode,
>             (void*)shell.get());
>     nsCOMPtr<nsIContent> content = do_QueryInterface(aNode);
>     if (content) {
>       printf(" Con: %s@%p",
>              NS_ConvertUTF16toUTF8(content->NodeInfo()->QualifiedName()).get(),
>              (void *)content.get());
>       nsAutoString buf;
>-      if (NS_SUCCEEDED(GetName(buf))) {
>+      if (NS_SUCCEEDED(Name(buf)))
>         printf(" Name:[%s]", NS_ConvertUTF16toUTF8(buf).get());
>-       }

since I'm pretty sure this ifdef hasn't worked for a while anyway I don't really care, but NS_FAILED() on a enum is wrong, and won't compile.

>   // In the end get the name from tooltip.
>   nsIAtom *tooltipAttr = nsnull;

nolonger used right?

> 
>-  if (mContent->IsHTML())
>-    tooltipAttr = nsGkAtoms::title;
>-  else if (mContent->IsXUL())
>-    tooltipAttr = nsGkAtoms::tooltiptext;
>+  if (mContent->IsHTML()) {
>+    if (mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::title, aName)) {
>+      aName.CompressWhitespace();
>+      return eNameFromTooltip;
>+    }
>+  }
>+  else if (mContent->IsXUL()) {

nit, else if on same line as } please, so
} else if (blah) {

>+    if (mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::tooltiptext, aName)) {
>+      aName.CompressWhitespace();
>+      return eNameFromTooltip;
>+    }
>+  }
>   else
>-    return NS_OK;
>-
>-  // XXX: if CompressWhiteSpace worked on nsAString we could avoid a copy.
>-  nsAutoString name;
>-  if (mContent->GetAttr(kNameSpaceID_None, tooltipAttr, name)) {
>-    name.CompressWhitespace();
>-    aName = name;
>-    return NS_OK_NAME_FROM_TOOLTIP;
>-  }
>+    return eNameOK;

nit, not bracing only one block is kind of funny, nd put else on same line as }.

>+/**
>+ * Name type flags.
>+ */
>+enum ENameValueFlag {
>+ eNameOK, // Name either present or aName.IsEmpty() or aName.IsVoid()

still doesn't describe what each of those states mean.

>+  xpAccessible->Name(name);
>+  if (!name.IsEmpty())
>+    return S_OK;

this is wrong, when Name() returns a name with text you'll now return the empty string and S_OK.

btw what about mac?

Comment 36

[Trevor Saunders (:tbsaunde)]

(In reply to Trevor Saunders (:tbsaunde) from comment #35)
> Comment on attachment 618867 [details] [diff] [review]
> Patch (v9)
> 
> >diff --git a/accessible/src/atk/ApplicationAccessibleWrap.h b/accessible/src/atk/ApplicationAccessibleWrap.h
> >--- a/accessible/src/atk/ApplicationAccessibleWrap.h
> >+++ b/accessible/src/atk/ApplicationAccessibleWrap.h
> >@@ -38,32 +38,33 @@
> >  *
> >  * ***** END LICENSE BLOCK ***** */
> > 
> > #ifndef MOZILLA_A11Y_APPLICATION_ACCESSIBLE_WRAP_H__
> > #define MOZILLA_A11Y_APPLICATION_ACCESSIBLE_WRAP_H__
> > 
> > #include "ApplicationAccessible.h"
> > 
> >+using namespace mozilla::a11y;
> 
> NO! putting using namespace declartions in headers especially exported ones
> is almost always very wrong, since anyone who pull this header in directly
> or indirectly will now be using that namespace.

virtual mozilla::a11y::eNameValue Name(nsString& aName); isn't terribly nice, ut it will get better when we finish namespaceing class and it can just be eNameValue

Comment 37

[Mark Capella [:capella]]

Attachment: Patch (v10)

Alex,

   I've changed the patch again. This version corrects the namespace in header file concern, removes the unused var that I forgot to pull previously, and fixes a technical error code return of S_OK vs. S_FALSE.

   I'd love to have credit for completing this, but if there is substantial changes yet desired, I'll have to ask you to re-assign this one, as I'm not sure how much time I have available for it in the immediate future.

Comment 38

[Trevor Saunders (:tbsaunde)]

these still aren't addressed, and really shouldn't take long.

> >--- a/accessible/src/base/nsAccessible.cpp
> >+++ b/accessible/src/base/nsAccessible.cpp
> >@@ -204,19 +204,18 @@ nsAccessible::nsAccessible(nsIContent* a
> >             (void*)static_cast<nsIAccessible*>(this), (void*)aNode,
> >             (void*)shell.get());
> >     nsCOMPtr<nsIContent> content = do_QueryInterface(aNode);
> >     if (content) {
> >       printf(" Con: %s@%p",
> >              NS_ConvertUTF16toUTF8(content->NodeInfo()->QualifiedName()).get(),
> >              (void *)content.get());
> >       nsAutoString buf;
> >-      if (NS_SUCCEEDED(GetName(buf))) {
> >+      if (NS_SUCCEEDED(Name(buf)))
> >         printf(" Name:[%s]", NS_ConvertUTF16toUTF8(buf).get());
> >-       }
> 
> since I'm pretty sure this ifdef hasn't worked for a while anyway I don't
> really care, but NS_FAILED() on a enum is wrong, and won't compile.
> 
> >   // In the end get the name from tooltip.
> >   nsIAtom *tooltipAttr = nsnull;
> 
> nolonger used right?
> 
> > 
> >-  if (mContent->IsHTML())
> >-    tooltipAttr = nsGkAtoms::title;
> >-  else if (mContent->IsXUL())
> >-    tooltipAttr = nsGkAtoms::tooltiptext;
> >+  if (mContent->IsHTML()) {
> >+    if (mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::title, aName)) {
> >+      aName.CompressWhitespace();
> >+      return eNameFromTooltip;
> >+    }
> >+  }
> >+  else if (mContent->IsXUL()) {
> 
> nit, else if on same line as } please, so
> } else if (blah) {
> 
> >+    if (mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::tooltiptext, aName)) {
> >+      aName.CompressWhitespace();
> >+      return eNameFromTooltip;
> >+    }
> >+  }
> >   else
> >-    return NS_OK;
> >-
> >-  // XXX: if CompressWhiteSpace worked on nsAString we could avoid a copy.
> >-  nsAutoString name;
> >-  if (mContent->GetAttr(kNameSpaceID_None, tooltipAttr, name)) {
> >-    name.CompressWhitespace();
> >-    aName = name;
> >-    return NS_OK_NAME_FROM_TOOLTIP;
> >-  }
> >+    return eNameOK;
> 
> nit, not bracing only one block is kind of funny, nd put else on same line
> as }.
> 
> >+/**
> >+ * Name type flags.
> >+ */
> >+enum ENameValueFlag {
> >+ eNameOK, // Name either present or aName.IsEmpty() or aName.IsVoid()
> 
> still doesn't describe what each of those states mean.
> 
> >+  xpAccessible->Name(name);
> >+  if (!name.IsEmpty())
> >+    return S_OK;
> 
> this is wrong, when Name() returns a name with text you'll now return the
> empty string and S_OK.

you didn't really address this, Name() returning a non empty string still results in msaa getting an empty string.

> btw what about mac?

Comment 39

[alexander :surkov (:asurkov)]

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

>    I'd love to have credit for completing this, but if there is substantial
> changes yet desired, I'll have to ask you to re-assign this one, as I'm not
> sure how much time I have available for it in the immediate future.

ok, I'll finish this one.

Comment 40

[alexander :surkov (:asurkov)]

Attachment: patch11

Comment 41

[Trevor Saunders (:tbsaunde)]

Comment on attachment 619513 [details] [diff] [review]
patch11

>+  } else if (mContent->IsXUL()) {
>+    if (mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::tooltiptext, aName)) {
>+      aName.CompressWhitespace();
>+      return eNameFromTooltip;
>+    }
>+  }
>+  else {

nit, else { on same line s }

>+enum ENameValueFlag {
>+  /**
>+   * Name either
>+   *  a) presented (not empty): !name.IsEmpty()

nit, should it be present not presented?

>+   *  c) no name (was missed): name.IsVoid()
>+   *  b) was left empty by the author on demand: name.IsEmpty() && !name.IsVoid()

nit, shouldn't the order be a b c?

Comment 42

[alexander :surkov (:asurkov)]

landed with nits fixed https://hg.mozilla.org/integration/mozilla-inbound/rev/221db28204cf

thanks everybody for the work!

Comment 43

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/221db28204cf