[Webkit-unassigned] [Bug 142836] Add events related to force click gesture

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=142836

--- Comment #30 from Darin Adler <darin at apple.com> ---
Comment on attachment 249124
  --> https://bugs.webkit.org/attachment.cgi?id=249124
Patch

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

>>> Source/WebCore/dom/Element.cpp:2137
>>> +#if ENABLE(MOUSE_FORCE_EVENTS)
>> 
>> Is there a reason we don't just disappear these functions if this isn't enabled?
> 
> Not necessarily a good one! I was just trying to minimize the number of spots I put the #ifs since they make code harder to read, but I am open to anything really.

My favorite pattern for this sort of thing is to have empty functions, inlined if necessary, to keep the #if statements out of all the call sites. I prefer separate functions because I don’t like all the UNUSED_PARAM you need if the conditionals are inside the functions, and also because it makes it easier to put them inlined and in headers.

> Source/WebCore/dom/WebKitMouseForceChangedEvent.cpp:36
> +    : force(0)

better to use C++11 member initialization syntax:

    double force { 0 };

Then we don’t need to declare or define a constructor at all.

> Source/WebCore/dom/WebKitMouseForceChangedEvent.cpp:41
> +    : m_force(0)

Same story here:

    double m_force { 0 };

Although in this case we should probably leave the default constructor, but just remove the explicit m_force initialization.

> Source/WebCore/dom/WebKitMouseForceChangedEvent.cpp:63
> +WebKitMouseForceChangedEvent::~WebKitMouseForceChangedEvent()
> +{
> +}

Seems better to just not declare or define this and let the compiler take care of it.

> Source/WebCore/dom/WebKitMouseForceChangedEvent.h:56
> +    virtual EventInterface eventInterface() const override;

Can this be private?

> Source/WebCore/dom/WebKitMouseForceChangedEvent.idl:29
> +    [InitializedByEventConstructor] readonly attribute float force;

It seems strange that this is float in the IDL and double in the code. Can it be double here too?

> Source/WebCore/html/HTMLAttributeNames.in:278
> +onwebkitmouseforcewillbegin
> +onwebkitmouseforceup

Better to keep these sorted in alphabetical order.

> Source/WebCore/html/HTMLBodyElement.cpp:125
> +        &onwebkitmouseforcewillbeginAttr,
> +        &onwebkitmouseforceupAttr,

Better to keep these sorted in alphabetical order.

>>> Source/WebCore/page/DOMWindow.idl:272
>>> +    [NotEnumerable] attribute EventHandler onwebkitmouseforcecancelled;
>> 
>> should these have Conditional=MOUSE_FORCE_EVENT?
> 
> SO!!! I am glad you asked, because I am really confused about this. Earlier versions of my patch included this. However, in http://trac.webkit.org/changeset/181507 Darin removed all conditionals from DOMWindow.idl, and I am very confused about why. I asked Anders, who reviewed the patch, and he wasn't totally sure about it either. I should really ask Darin. Oh, I will cc him!

It’s correct to include these unconditionally. We might decide some day we want to compile them out, but I suggest a pattern where the events are only there if needed, but the event handler machinery is always present.

However, you should add coverage for these new event handlers to the Layout test named fast/dom/event-handler-attributes.html

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150320/a7cd29e7/attachment-0002.html>