Closed Bug 783372 Opened 7 years ago Closed 7 years ago

use HyperTextAccessible directly in atk{,Editable}Text impl

Categories

(Core :: Disability Access APIs, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
Attached patch patchSplinter Review
Attachment #652554 - Flags: review?(surkov.alexander)
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
Attachment #652554 - Flags: review?(surkov.alexander) → review+
(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
(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.
(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
Summary: use HyperTextAccessible directly in atk{,Ediatable}Text impl → use HyperTextAccessible directly in atk{,Editable}Text impl
https://hg.mozilla.org/mozilla-central/rev/15cc8994c2be
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.