Last Comment Bug 917973 - tear off ISimpleDOMDocument
: tear off ISimpleDOMDocument
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Windows 7
-- normal (vote)
: mozilla27
Assigned To: alexander :surkov
:
: alexander :surkov
Mentors:
Depends on: 767756
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-18 11:31 PDT by alexander :surkov
Modified: 2013-09-20 19:42 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (23.66 KB, patch)
2013-09-18 11:31 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch2 (24.75 KB, patch)
2013-09-19 08:22 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description User image alexander :surkov 2013-09-18 11:31:49 PDT
Created attachment 806807 [details] [diff] [review]
patch

finishing tearoffing ISimpleDOM interfaces
Comment 1 User image Trevor Saunders (:tbsaunde) 2013-09-18 14:43:36 PDT
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 User image alexander :surkov 2013-09-19 08:12:07 PDT
(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 User image alexander :surkov 2013-09-19 08:22:49 PDT
Created attachment 807224 [details] [diff] [review]
patch2
Comment 4 User image Trevor Saunders (:tbsaunde) 2013-09-19 08:26:57 PDT
> > >-    // 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 User image alexander :surkov 2013-09-19 08:32:05 PDT
(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 User image Trevor Saunders (:tbsaunde) 2013-09-19 08:46:14 PDT
(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 User image alexander :surkov 2013-09-19 09:57:08 PDT
(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 9 User image Wes Kocher (:KWierso) 2013-09-20 19:42:17 PDT
https://hg.mozilla.org/mozilla-central/rev/57f32113d785

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