739130 - make GetDocumentNode() inline on nsDocAccessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

1. remove the definition of GetDocumentNode() on nsAccessNode (accessible/src/base/nsAccessNode.h)
2. rename GetDocumentNode() on nsDocAccessible (accessible/src/base/nsDocAccessible.h) to ContentDocument() and mark it as inline instead of virtual.
3. change the callers, to call ContentDocument(), and make sure they have a nsDocAccessible* instead of a nsAccessible* so the function can be called.

Comment 1

[alexander :surkov (:asurkov)]

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

> 2. rename GetDocumentNode() on nsDocAccessible
> (accessible/src/base/nsDocAccessible.h) to ContentDocument() and mark it as
> inline instead of virtual.

I think I like DocumentNode(). Content, Node and Element terms are used for DOM in a11y code. Document is referred to a11y document. Maybe confusing but anyway. Having a ContentDocument makes me think it's a Document and therefore it's a11y object. Having a DocumentContent is incorrect because DOM Document is not a Content object. So we have DocumentNode in the rest.

Comment 2

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #1)
> (In reply to Trevor Saunders (:tbsaunde) from comment #0)
> 
> > 2. rename GetDocumentNode() on nsDocAccessible
> > (accessible/src/base/nsDocAccessible.h) to ContentDocument() and mark it as
> > inline instead of virtual.
> 
> I think I like DocumentNode(). Content, Node and Element terms are used for
> DOM in a11y code. Document is referred to a11y document. Maybe confusing but
> anyway. Having a ContentDocument makes me think it's a Document and
> therefore it's a11y object. Having a DocumentContent is incorrect because
> DOM Document is not a Content object. So we have DocumentNode in the rest.

ok, that sounds fine by me.

Comment 3

[Mark Capella [:capella]]

(fmi, I might like to take this later, coding looks easy, the following is helpful background to me, maybe to someone else) ...

[00:20]tbsaunde surkov: how do you feel about making GetDocumentNode() inline on nsDocAccessible?
[00:21]surkov   mm, tbsaunde, you could keep it inline on nsAccessNode like return mContent->OwnerDoc()
[00:22]surkov   in long term it doesn't work
[00:22]surkov   anyway if we need it on nsDocAccessibe mostly then I'm fine
[00:25]tbsaunde surkov: why doesn't it work long term?
[00:25]tbsaunde surkov: I think the only thing we actually call it on is doc accessibles, and they already know there doc is why I suggested that
[00:30]surkov   tbsaunde: I think we should allow document accessible without mContent
[00:30]surkov   tbsaunde: then it makes sense
[00:32]tbsaunde surkov: ok, but would document accessibles still have a pointer to their content doc?
[00:32]surkov   nsIDocument? yes
[00:33]tbsaunde surkov: yeah
[00:34]tbsaunde so then afaik it'll be fine since currently GetDocumentNode() on doc accessibles just returns mDocument

Comment 4

[Michał Frontczak [:fxa90id] ♥]

Attachment: make GetDocumentNode() inline on nsDocAccessible

Comment 5

[Trevor Saunders (:tbsaunde)]

Comment on attachment 654332 [details] [diff] [review]
make GetDocumentNode() inline on nsDocAccessible

># HG changeset patch
># User fxa90id
># Date 1345652853 -7200
># Node ID c05ee8f606dcc1ae62bf6383508d4db2be9ebaf5
># Parent  2dfbe4b9403b18b3aeaa4af3644c11895fd827a3
>Bug 739130: make GetDocumentNode() inline on nsDocAccessible
>
>diff --git a/accessible/src/base/Logging.cpp b/accessible/src/base/Logging.cpp
>--- a/accessible/src/base/Logging.cpp
>+++ b/accessible/src/base/Logging.cpp
>@@ -533,7 +533,7 @@ logging::Address(const char* aDescr, Acc
>   }
> 
>   DocAccessible* doc = aAcc->Document();
>-  nsIDocument* docNode = aAcc->GetDocumentNode();
>+  nsIDocument* docNode = aAcc->DocumentNode();

that doesn't compile since aAcc isn't a DocAccessible*, please make sure your patch builds with --enable-debug

also btw please use 8 lines of context in future patches.

Comment 6

[alexander :surkov (:asurkov)]

Michał, are you still working on this?

Comment 7

[Michał Frontczak [:fxa90id] ♥]

Attachment: make GetDocumentNode() inline on nsDocAccessible

Comment 8

[Trevor Saunders (:tbsaunde)]

Comment on attachment 669988 [details] [diff] [review]
make GetDocumentNode() inline on nsDocAccessible

>   DocAccessible* doc = aAcc->Document();
>-  nsIDocument* docNode = aAcc->GetDocumentNode();
>+  nsIDocument* docNode = aAcc->Document()->DocumentNode();

you could just use doc here

>+  inline nsIDocument* DocumentNode() const { return mDocument; }

actually, inline is assumed for methods defined with in the class so you don't need it here

Comment 9

[alexander :surkov (:asurkov)]

btw, the patch is not applied cleanly to trunk. Please update it.

Comment 10

[Michał Frontczak [:fxa90id] ♥]

Attachment: make GetDocumentNode() inline on nsDocAccessible

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 670275 [details] [diff] [review]
make GetDocumentNode() inline on nsDocAccessible

Review of attachment 670275 [details] [diff] [review]:
-----------------------------------------------------------------

in general you don't need second review, please file new patch and add checkin-needed keyword

::: accessible/src/base/Logging.cpp
@@ +618,5 @@
>             static_cast<void*>(aAcc), static_cast<void*>(aAcc->GetNode()));
>    }
>  
>    DocAccessible* doc = aAcc->Document();
> +  nsIDocument* docNode = aAcc->Document()->DocumentNode();

Trev's comment is not addressed

::: accessible/src/generic/OuterDocAccessible.cpp
@@ +195,5 @@
>    }
>  
>  #ifdef A11Y_LOG
>    if (logging::IsEnabled(logging::eDocDestroy)) {
> +    logging::DocDestroy("remove document from outerdoc", 

nit: space at the end of line

@@ +196,5 @@
>  
>  #ifdef A11Y_LOG
>    if (logging::IsEnabled(logging::eDocDestroy)) {
> +    logging::DocDestroy("remove document from outerdoc", 
> +                        child->Document()->DocumentNode(), child->AsDoc());

nit: I would do child->AsDoc()->DocumentNode();

Comment 12

[Michał Frontczak [:fxa90id] ♥]

Attachment: make GetDocumentNode() inline on nsDocAccessible

Comment 13

[alexander :surkov (:asurkov)]

I'll take care to land it since it needs merging again because of my landings

Comment 14

[alexander :surkov (:asurkov)]

landed http://hg.mozilla.org/integration/mozilla-inbound/rev/645c3f9045fc
thanks for the fix!

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/645c3f9045fc