Bug 96894

Summary: Add DeviceController base-class to remove duplication of DeviceXXXControler
Product: WebKit Reporter: Kihong Kwon <kihong.kwon>
Component: WebCore Misc.Assignee: Kihong Kwon <kihong.kwon>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cdumez, dglazkov, donggwan.kim, eric, gns, gyuyoung.kim, haraken, japhet, mifenton, morrita, peter, philn, rakuco, rwlbuis, senorblanco, tonikitoo, vimff0, webkit.review.bot, xan.lopez, yutak
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 100856    
Bug Blocks: 92837, 97630, 102578    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Kihong Kwon 2012-09-16 22:56:50 PDT
DeviceOrientationController, DeviceMotionContoller and DeviceProximityController have almost same logic.
Therefore we can remove duplication in them by DeviceController base-class.
Comment 1 Kihong Kwon 2012-09-16 23:13:53 PDT
Created attachment 164340 [details]
Patch
Comment 2 Adam Barth 2012-09-17 00:05:45 PDT
There was some discussion of this patch in https://bugs.webkit.org/show_bug.cgi?id=93597
Comment 3 Hajime Morrita 2012-09-19 21:59:00 PDT
Could you set the blocking bug correctly?
Comment 4 Hajime Morrita 2012-09-19 22:05:47 PDT
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.
Comment 5 Kihong Kwon 2012-09-20 03:02:58 PDT
(In reply to comment #3)
> Could you set the blocking bug correctly?
Done.
Comment 6 Kihong Kwon 2012-09-20 03:18:13 PDT
(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.
Comment 7 Kihong Kwon 2012-09-20 03:57:33 PDT
Created attachment 164886 [details]
Patch
Comment 8 Hajime Morrita 2012-09-20 04:12:10 PDT
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.
Comment 9 Kihong Kwon 2012-09-20 05:09:59 PDT
(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.
Comment 10 Kihong Kwon 2012-09-20 18:36:38 PDT
Created attachment 165032 [details]
Patch
Comment 11 Hajime Morrita 2012-09-24 22:48:15 PDT
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.
Comment 12 Chris Dumez 2012-09-24 22:52:09 PDT
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)
Comment 13 Yuta Kitamura 2012-09-24 23:31:19 PDT
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.)
Comment 14 Kihong Kwon 2012-09-25 22:55:31 PDT
(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.
Comment 15 Hajime Morrita 2012-09-25 23:20:03 PDT
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.
Comment 16 Kihong Kwon 2012-10-17 02:03:08 PDT
Created attachment 169127 [details]
Patch
Comment 17 Build Bot 2012-10-17 02:41:34 PDT
Comment on attachment 169127 [details]
Patch

Attachment 169127 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14424010
Comment 18 Build Bot 2012-10-17 03:12:09 PDT
Comment on attachment 169127 [details]
Patch

Attachment 169127 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14400386
Comment 19 WebKit Review Bot 2012-10-17 03:26:52 PDT
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
Comment 20 Kihong Kwon 2012-10-19 04:42:57 PDT
Created attachment 169599 [details]
Patch
Comment 21 Build Bot 2012-10-19 04:48:00 PDT
Comment on attachment 169599 [details]
Patch

Attachment 169599 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14456466
Comment 22 Early Warning System Bot 2012-10-19 04:51:27 PDT
Comment on attachment 169599 [details]
Patch

Attachment 169599 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14457412
Comment 23 Early Warning System Bot 2012-10-19 04:51:54 PDT
Comment on attachment 169599 [details]
Patch

Attachment 169599 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14463444
Comment 24 Build Bot 2012-10-19 08:27:18 PDT
Comment on attachment 169599 [details]
Patch

Attachment 169599 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14456525
Comment 25 Kihong Kwon 2012-10-21 21:57:50 PDT
Created attachment 169829 [details]
Patch
Comment 26 Hajime Morrita 2012-10-22 00:36:36 PDT
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.
Comment 27 Kihong Kwon 2012-10-22 03:19:36 PDT
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.
Comment 28 Kihong Kwon 2012-10-30 21:51:09 PDT
Created attachment 171580 [details]
Patch
Comment 29 Hajime Morrita 2012-10-31 03:56:36 PDT
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.
Comment 30 Kihong Kwon 2012-10-31 04:48:24 PDT
Created attachment 171619 [details]
Patch
Comment 31 Kihong Kwon 2012-10-31 04:51:42 PDT
Thank you so much for reviewing, abarth and morrita.
Comment 32 WebKit Review Bot 2012-10-31 05:56:14 PDT
Comment on attachment 171619 [details]
Patch

Clearing flags on attachment: 171619

Committed r133016: <http://trac.webkit.org/changeset/133016>
Comment 33 WebKit Review Bot 2012-10-31 05:56:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 WebKit Review Bot 2012-10-31 07:16:45 PDT
Re-opened since this is blocked by bug 100856
Comment 35 Adam Barth 2012-10-31 10:28:11 PDT
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
Comment 36 Adam Barth 2012-10-31 10:28:38 PDT
Other than that one issue, this is a big improvement over the earlier patches.  Thanks!
Comment 37 Kihong Kwon 2012-10-31 21:58:05 PDT
I missed inspector related statement in the DeviceOrientationController.cpp
It's fixed.
Comment 38 Kihong Kwon 2012-11-01 00:27:41 PDT
Created attachment 171787 [details]
Patch
Comment 39 Kihong Kwon 2012-11-01 00:35:09 PDT
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.
Comment 40 Hajime Morrita 2012-11-01 03:29:08 PDT
Comment on attachment 171787 [details]
Patch

r=me again seeing that adam's comment is addressed.
Comment 41 WebKit Review Bot 2012-11-01 03:38:26 PDT
Comment on attachment 171787 [details]
Patch

Clearing flags on attachment: 171787

Committed r133143: <http://trac.webkit.org/changeset/133143>
Comment 42 WebKit Review Bot 2012-11-01 03:38:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 43 Stephen White 2012-11-01 07:11:58 PDT
(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).
Comment 44 Stephen White 2012-11-01 07:26:40 PDT
Reverted r133143 for reason:

Causing content_browsertests failures

Committed r133167: <http://trac.webkit.org/changeset/133167>
Comment 45 Kihong Kwon 2012-11-14 21:24:08 PST
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().
Comment 46 Kihong Kwon 2012-11-14 21:28:42 PST
Created attachment 174336 [details]
Patch
Comment 47 Kihong Kwon 2012-11-16 02:14:10 PST
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.
Comment 48 WebKit Review Bot 2012-11-16 02:20:16 PST
Comment on attachment 174336 [details]
Patch

Clearing flags on attachment: 174336

Committed r134918: <http://trac.webkit.org/changeset/134918>
Comment 49 WebKit Review Bot 2012-11-16 02:20:24 PST
All reviewed patches have been landed.  Closing bug.