Closed Bug 515141 Opened 12 years ago Closed 12 years ago

move events firing logic from nsDocAccessible and nsAccEvent into special classes

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 2 open bugs)

Details

(Keywords: access)

Attachments

(1 file, 2 obsolete files)

In the meantime delayed events handling is spread between nsDocAccessible and nsAccEvent classes. It would be nice to have nsEventUtils and nsEventQueue classes which will have all necessary logic to fire events. nsDocAccessible would create an instance of nsEventQueue class to deal with delayed events. nsEventUtills would keep common logic for normal and delayed events firing, for example, it could have FireShowHideEvents() method.
Agreed. This logic should sit in one place (class or otherwise). Are you working on this?
I plan to start working on this once I'll land patch of bug 514595.
Great! Can you post a WIP for discussion, or catch me on IRC? We might not want multiple classes for this. I just want to check with you about the design before you go too far down (with an enormous patch) :)
Attached patch not well formed idea (obsolete) — Splinter Review
1) probably it's nice to have to files (for nsEventQueue and nsEventUtils)
2) gLastFocusFrameType should live inside of event utils probably if so nsRootAcc should have access to it
3) think about PrepareEvents while you are here
4) probably it's worth to move FireTextEvent/FireShowHide/FireFocus into nsEventUtils
5) should nsDocAcc owns by nsEventQueue or it's preferable (more comfortable) to have its cache independently.
Thanks, as per our chat I might take this on so you can work on the table bugs. This will be a good bug to fix first, before our event perf bugs.
Wow I spent a lot of time tonight trying take this patch to a clean compile; maybe I was taking it in the wrong direction, not sure.

I'm more comfortable with making smaller changes, compiling as often as I can stand it... I think there is a lot of unfinished business in this patch :)

Hmmm, do you want to continue it, or should I start over?
Depends on: 540262
part 1: bug 540262 "get rid nsAccEvent static helpers"
part 2: bug 481396 -  rename nsAccessibleEventData file to nsAccEvent
Depends on: 481396
Depends on: 540265
part 3:  Bug 540265 -  move nsAccUtils::FireEvent into special class
Depends on: 540272
part 4:  Bug 540272 -  don't use nsAccessible::FireAccessibleEvent directly
Depends on: 540281
part 5:  Bug 540281 -  rename nsAccessible::FireAccessibleEvent into HandleAccEvent
Depends on: 540285
part 6:  Bug 540285 -  stop explicit usage of nsAccEvent::prepareEvent
Blocks: eventa11y
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Attachment #400027 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #422954 - Flags: superreview?(neil)
Attachment #422954 - Flags: review?(ginn.chen)
Attachment #422954 - Flags: review?(bolterbugz)
this patch is built upon the patch of bug 523785.
Depends on: 541352
Attached patch patch2Splinter Review
fix queue processing logic
Attachment #422954 - Attachment is obsolete: true
Attachment #422985 - Flags: superreview?(neil)
Attachment #422985 - Flags: review?(ginn.chen)
Attachment #422985 - Flags: review?(bolterbugz)
Attachment #422954 - Flags: superreview?(neil)
Attachment #422954 - Flags: review?(ginn.chen)
Attachment #422954 - Flags: review?(bolterbugz)
(In reply to comment #15)
> Created an attachment (id=422985) [details]
> patch2
> 
> fix queue processing logic

oops, sorry, missed return.
What do you think about inline Get...() method (like GetEventType() and etc) usage instead of making nsAccEventQueue a friend of nsAccEvent? Does it sound reasonable?
(In reply to comment #17)
> What do you think about inline Get...() method (like GetEventType() and etc)
> usage instead of making nsAccEventQueue a friend of nsAccEvent? Does it sound
> reasonable?
nsAccEventQueue uses those members a lot, so no, I think friend is better.
Comment on attachment 422985 [details] [diff] [review]
patch2

I didn't see any interesting changes...
Attachment #422985 - Flags: superreview?(neil)
Attachment #422985 - Flags: review?(bolterbugz) → review+
Comment on attachment 422985 [details] [diff] [review]
patch2

r=me, minor nit:

>+++ b/accessible/src/base/nsAccessNode.h
>@@ -163,19 +163,30 @@ class nsAccessNode: public nsIAccessNode
>      */
>     virtual nsresult Shutdown();
> 
>     /**
>      * Return frame for the given access node object.
>      */
>     virtual nsIFrame* GetFrame();
> 
>+  /**
>+   * Return the presentation shell the accessible belongs to.
>+   */
>+  already_AddRefed<nsIPresShell> GetPresShell();

I'd rather the comment didn't use "belongs" since we aren't a member of the pres shell. How about "Return the corresponding press shell for this accessible".

>+
>+  /**
>+   * Return true if the accessible still has presentation shell. Light-weight
>+   * version of IsDefunct() method.
>+   */
>+  PRBool HasWeakShell() const { return !!mWeakShell; }

Good, fast ;)
(In reply to comment #21)

> >+  /**
> >+   * Return the presentation shell the accessible belongs to.
> >+   */
> >+  already_AddRefed<nsIPresShell> GetPresShell();
> 
> I'd rather the comment didn't use "belongs" since we aren't a member of the
> pres shell. How about "Return the corresponding press shell for this
> accessible".

Done. As well I changed the uuid of nsAccessNode.
Attachment #422985 - Flags: review?(ginn.chen) → review+
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/0f180f44f0b4
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.