Last Comment Bug 873439 - Implement IAccessible2_2::relationTargetsOfType
: Implement IAccessible2_2::relationTargetsOfType
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All Windows 7
: -- normal (vote)
: mozilla27
Assigned To: alexander :surkov
:
: alexander :surkov
Mentors:
Depends on: IA2_1.3
Blocks: ia2
  Show dependency treegraph
 
Reported: 2013-05-17 05:42 PDT by alexander :surkov
Modified: 2013-10-25 03:00 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (7.19 KB, patch)
2013-10-03 08:45 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch2 (5.93 KB, patch)
2013-10-17 17:51 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2013-05-17 05:42:49 PDT
http://accessibility.linuxfoundation.org/a11yspecs/ia2/docs/html/interface_i_accessible2__2.html

should be a plain mapping into Accessible::RelationByType
Comment 1 alexander :surkov 2013-10-03 08:45:45 PDT
Created attachment 813545 [details] [diff] [review]
patch
Comment 2 alexander :surkov 2013-10-17 17:51:46 PDT
Created attachment 818796 [details] [diff] [review]
patch2
Comment 3 Trevor Saunders (:tbsaunde) 2013-10-22 10:32:42 PDT
Comment on attachment 818796 [details] [diff] [review]
patch2

>+  if (!Compatibility::IsIA2Off()) {

IA2Off is just a hack for really really ancient jaws so why don't we make it only effect IAccessible2 itself?

>+    if (IID_IAccessible2 == iid) {
>+      *ppv = static_cast<IAccessible2*>(this);
>+
>+    } else if (IID_IAccessible2_2 == iid) {

it seems like it would be better to check for the newer interface first

>+  bool matched = false;
>+  RelationType relationType;

why not use the mayBe<RelationType> trick?

>+  for (uint32_t idx = 0; idx < ArrayLength(sRelationTypePairs); idx++) {

size_t ?

>+    if (wcscmp(aType, sRelationTypePairs[idx].second) == 0) {
>+      relationType = sRelationTypePairs[idx].first;
>+      matched = true;

might as well break right?

>+  nsTArray<Accessible*> targets;
>+  Accessible* target = nullptr;
>+  while ((target = rel.Next()) &&
>+         static_cast<long>(targets.Length()) <= aMaxTargets)
>+    targets.AppendElement(target);

what happens if max targets is 0?

>+  for (int32_t i = 0; i < *aNTargets; i++) {

wouldn't long make more sense than int32_t? does it matter on win64 maybe?

>+  // IAccessible2_2
>+  virtual /* [propget] */ HRESULT STDMETHODCALLTYPE get_attribute(
>+    /* [in] */ BSTR name,
>+    /* [out, retval] */ VARIANT* attribute);

any reason to not make them override?
Comment 4 alexander :surkov 2013-10-22 11:40:09 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #3)

> >+  if (!Compatibility::IsIA2Off()) {
> 
> IA2Off is just a hack for really really ancient jaws so why don't we make it
> only effect IAccessible2 itself?

ok

> >+    if (IID_IAccessible2 == iid) {
> >+      *ppv = static_cast<IAccessible2*>(this);
> >+
> >+    } else if (IID_IAccessible2_2 == iid) {
> 
> it seems like it would be better to check for the newer interface first

hopefully this interface will picked up :)

> >+  bool matched = false;
> >+  RelationType relationType;
> 
> why not use the mayBe<RelationType> trick?

ok

> >+  for (uint32_t idx = 0; idx < ArrayLength(sRelationTypePairs); idx++) {
> 
> size_t ?

why?

> 
> >+    if (wcscmp(aType, sRelationTypePairs[idx].second) == 0) {
> >+      relationType = sRelationTypePairs[idx].first;
> >+      matched = true;
> 
> might as well break right?
> 
> >+  nsTArray<Accessible*> targets;
> >+  Accessible* target = nullptr;
> >+  while ((target = rel.Next()) &&
> >+         static_cast<long>(targets.Length()) <= aMaxTargets)
> >+    targets.AppendElement(target);
> 
> what happens if max targets is 0?

no targets returned

> >+  for (int32_t i = 0; i < *aNTargets; i++) {
> 
> wouldn't long make more sense than int32_t? does it matter on win64 maybe?

why?

> >+  // IAccessible2_2
> >+  virtual /* [propget] */ HRESULT STDMETHODCALLTYPE get_attribute(
> >+    /* [in] */ BSTR name,
> >+    /* [out, retval] */ VARIANT* attribute);
> 
> any reason to not make them override?

you mean MOZ_OVERRIDE? it's just a copy of what IDL generates, why to care?
Comment 6 Carsten Book [:Tomcat] 2013-10-25 03:00:22 PDT
https://hg.mozilla.org/mozilla-central/rev/98614ea8ce7f

Note You need to log in before you can comment on or make changes to this bug.