Closed Bug 917973 Opened 9 years ago Closed 9 years ago

tear off ISimpleDOMDocument

Categories

(Core :: Disability Access APIs, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
finishing tearoffing ISimpleDOM interfaces
Attachment #806807 - Flags: review?(trev.saunders)
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
(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?
Attached patch patch2Splinter Review
Attachment #806807 - Attachment is obsolete: true
Attachment #806807 - Flags: review?(trev.saunders)
Attachment #807224 - Flags: review?(trev.saunders)
> > >-    // 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.
(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.
(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
(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
Attachment #807224 - Flags: review?(trev.saunders) → review+
https://hg.mozilla.org/mozilla-central/rev/57f32113d785
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.