Bug 109278 – Factor EventContext and introduces MouseOrFocusEventContext.

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Hayato Ito]

This is a separated patch from bug 109156.

[Hayato Ito]

Created attachment 187277 [details]
Factor EventContext

[WebKit Review Bot]

Attachment 187277 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/EventContext.cpp', u'Source/WebCore/dom/EventContext.h', u'Source/WebCore/dom/EventDispatcher.cpp', u'Source/WebCore/dom/EventDispatcher.h', u'Source/WebCore/inspector/InspectorInstrumentation.cpp', u'Source/WebCore/inspector/InspectorInstrumentation.h']" exit_code: 1
Source/WebCore/dom/EventContext.h:65:  The parameter name "target" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dimitri Glazkov (Google)]

Comment on attachment 187277 [details]
Factor EventContext

View in context: https://bugs.webkit.org/attachment.cgi?id=187277&action=review

> Source/WebCore/dom/EventContext.h:61
> +typedef Vector<OwnPtr<EventContext>, 32> EventContextAncestors;

I am having trouble with this name. It seems more like an EventContextList. The word "ancestors" is completely out of place here, though I personally understand where it came from.

Another idea: we could use the DOM core terminology of "event path" (http://dom.spec.whatwg.org/#dispatching-events) and instead of EventContextAncestors and ensureEventContextAncestors (neither make good sense), we just say "EventPath" and "ensureEventPath"?

>> Source/WebCore/dom/EventContext.h:65
>> +    MouseOrFocusEventContext(PassRefPtr<Node>, PassRefPtr<EventTarget> currentTarget, PassRefPtr<EventTarget> target);
> 
> The parameter name "target" adds no information, so it should be removed.  [readability/parameter_name] [5]

Style elf is mistaken! :)

[Hayato Ito]

Thank you for the review. Let me land this after addressing the comment.

(In reply to comment #3)
> (From update of attachment 187277 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187277&action=review
> 
> > Source/WebCore/dom/EventContext.h:61
> > +typedef Vector<OwnPtr<EventContext>, 32> EventContextAncestors;
> 
> I am having trouble with this name. It seems more like an EventContextList. The word "ancestors" is completely out of place here, though I personally understand where it came from.
> 
> Another idea: we could use the DOM core terminology of "event path" (http://dom.spec.whatwg.org/#dispatching-events) and instead of EventContextAncestors and ensureEventContextAncestors (neither make good sense), we just say "EventPath" and "ensureEventPath"?

I like it. Let me use that.
We should use terminology from the spec unless there is a strong reason. We don't have a strong reason. :)

> 
> >> Source/WebCore/dom/EventContext.h:65
> >> +    MouseOrFocusEventContext(PassRefPtr<Node>, PassRefPtr<EventTarget> currentTarget, PassRefPtr<EventTarget> target);
> > 
> > The parameter name "target" adds no information, so it should be removed.  [readability/parameter_name] [5]
> 
> Style elf is mistaken! :)

Yeah :)

[Hayato Ito]

Created attachment 187761 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 187761 [details]
Patch for landing

Clearing flags on attachment: 187761

Committed r142575: <http://trac.webkit.org/changeset/142575>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.