Bug 57521 – Move coordinate-computing logic into MouseRelatedEvent.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dimitri Glazkov (Google)]

Move coordinate-computing logic into MouseRelatedEvent.

[Dimitri Glazkov (Google)]

Created attachment 87658 [details]
Patch

[Darin Adler]

Comment on attachment 87658 [details]
Patch

Generally speaking my goal was to have the events be simple data holders and keep the smarts in the class to a minimum. Since the DOM allows you to create events with arbitrary values, I figured the smarts of computing the arguments for an event to send didn’t belong in the event class itself. There had to be some exceptions, but my goal was to keep them to a minimum. Your patch heads in the other direction.

I think it’s OK to have this logic in the event class.

I also think it’s dangerous to change the meanings of the MouseRelatedEvent arguments. Before the constructor arguments and the init function arguments were exactly the same thing.

Despite those reservations this change seems OK. But not a great bell-ringer of an improvement.

[Dimitri Glazkov (Google)]

The reason I thought this would be a good thing is because MouseRelatedEvent already does a lot of coordinate-computing itself, so keeping the initialization code seemed odd.

I like the notion of Events as simple data holders, but in our code, we have three distinct piles of very narrowly-focused knowledge:
1) how to fire events according to DOM rules (that's what EventDispatcher does)
2) how to create events (and initialize them)
3) how to react to results of even dispatch (and perhaps issue a sequence of events, such as simulated clicking)

My plan was to put 2 and 3 in the same abstraction, and this abstraction was to be FooEvent, since that class knows all about, well, the specifics of firing Foo events.

However, if we go with the event-as-simple-object idea, then perhaps there's a place for another type of object, something like an event factory, which does 2 and 3.

[Darin Adler]

(In reply to comment #3)
> However, if we go with the event-as-simple-object idea, then perhaps there's a place for another type of object, something like an event factory, which does 2 and 3.

We can’t really make the event be a simple object because of the target-based coordinate properties like layerX and offsetX. I was forced to put that code into MouseRelatedEvent because the coordinates can’t be computed until the event is dispatched to a target.

[Dimitri Glazkov (Google)]

(In reply to comment #4)
> (In reply to comment #3)
> > However, if we go with the event-as-simple-object idea, then perhaps there's a place for another type of object, something like an event factory, which does 2 and 3.
> 
> We can’t really make the event be a simple object because of the target-based coordinate properties like layerX and offsetX. I was forced to put that code into MouseRelatedEvent because the coordinates can’t be computed until the event is dispatched to a target.

So maybe it's ok to view events as self-managing entities? :)

[Darin Adler]

I still think the long term best design for events is probably plain data holders. This is clearly the intent of the DOM since it provides a function that lets you construct an arbitrary event with any value for any property. It’s the non-standard DOM extensions that stray from this.

[Dimitri Glazkov (Google)]

(In reply to comment #6)
> I still think the long term best design for events is probably plain data holders. This is clearly the intent of the DOM since it provides a function that lets you construct an arbitrary event with any value for any property. It’s the non-standard DOM extensions that stray from this.

So we're back to some sort of a separate object to manage event cycle. Let me sketch out something here. Suppose we have a FooEventManager object, which has these two methods:

PassRefPtr<Event> prepareEvent();
bool dispatchEvent(EventDispatcher*);

At the callsite, we'll have something like this:

EventDispatcher::dispatchEvent(node, MouseEventManager(params,params));

Whose body does something like this:

RefPtr<Event> event = eventManager.prepareEvent();
EventDispatcher dispatcher(node);
return eventManager.dispatchEvent(dispatcher);

The def/decl live with the corresponding FooEvent.cpp. This should allow us to both keep events simpler and abstract the dispatch lifecycle logic pretty well. What do you think?

[Dimitri Glazkov (Google)]

(In reply to comment #7)
> (In reply to comment #6)
> > I still think the long term best design for events is probably plain data holders. This is clearly the intent of the DOM since it provides a function that lets you construct an arbitrary event with any value for any property. It’s the non-standard DOM extensions that stray from this.
> 
> So we're back to some sort of a separate object to manage event cycle. Let me sketch out something here. Suppose we have a FooEventManager object, which has these two methods:
> 
> PassRefPtr<Event> prepareEvent();
> bool dispatchEvent(EventDispatcher*);
> 
> At the callsite, we'll have something like this:
> 
> EventDispatcher::dispatchEvent(node, MouseEventManager(params,params));
> 
> Whose body does something like this:
> 
> RefPtr<Event> event = eventManager.prepareEvent();
> EventDispatcher dispatcher(node);
> return eventManager.dispatchEvent(dispatcher);

Probably don't even need the prepareEvent method. Can just stuff it all into dispatchEvent method.

> 
> The def/decl live with the corresponding FooEvent.cpp. This should allow us to both keep events simpler and abstract the dispatch lifecycle logic pretty well. What do you think?

[WebKit Commit Bot]

Comment on attachment 87658 [details]
Patch

Rejecting attachment 87658 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2

Last 500 characters of output:
......................................................................................................................
editing/selection ....................................................................................................................
editing/selection/drag-start-event-client-x-y.html -> failed

Exiting early after 1 failures. 5284 tests run.
116.40s total testing time

5283 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
2 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8311128

[Dimitri Glazkov (Google)]

(In reply to comment #9)
> (From update of attachment 87658 [details])
> Rejecting attachment 87658 [details] from commit-queue.
> 
> Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2
> 
> Last 500 characters of output:
> ......................................................................................................................
> editing/selection ....................................................................................................................
> editing/selection/drag-start-event-client-x-y.html -> failed
> 
> Exiting early after 1 failures. 5284 tests run.
> 116.40s total testing time
> 
> 5283 test cases (99%) succeeded
> 1 test case (<1%) had incorrect layout
> 2 test cases (<1%) had stderr output
> 
> Full output: http://queues.webkit.org/results/8311128

Ah, there's also a drag event callsite that needed to be tweaked.

[Dimitri Glazkov (Google)]

Committed r82584: <http://trac.webkit.org/changeset/82584>

[Dimitri Glazkov (Google)]

Darin, I put up a patch based on the sketch in bug 57562. Take a look and let me know what you think.