Last Comment Bug 652378 - dexpcom nsAccessible::getDescription()
: dexpcom nsAccessible::getDescription()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Trevor Saunders (:tbsaunde)
:
Mentors:
Depends on:
Blocks: dexpcoma11y
  Show dependency treegraph
 
Reported: 2011-04-23 18:49 PDT by Trevor Saunders (:tbsaunde)
Modified: 2011-05-18 08:11 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (31.87 KB, patch)
2011-04-23 18:56 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Splinter Review
patch (31.78 KB, patch)
2011-05-01 18:41 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2011-04-23 18:49:47 PDT
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
Comment 1 Trevor Saunders (:tbsaunde) 2011-04-23 18:56:14 PDT
Created attachment 527974 [details] [diff] [review]
patch
Comment 3 alexander :surkov 2011-04-24 23:53:43 PDT
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.
Comment 4 Trevor Saunders (:tbsaunde) 2011-04-25 00:12:30 PDT
(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 alexander :surkov 2011-04-25 00:39:11 PDT
(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.
Comment 6 Trevor Saunders (:tbsaunde) 2011-04-25 02:23:01 PDT
(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 neil@parkwaycc.co.uk 2011-04-26 04:19:12 PDT
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.
Comment 8 Trevor Saunders (:tbsaunde) 2011-04-26 05:05:56 PDT
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 neil@parkwaycc.co.uk 2011-04-26 05:53:34 PDT
(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.)
Comment 10 Trevor Saunders (:tbsaunde) 2011-04-26 06:10:29 PDT
(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?
Comment 11 neil@parkwaycc.co.uk 2011-04-26 07:13:36 PDT
You should use nsString for the parameters. (The local string in GetDescription could still be an nsAutoString; it's hard to tell without profiling.)
Comment 12 Trevor Saunders (:tbsaunde) 2011-04-26 08:48:12 PDT
(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
Comment 13 Trevor Saunders (:tbsaunde) 2011-05-01 18:41:00 PDT
Created attachment 529404 [details] [diff] [review]
patch

use nsString instead of nsAutoString
Comment 14 Marco Zehe (:MarcoZ) 2011-05-02 04:11:06 PDT
Trev, did you spin a try-build for this new patch, too?
Comment 15 Trevor Saunders (:tbsaunde) 2011-05-02 04:27:32 PDT
(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?
Comment 16 Marco Zehe (:MarcoZ) 2011-05-02 04:43:24 PDT
Just wanted to know whether this is ready for me to look at or not. No rush, let me know when you're done!
Comment 17 alexander :surkov 2011-05-02 18:55:02 PDT
Neil, are there advantages of nsString& usage over nsString* as method parameters? The best practice nsString& approach, right?
Comment 18 neil@parkwaycc.co.uk 2011-05-03 01:59:25 PDT
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 alexander :surkov 2011-05-03 05:06:35 PDT
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?
Comment 21 Trevor Saunders (:tbsaunde) 2011-05-04 22:10:27 PDT
landed on mozilla-central
http://hg.mozilla.org/mozilla-central/rev/ecfeced36f14

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