[webkit-reviews] review denied: [Bug 96894] Add DeviceController base-class to remove duplication of DeviceXXXControler : [Attachment 165032] Patch

[bugzilla-daemon at webkit.org]

Hajime Morrita <morrita at google.com> has denied Kihong Kwon
<kihong.kwon at samsung.com>'s request for review:
Bug 96894: Add DeviceController base-class to remove duplication of
DeviceXXXControler
https://bugs.webkit.org/show_bug.cgi?id=96894

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

------- Additional Comments from Hajime Morrita <morrita at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=165032&action=review


I'm sorry but I don't understand the underlying event lifecycle and why we need
m_event.
Could you explain the whole design briefly or drop me a link if you have?
Reviewing this patch without understanding the interaction between WebKit layer
is like blind man touching an elephant.

Thanks in advance.

> Source/WebCore/page/DeviceController.cpp:70
> +    m_event.clear();

Really?
This means m_event essentially never goes back null once it gets being set
since we re-set m_event just after it is cleared.

> Source/WebCore/page/DeviceController.cpp:93
> +	   listenerVector[i]->dispatchEvent(m_event);

You shouldn't dispatch same event object for different DOMWidows object
since its JavaScript object can be re-used and it can result a possible
information leak across different domain.
We should create a fresh event for each window instead.

Basically, passing Event objects from WebKit layer isn't good idea. Event is
WebCore concept and should be live inside WebCore.
For UI related event, WebKit has a low-level, "PlatformEvent" class for WebKit
layer abstraction. We might need similar idea here.