[webkit-reviews] review denied: [Bug 93597] Add DeviceController to support DOMWindow device events : [Attachment 160375] 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 160375: Patch
https://bugs.webkit.org/attachment.cgi?id=160375&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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>.