[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 #33 from Beth Dakin <bdakin at apple.com> ---
(In reply to comment #30)
> Comment on attachment 249124 [details]
> 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.
> 

I added separate functions, but for the time being, I added them to the implementation file rather than the header file. I did that because these functions need to be marked WEBCORE_EXPORT, and I think that WEBCORE_EXPORT functions cannot be inlined without breaking some of the bots. If this is an incorrect understanding, I will fix. (I'll chat with Alex to confirm.)

> > 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.
> 

Fixed all of this. 

> > 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.
> 

Removed this.

> > Source/WebCore/dom/WebKitMouseForceChangedEvent.h:56
> > +    virtual EventInterface eventInterface() const override;
> 
> Can this be private?
> 

Looks like it can! Done.

> > 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?
> 

Yes it can. Changed to a double in the idl.

> > Source/WebCore/html/HTMLAttributeNames.in:278
> > +onwebkitmouseforcewillbegin
> > +onwebkitmouseforceup
> 
> Better to keep these sorted in alphabetical order.
> 

Fixed.

> > Source/WebCore/html/HTMLBodyElement.cpp:125
> > +        &onwebkitmouseforcewillbeginAttr,
> > +        &onwebkitmouseforceupAttr,
> 
> Better to keep these sorted in alphabetical order.
> 

Fixed.

> >>> 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

Will do!

New patch coming.

-- 
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/20150323/c4751a91/attachment-0002.html>