[Webkit-unassigned] [Bug 93597] Add DeviceProximityController to support Proximity Events

[bugzilla-daemon at webkit.org]

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


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #157725|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #19 from Adam Barth <abarth at webkit.org>  2012-08-10 09:29:22 PST ---
(From update of attachment 157725)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.