783372 - use HyperTextAccessible directly in atk{,Editable}Text impl

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 652554 [details] [diff] [review]
patch

Review of attachment 652554 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/atk/nsMaiInterfaceEditableText.cpp
@@ +22,5 @@
>      return FALSE;
>  
> +  HyperTextAccessible* text = accWrap->AsHyperText();
> +  if (!text || !text->IsTextRole())
> +    return false;

FALSE, here and everywhere below

@@ +27,3 @@
>  
>      nsCOMPtr<nsISupports> attrSet;
>      /* how to insert attributes into nsISupports ??? */

please remove this comment while you are here because setAttributes aren't supposed to write something into nsISupports, nsISupports is supposed to contain attributes. But actually you could just return FALSE and remove all logic since it doesn't work and it doesn't look like we need it in foreseeable future.

@@ +27,5 @@
>  
>      nsCOMPtr<nsISupports> attrSet;
>      /* how to insert attributes into nsISupports ??? */
>  
> +    nsresult rv = text->SetAttributes(aStartOffset, aEndOffset, attrSet);

nit: you can fix indent here as well

@@ +45,5 @@
>  
>      MAI_LOG_DEBUG(("EditableText: setTextContentsCB, aString=%s", aString));
>  
>      NS_ConvertUTF8toUTF16 strContent(aString);
> +    text->SetTextContents(strContent);

nit: indent? here and below

::: accessible/src/atk/nsMaiInterfaceText.cpp
@@ +65,5 @@
>      nsAutoString autoStr;
>      PRInt32 startOffset = 0, endOffset = 0;
>      nsresult rv =
> +        text->GetTextAfterOffset(aOffset, aBoundaryType,
> +                                 &startOffset, &endOffset, autoStr);

fix indent pls? and below

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #2)
> Comment on attachment 652554 [details] [diff] [review]
> patch
> 
> Review of attachment 652554 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/atk/nsMaiInterfaceEditableText.cpp
> @@ +22,5 @@
> >      return FALSE;
> >  
> > +  HyperTextAccessible* text = accWrap->AsHyperText();
> > +  if (!text || !text->IsTextRole())
> > +    return false;
> 
> FALSE, here and everywhere below

are you concerned about binary compat, or consistancy? if the later maybe we should move towards only "false"?

> @@ +27,3 @@
> >  
> >      nsCOMPtr<nsISupports> attrSet;
> >      /* how to insert attributes into nsISupports ??? */
> 
> please remove this comment while you are here because setAttributes aren't
> supposed to write something into nsISupports, nsISupports is supposed to
> contain attributes. But actually you could just return FALSE and remove all
> logic since it doesn't work and it doesn't look like we need it in
> foreseeable future.

yeah, I considered that, I'll just do it, but actually it looks atk will just return false for us if we don't implement that function, so I just won't define it if that's fine iwth you

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)

> > FALSE, here and everywhere below
> 
> are you concerned about binary compat, or consistancy? if the later maybe we
> should move towards only "false"?

both, gboolean is int and FALSE/TRUE constants are used for it.

> > please remove this comment while you are here because setAttributes aren't
> > supposed to write something into nsISupports, nsISupports is supposed to
> > contain attributes. But actually you could just return FALSE and remove all
> > logic since it doesn't work and it doesn't look like we need it in
> > foreseeable future.
> 
> yeah, I considered that, I'll just do it, but actually it looks atk will
> just return false for us if we don't implement that function, so I just
> won't define it if that's fine iwth you

I can live with either way I think.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> 
> > > FALSE, here and everywhere below
> > 
> > are you concerned about binary compat, or consistancy? if the later maybe we
> > should move towards only "false"?
> 
> both, gboolean is int and FALSE/TRUE constants are used for it.

iirc bool values promote to inteer types just fine, but I guess I'm fine with staying with TRUE / FALSE

Comment 6

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15cc8994c2be

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/15cc8994c2be