tear off ISimpleDOMDocument

RESOLVED FIXED in mozilla27

Status

()

Core
Disability Access APIs
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

({access})

unspecified
mozilla27
x86
Windows 7
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

24.75 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
Created attachment 806807 [details] [diff] [review]
patch

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
(Assignee)

Comment 2

4 years ago
(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?
(Assignee)

Comment 3

4 years ago
Created attachment 807224 [details] [diff] [review]
patch2
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.
(Assignee)

Comment 5

4 years ago
(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
(Assignee)

Comment 7

4 years ago
(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+
(Assignee)

Comment 8

4 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/57f32113d785
https://hg.mozilla.org/mozilla-central/rev/57f32113d785
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.