make GetDocumentNode() inline on nsDocAccessible

RESOLVED FIXED in mozilla19

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: tbsaunde, Assigned: fxa90id)

Tracking

(Blocks: 1 bug)

unspecified
mozilla19
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
(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.
(Reporter)

Comment 2

6 years ago
(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.
(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
(Assignee)

Updated

6 years ago
Assignee: nobody → michaljev
(Assignee)

Comment 4

6 years ago
Created attachment 654332 [details] [diff] [review]
make GetDocumentNode() inline on nsDocAccessible
Attachment #654332 - Flags: review?(trev.saunders)
(Reporter)

Comment 5

6 years ago
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.
Attachment #654332 - Flags: review?(trev.saunders)
(Assignee)

Updated

6 years ago
Attachment #654332 - Attachment description: Merge Accessible::Init into constructor → make GetDocumentNode() inline on nsDocAccessible

Comment 6

6 years ago
Michał, are you still working on this?
(Assignee)

Comment 7

6 years ago
Created attachment 669988 [details] [diff] [review]
make GetDocumentNode() inline on nsDocAccessible
Attachment #654332 - Attachment is obsolete: true
Attachment #669988 - Flags: review?(trev.saunders)
(Reporter)

Comment 8

6 years ago
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
Attachment #669988 - Flags: review?(trev.saunders) → review+

Comment 9

6 years ago
btw, the patch is not applied cleanly to trunk. Please update it.
(Assignee)

Comment 10

6 years ago
Created attachment 670275 [details] [diff] [review]
make GetDocumentNode() inline on nsDocAccessible
Attachment #669988 - Attachment is obsolete: true
Attachment #670275 - Flags: review?(surkov.alexander)

Comment 11

6 years ago
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();
Attachment #670275 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 12

6 years ago
Created attachment 670299 [details] [diff] [review]
make GetDocumentNode() inline on nsDocAccessible
Attachment #670275 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed

Comment 13

6 years ago
I'll take care to land it since it needs merging again because of my landings
Keywords: checkin-needed

Comment 14

6 years ago
landed http://hg.mozilla.org/integration/mozilla-inbound/rev/645c3f9045fc
thanks for the fix!
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/645c3f9045fc
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.