865559 - get rid of nsAccessNodeWrap

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[Trevor Saunders (:tbsaunde)]

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

>+++ b/accessible/src/windows/msaa/nsWinUtils.h
>+#include "DocAccessible.h"

forward decl isn't good enough for the hash table? then why don't we make it file static (we should make it pointer to hash table too to get rid of static constructor but no need for that to happen today)

I think I'd prefer that the exception filtering thing and the result converter didn't live in AccessibleWrap could we have MSCOM.h for them and the IUnnown stuff?

Comment 2

[alexander :surkov (:asurkov)]

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

> I think I'd prefer that the exception filtering thing and the result
> converter didn't live in AccessibleWrap could we have MSCOM.h for them and
> the IUnnown stuff?

maybe put then into nsWinUtils.h file?

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #2)
> (In reply to Trevor Saunders (:tbsaunde) from comment #1)
> 
> > I think I'd prefer that the exception filtering thing and the result
> > converter didn't live in AccessibleWrap could we have MSCOM.h for them and
> > the IUnnown stuff?
> 
> maybe put then into nsWinUtils.h file?

I'd rather not drag all of nsWinUtils.h in everywhere, but if you need the SEH stuff you probably need the IUnknown stuff as well.

Comment 4

[alexander :surkov (:asurkov)]

I would avoid to introduce more files if they are 10 lines long.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> I would avoid to introduce more files if they are 10 lines long.

that's why I suggest to share file with IUnknown stuff :)

Comment 6

[alexander :surkov (:asurkov)]

so both exception and GetHResult -> IUnknownImpl.h?

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> so both exception and GetHResult -> IUnknownImpl.h?

sgtm

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> (In reply to alexander :surkov from comment #6)
> > so both exception and GetHResult -> IUnknownImpl.h?
> 
> sgtm

it'd be good if you talked in russian :) it made me to google it

fine with me

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 10

[Trevor Saunders (:tbsaunde)]

Comment on attachment 743965 [details] [diff] [review]
patch2

>+AccessibleWrap::FilterExceptions(unsigned int aCode,
>+                                 EXCEPTION_POINTERS* aExceptionInfo)
>+{
>+  if (aCode == EXCEPTION_ACCESS_VIOLATION) {
>+#ifdef MOZ_CRASHREPORTER
>+    // MSAA swallows crashes (because it is COM-based) but we still need to
>+    // learn about those crashes so we can fix them. Make sure to pass them to
>+    // the crash reporter.
>+    nsCOMPtr<nsICrashReporter> crashReporter =
>+      do_GetService("@mozilla.org/toolkit/crash-reporter;1");
>+    if (crashReporter)
>+      crashReporter->WriteMinidumpForException(aExceptionInfo);

you could file a bug or just convert this to crashreporter::WriteMinidumpForException() I think.  If you do that maybe we should stop disabling that msvc warning maybe?

>+++ b/accessible/src/windows/msaa/AccessibleWrap.h
>@@ -9,32 +9,55 @@
> 
> #include "nsCOMPtr.h"
> #include "Accessible.h"
> #include "Accessible2.h"
> #include "ia2AccessibleComponent.h"
> #include "ia2AccessibleHyperlink.h"
> #include "ia2AccessibleValue.h"
> 
>+#include "oleidl.h"
>+#include "oleacc.h"
>+#include <winuser.h>

do you actually need to include all of them here?  I have no idea

>+#ifdef MOZ_CRASHREPORTER
>+#include "nsICrashReporter.h"
>+#endif

include it in IUnknownImpl.h since that's what may need it?

>+  /**
>+   * Used to pass an exception to the crash reporter.
>+   */
>+  static int FilterExceptions(unsigned int aCode,
>+                              EXCEPTION_POINTERS* aExceptionInfo);

I thought you were going to move this to IUnknownImpl.h ?  I guess we need to include this almost everywhere anyway...

>+/**
>+ * Wrap every method body by these macroses to pass exception to the crash
>+ * reporter.

s/macroses/macros/

>+/**
>+ * Converts nsresult to HRESULT.
>+ */
>+HRESULT
>+GetHRESULT(nsresult aResult)

seems kind of large to put in a header, you might also have issues with multiple definitions since its in a header but not static.

Comment 11

[alexander :surkov (:asurkov)]

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

> 
> >+/**
> >+ * Converts nsresult to HRESULT.
> >+ */
> >+HRESULT
> >+GetHRESULT(nsresult aResult)
> 
> seems kind of large to put in a header, you might also have issues with
> multiple definitions since its in a header but not static.

I wouldn't really want to create a separate file for small function

Comment 12

[alexander :surkov (:asurkov)]

1) added IUnknownImpl.cpp
2) addressed comment #10

https://hg.mozilla.org/integration/mozilla-inbound/rev/8eccffccac1c

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8eccffccac1c

Comment 14

[Trevor Saunders (:tbsaunde)]

follow up remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9362f88a5651

Comment 15

[Kent James (:rkent)]

This patch is causing a build failure in my Thunderbird debug builds (VS2008, crash reported disabled):

c:/tb/2-central/src/mozilla/accessible/src/windows/msaa/IUnknownImpl.cpp(49) : error C3861: 'NS_NOTREACHED': identifier not found

I think you need to include nsDebug.h somewhere

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to Kent James (:rkent) from comment #15)
> This patch is causing a build failure in my Thunderbird debug builds
> (VS2008, crash reported disabled):
> 
> c:/tb/2-central/src/mozilla/accessible/src/windows/msaa/IUnknownImpl.cpp(49)
> : error C3861: 'NS_NOTREACHED': identifier not found
> 
> I think you need to include nsDebug.h somewhere

should be fixed by patch in comment 14

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9362f88a5651