Closed Bug 1301148 Opened 3 years ago Closed 3 years ago

Implement ia2AccessibleHyperlink methods in ProxyAccessible for Windows

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: michael.li11702, Assigned: michael.li11702)

References

Details

Attachments

(8 files, 5 obsolete files)

3.96 KB, patch
michael.li11702
: review+
Details | Diff | Splinter Review
3.49 KB, patch
michael.li11702
: review+
Details | Diff | Splinter Review
2.90 KB, patch
michael.li11702
: review+
Details | Diff | Splinter Review
2.14 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
9.18 KB, patch
aklotz
: review+
mshal
: review+
Details | Diff | Splinter Review
3.98 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
3.15 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
994 bytes, patch
aklotz
: review+
Details | Diff | Splinter Review
No description provided.
Blocks: 1288839
Assignee: nobody → mili
Attachment #8789067 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8789067 [details] [diff] [review]
Implement 3 ia2AccessibleHyperlink methods: StartOffset, EndOffset, and IsLinkValid

>+  RefPtr<IAccessible> acc;
>+  if (!GetCOMInterface((void**)getter_AddRefs(acc))) {
>+    return 0;
>+  }
>+
>+  RefPtr<IAccessibleHyperlink> acc2;
>+  if (FAILED(acc->QueryInterface(IID_IAccessibleHyperlink, (void**)getter_AddRefs(acc2)))) {
>+    return 0;
>+  }

I think you are going to end up repeating that a lot.  It might be nice to have a template function that QueryInterface()s to the right type after calling GetCOMInterface() you need a mapping from type to iid, and I'm not sure one is provided, so you can do something like this.

template<typename I> struct InterfaceIID {};

template struct InterfaceIID<IAccessibleHyperlink> { static const IIDTYPE value = IID_IAccessibleHyperlink; };
...


>+  /**
>+   * Return the start offset for the proxied accessible.
>+   */
>+  long StartOffset(bool* aOk);

It would be good for this to be the same exact interface as ProxyAccessible for other platforms I think.  Infact it might not be a bad idea to have a shared header declare a bunch of methods.
Attachment #8789067 - Flags: review?(tbsaunde+mozbugs)
StartOffset, EndOffset, and IsLinkValid

MozReview-Commit-ID: 3wUyQkH9yRP
Attachment #8790421 - Flags: review?(tbsaunde+mozbugs)
Attachment #8789067 - Attachment is obsolete: true
Comment on attachment 8790421 [details] [diff] [review]
Implement an IAccessible to REFIID mapping and 3 ia2AccessibleHyperlink methods

>+template<typename I>

Interface might be a better name.

