Bug 96894 – Add DeviceController base-class to remove duplication of DeviceXXXControler

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kihong Kwon]

DeviceOrientationController, DeviceMotionContoller and DeviceProximityController have almost same logic.
Therefore we can remove duplication in them by DeviceController base-class.

[Kihong Kwon]

Created attachment 164340 [details]
Patch

[Adam Barth]

There was some discussion of this patch in https://bugs.webkit.org/show_bug.cgi?id=93597

[Hajime Morrita]

Could you set the blocking bug correctly?

[Hajime Morrita]

Comment on attachment 164340 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164340&action=review

I think the root source of (seemingly unnecessary) complexity is the responsibility assignment between client and the controller.
It smells possible wrong design if we see client creating an event object.

Can we eliminate preservedEvent() and hasPreservedEvent() by moving the logic to the controller?
That would simplify the interaction between controller and client. 
Since there is no client implementation available for now, it's hard to see whether this complexity is really necessary.
And I expect it isn't. 

What do you think?

> Source/WebCore/page/DeviceController.cpp:39
> +    int isEmpty = m_listeners.size();

What does this mean?
- Why is "isEmpty" int not bool?
- Why non-empty listener vector implies isEmpty?

> Source/WebCore/page/DeviceController.cpp:41
> +    if (m_listeners.find(window) == m_listeners.end())

You can just add it. That's how set works.

[Kihong Kwon]

