Last Comment Bug 569356 - Coalescence of state change events is bad
: Coalescence of state change events is bad
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla21
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: eventa11y
  Show dependency treegraph
 
Reported: 2010-06-01 08:36 PDT by David Bolter [:davidb] ***PTO until 29th***
Modified: 2013-01-28 16:11 PST (History)
2 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (46.52 KB, patch)
2012-12-18 05:28 PST, alexander :surkov
tbsaunde+mozbugs: review+
mzehe: review+
Details | Diff | Splinter Review

Description David Bolter [:davidb] ***PTO until 29th*** 2010-06-01 08:36:17 PDT
Spin off from bug 564471 research and discussion with Alexander.

Looking over our current implementation I don't see how we avoid mistakenly removing 'duplicate' EVENT_STATE_CHANGE events (which can be different, e.g. EXT_STATE_FOO EXT_STATE_BAR).
Comment 1 David Bolter [:davidb] ***PTO until 29th*** 2010-06-01 08:55:08 PDT
So we need to consider two related things:

1. What to do when the delayed event has an initialized accessible but not an initialzed node.
2. What to do about false hits for removing duplicates.

For 1, we can compare accessibles as well, instead of just nodes.

For 2, we can consider comparing the state. This could be a general member of the base class (perhaps called mSubType or something) which could give us a general solution if we need this for other event types. For state changes we would just mSubType(aState).
Comment 2 David Bolter [:davidb] ***PTO until 29th*** 2010-06-01 09:03:54 PDT
For 1, we might consider always initializing the node member variable. We need to consider the life cycle of nodes and accessibles (and debug).
Comment 3 alexander :surkov 2012-12-18 05:28:57 PST
Created attachment 693350 [details] [diff] [review]
patch

