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, gustavo, 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
Kihong Kwon
2012-09-16 22:56:50 PDT
Created attachment 164340 [details]
Patch
There was some discussion of this patch in https://bugs.webkit.org/show_bug.cgi?id=93597 Could you set the blocking bug correctly? 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. (In reply to comment #3) > Could you set the blocking bug correctly? Done. (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. Created attachment 164886 [details]
Patch
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. (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. Created attachment 165032 [details]
Patch
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 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 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.) (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. 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. Created attachment 169127 [details]
Patch
Comment on attachment 169127 [details] Patch Attachment 169127 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14424010 Comment on attachment 169127 [details] Patch Attachment 169127 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14400386 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 Created attachment 169599 [details]
Patch
Comment on attachment 169599 [details] Patch Attachment 169599 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14456466 Comment on attachment 169599 [details] Patch Attachment 169599 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14457412 Comment on attachment 169599 [details] Patch Attachment 169599 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14463444 Comment on attachment 169599 [details] Patch Attachment 169599 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14456525 Created attachment 169829 [details]
Patch
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 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. Created attachment 171580 [details]
Patch
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. Created attachment 171619 [details]
Patch
Thank you so much for reviewing, abarth and morrita. Comment on attachment 171619 [details] Patch Clearing flags on attachment: 171619 Committed r133016: <http://trac.webkit.org/changeset/133016> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 100856 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 Other than that one issue, this is a big improvement over the earlier patches. Thanks! I missed inspector related statement in the DeviceOrientationController.cpp It's fixed. Created attachment 171787 [details]
Patch
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 on attachment 171787 [details]
Patch
r=me again seeing that adam's comment is addressed.
Comment on attachment 171787 [details] Patch Clearing flags on attachment: 171787 Committed r133143: <http://trac.webkit.org/changeset/133143> All reviewed patches have been landed. Closing bug. (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). Reverted r133143 for reason: Causing content_browsertests failures Committed r133167: <http://trac.webkit.org/changeset/133167> 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(). Created attachment 174336 [details]
Patch
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 on attachment 174336 [details] Patch Clearing flags on attachment: 174336 Committed r134918: <http://trac.webkit.org/changeset/134918> All reviewed patches have been landed. Closing bug. |