Comment on attachment 261386 [details] [diff] [review] patch + // The format is name:value;name:value with \ for escaping these + // characters ":;=,\". Please add the code to add the \ before those characters that need it. Please add a check on the first line of nsAccessible::GetAttributes(), if !mDOMNode fail. Nit, space before = *aAttributes=

Huh?

Comment on attachment 261400 [details] [diff] [review] patch2 Okay, but call charSet something like kCharsToEscape;

Comment on attachment 261400 [details] [diff] [review] patch2 Neil, can you check especially the work with string? (In reply to comment #3) > (From update of attachment 261400 [details] [diff] [review]) > Okay, but call charSet something like kCharsToEscape; > Ok

Comment on attachment 261400 [details] [diff] [review] patch2 >+ NS_ENSURE_TRUE(mDOMNode, NS_ERROR_FAILURE); NS_ENSURE_ARG(mDOMNode); perhaps? >+ // The format is name:value;name:value with \ for escaping these >+ // characters ":;=,\". So, trailing semicolon or no trailing semicolon? >+ PRUint32 offset = 0; Either move this to where you use it or don't uselessly define it here. >+ const char* charSet = ":;=,\\"; Should be char[] >+ nsCOMPtr<nsIPropertyElement> propElem; >+ propEnum->GetNext(getter_AddRefs(propElem)); Wrong, you should use nsCOMPtr<nsISupports> propSupports and then nsCOMPtr<nsIPropertyElement> propElem(do_QueryInterface(propSupports)); >+ strAttrs.Append(NS_ConvertUTF8toUTF16(name)); Please use AppendUTF8toUTF16 >+ strAttrs.AppendLiteral(":"); I prefer strAttrs.Append(':'); (and similarly for ';')

>+ NS_ENSURE_TRUE(mDOMNode, NS_ERROR_FAILURE); NS_ENSURE_ARG(mDOMNode); perhaps? I think both of these spew to the console. It's common for mDOMNode to be nsnull, for an accessible that's still held onto by the AT but has been shut down. To avoid too many messages to the console, I generally do it like this: if (!mDOMNode) { return NS_ERROR_FAILURE; // Node has been shut down }

(In reply to comment #5) > (From update of attachment 261400 [details] [diff] [review]) > >+ NS_ENSURE_TRUE(mDOMNode, NS_ERROR_FAILURE); > NS_ENSURE_ARG(mDOMNode); perhaps? I did like Aaron suggested. > > >+ // The format is name:value;name:value with \ for escaping these > >+ // characters ":;=,\". > So, trailing semicolon or no trailing semicolon? yes, for name and value. I changed comment. > >+ PRUint32 offset = 0; > Either move this to where you use it or don't uselessly define it here. Moved. > >+ const char* charSet = ":;=,\\"; > Should be char[] Fixed. > >+ nsCOMPtr<nsIPropertyElement> propElem; > >+ propEnum->GetNext(getter_AddRefs(propElem)); > Wrong, you should use nsCOMPtr<nsISupports> propSupports and then > nsCOMPtr<nsIPropertyElement> propElem(do_QueryInterface(propSupports)); Fixed. But why? > >+ strAttrs.Append(NS_ConvertUTF8toUTF16(name)); > Please use AppendUTF8toUTF16 > > >+ strAttrs.AppendLiteral(":"); > I prefer strAttrs.Append(':'); (and similarly for ';') > Fixed

Neil, any chance to review this?

Sorry for the delay, I was a bit busy this week. (In reply to comment #7) >>>+ nsCOMPtr<nsIPropertyElement> propElem; >>>+ propEnum->GetNext(getter_AddRefs(propElem)); >>Wrong, you should use nsCOMPtr<nsISupports> propSupports and then >>nsCOMPtr<nsIPropertyElement> propElem(do_QueryInterface(propSupports)); >Fixed. But why? Your code effectively does propElem = reinterpret_cast<nsIPropertyElement*> propSupports when it should do the equivalent of a dynamic_cast.

(In reply to comment #7) >>>+ // The format is name:value;name:value with \ for escaping these >>>+ // characters ":;=,\". >>So, trailing semicolon or no trailing semicolon? >yes, for name and value. I changed comment. No, that's not what I meant. The code generates name:value;name:value; with a final semicolon But the comment says name:value;name:value without it...

(In reply to comment #10) > (In reply to comment #7) > >>>+ // The format is name:value;name:value with \ for escaping these > >>>+ // characters ":;=,\". > >>So, trailing semicolon or no trailing semicolon? > >yes, for name and value. I changed comment. > No, that's not what I meant. > The code generates name:value;name:value; with a final semicolon > But the comment says name:value;name:value without it... > Ah, clear. I guess it's not big deal. If AT or IA2 teams will say it makes a difference then I change it.

(In reply to comment #9) > Sorry for the delay, I was a bit busy this week. No problem. > (In reply to comment #7) > >>>+ nsCOMPtr<nsIPropertyElement> propElem; > >>>+ propEnum->GetNext(getter_AddRefs(propElem)); > >>Wrong, you should use nsCOMPtr<nsISupports> propSupports and then > >>nsCOMPtr<nsIPropertyElement> propElem(do_QueryInterface(propSupports)); > >Fixed. But why? > Your code effectively does propElem = reinterpret_cast<nsIPropertyElement*> > propSupports when it should do the equivalent of a dynamic_cast. > Thank you for the clarification.

Comment on attachment 261646 [details] [diff] [review] patch3 >+ // The format is name:value;name:value with \ for escaping these Fix the comment to name:value;name:value; for check in.

for checkin

checked in