[webkit-reviews] review denied: [Bug 90944] Remove setController from BatteryClient : [Attachment 151671] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Kihong Kwon
<kihong.kwon at samsung.com>'s request for review:
Bug 90944: Remove setController from BatteryClient
https://bugs.webkit.org/show_bug.cgi?id=90944

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.