(In reply to comment #3)
> Could you set the blocking bug correctly?
Done.

[Kihong Kwon]

(In reply to comment #4)
> (From update of attachment 164340 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164340&action=review
> 
> I think the root source of (seemingly unnecessary) complexity is the responsibility assignment between client and the controller.
> It smells possible wrong design if we see client creating an event object.
> 
> Can we eliminate preservedEvent() and hasPreservedEvent() by moving the logic to the controller?
> That would simplify the interaction between controller and client. 
> Since there is no client implementation available for now, it's hard to see whether this complexity is really necessary.
> And I expect it isn't. 
> 
> What do you think?
It's better. I will change this like yours.

> 
> > Source/WebCore/page/DeviceController.cpp:39
> > +    int isEmpty = m_listeners.size();
> 
> What does this mean?
> - Why is "isEmpty" int not bool?
> - Why non-empty listener vector implies isEmpty?
I will change to bool and use isEmpty() of Set.
And I think wasEmpty is more reasonable name in this case.

> 
> > Source/WebCore/page/DeviceController.cpp:41
> > +    if (m_listeners.find(window) == m_listeners.end())
> 
> You can just add it. That's how set works.
You're right. I need to change this.

[Kihong Kwon]

Created attachment 164886 [details]
Patch

[Hajime Morrita]

Comment on attachment 164886 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164886&action=review

I thought we need to clear DeviceController::m_event in some timing but I couldn't find such a location.
Don't we need to clear it?

> Source/WebCore/page/DeviceController.cpp:34
> +    , m_event(0)

We don't need this. RefPtr does this for you.

[Kihong Kwon]

(In reply to comment #8)
> (From update of attachment 164886 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164886&action=review
> 
> I thought we need to clear DeviceController::m_event in some timing but I couldn't find such a location.
> Don't we need to clear it?
Yes, we need it.

> 
> > Source/WebCore/page/DeviceController.cpp:34
> > +    , m_event(0)
> 
> We don't need this. RefPtr does this for you.
I will remove it.

[Kihong Kwon]

Created attachment 165032 [details]
Patch

[Hajime Morrita]

Comment on attachment 165032 [details]
Patch

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.

[Chris Dumez]

Comment on attachment 165032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165032&action=review

> Source/WebCore/page/DeviceController.cpp:73
> +    copyToVector(m_listeners, listenerVector);

Why copy to a Vector? You could iterate over m_listeners directly.

> Source/WebCore/page/DeviceController.cpp:88
> +    copyToVector(m_lastEventListeners, listenerVector);

Why copy to a Vector? You could iterate over m_lastEventListeners directly.

> Source/WebCore/page/DeviceController.h:41
> +    void removeAllDeviceEventListener(DOMWindow*);

removeAllDeviceEventListeners() ? (plural)

[Yuta Kitamura]

Comment on attachment 165032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165032&action=review

(drive-by comments)

> Source/WebCore/page/DeviceClient.h:7
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Library General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2 of the License, or (at your option) any later version.

My copy of WebKit Committers Guideline (as of Sep 2010) says all WebKit code
must be governed by a BSD-style licence or LGPL 2.1. It also says (L)GPL 3.0
is NOT acceptable.

Your license statement looks like LGPL 2.0, which should be unacceptable.
I guess the clause "or (at your option) any later version" is also unacceptable.

Please use one of the "safe" licences listed above.

(I think there's an agreement that the BSD-style license is recommended for new
 files, but my memory may be wrong.)

[Kihong Kwon]

(In reply to comment #11)
> (From update of attachment 165032 [details])
> 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.

Basic idea of m_event is from lastOrientation of legacy DeviceOrientaionController.
If there is an event already fired when addDeviceEventListener is called from DOMWindow::addEventListener, we can fire an event for the listener using preserved event.(This can eliminate response time delaying)
IMHO, this is useful functionality because we can fire an event immediately without any delay, otherwise we don't guarantee when device event is fired.

> 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.

Here is the DeviceProximityController implementation using DeviceController.
https://bugs.webkit.org/attachment.cgi?id=165729&action=review

> 
> > 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.

Actually, all attributes of device events are readonly, therefore if we use same event object, there is no way to overwrite or modify an event data.
In addition, we already use an event like this in the Device[Orientation/Motion]Controller.

> 
> 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.

You are right. As you can see in the Bug 97630, we don't make an event in the WebKit layer. We generate an event in the WebCore.

Thank you for your reviewing.

[Hajime Morrita]

As we talked at IRC, this patch could be a refactoring which pulls 
DeviceController superclass up from DeviceOrientationController.
Then the mysterious timer usage will be explained well.

[Kihong Kwon]

Created attachment 169127 [details]
Patch

[Build Bot]

Comment on attachment 169127 [details]
Patch

Attachment 169127 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14424010

[Build Bot]

Comment on attachment 169127 [details]
Patch

Attachment 169127 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14400386

[WebKit Review Bot]

Comment on attachment 169127 [details]
Patch

Attachment 169127 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14400388

New failing tests:
inspector/device-orientation-success.html

[Kihong Kwon]

Created attachment 169599 [details]
Patch

[Build Bot]

Comment on attachment 169599 [details]
Patch

Attachment 169599 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14456466

[Early Warning System Bot]

Comment on attachment 169599 [details]
Patch

Attachment 169599 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14457412

[Early Warning System Bot]

Comment on attachment 169599 [details]
Patch

Attachment 169599 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14463444

[Build Bot]

Comment on attachment 169599 [details]
Patch

Attachment 169599 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14456525

[Kihong Kwon]

Created attachment 169829 [details]
Patch

[Hajime Morrita]

Comment on attachment 169829 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169829&action=review

Almost looks good. I added some small points.

> Source/WebCore/ChangeLog:8
> +        Add DeviceController base-class for DeviceOrientationController.

This is better to be explained as "extracted" . See http://refactoring.com/catalog/extractSuperclass.html

> Source/WebCore/ChangeLog:63
> +

Let's have per-file comment where it is appropriate. Apparently this is a big change and there are some non-trivial modificaitons like Document.cpp

> Source/WebCore/dom/DeviceOrientationController.cpp:42
> +    static_cast<DeviceOrientationClient*>(m_client)->setController(this);

Let's define deviceOriengationClient()  to encapsulate these down cast.
Ideally we could have some way to check the underlying client type, but that can be done in separate change.

> Source/WebCore/dom/Document.cpp:11
> + * Copyright (C) 2012 Samsung Electronics. All rights reserved.

I'd recommend not to do this unless you change the significant part of the file.

> Source/WebCore/dom/Document.cpp:-2184
> -    if (DeviceOrientationController* controller = DeviceOrientationController::from(page()))

Why can we remove this? That is worth mentioning in the ChangeLog.

> Source/WebCore/dom/Document.cpp:-2200
> -    if (DeviceOrientationController* controller = DeviceOrientationController::from(page()))

Ditto.

> Source/WebCore/page/DOMWindow.cpp:4
> + * Copyright (C) 2012 Samsung Electronics. All rights reserved.

Ditto.

> Source/WebCore/page/DeviceController.cpp:2
> + * Copyright (C) 2012 Samsung Electronics. All rights reserved.

I'd recommend to leave original copyright holder of the original file.

> Source/WebCore/page/DeviceController.h:2
> + * Copyright (C) 2012 Samsung Electronics. All rights reserved.

Ditto.

[Kihong Kwon]

Comment on attachment 169829 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169829&action=review

Thank you for your comments.

>> Source/WebCore/ChangeLog:8
>> +        Add DeviceController base-class for DeviceOrientationController.
> 
> This is better to be explained as "extracted" . See http://refactoring.com/catalog/extractSuperclass.html

It'will be changed.

>> Source/WebCore/ChangeLog:63
>> +
> 
> Let's have per-file comment where it is appropriate. Apparently this is a big change and there are some non-trivial modificaitons like Document.cpp

It'will be changed.

>> Source/WebCore/dom/DeviceOrientationController.cpp:42
>> +    static_cast<DeviceOrientationClient*>(m_client)->setController(this);
> 
> Let's define deviceOriengationClient()  to encapsulate these down cast.
> Ideally we could have some way to check the underlying client type, but that can be done in separate change.

I will add deviceOrientationClient().
And if we need type checking during implement other features(like device motion events), I will add it.

>> Source/WebCore/dom/Document.cpp:11
>> + * Copyright (C) 2012 Samsung Electronics. All rights reserved.
> 
> I'd recommend not to do this unless you change the significant part of the file.

OK.

>> Source/WebCore/dom/Document.cpp:-2184
>> -    if (DeviceOrientationController* controller = DeviceOrientationController::from(page()))
> 
> Why can we remove this? That is worth mentioning in the ChangeLog.

I will do that.

>> Source/WebCore/page/DeviceController.cpp:2
>> + * Copyright (C) 2012 Samsung Electronics. All rights reserved.
> 
> I'd recommend to leave original copyright holder of the original file.

OK.

[Kihong Kwon]

Created attachment 171580 [details]
Patch

[Hajime Morrita]

Comment on attachment 171580 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171580&action=review

looks god.

> Source/WebCore/loader/EmptyClients.h:576
> +    virtual void startUpdating() { }

Use OVERRIDE here.

> Source/WebCore/loader/EmptyClients.h:577
> +    virtual void stopUpdating() { }

DItto.

[Kihong Kwon]

Created attachment 171619 [details]
Patch

[Kihong Kwon]

Thank you so much for reviewing, abarth and morrita.

[WebKit Review Bot]

Comment on attachment 171619 [details]
Patch

Clearing flags on attachment: 171619

Committed r133016: <http://trac.webkit.org/changeset/133016>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 100856

[Adam Barth]

Comment on attachment 171619 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171619&action=review

> Source/WebCore/page/DeviceController.cpp:83
> +    Vector<DOMWindow*> listenerVector;
> +    copyToVector(m_listeners, listenerVector);
> +    for (size_t i = 0; i < listenerVector.size(); ++i) {
> +        if (!listenerVector[i]->document()->activeDOMObjectsAreSuspended()
> +            && !listenerVector[i]->document()->activeDOMObjectsAreStopped())
> +        listenerVector[i]->dispatchEvent(event);
> +    }

Is it possible that these DOMWindow* become garbage during this iteration?  We might want to change to Vector<RefPtr<DOMWindow> > here and then null-check listenerVector[i]->document() in each iteration.

> Source/WebCore/page/DeviceController.cpp:92
> +    Vector<DOMWindow*> listenerVector;

ditto

[Adam Barth]

Other than that one issue, this is a big improvement over the earlier patches.  Thanks!

[Kihong Kwon]

I missed inspector related statement in the DeviceOrientationController.cpp
It's fixed.

[Kihong Kwon]

Created attachment 171787 [details]
Patch

[Kihong Kwon]

Comment on attachment 171787 [details]
Patch

I got r+ from morrita this, but I think I need to confirm this patch one more time, because this patch is changed little bit.

[Hajime Morrita]

Comment on attachment 171787 [details]
Patch

r=me again seeing that adam's comment is addressed.

[WebKit Review Bot]

Comment on attachment 171787 [details]
Patch

Clearing flags on attachment: 171787

Committed r133143: <http://trac.webkit.org/changeset/133143>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen White]

(In reply to comment #42)
> All reviewed patches have been landed.  Closing bug.

This patch seems to be causing a failure in content_browsertests on at least Linux and MacOS 10.6:

DeviceOrientationBrowserTest.BasicTest: 
[25419:25428:1101/063250:4481937548:ERROR:object_proxy.cc(608)] Failed to get name owner. Got org.freedesktop.DBus.Error.NameHasNoOwner: Could not get owner of name 'org.freedesktop.NetworkManager': no such name
[25419:25439:1101/063250:4482017847:WARNING:proxy_service.cc(885)] PAC support disabled because there is no system implementation
Killed (timed out).

[Stephen White]

Reverted r133143 for reason:

Causing content_browsertests failures

Committed r133167: <http://trac.webkit.org/changeset/133167>

[Kihong Kwon]

Comment on attachment 171787 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171787&action=review

There has been a bug in this patch, it will be fixed.

> Source/WebCore/page/DeviceController.cpp:54
> +    if (!wasEmpty)

"!" has to be eliminated to call startUpdating().

[Kihong Kwon]

Created attachment 174336 [details]
Patch

[Kihong Kwon]

Comment on attachment 174336 [details]
Patch

This is already reviewed.
But, there is a bug which makes a regression in the content_browsertests.
I only eliminate "!" from DeviceController::addDeviceEventListener() for fixing regression.

[WebKit Review Bot]

Comment on attachment 174336 [details]
Patch

Clearing flags on attachment: 174336

Committed r134918: <http://trac.webkit.org/changeset/134918>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.