Closed Bug 1303447 Opened 8 years ago Closed 8 years ago

Implement ia2AccessibleText methods for ProxyAccessible in Windows

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: michael.li11702, Assigned: tbsaunde)

References

Details

Attachments

(1 file, 1 obsolete file)

MozReview-Commit-ID: IBttKLZdOgU
Compatible with shared header patch of bug 1303040 and contains QueryInterface wrappers from bug 1301148
Attachment #8792115 - Flags: review?(tbsaunde+mozbugs)
Blocks: 1288839
Depends on: 1303040
Attachment #8792115 - Attachment is obsolete: true
Attachment #8792115 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8802155 [details] [diff] [review]
implement text interface methods for the windows ProxyAccessible r?aklotz

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

I'm not sure how the mingw people are going to feel about our use of _bstr_t, whose implementation is provided by MSVC and not the Windows SDK. I don't know whether or not mingw has a polyfill for that. OTOH, they're usually prompt about letting us know when they don't! ;-)

My main concern is with the cast in ScrollSubstringToPoint (see below). r=me with that fixed.

::: accessible/ipc/win/ProxyAccessible.cpp
@@ +389,5 @@
> +      return IA2_TEXT_BOUNDARY_WORD;
> +    case nsIAccessibleText::BOUNDARY_LINE_START:
> +      return IA2_TEXT_BOUNDARY_LINE;
> +    default:
> +      MOZ_ASSERT(false);

What about in the case of release builds? Should this be a release assert? Meh, I guess it's really only going to be used by our own test code, right?

@@ +394,5 @@
> +  }
> +}
> +
> +bool
> +ProxyAccessible::TextSubstring(int32_t aStartOffset, int32_t aEndOfset,

nit: s/EndOfset/EndOffset/

@@ +500,5 @@
> +  if (!acc) {
> +    return false;
> +  }
> +
> +  return !FAILED(acc->addSelection(static_cast<long>(aStartOffset),

s/!FAILED/SUCCEEDED/

@@ +512,5 @@
> +  if (!acc) {
> +    return false;
> +  }
> +
> +  return !FAILED(acc->removeSelection(static_cast<long>(aSelectionNum)));

s/!FAILED/SUCCEEDED/

@@ +545,5 @@
> +}
> +
> +void
> +ProxyAccessible::ScrollSubstringTo(int32_t aStartOffset, int32_t aEndOffset,
> +                                   uint32_t aScrollType)

Let's add a comment above this function definition pointing out that aScrollType should be a nsIAccessibleScrollType value.

@@ +554,5 @@
> +  }
> +
> +  acc->scrollSubstringTo(static_cast<long>(aStartOffset),
> +                         static_cast<long>(aEndOffset),
> +                         static_cast<IA2ScrollType>(aScrollType));

Could we maybe add a comment to the declaration of nsIAccessibleScrollType indicating that its values have a 1:1 correspondence with IA2ScrollType? That might allow an uneducated reader like me to save a few seconds looking up the IA2ScrollType IDL to compare. ;-)

@@ +559,5 @@
> +}
> +
> +void
> +ProxyAccessible::ScrollSubstringToPoint(int32_t aStartOffset, int32_t aEndOffset,
> +                                        uint32_t aCoordinateType, int32_t aX,

Comment that aCoordinateType is a nsIAccessibleCoordinateType value?

@@ +569,5 @@
> +  }
> +
> +  acc->scrollSubstringToPoint(static_cast<long>(aStartOffset),
> +                              static_cast<long>(aEndOffset),
> +                              static_cast<IA2CoordinateType>(aCoordinateType),

This look wrong given that IA2CoordinateType does not have an analog to  nsIAccessibleCoordinateType::COORDTYPE_WINDOW_RELATIVE. I don't think we can just cast this.
Attachment #8802155 - Flags: review?(aklotz) → review+
(In reply to Aaron Klotz [:aklotz] from comment #3)
> ::: accessible/ipc/win/ProxyAccessible.cpp
> @@ +389,5 @@
> > +      return IA2_TEXT_BOUNDARY_WORD;
> > +    case nsIAccessibleText::BOUNDARY_LINE_START:
> > +      return IA2_TEXT_BOUNDARY_LINE;
> > +    default:
> > +      MOZ_ASSERT(false);
> 
> What about in the case of release builds? Should this be a release assert?
> Meh, I guess it's really only going to be used by our own test code, right?
> 

MOZ_ASSERT_UNREACHABLE perhaps?
Status: UNCONFIRMED → NEW
Ever confirmed: true
> I'm not sure how the mingw people are going to feel about our use of
> _bstr_t, whose implementation is provided by MSVC and not the Windows SDK. I
> don't know whether or not mingw has a polyfill for that. OTOH, they're
> usually prompt about letting us know when they don't! ;-)

I just checked the mingw headers on my machine, and it looks like there's a _bstr_t class, so I expect it'll work.

I hope you mean _bstr_t is a header that comes from the compiler not it being some magic builtin, that sounds like madness.

> ::: accessible/ipc/win/ProxyAccessible.cpp
> @@ +389,5 @@
> > +      return IA2_TEXT_BOUNDARY_WORD;
> > +    case nsIAccessibleText::BOUNDARY_LINE_START:
> > +      return IA2_TEXT_BOUNDARY_LINE;
> > +    default:
> > +      MOZ_ASSERT(false);
> 
> What about in the case of release builds? Should this be a release assert?
> Meh, I guess it's really only going to be used by our own test code, right?

I agree it doesn't matter, but I guess we might as well here.

> 
> @@ +554,5 @@
> > +  }
> > +
> > +  acc->scrollSubstringTo(static_cast<long>(aStartOffset),
> > +                         static_cast<long>(aEndOffset),
> > +                         static_cast<IA2ScrollType>(aScrollType));
> 
> Could we maybe add a comment to the declaration of nsIAccessibleScrollType
> indicating that its values have a 1:1 correspondence with IA2ScrollType?
> That might allow an uneducated reader like me to save a few seconds looking
> up the IA2ScrollType IDL to compare. ;-)

it looks like there is one? though maybe it should be more visible / stronger?

> @@ +559,5 @@
> > +}
> > +
> > +void
> > +ProxyAccessible::ScrollSubstringToPoint(int32_t aStartOffset, int32_t aEndOffset,
> > +                                        uint32_t aCoordinateType, int32_t aX,
> 
> Comment that aCoordinateType is a nsIAccessibleCoordinateType value?

sure, blarg at xpidl for not allowing nice things.

> @@ +569,5 @@
> > +  }
> > +
> > +  acc->scrollSubstringToPoint(static_cast<long>(aStartOffset),
> > +                              static_cast<long>(aEndOffset),
> > +                              static_cast<IA2CoordinateType>(aCoordinateType),
> 
> This look wrong given that IA2CoordinateType does not have an analog to 
> nsIAccessibleCoordinateType::COORDTYPE_WINDOW_RELATIVE. I don't think we can
> just cast this.

good catch, I guess I should have been more careful when looking over this.

I'm not really sure what to do here with the window relative case, and for that matter I'm not terribly confident the bounds adjustment stuff for windows will work right with e10s anyway, but I guess that's what tests are for.
Pushed by tsaunders@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1e487f723a0
implement text interface methods for the windows ProxyAccessible r=aklotz
https://hg.mozilla.org/mozilla-central/rev/d1e487f723a0
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee: nobody → tbsaunde+mozbugs
You need to log in before you can comment on or make changes to this bug.