952782 - Use nsIDocument instead of nsIDOMDocument in accessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 1

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8350931 [details] [diff] [review]
Patch

>From: David Zbarsky <[e-mail]>
>
>diff --git a/accessible/src/base/nsCoreUtils.cpp b/accessible/src/base/nsCoreUtils.cpp
>--- a/accessible/src/base/nsCoreUtils.cpp
>+++ b/accessible/src/base/nsCoreUtils.cpp
>@@ -5,17 +5,16 @@
> 
> #include "nsCoreUtils.h"
> 
> #include "nsIAccessibleTypes.h"
> 
> #include "nsIBaseWindow.h"
> #include "nsIDocShellTreeOwner.h"
> #include "nsIDocument.h"
>-#include "nsIDOMDocument.h"
> #include "nsIDOMHTMLDocument.h"
> #include "nsIDOMHTMLElement.h"
> #include "nsRange.h"
> #include "nsIBoxObject.h"
> #include "nsIDOMXULElement.h"
> #include "nsIDocShell.h"
> #include "nsEventListenerManager.h"
> #include "nsIPresShell.h"
>@@ -211,28 +210,26 @@ nsCoreUtils::GetDOMNodeFromDOMPoint(nsIN
>   return aNode;
> }
> 
> nsIContent*
> nsCoreUtils::GetRoleContent(nsINode *aNode)
> {
>   nsCOMPtr<nsIContent> content(do_QueryInterface(aNode));
>   if (!content) {
>-    nsCOMPtr<nsIDOMDocument> domDoc(do_QueryInterface(aNode));
>-    if (domDoc) {
>+    nsCOMPtr<nsIDocument> doc(do_QueryInterface(aNode));
>+    if (doc) {
>       nsCOMPtr<nsIDOMHTMLDocument> htmlDoc(do_QueryInterface(aNode));

I guess you could use AsHTMLDocument() here

>       else {
>-        nsCOMPtr<nsIDOMElement> docElement;
>-        domDoc->GetDocumentElement(getter_AddRefs(docElement));
>-        content = do_QueryInterface(docElement);
>+        content = doc->GetDocumentElement();

return doc->GetDocumentElement() would save AddRef / Release right?

> Accessible::GetFirstAvailableAccessible(nsINode *aStartNode) const
>+  nsCOMPtr<nsIDocument> doc = aStartNode->OwnerDoc();
>+  NS_ENSURE_TRUE(doc, nullptr);

that can never fail right?

>+
>+  nsCOMPtr<nsINode> currentNode = aStartNode;

seems like we don't need to hold a ref here.

>+  walker->SetCurrentNode(*currentNode, rv);
>+  if (rv.Failed()) {
>+    return nullptr;
>+  }

local style seems to be not bracing single line ifs so please do that.

>+++ b/accessible/src/generic/RootAccessible.cpp
>@@ -79,19 +79,19 @@ RootAccessible::Name(nsString& aName)
>+  if (mDocumentNode) {
>+    mDocumentNode->GetTitle(aName);
>+  }

mDocument should never be null here.

>diff --git a/content/base/src/moz.build b/content/base/src/moz.build
>--- a/content/base/src/moz.build
>+++ b/content/base/src/moz.build
>@@ -12,30 +12,32 @@ EXPORTS += [

Does some other header include these or why do you need to change them?  In any case I'm probably not the person to say exporting them is ok.


sorry about the long lag :(  I'd like to see next version, but thanks for doing this.

Comment 2

[David Zbarsky (:dzbarsky)]

(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> Comment on attachment 8350931 [details] [diff] [review]
> Patch
> 
> 
> return doc->GetDocumentElement() would save AddRef / Release right?
> 

Yep, changed.

> > Accessible::GetFirstAvailableAccessible(nsINode *aStartNode) const
> >+  nsCOMPtr<nsIDocument> doc = aStartNode->OwnerDoc();
> >+  NS_ENSURE_TRUE(doc, nullptr);
> 
> that can never fail right?

Yep.

> 
> >+
> >+  nsCOMPtr<nsINode> currentNode = aStartNode;
> 
> seems like we don't need to hold a ref here.

I left it like that to be safe but I guess it doesn't matter.  I'll tryserver to be sure.


> 
> >diff --git a/content/base/src/moz.build b/content/base/src/moz.build
> >--- a/content/base/src/moz.build
> >+++ b/content/base/src/moz.build
> >@@ -12,30 +12,32 @@ EXPORTS += [
> 
> Does some other header include these or why do you need to change them?  In
> any case I'm probably not the person to say exporting them is ok.
> 

nsIDocument.h includes these.

Comment 3

[David Zbarsky (:dzbarsky)]

(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> Comment on attachment 8350931 [details] [diff] [review]
> Patch
> 
> 
> I guess you could use AsHTMLDocument() here
> 

That requires exporting nsHTMLDocument.h and nsDocument.h and possibly other things so let's not do that for now.

> 
> >+
> >+  nsCOMPtr<nsINode> currentNode = aStartNode;
> 
> seems like we don't need to hold a ref here.
> 

We actually need an nsCOMPtr<nsINode> local because GetNextNode() returns already_AddRefed.

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 5

[David Zbarsky (:dzbarsky)]

Comment on attachment 8361188 [details] [diff] [review]
Patch

Bz, can you sign off on the change to exports?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8361188 [details] [diff] [review]
Patch

Seems ok to me...

Comment 7

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8361188 [details] [diff] [review]
Patch

I don't even have an execuse for talking forever this time :(

Comment 8

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92e6323c830f

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/92e6323c830f