Bug 90944 – Remove setController from BatteryClient

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

[Kihong Kwon]

BatteryClient doesn't need to keep m_controller becuase, we can use BatteryController by BatteryController::from().
Therefore we can remove setController from BatteryClient.

[Kihong Kwon]

Created attachment 151671 [details]
Patch

[Chris Dumez]

Comment on attachment 151671 [details]
Patch

LGTM.

[Gyuyoung Kim]

Comment on attachment 151671 [details]
Patch

Because BatteryController is already supplementable, we don't need to use a member variable for controller. Looks fine.

[Adam Barth]

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.

[Kihong Kwon]

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.

[Kihong Kwon]

Created attachment 152161 [details]
Patch

[Kihong Kwon]

Created attachment 152175 [details]
Patch.

I used geolocation implementation by reference for chromium port.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Adam Barth]

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

[Kihong Kwon]

Created attachment 152404 [details]
Patch

[Adam Barth]

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.

[WebKit Review Bot]

Comment on attachment 152404 [details]
Patch

Clearing flags on attachment: 152404

Committed r122743: <http://trac.webkit.org/changeset/122743>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kihong Kwon]

(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. :-)