Last Comment Bug 760354 - implement IsInDocument as accessible flag
: implement IsInDocument as accessible flag
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Mark Capella [:capella]
:
: alexander :surkov
Mentors:
Depends on:
Blocks: 759875
  Show dependency treegraph
 
Reported: 2012-05-31 21:12 PDT by alexander :surkov
Modified: 2012-06-12 18:33 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (5.36 KB, patch)
2012-06-09 09:00 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v2) (4.87 KB, patch)
2012-06-11 02:00 PDT, Mark Capella [:capella]
surkov.alexander: feedback+
Details | Diff | Splinter Review
Patch (v3) (5.16 KB, patch)
2012-06-11 08:53 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
Patch (v4) (5.74 KB, patch)
2012-06-11 09:52 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v5) (5.76 KB, patch)
2012-06-11 10:42 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-05-31 21:12:46 PDT
spun off bug 759875. DocAccessible::IsInDocument returns wrong result for an accessible that was destroyed and new accessible for the same DOM node was created.

1) add eIsNotInDocument flag to Accessible::StateFlags (don't forget to update AccessibleTypes enum)
2) add IsInDocument() method to Accessible
3) remove DocAccessible::IsInDocument
4) make DocAccessible::UncacheChildrenInSubtree to set IsNotInDocument flag on the accessible
Comment 1 Mark Capella [:capella] 2012-06-09 09:00:05 PDT
Created attachment 631667 [details] [diff] [review]
Patch (v1)

I followed what I thought the intent was here, but I'm getting test failures in test_stale.html. Did I mess up the code? Or simply expose the problem we're trying to fix?

Ties into original Bug 676267 ... ?
Comment 2 alexander :surkov 2012-06-09 09:02:07 PDT
I think because of
inline bool IsInDocument() const { return mFlags & eIsNotInDocument; }
Comment 3 alexander :surkov 2012-06-09 09:03:42 PDT
and because of this
DocAccessible::UncacheChildrenInSubtree(Accessible* aRoot)
{
  mFlags |= eIsNotInDocument;
Comment 4 Mark Capella [:capella] 2012-06-11 02:00:49 PDT
Created attachment 631831 [details] [diff] [review]
Patch (v2)

Lord, I've got to stop jumping on bugs I don't fully understand :)

This seems to make more sense, and it passes all tests ...
Comment 5 alexander :surkov 2012-06-11 05:55:23 PDT
Comment on attachment 631831 [details] [diff] [review]
Patch (v2)

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

::: accessible/src/generic/Accessible.h
@@ +479,5 @@
>      return mContent->IsHTML() &&
>        (mContent->Tag() == nsGkAtoms::abbr || mContent->Tag() == nsGkAtoms::acronym);
>    }
>  
> +  inline bool IsInDocument(Accessible* aAccessible) const

no inline, I'd keep it near of IsDefunct, please comment it

@@ +484,5 @@
> +  {
> +    Accessible* acc = aAccessible;
> +    while (acc && !acc->IsPrimaryForNode())
> +      acc = acc->Parent();
> +

you don't need that because you set the flag on each accessible you uncache from document

@@ +488,5 @@
> +
> +    return acc ? !(acc->mFlags & eIsNotInDocument) : false;
> +  }
> +
> +  inline void SetIsNotInDocument() { mFlags |= eIsNotInDocument; }

not sure I like it's public (you could friend classes) but perhaps it's ok.
Comment 6 Mark Capella [:capella] 2012-06-11 08:53:21 PDT
Created attachment 631904 [details] [diff] [review]
Patch (v3)

I went ahead and tweaked those bits while waiting ...
Comment 7 alexander :surkov 2012-06-11 08:56:23 PDT
Comment on attachment 631904 [details] [diff] [review]
Patch (v3)

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

::: accessible/src/generic/Accessible.h
@@ +694,5 @@
> +   */
> +  bool IsInDocument(Accessible* aAccessible) const
> +  {
> +    return aAccessible ? !(aAccessible->mFlags & eIsNotInDocument) : false;
> +  }

you're on accessible so you don't need aAccessible argument, jsut
return !(mFlags & eIsNotInDocument);
Comment 8 Mark Capella [:capella] 2012-06-11 08:58:35 PDT
I though I tried that and it was failinng ... lemme try again ...
Comment 9 Trevor Saunders (:tbsaunde) 2012-06-11 09:16:10 PDT
Comment on attachment 631904 [details] [diff] [review]
Patch (v3)

># HG changeset patch
># Parent 0f6ae110182a267ab1a20bca6730194cbff72ea3
># User Mark Capella <markcapella@twcny.rr.com>
>Bug 760354 - implement IsInDocument as accessible flag, r=surkov, f=tbsaunde

might want to update that ;-)

>+  bool IsInDocument(Accessible* aAccessible) const
>+  {
>+    return aAccessible ? !(aAccessible->mFlags & eIsNotInDocument) : false;

if we're going to move this from DocAccessible to Accessible we should get rid of the argument and make it work on this, and then update the callers.  Hopefully that'll help perf some since callers won't need to get the doc anymore.
Comment 10 Mark Capella [:capella] 2012-06-11 09:52:42 PDT
Created attachment 631927 [details] [diff] [review]
Patch (v4)

This is how I had the patch without passing an argument ... it fails test_stale.html with:

failed | wrong extra state bits for ['div node', address: [object HTMLDivElement], role: section, address: [xpconnect wrapped nsIAccessible]]!got '0', expected 'stale'

Changing it to take the arg works / passes ... (I had the debugger out and was hacking at it for awhile before I tried the new approach).
Comment 11 Mark Capella [:capella] 2012-06-11 10:42:03 PDT
Created attachment 631942 [details] [diff] [review]
Patch (v5)

Changed the caller and this works again ...
Comment 12 Mark Capella [:capella] 2012-06-11 17:05:49 PDT
push to inbound TRY
https://tbpl.mozilla.org/?tree=Try&rev=314aed2afe92
Comment 13 Mark Capella [:capella] 2012-06-12 02:50:04 PDT
previous push was in the middle of many TRY and INBOUND issues ... lotsa backouts and such ... waited, and am trying again:

https://tbpl.mozilla.org/?tree=Try&rev=982d37b69ab6
Comment 14 Mark Capella [:capella] 2012-06-12 08:27:15 PDT
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ab582792079c
Comment 15 Matt Brubeck (:mbrubeck) 2012-06-12 18:33:23 PDT
https://hg.mozilla.org/mozilla-central/rev/ab582792079c

Note You need to log in before you can comment on or make changes to this bug.