Add down-casting to nsXULTreeAccessible

RESOLVED FIXED in mozilla14

Status

()

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

People

(Reporter: capella, Assigned: capella)

Tracking

({access})

unspecified
mozilla14
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Add down-casting to nsXULTreeAccessible class similar to work done for nsRootAccessible in bug#636845 to support DeCOMtamination effort.

https://wiki.mozilla.org/Gecko:DeCOMtamination
(Assignee)

Comment 1

6 years ago
bug#636945
(Assignee)

Updated

6 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Depends on: 739524
(Assignee)

Comment 2

6 years ago
Created attachment 611569 [details] [diff] [review]
Patch (v1)

Ok, first try at this patch ... builds ok, but fails during testing. Can you see anything? I've narrowed it to the following (it works if I comment it out).

test_focus_general.xul fails while running this one test:

   gQueue.push(new synthDownKey("radiosweater",
                                 new focusChecker("radiojacket")));
Attachment #611569 - Flags: feedback?(surkov.alexander)

Comment 3

6 years ago
(In reply to Mark Capella [:capella] from comment #2)
> Created attachment 611569 [details] [diff] [review]
> Patch (v1)
> 
> Ok, first try at this patch ... builds ok, but fails during testing. Can you
> see anything? I've narrowed it to the following (it works if I comment it
> out).

it looks correct

> 
> test_focus_general.xul fails while running this one test:
> 
>    gQueue.push(new synthDownKey("radiosweater",
>                                  new focusChecker("radiojacket")));

no XUL tree at all here, no known intermittent failures for test_focus_general.xul and no XUL tree tests there. Can you make sure you build this patch against trunk and no other parches are applied

Comment 4

6 years ago
Comment on attachment 611569 [details] [diff] [review]
Patch (v1)

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

::: accessible/src/base/nsRootAccessible.cpp
@@ +394,1 @@
>      if (treeAcc) {

you don't need to check NodeInfo, just do
nsXULTreeAccessible* treeAcc = accessible->AsXULTree();
if (treeAcc) {
Attachment #611569 - Flags: review?(trev.saunders)
Attachment #611569 - Flags: feedback?(surkov.alexander)
Attachment #611569 - Flags: feedback+
(Assignee)

Comment 5

6 years ago
Created attachment 611718 [details] [diff] [review]
Patch (v2)

It looks like I had the patch re-based after all. Alexs suggestion seems have been the missing piece ... builds and tests out now just fine !

Comment 6

6 years ago
Mark, could you please mark patches as obsolete when you upload new one and please move '?' flags (like review requests) to new patch?

Updated

6 years ago
Attachment #611569 - Attachment is obsolete: true
Attachment #611569 - Flags: review?(trev.saunders)

Updated

6 years ago
Attachment #611718 - Flags: review?(trev.saunders)
Comment on attachment 611718 [details] [diff] [review]
Patch (v2)

>+    if (eventType.EqualsLiteral("TreeRowCountChanged")) {
>+      HandleTreeRowCountChangedEvent(aDOMEvent, treeAcc);
>+      return;
>+    }
>+    if (eventType.EqualsLiteral("TreeInvalidated")) {

nit, blank line between ifs would be nice

>+inline nsXULTreeAccessible*
>+nsAccessible::AsXULTree()
>+{
>+  return mFlags & eXULTreeAccessible ?

you could use IsXULTree() to be sure we are consistant.

btw I don't see changes to nsAccessibilityService.cpp in this patch, so you missed one place where we use do_QueryObject ot get an nsXULTreeAccessible.  If you wnat to be sure you got all of them I'm pretty sure you could rm the class ID xpcom macros from nsXULTreeAccessible.
Attachment #611718 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 8

6 years ago
Created attachment 611732 [details] [diff] [review]
Patch (v3)
Attachment #611718 - Attachment is obsolete: true

Comment 9

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f85a5488ffb

Updated

6 years ago
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/8f85a5488ffb
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.