[Webkit-unassigned] [Bug 93597] Add DeviceController to support DOMWindow device 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 #160375|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #53 from Adam Barth <abarth at webkit.org>  2012-08-27 11:27:55 PST ---
(From update of attachment 160375)
View in context: https://bugs.webkit.org/attachment.cgi?id=160375&action=review

I think we can improve this design substantially.  You're trying to have DeviceController do too much.  It's responsible for handling all types of events for all documents in a Page.  Instead, we should subclass DeviceController for each type of event and keep the per-DOMWindow state in a Supplement<DOMWindow>.  That way this state will be owned by the DOMWindow instead of needing to use a RefPtr<DOMWindow> to keep the DOMWindow alive.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:28545
>  				FD537352137B651800008DCE /* ZeroPole.cpp in Sources */,
> +				CC24D64A15E212E90079DD79 /* DeviceController.cpp in Sources */,

When you modify the Xcodeproj file, you might want to run Tools/Scrips/sort-xcodeproj .  That keeps the file sorted and reduces the risk of merge conflicts.

> Source/WebCore/page/DeviceController.cpp:51
> +    if (!m_clients.contains(eventType))
> +        return;
> +
> +    DeviceClientMap::iterator iterClient = m_clients.find(eventType);

You can merge these calls together so we only do a find once.  The iterator will be end() if the eventType isn't in the map.

iterClient -> client

> Source/WebCore/page/DeviceController.cpp:52
> +    if (iterClient != m_clients.end() && iterClient->second->lastEvent()) {

iterClient != m_clients.end()   <--  This is redundant with the contains check

> Source/WebCore/page/DeviceController.cpp:53
> +        m_lastEventHandler.append(LastEventHandler(window, iterClient->second->lastEvent()));

It's strange that window is being passed in here...  I'll keep reading to see how the lifetime issues work out.

> Source/WebCore/page/DeviceController.cpp:74
> +    ListenersCountedSet* listeners = m_listeners.find(eventType)->second.get();

Similar problem as above.  It's inefficient to do the contains and the find operation separately.

> Source/WebCore/page/DeviceController.cpp:82
> +    size_t lastEventHandlerCount = m_lastEventHandler.size();
> +    for (size_t i = 0; i < lastEventHandlerCount; ++i)

You don't need the lastEventHandlerCount variable.  You can just put m_lastEventHandler.size() in the loop condition.

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

ditto

> Source/WebCore/page/DeviceController.cpp:101
> +            size_t lastEventHandlerCount = m_lastEventHandler.size();
> +            for (size_t i = 0; i < lastEventHandlerCount; ++i)

ditto

> Source/WebCore/page/DeviceController.cpp:127
> +    Vector<RefPtr<DOMWindow> > listeners;

It's not a good idea to grab a RefPtr to the DOMWindow.  DOMWindow is only RefCounted because for some odd cases involving initial documents.  This could shouldn't be holding references to DOMWindows.

> Source/WebCore/page/Page.cpp:118
> +    , m_deviceController(DeviceController::create(this))

We should avoid spamming Page with a bunch of random objects.  Instead, DeviceController should be a Supplement<Page>.

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