If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

nsXULTreeGridRowAccesible should cache its kids as nsXULTreeGridCellAccessibles not generic accessibles

RESOLVED FIXED in mozilla36

Status

()

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

People

(Reporter: tbsaunde, Assigned: dij, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

6 years ago
1. change nsAccessibleHashtable to nsRefPtrHashtable<nsHashKey<const void>, nsXULTreeGridCellAccessible> as the type of mAccessibleCache in nsXULTreeGridRowAccessible (accessible/src/xul/nsXULTreeGridAccessible.h)
2. change GetCellAccessible to return a nsXULTreeGridCellAccessible* instead of a nsAccessible*
3. while your here and since it won't be needed any more remove the cid macro stuff from nsXULTreeGridCellAccessible
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
How do you want to handle consumers of the GetCellAceessible function? GetCellAt() might be ok as is, RowInvalidated() might be modified with a recast (?), but ChildAtPoint(), GetChildAt(), and GetSiblingAt() return nsAccessible values.

I guess I should have asked what the benefit of changing the internal cache contents is here...
(Reporter)

Comment 2

6 years ago
(In reply to Mark Capella [:capella] from comment #1)
> How do you want to handle consumers of the GetCellAceessible function?
> GetCellAt() might be ok as is, RowInvalidated() might be modified with a
> recast (?), but ChildAtPoint(), GetChildAt(), and GetSiblingAt() return
> nsAccessible values.

an nsXULTreeGridCellAccessible* can be used wherever you need a nsAccessible*, so as I sort of said in the description if the caller downcasts the result then remove that since you already have the right type, and if the caller doesn't need a nsXULTreeGridCellAccessible* only a nsAccessible* it should just keep working.

> I guess I should have asked what the benefit of changing the internal cache
> contents is here...

it means we don't need to downcast what we pull out of it.
Nah, I didn't see anything like that in the description, so I thought I'd ask.

This seems to be growing in complexity anyhow, since now I see the CycleCollectorTraverse macros / templates in nsAccCache.h such as CycleCollectorTraverseCache, ClearCache, ... etc are build around the type / keyword "nsAccessibleHashtable".

In other words unless I'm way off, we can't simply replace "nsAccessibleHashtable to nsRefPtrHashtable<nsHashKey<const void>, nsXULTreeGridCellAccessible>" without affecting cycle collection, and cloning blocks of code to support it.

Is this something you're familiar with?

Comment 4

6 years ago
how about to provide templated versions of ClearCache etc and do template instantiation for nsAccessibleHashtable?
(Reporter)

Comment 5

6 years ago
(In reply to alexander :surkov from comment #4)
> how about to provide templated versions of ClearCache etc and do template
> instantiation for nsAccessibleHashtable?

hm, hadn't thought about that problem, but that solution seems fine.
Created attachment 616168 [details] [diff] [review]
Patch (v1) - WIP

Attaching my current patch for the record and as-is before any GG changes ...

Comment 7

6 years ago
Comment on attachment 616168 [details] [diff] [review]
Patch (v1) - WIP

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

::: accessible/src/xul/nsXULTreeAccessible.cpp
@@ +243,5 @@
>    if (aWhichChild == eDeepestChild && child) {
>      // Look for accessible cell for the found item accessible.
>      nsRefPtr<nsXULTreeItemAccessibleBase> treeitem = do_QueryObject(child);
>  
> +    nsAccessible *cell = reinterpret_cast<nsAccessible*>(treeitem->GetCellAccessible(column));

type* name

it returns nsXULTreeGridCellAccessible*, you don't need casting here

@@ +598,5 @@
>    // Fire destroy event for removed tree items and delete them from caches.
>    for (PRInt32 rowIdx = aRow; rowIdx < aRow - aCount; rowIdx++) {
>  
>      void* key = reinterpret_cast<void*>(rowIdx);
> +    nsAccessible* treeItem = reinterpret_cast<nsAccessible*>(mAccessibleCache.GetWeak(key));

same

@@ +624,5 @@
>  
>    for (PRInt32 rowIdx = newRowCount; rowIdx < oldRowCount; ++rowIdx) {
>  
>      void *key = reinterpret_cast<void*>(rowIdx);
> +    nsAccessible* treeItem = reinterpret_cast<nsAccessible*>(mAccessibleCache.GetWeak(key));

same

::: accessible/src/xul/nsXULTreeAccessible.h
@@ +216,5 @@
>    /**
>     * Return cell accessible for the given column. If XUL tree accessible is not
>     * accessible table then return null.
>     */
> +  virtual nsXULTreeGridCellAccessible* GetCellAccessible(nsITreeColumn *aColumn)

type* name

::: accessible/src/xul/nsXULTreeGridAccessible.cpp
@@ +726,5 @@
>    // Return if we failed to find tree cell in the row for the given point.
>    if (row != mRow || !column)
>      return nsnull;
>  
> +  return reinterpret_cast<nsAccessible*>(GetCellAccessible(column));

no casting and below
(Reporter)

Comment 8

6 years ago
Comment on attachment 616168 [details] [diff] [review]
Patch (v1) - WIP

>+      nsXULTreeGridCellAccessible* cellAccessible = GetCellAccessible(column);
>       if (cellAccessible) {
>         nsRefPtr<nsXULTreeGridCellAccessible> cellAcc = do_QueryObject(cellAccessible);

no longer needed

Comment 9

6 years ago
Mark, please address comments and request review.
Created attachment 627433 [details] [diff] [review]
Patch (v2)

Ok, to catch this one up, I've attached the patch as-is with code nits addressed ... but I'm still looking at comment 4 as templates are new to me.

I'm also attaching a screen shot of the build errors this causes ... it shows the GC errors that are expected at this point, but also the reasons why I had placed the reinterpret_casts into the code ...
Attachment #616168 - Attachment is obsolete: true
Created attachment 627434 [details]
GC and reinterpret_cast Build errors
(Reporter)

Comment 12

6 years ago
(In reply to Mark Capella [:capella] from comment #10)
> Created attachment 627433 [details] [diff] [review]
> Patch (v2)
> 
> Ok, to catch this one up, I've attached the patch as-is with code nits
> addressed ... but I'm still looking at comment 4 as templates are new to me.

for this they should be pretty simple just take a template argument of the type, and update the callers to pass the type of accessible in the cache as the template rg.
Freeing up items I'm not active on @ the moment ...
Assignee: markcapella → nobody
Status: ASSIGNED → NEW
Mark, do you think this is still a good first bug or something is changed in the meanwhile?
Flags: needinfo?(markcapella)

Updated

4 years ago
Flags: needinfo?(markcapella) → needinfo?(trev.saunders)
(Reporter)

Comment 15

4 years ago
if anything this should be simpler now, there's a generic support for cycle collecting nsRefPtrHashtables now so it should be possible to remove the stuff in nsAccCache.h and just replace it with NS_IMPL_TRAVERSE(mCache) or whatever the macro is.s
Flags: needinfo?(trev.saunders)

Comment 16

4 years ago
Hello everyone! I am new here and I would like to try to fix this bug. I have read the comments so far, however from what I've understood, things have changed recently (comment 15) and the approach should differ from what has been done so far. Could someone help me and please bring me up to date with what should be done here and some hints to how to do it? Thank you!
Flags: needinfo?

Updated

4 years ago
Flags: needinfo? → needinfo?(trev.saunders)
(Reporter)

Comment 17

4 years ago
(In reply to Iosif Adrian Mihai from comment #16)
> Hello everyone! I am new here and I would like to try to fix this bug. I
> have read the comments so far, however from what I've understood, things
> have changed recently (comment 15) and the approach should differ from what
> has been done so far. Could someone help me and please bring me up to date
> with what should be done here and some hints to how to do it? Thank you!

I think comment 15 explained that.  If you have specific question maybe I can help more, otherwise I guess you should just try and fix it and see what happens.
Flags: needinfo?(trev.saunders)
Mentor: trev.saunders@gmail.com
Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++] → [good first bug][lang=c++]
(Assignee)

Comment 18

3 years ago
hello, is somebody working on this bug because i would like to work on this bug, this would be my first bug so i might need some extra guidance debugging it. thanks!
(Assignee)

Comment 19

3 years ago
Created attachment 8512927 [details] [diff] [review]
bug739568.diff
(Assignee)

Comment 20

3 years ago
Created attachment 8515598 [details] [diff] [review]
bug739568.patch

i didn't remove the cid macro stuff from nsXULTreeGridCellAccessible because it is being used by another macro NS_INTERFACE_TABLE_INHERITED(XULTreeGridCellAccessible,
                               XULTreeGridCellAccessible) in XULTreeGridAccessible.cpp, do we really need to remove that it seems like it is being used here.
Assignee: nobody → dj.dij123
Attachment #627433 - Attachment is obsolete: true
Attachment #627434 - Attachment is obsolete: true
Attachment #8512927 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8515598 - Flags: review?(tbsaunde+mozbugs)

Comment 21

3 years ago
(In reply to diwas joshi [:dij] from comment #20)
> Created attachment 8515598 [details] [diff] [review]
> bug739568.patch
> 
> i didn't remove the cid macro stuff from nsXULTreeGridCellAccessible because
> it is being used by another macro
> NS_INTERFACE_TABLE_INHERITED(XULTreeGridCellAccessible,
>                                XULTreeGridCellAccessible)

if you don't have queries left then you don't need this macros anymore (make sure to fix .h part)
(Reporter)

Comment 22

3 years ago
Comment on attachment 8515598 [details] [diff] [review]
bug739568.patch

>+++ b/accessible/xul/XULTreeAccessible.cpp
>@@ -177,24 +177,24 @@ XULTreeAccessible::NativeRole()
> 
> ////////////////////////////////////////////////////////////////////////////////
> // XULTreeAccessible: Accessible implementation (DON'T put methods here)
> 
> Accessible*
> XULTreeAccessible::ChildAtPoint(int32_t aX, int32_t aY,
>                                 EWhichChildAtPoint aWhichChild)
> {
>-  nsIFrame *frame = GetFrame();
>+  nsIFrame* frame = GetFrame();

this is extranious,  please remove it

>   if (!frame)
>     return nullptr;
> 
>-  nsPresContext *presContext = frame->PresContext();
>+  nsPresContext* presContext = frame->PresContext();

likewise, and several other places below.

>@@ -208,17 +208,17 @@ XULTreeAccessible::ChildAtPoint(int32_t 
>   if (row == -1 || !column)
>     return AccessibleWrap::ChildAtPoint(aX, aY, aWhichChild);
> 
>   Accessible* child = GetTreeItemAccessible(row);
>   if (aWhichChild == eDeepestChild && child) {
>     // Look for accessible cell for the found item accessible.
>     nsRefPtr<XULTreeItemAccessibleBase> treeitem = do_QueryObject(child);
> 
>-    Accessible* cell = treeitem->GetCellAccessible(column);
>+    Accessible* cell = reinterpret_cast<Accessible*>(treeitem->GetCellAccessible(column));

cast should be unnecessary.

>+++ b/accessible/xul/XULTreeAccessible.h
>@@ -11,16 +11,17 @@
> #include "nsITreeColumns.h"
> #include "XULListboxAccessible.h"
> 
> class nsTreeBodyFrame;
> 
> namespace mozilla {
> namespace a11y {
> 
>+class XULTreeGridCellAccessible;

blank line here please

>+++ b/accessible/xul/XULTreeGridAccessible.cpp
>@@ -126,17 +126,17 @@ XULTreeGridAccessible::CellAt(uint32_t a
>     nsCoreUtils::GetSensibleColumnAt(mTree, aColumnIndex);
>   if (!column)
>     return nullptr;
> 
>   nsRefPtr<XULTreeItemAccessibleBase> rowAcc = do_QueryObject(row);
>   if (!rowAcc)
>     return nullptr;
> 
>-  return rowAcc->GetCellAccessible(column);
>+  return reinterpret_cast<Accessible*>(rowAcc->GetCellAccessible(column));

cast shouldn't be required, here and a couple places below.

>+++ b/accessible/xul/XULTreeGridAccessible.h
>@@ -10,16 +10,17 @@
> #include "TableAccessible.h"
> #include "TableCellAccessible.h"
> #include "xpcAccessibleTable.h"
> #include "xpcAccessibleTableCell.h"
> 
> namespace mozilla {
> namespace a11y {
> 
>+class XULTreeGridCellAccessible;

blank line before comment please

> 
>   // XULTreeItemAccessibleBase
>-  virtual Accessible* GetCellAccessible(nsITreeColumn* aColumn) const MOZ_OVERRIDE;
>+  virtual XULTreeGridCellAccessible* GetCellAccessible(nsITreeColumn* aColumn)
>+    const MOZ_OVERRIDE;

MOZ_FINAL too please
Attachment #8515598 - Flags: review?(tbsaunde+mozbugs) → feedback+
(Assignee)

Comment 23

3 years ago
Created attachment 8516159 [details] [diff] [review]
bug739568.patch
Attachment #8515598 - Attachment is obsolete: true
Attachment #8516159 - Flags: review?(tbsaunde+mozbugs)
(Reporter)

Comment 24

3 years ago
Comment on attachment 8516159 [details] [diff] [review]
bug739568.patch

>+++ b/accessible/xul/XULTreeAccessible.cpp
>@@ -26,17 +26,17 @@
> #include "nsIDOMXULMenuListElement.h"
> #include "nsIDOMXULMultSelectCntrlEl.h"
> #include "nsIDOMXULTreeElement.h"
> #include "nsITreeSelection.h"
> #include "nsIMutableArray.h"
> #include "nsTreeBodyFrame.h"
> #include "nsTreeColumns.h"
> #include "nsTreeUtils.h"
>-
>+#include "XULTreeGridAccessible.h"

include it after States.h instead of here

>     nsCOMPtr<nsITreeColumn> column;
>     treeColumns->GetColumnAt(colIdx, getter_AddRefs(column));
>     if (column && !nsCoreUtils::IsColumnHidden(column)) {
>-      Accessible* cellAccessible = GetCellAccessible(column);
>+      XULTreeGridCellAccessible* cellAccessible = GetCellAccessible(column);
>       if (cellAccessible) {
>-        nsRefPtr<XULTreeGridCellAccessible> cellAcc = do_QueryObject(cellAccessible);
>-
>-        nameChanged |= cellAcc->CellInvalidated();
>+        nameChanged |= cellAccessible->CellInvalidated();

you should use nsRefPtr<XULTreeGridCellAccessible> here since otherwise its possible for all refs to the cell to go away before the function finishes.  (that's totally horrible, but that's the way XUL is).

>-// XULTreeGridCellAccessible: nsISupports implementation
>-
>-NS_IMPL_CYCLE_COLLECTION_INHERITED(XULTreeGridCellAccessible, LeafAccessible,
>-                                   mTree, mColumn)

you should leave that

>-
>-NS_INTERFACE_TABLE_HEAD_CYCLE_COLLECTION_INHERITED(XULTreeGridCellAccessible)
>-  NS_INTERFACE_TABLE_INHERITED(XULTreeGridCellAccessible,
>-                               XULTreeGridCellAccessible)
>-NS_INTERFACE_TABLE_TAIL_INHERITING(LeafAccessible)
>-NS_IMPL_ADDREF_INHERITED(XULTreeGridCellAccessible, LeafAccessible)
>-NS_IMPL_RELEASE_INHERITED(XULTreeGridCellAccessible, LeafAccessible)

and replace this by NS_IMPL_ISSUPORTS_INHERITED0

> class XULTreeGridCellAccessible : public LeafAccessible,
>                                   public TableCellAccessible
> {
> public:
> 
>   XULTreeGridCellAccessible(nsIContent* aContent, DocAccessible* aDoc,
>                             XULTreeGridRowAccessible* aRowAcc,
>                             nsITreeBoxObject* aTree, nsITreeView* aTreeView,
>                             int32_t aRow, nsITreeColumn* aColumn);
> 
>-  // nsISupports
>-  NS_DECL_ISUPPORTS_INHERITED
>-  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(XULTreeGridCellAccessible,
>-                                           LeafAccessible)

keep these
Attachment #8516159 - Flags: review?(tbsaunde+mozbugs) → feedback+
(Assignee)

Comment 25

3 years ago
Created attachment 8517497 [details] [diff] [review]
bug739568.patch

i have corrected the mistakes in the previous patch.
Attachment #8516159 - Attachment is obsolete: true
Attachment #8517497 - Flags: review?(tbsaunde+mozbugs)
(Reporter)

Updated

3 years ago
Attachment #8517497 - Flags: review?(tbsaunde+mozbugs) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Can we please run this through Try first? :)
Keywords: checkin-needed

Comment 27

3 years ago
there are failures (https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3000937&repo=try)


Assertion failure: p == &_cycleCollectorGlobal (XULTreeGridCellAccessible should QI to its own CC participant), at /builds/slave/try-l64-d-00000000000000000000/build/accessible/xul/XULTreeGridAccessible.h:127

26 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/attributes/test_obj.html | application terminated with exit code 11

PROCESS-CRASH | chrome://mochitests/content/a11y/accessible/tests/mochitest/attributes/test_obj.html | application crashed [@ mozilla::a11y::XULTreeGridCellAccessible::CheckForRightParticipant()]

TEST-UNEXPECTED-FAIL | leakcheck | default process: missing output line for total leaks!

Return code: 1
(Reporter)

Comment 28

3 years ago
(In reply to alexander :surkov from comment #27)
> there are failures
> (https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3000937&repo=try)
> 
> 
> Assertion failure: p == &_cycleCollectorGlobal (XULTreeGridCellAccessible
> should QI to its own CC participant), at
> /builds/slave/try-l64-d-00000000000000000000/build/accessible/xul/
> XULTreeGridAccessible.h:127

I guess NS_IMPL_ISUPPORTS_INHERITED0 doesn't work for ccable things, so you need to find the right set of macros that do.
https://hg.mozilla.org/mozilla-central/rev/992784f79f96
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.