>+HRESULT ProxyAccessible::QueryInterface(RefPtr<IAccessible> aIAccessible,

return type goes on its own line.  I don't really see a reason to return a hresult, it should be fine to just return already_AddRefed<Interface> which should help hide the casting stuff here.

Since you always pass the result of GetCOMInterface to this function it would seem to make sense for it to do that part too.

> void
> ProxyAccessible::Attributes(nsTArray<Attribute>* aAttrs) const
> {
>   aAttrs->Clear();
> 
>   RefPtr<IAccessible> acc;
>   if (!GetCOMInterface((void**)getter_AddRefs(acc))) {
>     return;
>   }
> 
>   RefPtr<IAccessible2> acc2;
>-  if (FAILED(acc->QueryInterface(IID_IAccessible2, (void**)getter_AddRefs(acc2)))) {
>+  if (FAILED(QueryInterface<IAccessible2>(acc, (void**)getter_AddRefs(acc2)))) {

it would be good to separate the cleanup part of this into its own patch.  I suspect there are more places you can clean up too.

>+  /**
>+   * Specializations of this template map an IAccessible type to its REFIID

s/REFIID/IID/

>+   * QueryInterface an IAccessible using its correct REFIID

Needs to be updated if you change the above, probably say something like get the com proxy for this proxy accessible and QueryInterface it to the given type.

>+   */
>+  template<typename I>
>+  HRESULT ProxyAccessible::QueryInterface(RefPtr<IAccessible> aIAccessible,

ProxyAccessible:: shouldn't be needed.
Attachment #8790421 - Flags: review?(tbsaunde+mozbugs)
Attachment #8791423 - Flags: review?(tbsaunde+mozbugs)
Attachment #8790421 - Attachment is obsolete: true
StartOffset, EndOffset, and IsLinkValid
Attachment #8791424 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8791423 [details] [diff] [review]
Setup template for IAccessible to IID mapping and apply to IAccessible2

># HG changeset patch
># User Michael Li <michael.li11702@gmail.com>
>
>bug 1301148 - Setup template for IAccessible to IID mapping and
>
>apply to IAccessible2. r?tbsaunde

you could just say simplify getting a IAccessible2* for a ProxyAccessible.

>+/**
>+ * Template specializations to map an IAccessible type to its REFIID
>+ */
>+template<>
>+struct ProxyAccessible::InterfaceIID<IAccessible2>
>+{
>+  static REFIID Value()
>+  { return IID_IAccessible2; }
>+};
>+
>+template<>
>+struct ProxyAccessible::InterfaceIID<IAccessibleHyperlink>
>+{
>+  static REFIID Value()
>+  { return IID_IAccessibleHyperlink; }
>+};

it would be better if you added this one in the patch using it, but whatever I guess.

>+
>+template<typename Interface>
>+already_AddRefed<Interface>
>+QueryInterface(const ProxyAccessible* aProxy)

might as well be static I guess.

>+   * Specializations of this template map an IAccessible type to its IID
>+   */
>+  template<typename Interface>
>+  struct InterfaceIID {};

I don't see any reason to export this, especially since its not useful without the specializations.  So you might as well put in ProxyAccessible.cpp

>+ */
>+template<typename Interface>
>+already_AddRefed<Interface>
>+QueryInterface(const ProxyAccessible* aProxy);

might as well leave it static to ProxyAccessible.cpp
Attachment #8791423 - Flags: review?(tbsaunde+mozbugs) → review+
Comment on attachment 8791424 [details] [diff] [review]
Implement 3 ia2AccessibleHyperlink methods

># HG changeset patch
># User Michael Li <michael.li11702@gmail.com>
>
>bug 1301148 - Implement 3 ia2AccessibleHyperlink methods. r?tbsaunde
>
>StartOffset, EndOffset, and IsLinkValid

no real reason to specify which ones, if somebody cares to know what you did they can read the patch.
Attachment #8791424 - Flags: review?(tbsaunde+mozbugs) → review+
Attachment #8791423 - Attachment is obsolete: true
Attachment #8791746 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8791746 [details] [diff] [review]
Implement AnchorAt and conversion from IUnknown to ProxyAccessible

># HG changeset patch
># User Michael Li <michael.li11702@gmail.com>
>
>bug 1301148 - Implement AnchorAt and conversion from IUnknown to ProxyAccessible. r?tbsaunde
>
>diff --git a/accessible/ipc/win/ProxyAccessible.cpp b/accessible/ipc/win/ProxyAccessible.cpp
>index 95588dc..3ca9b11 100644
>--- a/accessible/ipc/win/ProxyAccessible.cpp
>+++ b/accessible/ipc/win/ProxyAccessible.cpp
>@@ -357,10 +357,45 @@ ProxyAccessible::IsLinkValid()
>   boolean isValid = false;
>   HRESULT hr = acc->get_valid(&isValid);
>   if (FAILED(hr)) {
>     return false;
>   }
>   return static_cast<bool>(isValid);
> }
> 
>+ProxyAccessible*
>+ProxyAccessible::AnchorAt(uint32_t aIndex)
>+{
>+  RefPtr<IAccessibleHyperlink> acc = QueryInterface<IAccessibleHyperlink>(this);
>+  if (!acc) {
>+    return nullptr;
>+  }
>+
>+  VARIANT anchor;
>+  HRESULT hr = acc->get_anchor(aIndex, &anchor);
>+  if (FAILED(hr)) {
>+    return nullptr;
>+  }
>+
>+  return ConvertIUnknownToProxy(*anchor.ppunkVal);

you need to check the variant holds a IUnknown.

>+}
>+
>+ProxyAccessible*
>+ProxyAccessible::ConvertIUnknownToProxy(IUnknown* aIUnknown)
>+{
>+  RefPtr<IAccessible2> acc;
>+  if (FAILED(aIUnknown->QueryInterface(InterfaceIID<IAccessible2>::Value(),

might as well just use IID_IAccessible2
Attachment #8791746 - Flags: review?(tbsaunde+mozbugs) → review+
Rebased with the shared header patch from bug 1303040
Attachment #8792093 - Flags: review+
Attachment #8791424 - Attachment is obsolete: true
Fixed stuff from comment 11, rebased with shared header patch from bug 1303040,
deleted #ifdef from xpcAccessibleHyperlink::GetAnchor
Attachment #8792094 - Flags: review+
Attachment #8791746 - Attachment is obsolete: true
Depends on: 1303040
Attachment #8811134 - Flags: review?(mshal)
Attachment #8811134 - Flags: review?(aklotz)
Comment on attachment 8811134 [details] [diff] [review]
add custom gecko interface

>diff --git a/accessible/interfaces/gecko/Makefile.in b/accessible/interfaces/gecko/Makefile.in
>new file mode 100644
>index 0000000..24343dd
>--- /dev/null
>+++ b/accessible/interfaces/gecko/Makefile.in
>@@ -0,0 +1,31 @@
>+# This Source Code Form is subject to the terms of the Mozilla Public
>+# License, v. 2.0. If a copy of the MPL was not distributed with this
>+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
>+
>+GARBAGE += $(MIDL_GENERATED_FILES) done_gen dlldata.c
>+
>+MIDL_GENERATED_FILES = \
>+	IGeckoCustom.h \
>+	IGeckoCustom_p.c \
>+	IGeckoCustom_i.c \
>+	$(NULL)
>+
>+$(MIDL_GENERATED_FILES): done_gen
>+
>+done_gen: IGeckoCustom.idl
>+	$(MIDL) $(MIDL_FLAGS) -I $(srcdir) -Oicf $(srcdir)/IGeckoCustom.idl
>+	touch $@
>+
>+export:: done_gen
>+
>+EMBED_MANIFEST_AT = 2
>+
>+midl_exports := \
>+    IGeckoCustom.h \
>+    IGeckoCustom_i.c \
>+    $(NULL)
>+
>+INSTALL_TARGETS += midl_exports
>+midl_exports_FILES := $(midl_exports)
>+midl_exports_DEST = $(DIST)/include
>+midl_exports_TARGET := export

Wish we had bug 1256013! The midl_exports stuff can probably just be EXPORTS in moz.build, but since the other midl-Makefile.ins do this it's something we'd have to address when midl is ported anyway.

>diff --git a/toolkit/library/moz.build b/toolkit/library/moz.build
>index d73fbdf..92e9f58 100644
>--- a/toolkit/library/moz.build
>+++ b/toolkit/library/moz.build
>@@ -44,16 +44,17 @@ def Libxul(name):
>     if CONFIG['OS_ARCH'] == 'WINNT':
>         DELAYLOAD_DLLS += [
>             'api-ms-win-core-winrt-l1-1-0.dll',
>             'api-ms-win-core-winrt-string-l1-1-0.dll',
>         ]
> 
>     if CONFIG['OS_ARCH'] == 'WINNT' and not CONFIG['GNU_CC']:
>         LOCAL_INCLUDES += [
>+            '!/accessible/interfaces/gecko/',

Why is this needed? It looks like the .h files are already exported to dist/include, so I don't see why you should require an include directory here. Can you just remove this line? Or do additional headers need to be exported?

Note I did not review the contents of the .cpp / .h files, only the build system changes.
Attachment #8811134 - Flags: review?(mshal) → review+
Attachment #8811133 - Flags: review?(aklotz) → review+
Comment on attachment 8811134 [details] [diff] [review]
add custom gecko interface

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

r=me with comments addressed.

::: accessible/interfaces/gecko/IGeckoCustom.idl
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Nit: Can you add the vim modeline here too?
/me is a vim user ;-)

::: accessible/interfaces/gecko/Makefile.in
@@ +17,5 @@
> +	touch $@
> +
> +export:: done_gen
> +
> +EMBED_MANIFEST_AT = 2

You can remove EMBED_MANIFEST_AT: we don't want that in the case of xul.dll.

::: accessible/windows/msaa/AccessibleWrap.cpp
@@ +160,5 @@
>        return hr;
>    }
>  
> +  if (!*ppv && iid == IID_IGeckoCustom) {
> +    RefPtr<GeckoCustom> gkCrap = new GeckoCustom(this);

LOL @ gkCrap :-)

::: accessible/windows/msaa/GeckoCustom.cpp
@@ +9,5 @@
> +using namespace mozilla;
> +using namespace mozilla::a11y;
> +
> +////////////////////////////////////////////////////////////////////////////////
> +// ChildrenEnumVariant

You probably want to remove the copypasta from this comment ;-)

::: accessible/windows/msaa/GeckoCustom.h
@@ +20,5 @@
> + */
> +class GeckoCustom final : public IGeckoCustom
> +{
> +public:
> +  GeckoCustom(AccessibleWrap* aAcc) : mAcc(aAcc) {}

explicit keyword please

::: toolkit/library/moz.build
@@ +48,5 @@
>          ]
>  
>      if CONFIG['OS_ARCH'] == 'WINNT' and not CONFIG['GNU_CC']:
>          LOCAL_INCLUDES += [
> +            '!/accessible/interfaces/gecko/',

You probably want to make this conditional on CONFIG['ACCESSIBILITY'].

::: toolkit/library/xulrunner.rc
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "widget.rc"
> +1 typelib IGeckoCustom.tlb

You probably want to #ifdef ACCESSIBILITY around this line.
Attachment #8811134 - Flags: review?(aklotz) → review+
Comment on attachment 8811135 [details] [diff] [review]
implement anchor count for windows

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

::: accessible/windows/msaa/GeckoCustom.cpp
@@ +16,5 @@
>  IMPL_IUNKNOWN_QUERY_HEAD(GeckoCustom)
>  IMPL_IUNKNOWN_QUERY_IFACE(IGeckoCustom)
>  IMPL_IUNKNOWN_QUERY_TAIL_AGGREGATED(mAcc)
> +
> +STDMETHODIMP

When defining a STDMETHODIMP function, just use HRESULT as the return type.

::: accessible/windows/msaa/GeckoCustom.h
@@ +25,5 @@
>  
>    // IUnknown
>    DECL_IUNKNOWN
>  
> +    virtual HRESULT STDMETHODCALLTYPE get_anchorCount(long* aCount);

Nit: indentation

Typically HRESULT STDMETHDOCALLTYPE is just declared as STDMETHODIMP.
Attachment #8811135 - Flags: review?(aklotz) → review+
Comment on attachment 8811136 [details] [diff] [review]
allow getting a proxy accessible from a com proxy

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

::: accessible/ipc/win/ProxyAccessible.cpp
@@ +107,5 @@
> +{
> +  RefPtr<IGeckoCustom> custom;
> +  if (FAILED(aCOMProxy->QueryInterface(IID_IGeckoCustom,
> +                                       (void**) getter_AddRefs(custom)))) {
> +      return nullptr;

Your indentation is messed up beginning at this line and continuing to the end of the function.
Attachment #8811136 - Flags: review?(aklotz) → review+
Attachment #8811137 - Flags: review?(aklotz) → review+
Comment on attachment 8811134 [details] [diff] [review]
add custom gecko interface

>+INSTALL_TARGETS += midl_exports
>+midl_exports_FILES := $(midl_exports)
>+midl_exports_DEST = $(DIST)/include
>+midl_exports_TARGET := export

FYI this construction may change slightly in bug 1319350.
Blocks: 1269369
Pushed by tsaunders@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f805db8e59e
implement easy parts of nsIAccessibleHyperlink r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fb5cdbd3d10
add custom gecko interface r=aklotz, mshal
https://hg.mozilla.org/integration/mozilla-inbound/rev/01d2197f9141
implement anchor count for windows r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0d9db0f5ca1
allow getting a proxy accessible from a com proxy r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/005ae596c1e9
implement anchor at for windows r=aklotz
Depends on: 1329697
You need to log in before you can comment on or make changes to this bug.