Last Comment Bug 740747 - dexpcom nsAccessible::GetName
: dexpcom nsAccessible::GetName
Status: RESOLVED FIXED
[good first bug][mentor=eitan@monoton...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: alexander :surkov
:
:
Mentors:
Depends on: 751623
Blocks: 745429 748724
  Show dependency treegraph
 
Reported: 2012-03-30 02:53 PDT by alexander :surkov
Modified: 2012-05-03 10:28 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (34.23 KB, patch)
2012-04-13 10:14 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Error (65.33 KB, image/png)
2012-04-13 10:14 PDT, Mark Capella [:capella]
no flags Details
Patch (v2) (37.06 KB, patch)
2012-04-15 04:22 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v3) (37.04 KB, patch)
2012-04-15 06:34 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v4) (36.99 KB, patch)
2012-04-16 00:55 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v5) (36.99 KB, patch)
2012-04-16 02:50 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v6) (34.30 KB, patch)
2012-04-16 06:27 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review
Patch (v7) (34.13 KB, patch)
2012-04-17 03:15 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v8) (34.79 KB, patch)
2012-04-17 23:59 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v9) (39.58 KB, patch)
2012-04-26 17:19 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: feedback-
Details | Diff | Splinter Review
Patch (v10) (36.25 KB, patch)
2012-04-28 02:38 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
patch11 (36.88 KB, patch)
2012-04-30 03:24 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-03-30 02:53:05 PDT
identical to bug 652378, just follow the idea of the patch.
Comment 1 alexander :surkov 2012-03-30 03:28:03 PDT
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] 2012-04-13 10:14:22 PDT
Created attachment 614844 [details] [diff] [review]
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] 2012-04-13 10:14:51 PDT
Created attachment 614845 [details]
Error
Comment 4 alexander :surkov 2012-04-13 21:15:13 PDT
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) 2012-04-13 21:38:07 PDT
> 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 2012-04-13 22:04:52 PDT
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] 2012-04-15 04:22:59 PDT
Created attachment 615133 [details] [diff] [review]
Patch (v2)

Problem with patch failing "name" mochitests corrected ... nits addressed ... rebuilt and tested ...
Comment 8 Mozilla RelEng Bot 2012-04-15 05:50:30 PDT
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 2012-04-15 06:32:18 PDT
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/autolanduser@mozilla.com-fc63bc2ae088
Comment 10 Mark Capella [:capella] 2012-04-15 06:34:09 PDT
Created attachment 615143 [details] [diff] [review]
Patch (v3)

last patch burned on Linux ... bug corrected
Comment 11 Mozilla RelEng Bot 2012-04-15 06:46:05 PDT
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 2012-04-15 10:17:43 PDT
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/autolanduser@mozilla.com-8cdc94191df3
Comment 13 alexander :surkov 2012-04-15 19:13:57 PDT
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] 2012-04-15 19:38:13 PDT
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 2012-04-15 21:02:49 PDT
(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 2012-04-15 21:03:34 PDT
So what constants do you want to have?
Comment 17 Mark Capella [:capella] 2012-04-15 21:14:26 PDT
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 2012-04-15 21:23:30 PDT
(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] 2012-04-16 00:55:14 PDT
Created attachment 615266 [details] [diff] [review]
Patch (v4)


Changed, rebuilt and tested locally ... all looks good :)
Comment 20 alexander :surkov 2012-04-16 01:26:14 PDT
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] 2012-04-16 02:50:48 PDT
Created attachment 615284 [details] [diff] [review]
Patch (v5)

Revised, try - try - again ...
Comment 22 alexander :surkov 2012-04-16 03:07:31 PDT
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] 2012-04-16 06:27:00 PDT
Created attachment 615318 [details] [diff] [review]
Patch (v6)

Ah ok ... I forgot to QREFRESH before posting the last tested patch ... here it is ...
Comment 24 alexander :surkov 2012-04-17 01:10:02 PDT
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] 2012-04-17 01:29:38 PDT
   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 2012-04-17 01:33:27 PDT
(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] 2012-04-17 03:15:09 PDT
Created attachment 615642 [details] [diff] [review]
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) 2012-04-17 23:10:58 PDT
> 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] 2012-04-17 23:59:27 PDT
Created attachment 616032 [details] [diff] [review]
Patch (v8)

Cleaned-up that method, rebuild and tested locally.
Comment 30 Trevor Saunders (:tbsaunde) 2012-04-18 21:34:47 PDT
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) 2012-04-19 09:39:37 PDT
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 2012-04-22 20:52:21 PDT
(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] 2012-04-26 17:19:10 PDT
Created attachment 618867 [details] [diff] [review]
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 2012-04-26 23:02:29 PDT
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) 2012-04-27 23:49:27 PDT
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) 2012-04-27 23:51:17 PDT
(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] 2012-04-28 02:38:09 PDT
Created attachment 619267 [details] [diff] [review]
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) 2012-04-28 20:24:55 PDT
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 2012-04-29 18:10:17 PDT
(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 2012-04-30 03:24:03 PDT
Created attachment 619513 [details] [diff] [review]
patch11
Comment 41 Trevor Saunders (:tbsaunde) 2012-04-30 10:59:41 PDT
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 2012-04-30 20:09:11 PDT
landed with nits fixed https://hg.mozilla.org/integration/mozilla-inbound/rev/221db28204cf

thanks everybody for the work!

Note You need to log in before you can comment on or make changes to this bug.