Closed Bug 952782 Opened 6 years ago Closed 6 years ago

Use nsIDocument instead of nsIDOMDocument in accessible

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
No description provided.
Attachment #8350931 - Attachment is patch: true
Attachment #8350931 - Attachment mime type: message/rfc822 → text/plain
Attachment #8350931 - Flags: review?(trev.saunders)
Comment on attachment 8350931 [details] [diff] [review]
Patch

>From: David Zbarsky <dzbarsky@gmail.com>
>
>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.
Attachment #8350931 - Flags: review?(trev.saunders)
(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.
(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.
Attached patch PatchSplinter Review
Assignee: nobody → dzbarsky
Attachment #8350931 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8361188 - Flags: review?(trev.saunders)
Comment on attachment 8361188 [details] [diff] [review]
Patch

Bz, can you sign off on the change to exports?
Attachment #8361188 - Flags: review?(bzbarsky)
Comment on attachment 8361188 [details] [diff] [review]
Patch

Seems ok to me...
Attachment #8361188 - Flags: review?(bzbarsky) → review+
Comment on attachment 8361188 [details] [diff] [review]
Patch

I don't even have an execuse for talking forever this time :(
Attachment #8361188 - Flags: review?(trev.saunders) → review+
https://hg.mozilla.org/mozilla-central/rev/92e6323c830f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.