Closed
Bug 652378
Opened 13 years ago
Closed 13 years ago
dexpcom nsAccessible::getDescription()
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla6
People
(Reporter: tbsaunde, Assigned: tbsaunde)
References
Details
Attachments
(1 file, 1 obsolete file)
31.78 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
Assignee: nobody → trev.saunders
Blocks: dexpcoma11y
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #527974 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 2•13 years ago
|
||
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/trev.saunders@gmail.com-1f8e1a3a8091
Comment 3•13 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•13 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•13 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•13 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•13 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•13 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•13 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•13 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?
Comment 11•13 years ago
|
||
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•13 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•13 years ago
|
||
use nsString instead of nsAutoString
Attachment #527974 -
Attachment is obsolete: true
Attachment #527974 -
Flags: review?(surkov.alexander)
Attachment #529404 -
Flags: review?(surkov.alexander)
Comment 14•13 years ago
|
||
Trev, did you spin a try-build for this new patch, too?
Assignee | ||
Comment 15•13 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?
Comment 16•13 years ago
|
||
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•13 years ago
|
Summary: dexpcom nsAccessible::getdeSCription() → dexpcom nsAccessible::getDescription()
Comment 17•13 years ago
|
||
Neil, are there advantages of nsString& usage over nsString* as method parameters? The best practice nsString& approach, right?
Comment 18•13 years ago
|
||
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•13 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•13 years ago
|
||
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/trev.saunders@gmail.com-b3792ad14d6b
Assignee | ||
Comment 21•13 years ago
|
||
landed on mozilla-central http://hg.mozilla.org/mozilla-central/rev/ecfeced36f14
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Target Milestone: --- → mozilla6
Version: unspecified → Trunk
You need to log in
before you can comment on or make changes to this bug.
Description
•