Closed Bug 652378 Opened 13 years ago Closed 13 years ago

dexpcom nsAccessible::getDescription()

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

Details

Attachments

(1 file, 1 obsolete file)

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: nobody → trev.saunders
Blocks: dexpcoma11y
Attached patch patch (obsolete) — Splinter Review
Attachment #527974 - Flags: review?(surkov.alexander)
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.
(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?
(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.
(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.
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.
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?
(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.)
(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.)
(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
Attached patch patchSplinter Review
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?
(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!
Summary: dexpcom nsAccessible::getdeSCription() → dexpcom nsAccessible::getDescription()
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 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+
landed on mozilla-central
http://hg.mozilla.org/mozilla-central/rev/ecfeced36f14
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: