[webkit-reviews] review denied: [Bug 93597] Add DeviceProximityController to support Proximity Events : [Attachment 157725] 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 DeviceProximityController to support Proximity Events
https://bugs.webkit.org/show_bug.cgi?id=93597

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

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


> Source/WebCore/Modules/proximity/DeviceProximityClient.h:34
> +    virtual void DeviceProximityControllerDestroyed() = 0;

DeviceProximityControllerDestroyed -> deviceProximityControllerDestroyed

> Source/WebCore/Modules/proximity/DeviceProximityController.cpp:44
> +DeviceProximityController::~DeviceProximityController()
> +{
> +    m_client->DeviceProximityControllerDestroyed();
> +}

I was under the impression that most objects like this don't need this sort of
callback.  Typically the client and the controller are owned by the Page and
get their destructors called at the same time.

> Source/WebCore/Modules/proximity/DeviceProximityController.cpp:83
> +void DeviceProximityController::addListener(DOMWindow* window)
> +{
> +    bool isEmpty = m_listeners.isEmpty();
> +    m_listeners.add(window);
> +    if (isEmpty)
> +	   m_client->startUpdating();
> +}
> +
> +void DeviceProximityController::removeListener(DOMWindow* window)
> +{
> +    m_listeners.remove(window);
> +    m_suspendedListeners.remove(window);
> +    if (m_listeners.isEmpty())
> +	   m_client->stopUpdating();
> +}
> +
> +void DeviceProximityController::removeAllListeners(DOMWindow* window)
> +{
> +    if (!m_listeners.contains(window))
> +	   return;
> +
> +    m_listeners.removeAll(window);
> +    m_suspendedListeners.removeAll(window);
> +    if (m_listeners.isEmpty())
> +	   m_client->stopUpdating();
> +}
> +
> +void DeviceProximityController::suspendEventsForAllListeners(DOMWindow*
window)
> +{
> +    if (!m_listeners.contains(window))
> +	   return;
> +
> +    int count = m_listeners.count(window);
> +    removeAllListeners(window);
> +    while (count--)
> +	   m_suspendedListeners.add(window);
> +}

All this stuff doesn't look specific to DeviceProximity.  There are a number of
features that follow this pattern.  Should we extract a base class to share
this code?

> Source/WebCore/dom/Document.cpp:2214
> +#if ENABLE(PROXIMITY_EVENTS)
> +    if (DeviceProximityController* controller =
DeviceProximityController::from(page()))
> +	   controller->suspendEventsForAllListeners(domWindow());
>  #endif

We shouldn't need to do this.  Perhaps DeviceProximityController should be an
ActiveDOMObject?  I was looking at this code the other day and had the same
thought about DeviceMotionController and DeviceOrientationController.

> Source/WebCore/dom/Document.cpp:2233
> +    if (!page())
> +	   return;

At the very least, this shouldn't be copy/pasted from line 2222

> Source/WebCore/dom/Document.cpp:2235
> +    if (DeviceProximityController* controller =
DeviceProximityController::from(page()))
> +	   controller->resumeEventsForAllListeners(domWindow());

Ditto.