BatteryClient doesn't need to keep m_controller becuase, we can use BatteryController by BatteryController::from(). Therefore we can remove setController from BatteryClient.
Created attachment 151671 [details] Patch
Comment on attachment 151671 [details] Patch LGTM.
Comment on attachment 151671 [details] Patch Because BatteryController is already supplementable, we don't need to use a member variable for controller. Looks fine.
Comment on attachment 151671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151671&action=review > Source/WebKit/blackberry/Api/WebPage.cpp:522 > - WebCore::provideBatteryTo(m_page, new WebCore::BatteryClientBlackBerry); > + WebCore::provideBatteryTo(m_page, new WebCore::BatteryClientBlackBerry(this)); Where is the BatteryClientBlackBerry deleted? I see. There's the delete this pattern in batteryControllerDestroyed. That's a bad design. > Source/WebKit/chromium/src/BatteryClientImpl.cpp:45 > +BatteryClientImpl::BatteryClientImpl(WebViewImpl* webView) > + : m_webView(webView) This isn't the usual pattern for subclasses of WebCore clients. Typically the WebCore clients are wired directly to the WebKit API clients. > Source/WebKit/chromium/src/BatteryClientImpl.cpp:69 > void BatteryClientImpl::batteryControllerDestroyed() > { > - m_controller = 0; > } It seems unlikely that this function should both exist and do nothing. > Source/WebKit/chromium/src/WebViewImpl.h:819 > - OwnPtr<BatteryClientImpl> m_batteryClient; > + BatteryClientImpl m_batteryClientImpl; This isn't the usual pattern for subclasses of WebCore clients. It's more likely that we should follow the usual conventions instead of having the battery client be weird.
Comment on attachment 151671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151671&action=review Basically I think we don't need to keep the controller which inherits Supplement. As you know, we can access controller by Supplement::from. But I wondering how you think about this? Thank you for reviewing. :) >> Source/WebKit/blackberry/Api/WebPage.cpp:522 >> + WebCore::provideBatteryTo(m_page, new WebCore::BatteryClientBlackBerry(this)); > > Where is the BatteryClientBlackBerry deleted? I see. There's the delete this pattern in batteryControllerDestroyed. That's a bad design. Usually blackberry port use client like this. For instance, WebCore::provideDeviceOrientationTo(m_page, new DeviceOrientationClientBlackBerry(this)); WebCore::provideDeviceMotionTo(m_page, new DeviceMotionClientBlackBerry(this)); If we change this design, we need to raise an issue about this to blackberry guys. >> Source/WebKit/chromium/src/BatteryClientImpl.cpp:45 >> + : m_webView(webView) > > This isn't the usual pattern for subclasses of WebCore clients. Typically the WebCore clients are wired directly to the WebKit API clients. In this case(using supplement::from instead of m_controller) we need to access page instance. But, actually there is no way to access page instance of WebView in the client. And this way is not much different from DragClientImpl or EditorClientImpl. If I'm wrong, please let me know. >> Source/WebKit/chromium/src/BatteryClientImpl.cpp:69 >> } > > It seems unlikely that this function should both exist and do nothing. In this way, we don't need to use this function in the chromium port, but other ports still need this. >> Source/WebKit/chromium/src/WebViewImpl.h:819 >> + BatteryClientImpl m_batteryClientImpl; > > This isn't the usual pattern for subclasses of WebCore clients. It's more likely that we should follow the usual conventions instead of having the battery client be weird. OK. I will fix it.
Created attachment 152161 [details] Patch
Created attachment 152175 [details] Patch. I used geolocation implementation by reference for chromium port.
Comment on attachment 152175 [details] Patch. Attachment 152175 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13221571 New failing tests: http/tests/w3c/webperf/approved/navigation-timing/html/test_performance_attributes_exist_in_object.html
Created attachment 152200 [details] Archive of layout-test-results from gce-cr-linux-07 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
> http/tests/w3c/webperf/approved/navigation-timing/html/test_performance_attributes_exist_in_object.html Sorry for the spam. I've marked this test as flaky.
Created attachment 152404 [details] Patch
Comment on attachment 152404 [details] Patch Ok. It's a bit odd that this works differently for different ports, but I don't see a problem with it.
Comment on attachment 152404 [details] Patch Clearing flags on attachment: 152404 Committed r122743: <http://trac.webkit.org/changeset/122743>
All reviewed patches have been landed. Closing bug.
(In reply to comment #12) > (From update of attachment 152404 [details]) > Ok. It's a bit odd that this works differently for different ports, but I don't see a problem with it. Thank you for your review, adam. :-)