1) make state change events coalescence (Trevor)
2) change eventQueue to handle alternatives (named as scenarios), otherwise we'll regress testing from bug 446469 (Marco).
Comment 4 David Bolter [:davidb] ***PTO until 29th*** 2012-12-18 06:42:20 PST
Wow thanks for getting to this bug!
(nit: don't forget to recomment gA11yEventDumpToConsole)
Comment 5 alexander :surkov 2012-12-18 06:45:19 PST
(In reply to David Bolter [:davidb] from comment #4)
> Wow thanks for getting to this bug!

why is it important for you?

> (nit: don't forget to recomment gA11yEventDumpToConsole)

yeah, thanks, usually I skim the patch after I uploaded it so I fix some things after that.
Comment 6 David Bolter [:davidb] ***PTO until 29th*** 2012-12-18 06:46:44 PST
(In reply to alexander :surkov from comment #5)
> (In reply to David Bolter [:davidb] from comment #4)
> > Wow thanks for getting to this bug!
> 
> why is it important for you?

Just feeling badly that it sat for so long.
Comment 7 alexander :surkov 2012-12-18 06:48:36 PST
(In reply to David Bolter [:davidb] from comment #6)

> Just feeling badly that it sat for so long.

yeah, you must be feeling bad permanently ;)
Comment 8 Trevor Saunders (:tbsaunde) 2012-12-19 03:31:28 PST
Comment on attachment 693350 [details] [diff] [review]
patch

>+          if (thisSCEvent->mState == tailSCEvent->mState) {
>+            thisEvent->mEventRule = AccEvent::eDoNotEmit;
>+            if (thisSCEvent->mIsEnabled != tailSCEvent->mIsEnabled)
>+              tailEvent->mEventRule = AccEvent::eDoNotEmit;

nit, assert the mIsEnabled are different?

r=me for accessible/src/ (I didn't read the tests)
Comment 9 alexander :surkov 2012-12-19 06:28:51 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> Comment on attachment 693350 [details] [diff] [review]
> patch
> 
> >+          if (thisSCEvent->mState == tailSCEvent->mState) {
> >+            thisEvent->mEventRule = AccEvent::eDoNotEmit;
> >+            if (thisSCEvent->mIsEnabled != tailSCEvent->mIsEnabled)
> >+              tailEvent->mEventRule = AccEvent::eDoNotEmit;
> 
> nit, assert the mIsEnabled are different?

why?

> r=me for accessible/src/ (I didn't read the tests)

those are really two different but cycle dependent bugs.
Comment 10 Trevor Saunders (:tbsaunde) 2012-12-26 19:18:01 PST
(In reply to alexander :surkov from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > Comment on attachment 693350 [details] [diff] [review]
> > patch
> > 
> > >+          if (thisSCEvent->mState == tailSCEvent->mState) {
> > >+            thisEvent->mEventRule = AccEvent::eDoNotEmit;
> > >+            if (thisSCEvent->mIsEnabled != tailSCEvent->mIsEnabled)
> > >+              tailEvent->mEventRule = AccEvent::eDoNotEmit;
> > 
> > nit, assert the mIsEnabled are different?
> 
> why?
Comment 11 Trevor Saunders (:tbsaunde) 2012-12-26 19:19:52 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> (In reply to alexander :surkov from comment #9)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > > Comment on attachment 693350 [details] [diff] [review]
> > > patch
> > > 
> > > >+          if (thisSCEvent->mState == tailSCEvent->mState) {
> > > >+            thisEvent->mEventRule = AccEvent::eDoNotEmit;
> > > >+            if (thisSCEvent->mIsEnabled != tailSCEvent->mIsEnabled)
> > > >+              tailEvent->mEventRule = AccEvent::eDoNotEmit;
> > > 
> > > nit, assert the mIsEnabled are different?
> > 
> > why?

because I forgoforget it, I forgot we might have already coalesced an intermediate state change event away.  What I wanted to assert was that we never fire state foo enabled event then another state foo enabled event witout disabling state foo in between.
Comment 12 Marco Zehe (:MarcoZ) 2013-01-07 05:07:49 PST
Comment on attachment 693350 [details] [diff] [review]
patch

>+    gA11yEventDumpToConsole = true; // debug stuff

Nit: Remove this before checkin, which will obsolete any diffs to actions/anchors.html.

>+ *     // [optional] if true then event sequances are ignored (no failure if

Nit: sequences, not sequances.

>+                ok(false,
>+                   "We have a matched scendario at index " + matchIdx + " already.");

Nit: "...scenario".

>+        if (!checker.unexptected && checker.unique &&

This looks like a typo which may have borked your tests in some way. Please check that you've not missed anything.

r=me with those nits fixed and the last point checked.
Comment 13 alexander :surkov 2013-01-07 17:48:31 PST
(In reply to Marco Zehe (:MarcoZ) from comment #12)

> >+        if (!checker.unexptected && checker.unique &&
> 
> This looks like a typo which may have borked your tests in some way. Please
> check that you've not missed anything.

comment was updated to:

// Report an error if we hanlded not expected event of unique type
// (i.e. event types are matched, targets differs).
Comment 15 David Bolter [:davidb] ***PTO until 29th*** 2013-01-08 07:41:27 PST
(In reply to alexander :surkov from comment #14)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/696264e2414c

Looks like you accidentally missed Marco's nit about 'unexptected'...
Comment 16 David Bolter [:davidb] ***PTO until 29th*** 2013-01-08 07:52:48 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/da88cd4bc6db
Comment 17 David Bolter [:davidb] ***PTO until 29th*** 2013-01-08 07:57:52 PST
(I should mention it really wasn't a 'nit' in case that matters later)
Comment 18 alexander :surkov 2013-01-08 17:14:55 PST
thanks!
Comment 20 Ed Morley [:emorley] 2013-01-09 03:49:02 PST
Backed out for intermittent, but pretty frequent test_doc_busy.html timeouts:
https://tbpl.mozilla.org/php/getParsedLog.php?id=18616115&tree=Firefox

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a0794537c6d
Comment 21 Ed Morley [:emorley] 2013-01-09 05:42:29 PST
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/4a0794537c6d
Comment 22 alexander :surkov 2013-01-18 16:02:39 PST
landing by parts:
part 1 (support the event scenarios in mochitests): http://hg.mozilla.org/integration/mozilla-inbound/rev/3bf0fc40df43
Comment 23 :Ms2ger (⌚ UTC+1/+2) 2013-01-19 10:29:55 PST
https://hg.mozilla.org/mozilla-central/rev/3bf0fc40df43
Comment 24 alexander :surkov 2013-01-28 01:20:17 PST
2nd part: http://hg.mozilla.org/integration/mozilla-inbound/rev/d00f2767e57c, logging is enabled in case of failures
Comment 25 Ryan VanderMeulen [:RyanVM] 2013-01-28 16:11:06 PST
https://hg.mozilla.org/mozilla-central/rev/d00f2767e57c

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