The default bug view has changed. See this FAQ.

dexpcom nsAccessible::getDescription()

RESOLVED FIXED in mozilla6

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

Trunk
mozilla6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:6.0a1) Gecko/20110418 Firefox/6.0a1
Build Identifier: 

 provide void Description(nsAutoString& aDescription) then make the xpcom method call it

Reproducible: Always
(Assignee)

Updated

6 years ago
Assignee: nobody → trev.saunders
Blocks: 574336
(Assignee)

Comment 1

6 years ago
Created attachment 527974 [details] [diff] [review]
patch
Attachment #527974 - Flags: review?(surkov.alexander)
(Assignee)

Comment 2

6 years ago
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/trev.saunders@gmail.com-1f8e1a3a8091

Comment 3

6 years ago
nsAutoString is intended for local variables usage. When you replace nsAString on nsAutoString in method arguments then what do you solve? Extra objects creations/string copies?

Btw, XPCOM GetDecription version has extra string copy.
(Assignee)

Comment 4

6 years ago
(In reply to comment #3)
> nsAutoString is intended for local variables usage. When you replace nsAString
> on nsAutoString in method arguments then what do you solve? Extra objects
> creations/string copies?

yeah, there are a couple places where we have comments saying we wouldn't need this copy if nsAString  I got rid of those copies, would you prefer nsAString or some other string type?  I'm not really familiar with  how we deal with strings.

> Btw, XPCOM GetDecription version has extra string copy.

can that be avoided?

Comment 5

6 years ago
(In reply to comment #4)
> (In reply to comment #3)
> > nsAutoString is intended for local variables usage. When you replace nsAString
> > on nsAutoString in method arguments then what do you solve? Extra objects
> > creations/string copies?
> 
> yeah, there are a couple places where we have comments saying we wouldn't need
> this copy if nsAString  I got rid of those copies, 

You mean nsAString doesn't have methods we need to use and therefore we deal with nsAutoString and then copy it back to nsAString argument?

I suggest to ask Neil. Perhaps we should extend nsAString interface then.
(Assignee)

Comment 6

6 years ago
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > nsAutoString is intended for local variables usage. When you replace nsAString
> > > on nsAutoString in method arguments then what do you solve? Extra objects
> > > creations/string copies?
> > 
> > yeah, there are a couple places where we have comments saying we wouldn't need
> > this copy if nsAString  I got rid of those copies, 
> 
> You mean nsAString doesn't have methods we need to use and therefore we deal
> with nsAutoString and then copy it back to nsAString argument?

YES, PARTICULARLY  compressWHITESPACE() SEE NShtmlselectACCESSIBLE::getNAME() HAS A COMMENT TO THIS EFFECT, SEE  NShtmlselectACCESSIBLE.CPP LINE 217

> I suggest to ask Neil. Perhaps we should extend nsAString interface then.

IF WE CAN THAT SEEMS IDEAL.

Comment 7

6 years ago
For non-XPCOM methods, please use nsString& parameters, not nsAutoString&

(In reply to comment #6)
> Yes, particularly CompressWhitespace()
It's always nice to be able to harmonise the internal and external APIs. Here the external API provides nsAString::CompressWhitespace while the internal API provides nsString::CompressWhitespace and nsCString::CompressWhitespace.

On the other hand the internal APIs often make use of specialised internal knowledge, and certainly the code here is convoluted and not obviously portable to the internal nsAString class.
(Assignee)

Comment 8

6 years ago
In reply to comment #7)
> For non-XPCOM methods, please use nsString& parameters, not nsAutoString&

ok, that's what we're trying to figure out how to do :)

> 
> (In reply to comment #6)
> > Yes, particularly CompressWhitespace()
> It's always nice to be able to harmonise the internal and external APIs. Here
> the external API provides nsAString::CompressWhitespace while the internal API
> provides nsString::CompressWhitespace and nsCString::CompressWhitespace.

ok, I'm a little confused
I'm not seeing CompressWhitespace() on 
https://developer.mozilla.org/en/nsAString
is that just docs being out of date? or is that a different api than you mean?

> 
> On the other hand the internal APIs often make use of specialised internal
> knowledge, and certainly the code here is convoluted and not obviously portable
> to the internal nsAString class.

so, who is internal and external here?  before  the xpcom methods passed  nsASTRING& will that  still work?  can we call CompressWhitespace() on that string?

Comment 9

6 years ago
(In reply to comment #8)
> I'm not seeing CompressWhitespace() on 
> https://developer.mozilla.org/en/nsAString
> is that just docs being out of date? or is that a different api than you mean?
That document is for the internal API, and you're definitely using the internal API. (The external API is documented in the external string guide.)
(Assignee)

Comment 10

6 years ago
(In reply to comment #9)
> (In reply to comment #8)
> > I'm not seeing CompressWhitespace() on 
> > https://developer.mozilla.org/en/nsAString
> > is that just docs being out of date? or is that a different api than you mean?
> That document is for the internal API, and you're definitely using the internal
> API. (The external API is documented in the external string guide.)

SO THE EXTERNAL API WHICH WE CAN'T USE HAS A METHOD THE INTERNAL ONE WE CAN USE DOESN'T HAVE. then should we use nsString?
You should use nsString for the parameters. (The local string in GetDescription could still be an nsAutoString; it's hard to tell without profiling.)
(Assignee)

Comment 12

6 years ago
(In reply to comment #11)
> You should use nsString for the parameters. (The local string in GetDescription
> could still be an nsAutoString; it's hard to tell without profiling.)

OK, THANKS
(Assignee)

Comment 13

6 years ago
Created attachment 529404 [details] [diff] [review]
patch

use nsString instead of nsAutoString
Attachment #527974 - Attachment is obsolete: true
Attachment #527974 - Flags: review?(surkov.alexander)
Attachment #529404 - Flags: review?(surkov.alexander)
Trev, did you spin a try-build for this new patch, too?
(Assignee)

Comment 15

6 years ago
(In reply to comment #14)
> Trev, did you spin a try-build for this new patch, too?

not yet, I was expecting to build up another patch or two this week, and was going to push them together.  Why?
Just wanted to know whether this is ready for me to look at or not. No rush, let me know when you're done!

Updated

6 years ago
Summary: dexpcom nsAccessible::getdeSCription() → dexpcom nsAccessible::getDescription()

Comment 17

6 years ago
Neil, are there advantages of nsString& usage over nsString* as method parameters? The best practice nsString& approach, right?
Yes, best practice is to use nsString& rather than nsString* which would only be useful for heap-allocated strings. (Although the string could be optional, with nsString you can use SetIsVoid(PR_TRUE) to indicate a null string.)

Comment 19

6 years ago
Comment on attachment 529404 [details] [diff] [review]
patch

Review of attachment 529404 [details] [diff] [review]:

r=me with comments addressed

::: accessible/src/base/nsAccessible.cpp
@@ +280,5 @@
 
+  nsAutoString desc;
+  Description(desc);
+  aDescription.Assign(desc);
+

you should be able to pass nsAString

::: accessible/src/base/nsApplicationAccessible.h
@@ +121,5 @@
   virtual bool IsPrimaryForNode() const;
 
   // nsAccessible
   virtual void ApplyARIAState(PRUint64* aState);
+  virtual void Description(nsString &aDescription);

nit: nsString& aDescription

::: accessible/src/mac/mozAccessible.mm
@@ +518,5 @@
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;
 
   nsAutoString desc;
+  mGeckoAccessible->Description (desc);

nit: remove space after method name while you're here

::: accessible/src/xforms/nsXFormsAccessible.cpp
@@ +209,2 @@
   // search the xforms:hint element
+  GetBoundChildElementValue(NS_LITERAL_STRING("hint"), aDescription);

nit: I'd change this to
if (aDescription.IsEmpty())
  GetBoundChildEleemtnValue

::: accessible/src/xul/nsXULComboboxAccessible.cpp
@@ +127,5 @@
     do_QueryInterface(focusedOptionItem);
+  if (focusedOptionContent)
+    GetAccService()->
+      GetAccessibleInWeakShell(focusedOptionContent, mWeakShell)->
+      Description(aDescription);

no focusedOption null check anymore, having it null is possible, right?

::: accessible/src/xul/nsXULListboxAccessible.h
@@ +127,2 @@
   // nsAccessible
+  virtual void Description(nsString& aDesc) { nsAccessibleWrap::Description(aDesc); }

while you're here can you please uninline it?
Attachment #529404 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 20

6 years ago
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/trev.saunders@gmail.com-b3792ad14d6b
(Assignee)

Comment 21

6 years ago
landed on mozilla-central
http://hg.mozilla.org/mozilla-central/rev/ecfeced36f14
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Target Milestone: --- → mozilla6
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.