Last Comment Bug 865588 - tear off ISimpleDOMText
: tear off ISimpleDOMText
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla23
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-25 00:37 PDT by alexander :surkov
Modified: 2013-05-06 11:36 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (31.31 KB, patch)
2013-04-25 00:37 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2013-04-25 00:37:10 PDT
Created attachment 741712 [details] [diff] [review]
patch
Comment 1 Trevor Saunders (:tbsaunde) 2013-04-26 15:41:48 PDT
Comment on attachment 741712 [details] [diff] [review]
patch

>+++ b/accessible/src/windows/msaa/ServiceProvider.cpp
>@@ -18,19 +18,18 @@
> 
> #include "ISimpleDOMNode_i.c"
> 
> namespace mozilla {
> namespace a11y {
> 
> IMPL_IUNKNOWN_QUERY_HEAD(ServiceProvider)
>   IMPL_IUNKNOWN_QUERY_IFACE(IServiceProvider)
>-  return mAccessible->QueryInterface(aIID, aInstancePtr);
>-A11Y_TRYBLOCK_END
>-  }
>+IMPL_IUNKNOWN_QUERY_TAIL_AGGREGATED(mAccessible)

isn't it a bug that this returns the accessibles IUnknown not the one for IServiceProvider?

>+IMPL_IUNKNOWN_QUERY_HEAD(TextLeafAccessibleWrap)
>+  if (aIID == IID_ISimpleDOMText) {
>     statistics::ISimpleDOMUsed();
>-    *ppv = static_cast<ISimpleDOMText*>(this);
>-  } else {
>-    return AccessibleWrap::QueryInterface(iid, ppv);
>+    *aInstancePtr = static_cast<ISimpleDOMText*>(new sdnTextAccessible(this));

I don't think you need the cast since it only inherits from ISimpleDOMText

>+
>+/**
>+ * Implement ISimpleDOMText as a tear off.

funny wording for the description of a class maybe "Wrap TextLeafAccessible so we can expose ISimpleDOMText as a native  interface with a teer off" or something?

>+  nsCOMPtr<nsIDOMNode> DOMNode(do_QueryInterface(mAccessible->GetContent()));
>   DOMNode->GetNodeValue(nodeValue);
>   if (nodeValue.IsEmpty())I think there's a version of that method on nsINode now so it would be nice to file a bug or just get rid of the nsIDOMNode

>copy to accessible/src/windows/sdn/sdnTextAccessible.h
>--- a/accessible/src/windows/msaa/TextLeafAccessibleWrap.h
>+++ b/accessible/src/windows/sdn/sdnTextAccessible.h
>-#include "TextLeafAccessible.h"
>+//#include "TextLeafAccessible.h"
> #include "ISimpleDOMText.h"
>-#include "nsRect.h"
>+//#include "nsRect.h"

any reason for it?

>+#include "AccessibleWrap.h"

I guess you need that because of inline ctor / dtor and refptr? :(

>+  sdnTextAccessible(AccessibleWrap* aAccessible) : mAccessible(aAccessible) {};
>+  virtual ~sdnTextAccessible() {}

class is final so no need for it to be virtual

>+  nsRefPtr<AccessibleWrap> mAccessible;

I'm not really a fan, but holding onto content is probably hard, and its not really any worse than what we had.
Comment 2 alexander :surkov 2013-04-27 19:09:55 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #1)
 
> isn't it a bug that this returns the accessibles IUnknown not the one for
> IServiceProvider?

I checked and it goes with tear off implementation in ATL

> >-    return AccessibleWrap::QueryInterface(iid, ppv);
> >+    *aInstancePtr = static_cast<ISimpleDOMText*>(new sdnTextAccessible(this));
> 
> I don't think you need the cast since it only inherits from ISimpleDOMText

vt address is the same with the object address? what about IUnknown?

I added it to stay consistent and be on safe side.

> >+
> >+/**
> >+ * Implement ISimpleDOMText as a tear off.
> 
> funny wording for the description of a class maybe "Wrap TextLeafAccessible
> so we can expose ISimpleDOMText as a native  interface with a teer off" or
> something?

ok

> >+  nsCOMPtr<nsIDOMNode> DOMNode(do_QueryInterface(mAccessible->GetContent()));
> >   DOMNode->GetNodeValue(nodeValue);
> >   if (nodeValue.IsEmpty())I think there's a version of that method on nsINode now so it would be nice to file a bug or just get rid of the nsIDOMNode

a bug should be ok

> >+#include "AccessibleWrap.h"
> 
> I guess you need that because of inline ctor / dtor and refptr? :(

I guess so but since this header is included only in one file then maybe it doesn't matter.
> 
> >+  nsRefPtr<AccessibleWrap> mAccessible;
> 
> I'm not really a fan, but holding onto content is probably hard, and its not
> really any worse than what we had.

same as in IServiceProvider, I think to have a bug 678429 for this
Comment 4 Ryan VanderMeulen [:RyanVM] 2013-05-06 11:36:00 PDT
https://hg.mozilla.org/mozilla-central/rev/96e445cf42fb

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