917973 - tear off ISimpleDOMDocument

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

finishing tearoffing ISimpleDOM interfaces

Comment 1

[Trevor Saunders (:tbsaunde)]

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

>+IMPL_IUNKNOWN_QUERY_HEAD(DocAccessibleWrap)
>+  if (aIID == IID_ISimpleDOMDocument) {
>+    statistics::ISimpleDOMUsed();
>+    *aInstancePtr = static_cast<ISimpleDOMDocument*>(new sdnDocAccessible(this));
>+    AddRef();

you mean to AddRef the sdnDocAccessible right?

>-    virtual /* [id] */ HRESULT STDMETHODCALLTYPE put_alternateViewMediaTypes( 
>-        /* [in] */ BSTR __RPC_FAR *commaSeparatedMediaTypes);
>-
>-    // IAccessible
>+  // IAccessible

its useless why not rm it?

>copy to accessible/src/windows/sdn/sdnDocAccessible.cpp
>--- a/accessible/src/windows/msaa/DocAccessibleWrap.cpp
>+++ b/accessible/src/windows/sdn/sdnDocAccessible.cpp
>@@ -1,319 +1,180 @@
> /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

vim mode line too please

> 
> #include "nsIDocShell.h"
> #include "nsIDocShellTreeNode.h"
> #include "nsIInterfaceRequestorUtils.h"
> #include "nsISelectionController.h"
> #include "nsIServiceManager.h"
> #include "nsIURI.h"
> #include "nsViewManager.h"
> #include "nsIWebNavigation.h"

do you actually need all these?

>+sdnDocAccessible::~sdnDocAccessible() { }

why not inline it? (or actually I think you could let the compiler generate it for you.

>copy to accessible/src/windows/sdn/sdnDocAccessible.h
>--- a/accessible/src/windows/msaa/DocAccessibleWrap.h
>+++ b/accessible/src/windows/sdn/sdnDocAccessible.h
>+class sdnDocAccessible : public ISimpleDOMDocument

MOZ_FINAL?

> {
> public:
>-  DocAccessibleWrap(nsIDocument* aDocument, nsIContent* aRootContent,
>-                    nsIPresShell* aPresShell);
>-  virtual ~DocAccessibleWrap();

I don't think it needs to be virtual

Comment 2

[alexander :surkov (:asurkov)]

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

> >+    *aInstancePtr = static_cast<ISimpleDOMDocument*>(new sdnDocAccessible(this));
> >+    AddRef();
> 
> you mean to AddRef the sdnDocAccessible right?

right, copy/paste, sorry

> >-    // IAccessible
> >+  // IAccessible
> 
> its useless why not rm it?

a common pattern

> > /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> 
> vim mode line too please

k

> > #include "nsIWebNavigation.h"
> 
> do you actually need all these?

no, I don't

> >+sdnDocAccessible::~sdnDocAccessible() { }
> 
> why not inline it? (or actually I think you could let the compiler generate
> it for you.

inlined

> >+class sdnDocAccessible : public ISimpleDOMDocument
> 
> MOZ_FINAL?

ok

> > 
> >-  virtual ~DocAccessibleWrap();
> 
> I don't think it needs to be virtual

they all in inheritance chain are virtual, why this one shouldnt' be?

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 4

[Trevor Saunders (:tbsaunde)]

> > >-    // IAccessible
> > >+  // IAccessible
> > 
> > its useless why not rm it?
> 
> a common pattern

sure, but the section is empty the next thing is another such comment.

> > > 
> > >-  virtual ~DocAccessibleWrap();
> > 
> > I don't think it needs to be virtual
> 
> they all in inheritance chain are virtual, why this one shouldnt' be?

what inheritance chain?  iirc interface classes let the compiler autogenerate they're dtor, so its not virtual and that's all this inherits from.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> > > >-    // IAccessible
> > > >+  // IAccessible
> > > 
> > > its useless why not rm it?
> > 
> > a common pattern
> 
> sure, but the section is empty the next thing is another such comment.

it's not really empty, it has get_accValue

> > > > 
> > > >-  virtual ~DocAccessibleWrap();
> > > 
> > > I don't think it needs to be virtual
> > 
> > they all in inheritance chain are virtual, why this one shouldnt' be?
> 
> what inheritance chain? 

of DocAccessibleWrap class

> iirc interface classes let the compiler
> autogenerate they're dtor, so its not virtual and that's all this inherits
> from.

I must be misssing something, if this one wasn't virtual then how would it be called if I destroy docacc instance having Accessible* pointer.

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #5)
> (In reply to Trevor Saunders (:tbsaunde) from comment #4)
> > > > >-    // IAccessible
> > > > >+  // IAccessible
> > > > 
> > > > its useless why not rm it?
> > > 
> > > a common pattern
> > 
> > sure, but the section is empty the next thing is another such comment.
> 
> it's not really empty, it has get_accValue

oh, yeah ok

> > > > > 
> > > > >-  virtual ~DocAccessibleWrap();
> > > > 
> > > > I don't think it needs to be virtual
> > > 
> > > they all in inheritance chain are virtual, why this one shouldnt' be?
> > 
> > what inheritance chain? 
> 
> of DocAccessibleWrap class
> 
> > iirc interface classes let the compiler
> > autogenerate they're dtor, so its not virtual and that's all this inherits
> > from.
> 
> I must be misssing something, if this one wasn't virtual then how would it
> be called if I destroy docacc instance having Accessible* pointer.

err, quoted wrong part of diff, I meant to get the + virtual ~sdnDocAccessible() bit

Comment 7

[alexander :surkov (:asurkov)]

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

> err, quoted wrong part of diff, I meant to get the + virtual
> ~sdnDocAccessible() bit

got it, will remove 'virtual' keyword

Comment 8

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/57f32113d785

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/57f32113d785