934039 - isolate XPCOM text interfaces implementation

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[Trevor Saunders (:tbsaunde)]

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

>+++ b/accessible/src/generic/HyperTextAccessible-inl.h
>+HyperTextAccessible::IsValidRange(int32_t aStartOffset, int32_t aEndOffset)
>+{
>+  int32_t startOffset = ConvertMagicOffset(aStartOffset);
>+  if (startOffset < 0 || startOffset > static_cast<int32_t>(CharacterCount()))
>+    return false;
>+  int32_t endOffset = ConvertMagicOffset(aEndOffset);
>+  if (endOffset < 0 || endOffset > static_cast<int32_t>(CharacterCount()))
>+    return false;
>+
>+  return startOffset <= endOffset;

hm, would it be faster to write this as

startOffset = ConvertMagicOffset(aStart);
if (startOffset < 0)
 return false;

endOffset = ConvertMagicOffset(aEndOffset);
if (start offset > endOffset)
  return false;

return endOffset <= CharacterCount()

do we allow endOffset == Character count anyway? that seems a little odd.

>+HyperTextAccessible::AddToSelection(int32_t aStartOffset, int32_t aEndOffset)

death to selections with > 1 range

>+inline void
>+HyperTextAccessible::CopyText(int32_t aStartPos, int32_t aEndPos)
>+inline void
>+HyperTextAccessible::CutText(int32_t aStartPos, int32_t aEndPos)
>+inline void
>+HyperTextAccessible::DeleteText(int32_t aStartPos, int32_t aEndPos)
>+inline void
>+HyperTextAccessible::PasteText(int32_t aPosition)

those are all depricated right?  I don't see any reason to make them fast at the expense of a little more compilation.

> nsresult
> HyperTextAccessible::QueryInterface(REFNSIID aIID, void** aInstancePtr)
> {
>-  *aInstancePtr = nullptr;
>-
>-  // ARIA roles that these interfaces are not appropriate for.
>-  if (!IsTextRole())
>-    return Accessible::QueryInterface(aIID, aInstancePtr);
>-
>-  if (aIID.Equals(NS_GET_IID(nsIAccessibleText))) {
>-    *aInstancePtr = static_cast<nsIAccessibleText*>(this);
>-    NS_ADDREF_THIS();
>-    return NS_OK;
>-  }
>-
>-  if (aIID.Equals(NS_GET_IID(nsIAccessibleHyperText))) {
>-    *aInstancePtr = static_cast<nsIAccessibleHyperText*>(this);
>-    NS_ADDREF_THIS();
>-    return NS_OK;
>-  }
>-
>-  if (aIID.Equals(NS_GET_IID(nsIAccessibleEditableText))) {
>-    *aInstancePtr = static_cast<nsIAccessibleEditableText*>(this);
>-    NS_ADDREF_THIS();
>-    return NS_OK;
>-  }
>-
>-  return Accessible::QueryInterface(aIID, aInstancePtr);
>+  xpcAccessibleHyperText::QueryInterface(aIID, aInstancePtr);
>+  return *aInstancePtr ? NS_OK : Accessible::QueryInterface(aIID, aInstancePtr);

aren't you loosing the IsTextRole check?

>+void
>+HyperTextAccessible::TextSubstring(int32_t aStartOffset, int32_t aEndOffset,
>+                                   nsAString& aText)

call it TextBetween maybe?

>   if (aBoundaryType == BOUNDARY_CHAR) {
>     GetCharAt(aOffset, eGetBefore, aText, aStartOffset, aEndOffset);
>-    return NS_OK;
>+    return;
>   }
> 
>+  *aStartOffset = *aEndOffset = 0;
>+  aText.Truncate();
>+
>   int32_t convertedOffset = ConvertMagicOffset(aOffset);
>   if (convertedOffset < 0)
>-    return NS_ERROR_INVALID_ARG;
>+    return;

assert?

>   int32_t adjustedOffset = ConvertMagicOffset(aOffset);
>   if (adjustedOffset < 0)
>-    return NS_ERROR_INVALID_ARG;
>+    return;

same

note to self stopped at GetBounds stuff.

Comment 2

[Trevor Saunders (:tbsaunde)]

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

> HyperTextAccessible::ScrollSubstringTo(int32_t aStartIndex, int32_t aEndIndex,
>                                        uint32_t aScrollType)
> {
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;
>-
>   nsRefPtr<nsRange> range = new nsRange(mContent);
>   nsresult rv = HypertextOffsetsToDOMRange(aStartIndex, aEndIndex, range);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  return nsCoreUtils::ScrollSubstringTo(GetFrame(), range, aScrollType);
>+  if (NS_SUCCEEDED(rv))
>+    nsCoreUtils::ScrollSubstringTo(GetFrame(), range, aScrollType);

are you really sure that should never fail or callers shouldn't care about failure?

> HyperTextAccessible::ScrollSubstringToPoint(int32_t aStartIndex,
>                                             int32_t aEndIndex,
>                                             uint32_t aCoordinateType,
>                                             int32_t aX, int32_t aY)
> {
>   nsIFrame *frame = GetFrame();
>   if (!frame)
>-    return NS_ERROR_FAILURE;
>+    return;
> 
>   nsIntPoint coords = nsAccUtils::ConvertToScreenCoords(aX, aY, aCoordinateType,
>                                                         this);
> 
>   nsRefPtr<nsRange> range = new nsRange(mContent);
>   nsresult rv = HypertextOffsetsToDOMRange(aStartIndex, aEndIndex, range);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  if (NS_FAILED(rv))
>+    return;

same here

>+  nsIntRect CharBounds(int32_t aOffset, uint32_t aCoordType)
>+    { return TextBounds(aOffset, aOffset + 1, aCoordType); }

is that supposed to work when the offset is a magic offset, or when the offset is the last char?

>+    int32_t start = 0, end = 0;
>+    mGeckoTextAccessible->SelectionBoundsAt(0, &start, &end);
>+    mGeckoTextAccessible->DeleteText(start, end - start);
> 
>     nsString text;
>     nsCocoaUtils::GetStringForNSString(stringValue, text);
>-    rv = mGeckoTextAccessible->InsertText(text, start);
>-    NS_ENSURE_SUCCESS(rv,);
>-
>-    return;
>+    mGeckoTextAccessible->InsertText(text, start);

I thought there was a SetText method?

>+  if (!textAcc->IsValidRange(aStartOffset, aEndOffset))
>+    return E_INVALIDARG;
>+
>+  textAcc->DeleteText(aStartOffset, aEndOffset);
> 
>   uint32_t length = ::SysStringLen(*aText);
>   nsAutoString text(*aText, length);
>+  textAcc->InsertText(text, aStartOffset);

same

>+ia2AccessibleText::get_text(long aStartOffset, long aEndOffset, BSTR* aText)
> {
>   A11Y_TRYBLOCK_BEGIN
> 
>   if (!aText)
>     return E_INVALIDARG;
> 
>   *aText = nullptr;
> 
>   HyperTextAccessible* textAcc = static_cast<HyperTextAccessibleWrap*>(this);
>   if (textAcc->IsDefunct())
>     return CO_E_OBJNOTCONNECTED;
> 
>   nsAutoString text;
>-  nsresult rv = textAcc->GetText(aStartOffset, aEndOffset, text);
>-  if (NS_FAILED(rv))
>-    return GetHRESULT(rv);
>-
>+  textAcc->TextSubstring(aStartOffset, aEndOffset, text);

don't you need to check those are valid offsets?

> ia2AccessibleText::get_textAfterOffset(long aOffset,

stopped here

Comment 3

[Trevor Saunders (:tbsaunde)]

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

> ia2AccessibleText::get_textAtOffset(long aOffset,
>                                     enum IA2TextBoundaryType aBoundaryType,
>-                                    long *aStartOffset, long *aEndOffset,
>-                                    BSTR *aText)
>+                                    long* aStartOffset, long* aEndOffset,
>+                                    BSTR* aText)
> {
>   A11Y_TRYBLOCK_BEGIN
> 
>   if (!aStartOffset || !aEndOffset || !aText)
>     return E_INVALIDARG;
> 
>-  *aStartOffset = 0;
>-  *aEndOffset = 0;
>+  *aStartOffset = *aEndOffset = 0;
>   *aText = nullptr;
> 
>   HyperTextAccessible* textAcc = static_cast<HyperTextAccessibleWrap*>(this);
>   if (textAcc->IsDefunct())
>     return CO_E_OBJNOTCONNECTED;
> 
>-  nsresult rv = NS_OK;
>   nsAutoString text;
>   int32_t startOffset = 0, endOffset = 0;
>-
>   if (aBoundaryType == IA2_TEXT_BOUNDARY_ALL) {
>     startOffset = 0;
>     endOffset = textAcc->CharacterCount();
>-    rv = textAcc->GetText(startOffset, endOffset, text);
>+    textAcc->TextSubstring(startOffset, endOffset, text);
>   } else {
>     AccessibleTextBoundary boundaryType = GetGeckoTextBoundary(aBoundaryType);
>     if (boundaryType == -1)
>       return S_FALSE;
>-    rv = textAcc->GetTextAtOffset(aOffset, boundaryType,
>-                                  &startOffset, &endOffset, text);
>+    textAcc->TextAtOffset(aOffset, boundaryType, &startOffset, &endOffset, text);

what if bounds are invalid? same for before / after offset too I assume.

>+++ b/accessible/src/xpcom/xpcAccessibleHyperText.cpp
+
>+#include "HyperTextAccessible-inl.h"

include xpcAccessibleHyperText.h first pls to be sure it compiles on its own.

>+xpcAccessibleHyperText::QueryInterface(REFNSIID aIID, void** aInstancePtr)
>+{
>+  *aInstancePtr = nullptr;
>+
>+  HyperTextAccessible* text = static_cast<HyperTextAccessible*>(this);
>+  if (!text->IsTextRole())
>+    return NS_ERROR_NOT_IMPLEMENTED;

wouldn't this make more sense in HyperTextAccessible::QI?

btw I think that's the wrong error code.

>+  else
>+    return NS_ERROR_NOT_IMPLEMENTED;

isn't it NS_ERROR_NOINTERFACE?

>+  nsIntRect rect = text->CharBounds(aOffset, aCoordType);
>+  *aX = rect.x; *aY = rect.y; *aWidth = rect.width; *aHeight = rect.height;

gross

>+  nsIntRect rect = text->TextBounds(aStartOffset, aEndOffset, aCoordType);
>+  *aX = rect.x; *aY = rect.y; *aWidth = rect.width; *aHeight = rect.height;

some reason a couple lines would be bad esp when this goes over 80 chars?

>+xpcAccessibleHyperText::SetScriptableCaretOffset(int32_t aCaretOffset)
>+{
>+  HyperTextAccessible* text = static_cast<HyperTextAccessible*>(this);
>+  if (text->IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  text->SetCaretOffset(aCaretOffset);

what if its an ivalid offset?

>+xpcAccessibleHyperText::RemoveSelection(int32_t aSelectionNum)
>+{
>+  HyperTextAccessible* text = static_cast<HyperTextAccessible*>(this);
>+  if (text->IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  text->RemoveFromSelection(aSelectionNum);

same

>+xpcAccessibleHyperText::ScriptableScrollSubstringTo(int32_t aStartOffset,
>+                                                    int32_t aEndOffset,
>+                                                    uint32_t aScrollType)
>+{
>+  HyperTextAccessible* text = static_cast<HyperTextAccessible*>(this);
>+  if (text->IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  text->ScrollSubstringTo(aStartOffset, aEndOffset, aScrollType);

same

>+xpcAccessibleHyperText::ScriptableScrollSubstringToPoint(int32_t aStartOffset,
>+                                                         int32_t aEndOffset,
>+                                                         uint32_t aCoordinateType,
>+                                                         int32_t aX, int32_t aY)
>+{
>+  HyperTextAccessible* text = static_cast<HyperTextAccessible*>(this);
>+  if (text->IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  text->ScrollSubstringToPoint(aStartOffset, aEndOffset, aCoordinateType, aX, aY);

same

>+xpcAccessibleHyperText::ScriptableInsertText(const nsAString& aText,
>+                                             int32_t aOffset)
>+{
>+  HyperTextAccessible* text = static_cast<HyperTextAccessible*>(this);
>+  if (text->IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  text->InsertText(aText, aOffset);

same

>+xpcAccessibleHyperText::ScriptableCopyText(int32_t aStartOffset,
>+                                           int32_t aEndOffset)
>+{
>+  HyperTextAccessible* text = static_cast<HyperTextAccessible*>(this);
>+  if (text->IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  text->CopyText(aStartOffset, aEndOffset);

same

>+xpcAccessibleHyperText::ScriptableCutText(int32_t aStartOffset,
>+                                          int32_t aEndOffset)
>+{
>+  HyperTextAccessible* text = static_cast<HyperTextAccessible*>(this);
>+  if (text->IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  text->CutText(aStartOffset, aEndOffset);

same

>+xpcAccessibleHyperText::ScriptableDeleteText(int32_t aStartOffset,
>+                                             int32_t aEndOffset)
>+{
>+  HyperTextAccessible* text = static_cast<HyperTextAccessible*>(this);
>+  if (text->IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  text->DeleteText(aStartOffset, aEndOffset);

same

>+xpcAccessibleHyperText::ScriptablePasteText(int32_t aOffset)

same

>+xpcAccessibleHyperText::GetLinkAt(int32_t aIndex, nsIAccessibleHyperLink** aLink)
>+{
>+  NS_ENSURE_ARG_POINTER(aLink);
>+  *aLink = nullptr;
>+
>+  HyperTextAccessible* text = static_cast<HyperTextAccessible*>(this);
>+  if (text->IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  Accessible* link = text->LinkAt(aIndex);
>+  if (link)
>+    CallQueryInterface(link, aLink);
>+
>+  return NS_OK;

why not just do
nsCOMPtr<nsIAccessibleHyperLink> link = text->LinkAt(i);
link.forget(aLink);
return *aLink ? NS_OK : NS_ERROR_IVALID_ARG;

>+xpcAccessibleHyperText::GetLinkIndexAtOffset(int32_t aOffset,
>+                                             int32_t* aLinkIndex)
>+{
>+  NS_ENSURE_ARG_POINTER(aLinkIndex);
>+  *aLinkIndex = -1; // API says this magic value means 'not found'
>+
>+  HyperTextAccessible* text = static_cast<HyperTextAccessible*>(this);
>+  if (text->IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  *aLinkIndex = text->LinkIndexAtOffset(aOffset);
>+  return NS_OK;

what about invalid offset?

Comment 4

[alexander :surkov (:asurkov)]

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

> do we allow endOffset == Character count anyway? that seems a little odd.

yes, it's offset after last character


> >+HyperTextAccessible::AddToSelection(int32_t aStartOffset, int32_t aEndOffset)
> 
> death to selections with > 1 range

why?

> >+HyperTextAccessible::PasteText(int32_t aPosition)
> 
> those are all depricated right?  I don't see any reason to make them fast at
> the expense of a little more compilation.

in IA2 world only

> > nsresult
> > HyperTextAccessible::QueryInterface(REFNSIID aIID, void** aInstancePtr)
> >+  xpcAccessibleHyperText::QueryInterface(aIID, aInstancePtr);
> >+  return *aInstancePtr ? NS_OK : Accessible::QueryInterface(aIID, aInstancePtr);
> 
> aren't you loosing the IsTextRole check?

xpcAccessibleHyperText::QueryInterface has it

> >+HyperTextAccessible::TextSubstring(int32_t aStartOffset, int32_t aEndOffset,
> >+                                   nsAString& aText)
> 
> call it TextBetween maybe?

that was my initial thinking but then I've inclined for TextSubstring, it appears it's more native

> >   int32_t convertedOffset = ConvertMagicOffset(aOffset);
> >   if (convertedOffset < 0)
> >-    return NS_ERROR_INVALID_ARG;
> >+    return;
> 
> assert?

ok, it would be nicer if ConvertMagicOffset returned unsigned

(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > HyperTextAccessible::ScrollSubstringTo(int32_t aStartIndex, int32_t aEndIndex,
> >                                        uint32_t aScrollType)

> are you really sure that should never fail or callers shouldn't care about
> failure?

caller should take care to check in arguments, otherwise it shouldn't fail

> 
> >+  nsIntRect CharBounds(int32_t aOffset, uint32_t aCoordType)
> >+    { return TextBounds(aOffset, aOffset + 1, aCoordType); }
> 
> is that supposed to work when the offset is a magic offset, or when the
> offset is the last char?

I think it should but I tried to copy what we have

> >+    mGeckoTextAccessible->InsertText(text, start);
> 
> I thought there was a SetText method?

where?

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> Comment on attachment 826212 [details] [diff] [review]
> patch
> 
> > ia2AccessibleText::get_textAtOffset(long aOffset,
> 
> what if bounds are invalid? same for before / after offset too I assume.

I added a check for in arg

> >+++ b/accessible/src/xpcom/xpcAccessibleHyperText.cpp
> +
> >+#include "HyperTextAccessible-inl.h"
> 
> include xpcAccessibleHyperText.h first pls to be sure it compiles on its own.

ok

> >+xpcAccessibleHyperText::QueryInterface(REFNSIID aIID, void** aInstancePtr)
> >+{
> >+  *aInstancePtr = nullptr;
> >+
> >+  HyperTextAccessible* text = static_cast<HyperTextAccessible*>(this);
> >+  if (!text->IsTextRole())
> >+    return NS_ERROR_NOT_IMPLEMENTED;
> 
> wouldn't this make more sense in HyperTextAccessible::QI?

not sure, one day we should remove this class from inheritance chain so all QI logic will get here

> >+  else
> >+    return NS_ERROR_NOT_IMPLEMENTED;
> 
> isn't it NS_ERROR_NOINTERFACE?

right

> >+  nsIntRect rect = text->CharBounds(aOffset, aCoordType);
> >+  *aX = rect.x; *aY = rect.y; *aWidth = rect.width; *aHeight = rect.height;
> >+  nsIntRect rect = text->TextBounds(aStartOffset, aEndOffset, aCoordType);
> >+  *aX = rect.x; *aY = rect.y; *aWidth = rect.width; *aHeight = rect.height;
> 
> some reason a couple lines would be bad esp when this goes over 80 chars?

ok, couple lines, btw, it's not over 80 chars

> >+xpcAccessibleHyperText::SetScriptableCaretOffset(int32_t aCaretOffset)

> >+  text->SetCaretOffset(aCaretOffset);
> 
> what if its an ivalid offset?

dunno, does it make a difference, XPCOM interface doesn't say much about error handling. What do you think?

> >+  Accessible* link = text->LinkAt(aIndex);
> >+  if (link)
> >+    CallQueryInterface(link, aLink);
> >+
> >+  return NS_OK;
> 
> why not just do
> nsCOMPtr<nsIAccessibleHyperLink> link = text->LinkAt(i);
> link.forget(aLink);
> return *aLink ? NS_OK : NS_ERROR_IVALID_ARG;

nicer, no extra object

> >+xpcAccessibleHyperText::GetLinkIndexAtOffset(int32_t aOffset,
> >+                                             int32_t* aLinkIndex)
> >+{
> >+  NS_ENSURE_ARG_POINTER(aLinkIndex);
> >+  *aLinkIndex = -1; // API says this magic value means 'not found'
> >+  *aLinkIndex = text->LinkIndexAtOffset(aOffset);
> >+  return NS_OK;
> 
> what about invalid offset?

it says -1

Comment 5

[Trevor Saunders (:tbsaunde)]

> > >+HyperTextAccessible::AddToSelection(int32_t aStartOffset, int32_t aEndOffset)
> > 
> > death to selections with > 1 range
> 
> why?

in this case because they made this interface much more complicated, and generally the same reason since they don't really make much sense anyway (accept maybe just maybe tables)

> > >+HyperTextAccessible::PasteText(int32_t aPosition)
> > 
> > those are all depricated right?  I don't see any reason to make them fast at
> > the expense of a little more compilation.
> 
> in IA2 world only

hm ok, still making them fast doesn't seem very worth while.

> > > nsresult
> > > HyperTextAccessible::QueryInterface(REFNSIID aIID, void** aInstancePtr)
> > >+  xpcAccessibleHyperText::QueryInterface(aIID, aInstancePtr);
> > >+  return *aInstancePtr ? NS_OK : Accessible::QueryInterface(aIID, aInstancePtr);
> > 
> > aren't you loosing the IsTextRole check?
> 
> xpcAccessibleHyperText::QueryInterface has it

yeah saw that later.

> > >+HyperTextAccessible::TextSubstring(int32_t aStartOffset, int32_t aEndOffset,
> > >+                                   nsAString& aText)
> > 
> > call it TextBetween maybe?
> 
> that was my initial thinking but then I've inclined for TextSubstring, it
> appears it's more native

not sure what you mean by that.

> > >   int32_t convertedOffset = ConvertMagicOffset(aOffset);
> > >   if (convertedOffset < 0)
> > >-    return NS_ERROR_INVALID_ARG;
> > >+    return;
> > 
> > assert?
> 
> ok, it would be nicer if ConvertMagicOffset returned unsigned

yeah, I guess we could do that now or maybe push conversion of offsets out to the platform and not deal with it here.

> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > > HyperTextAccessible::ScrollSubstringTo(int32_t aStartIndex, int32_t aEndIndex,
> > >                                        uint32_t aScrollType)
> 
> > are you really sure that should never fail or callers shouldn't care about
> > failure?
> 
> caller should take care to check in arguments, otherwise it shouldn't fail

I'd prefer to have made it return void first then, but oh well.
> > 
> > >+  nsIntRect CharBounds(int32_t aOffset, uint32_t aCoordType)
> > >+    { return TextBounds(aOffset, aOffset + 1, aCoordType); }
> > 
> > is that supposed to work when the offset is a magic offset, or when the
> > offset is the last char?
> 
> I think it should but I tried to copy what we have

ok

> > >+    mGeckoTextAccessible->InsertText(text, start);
> > 
> > I thought there was a SetText method?
> 
> where?

I think I was thinking of ReplaceText which is what this does right?

> > >+xpcAccessibleHyperText::QueryInterface(REFNSIID aIID, void** aInstancePtr)
> > >+{
> > >+  *aInstancePtr = nullptr;
> > >+
> > >+  HyperTextAccessible* text = static_cast<HyperTextAccessible*>(this);
> > >+  if (!text->IsTextRole())
> > >+    return NS_ERROR_NOT_IMPLEMENTED;
> > 
> > wouldn't this make more sense in HyperTextAccessible::QI?
> 
> not sure, one day we should remove this class from inheritance chain so all
> QI logic will get here

well, when we change that we'll need to rework a lot of the qi logic anyway so moving it shouldn't be big deal.

> > >+xpcAccessibleHyperText::SetScriptableCaretOffset(int32_t aCaretOffset)
> 
> > >+  text->SetCaretOffset(aCaretOffset);
> > 
> > what if its an ivalid offset?
> 
> dunno, does it make a difference, XPCOM interface doesn't say much about
> error handling. What do you think?

generally silent failure is pretty bad, if you can't do something you should complain loudly.

> > >+  Accessible* link = text->LinkAt(aIndex);
> > >+  if (link)
> > >+    CallQueryInterface(link, aLink);
> > >+
> > >+  return NS_OK;
> > 
> > why not just do
> > nsCOMPtr<nsIAccessibleHyperLink> link = text->LinkAt(i);
> > link.forget(aLink);
> > return *aLink ? NS_OK : NS_ERROR_IVALID_ARG;
> 
> nicer, no extra object

yeah, but it doesn't call qi when you just need to AddRef, and it reports the failure.

> > >+xpcAccessibleHyperText::GetLinkIndexAtOffset(int32_t aOffset,
> > >+                                             int32_t* aLinkIndex)
> > >+{
> > >+  NS_ENSURE_ARG_POINTER(aLinkIndex);
> > >+  *aLinkIndex = -1; // API says this magic value means 'not found'
> > >+  *aLinkIndex = text->LinkIndexAtOffset(aOffset);
> > >+  return NS_OK;
> > 
> > what about invalid offset?
> 
> it says -1

shouldn't it throw though?

Comment 6

[alexander :surkov (:asurkov)]

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

> hm ok, still making them fast doesn't seem very worth while.

isn't it preferable to have faster software over lesser compilation time?

> > that was my initial thinking but then I've inclined for TextSubstring, it
> > appears it's more native
> 
> not sure what you mean by that.

I meant I prefer TextSubstirng over TextBetween since it sounds more usual and seems more descriptive

> > ok, it would be nicer if ConvertMagicOffset returned unsigned
> 
> yeah, I guess we could do that now or maybe push conversion of offsets out
> to the platform and not deal with it here.

perhaps I would keep it as follow up to not increase more the patch and changes.

> > > >+    mGeckoTextAccessible->InsertText(text, start);
> > > 
> > > I thought there was a SetText method?
> > 
> > where?
> 
> I think I was thinking of ReplaceText which is what this does right?

ReplaceText replaces all text, here we need to replace the substring. We could have one more method for that but it shouldn't be a big deal

> > not sure, one day we should remove this class from inheritance chain so all
> > QI logic will get here
> 
> well, when we change that we'll need to rework a lot of the qi logic anyway
> so moving it shouldn't be big deal.

so either way it shouldn't be a big deal

> > dunno, does it make a difference, XPCOM interface doesn't say much about
> > error handling. What do you think?
> 
> generally silent failure is pretty bad, if you can't do something you should
> complain loudly.

that's right, but I don't like much exceptions in JS and designing a better XPCOM a11y interfaces is not something I would want to do here or even soon. Since we don't have many consumers and known consumers (accessFu + testing) should be ok with silent version then I'd save that for future.

> > > nsCOMPtr<nsIAccessibleHyperLink> link = text->LinkAt(i);
> > > link.forget(aLink);
> > > return *aLink ? NS_OK : NS_ERROR_IVALID_ARG;
> > 
> > nicer, no extra object
> 
> yeah, but it doesn't call qi when you just need to AddRef, and it reports
> the failure.

iirc nsCOMPtr always makes qi, maybe
if (link && link->IsLink()) {
  *aLink = static_cast<nsIAccesisbleHyperLink*>(link);
  NS_ADDREF(*aLink);
}

> > > >+xpcAccessibleHyperText::GetLinkIndexAtOffset(int32_t aOffset,
> > it says -1
> 
> shouldn't it throw though?

I don't know honestly, our XPCOM interfaces are far from being ideal

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > hm ok, still making them fast doesn't seem very worth while.
> 
> isn't it preferable to have faster software over lesser compilation time?
> 
> > > that was my initial thinking but then I've inclined for TextSubstring, it
> > > appears it's more native
> > 
> > not sure what you mean by that.
> 
> I meant I prefer TextSubstirng over TextBetween since it sounds more usual
> and seems more descriptive

I'd say that about TextBetween, but don't really like either.

> > > ok, it would be nicer if ConvertMagicOffset returned unsigned
> > 
> > yeah, I guess we could do that now or maybe push conversion of offsets out
> > to the platform and not deal with it here.
> 
> perhaps I would keep it as follow up to not increase more the patch and
> changes.

yeah, just thinking about how it should work.

> > > > >+    mGeckoTextAccessible->InsertText(text, start);
> > > > 
> > > > I thought there was a SetText method?
> > > 
> > > where?
> > 
> > I think I was thinking of ReplaceText which is what this does right?
> 
> ReplaceText replaces all text, here we need to replace the substring. We
> could have one more method for that but it shouldn't be a big deal

yeah, I missed it only removed selected stuff.

> > > not sure, one day we should remove this class from inheritance chain so all
> > > QI logic will get here
> > 
> > well, when we change that we'll need to rework a lot of the qi logic anyway
> > so moving it shouldn't be big deal.
> 
> so either way it shouldn't be a big deal

I guess just makes little more sense in other place.

> > > dunno, does it make a difference, XPCOM interface doesn't say much about
> > > error handling. What do you think?
> > 
> > generally silent failure is pretty bad, if you can't do something you should
> > complain loudly.
> 
> that's right, but I don't like much exceptions in JS and designing a better

well, unles you want to use error codes or have everything return bool or something they're what you get

> XPCOM a11y interfaces is not something I would want to do here or even soon.
> Since we don't have many consumers and known consumers (accessFu + testing)
> should be ok with silent version then I'd save that for future.

Well, I bet it doesn't make lives of people writting access foo / tests nice, but their life already sucks so meh

> > > > nsCOMPtr<nsIAccessibleHyperLink> link = text->LinkAt(i);
> > > > link.forget(aLink);
> > > > return *aLink ? NS_OK : NS_ERROR_IVALID_ARG;
> > > 
> > > nicer, no extra object
> > 
> > yeah, but it doesn't call qi when you just need to AddRef, and it reports
> > the failure.
> 
> iirc nsCOMPtr always makes qi, maybe
> if (link && link->IsLink()) {
>   *aLink = static_cast<nsIAccesisbleHyperLink*>(link);
>   NS_ADDREF(*aLink);
> }

I don't think so, but if you want to be careful you can just use nsRefPtr.

> > > > >+xpcAccessibleHyperText::GetLinkIndexAtOffset(int32_t aOffset,
> > > it says -1
> > 
> > shouldn't it throw though?
> 
> I don't know honestly, our XPCOM interfaces are far from being ideal

yeah, guess I don't really care

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 9

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/59087c617ba0

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/59087c617ba0