[webkit-reviews] review denied: [Bug 93597] Add DeviceController to support DOMWindow device events : [Attachment 159363] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Kihong Kwon
<kihong.kwon at samsung.com>'s request for review:
Bug 93597: Add DeviceController to support DOMWindow device events
https://bugs.webkit.org/show_bug.cgi?id=93597

Attachment 159363: Patch
https://bugs.webkit.org/attachment.cgi?id=159363&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=159363&action=review


OK.  This is getting close.  Can we remove the PROXIMITY_EVENTS-specific code
from this patch?  It doesn't seem like it actually does anything because
nothing inherits from DeviceClient.

Rather than adding support for PROXIMITY_EVENTS in this patch, I'd prefer if
we:
1) Landed this patch
2) Changed the existing device code to use this mechanism (probably one per
patch)
3) Added support for device proximity

Is that a reasonable course of action?

> Source/WebCore/page/DeviceController.cpp:44
> +#if ENABLE(PROXIMITY_EVENTS)
> +    if (eventType == eventNames().webkitdeviceproximityEvent)
> +	   return true;
> +#endif

Should we do the PROXIMITY_EVENTS specific work in the next patch?

> Source/WebCore/page/DeviceController.cpp:58
> +	   listeners = new ListenersCountedSet();

This is a naked new.  Presumably we want adoptPtr here.

> Source/WebCore/page/DeviceController.cpp:77
> +	   delete listeners;

Raw delete.  Let's use OwnPtr instead!

> Source/WebCore/page/DeviceController.cpp:87
> +    for (int i = 0; i < m_listeners.size(); ++i) {

int -> size_t ?

> Source/WebCore/page/DeviceController.cpp:116
> +	   if (!listeners[i]->document()->activeDOMObjectsAreSuspended())

Is this the right semantics for suspended documents?  It's fine to just drop
the events on the floor?