The destructor of BatteryProviderEfl should not call stopUpdating() because it invokes e_ukit_shutdown() even though e_ukit_init() is not called. The e_ukit_shutdown() calls the e_dbus_shutdown() and it returns error that the init is not called properly. EAPI int e_dbus_shutdown(void) { if (_edbus_init_count <= 0) { EINA_LOG_ERR("Init count not greater than 0 in shutdown."); return 0; } ... }
Created attachment 161818 [details] patch.
Comment on attachment 161818 [details] patch. View in context: https://bugs.webkit.org/attachment.cgi?id=161818&action=review > Source/WebKit2/ChangeLog:9 > + The destructor of BatteryProviderEfl should not call stopUpdating() > + because it invokes e_ukit_shutdown() even though e_ukit_init() is not called. e_ukit_init() is called in BatteryProviderEFl::startUpdating(), so there seems to be some other problem here.
Comment on attachment 161818 [details] patch. View in context: https://bugs.webkit.org/attachment.cgi?id=161818&action=review >> Source/WebKit2/ChangeLog:9 >> + because it invokes e_ukit_shutdown() even though e_ukit_init() is not called. > > e_ukit_init() is called in BatteryProviderEFl::startUpdating(), so there seems to be some other problem here. Well, the problem is possibly that stopUpdating() is called several times? > Source/WebKit2/UIProcess/API/efl/BatteryProvider.cpp:-55 > - m_provider.stopUpdating(); Could we maybe add a boolean property to the class to know if we are updating or not? This way we should call m_provider.stopUpdating() only we we are still updating (i.e. stopUpdating() was not explicitly called). I think this would be more reliable.
Actually, the fix seems consistent with the WebKit1 implementation so OK. In addition to your fix, I would also add a check in BatteryProviderEfl::stopUpdating() / BatteryProviderEfl::startUpdating() to avoid such issue in the future. It should not cause any problem to call BatteryProviderEfl::stopUpdating() twice or BatteryProviderEfl::startUpdating() twice.
(In reply to comment #2) > (From update of attachment 161818 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161818&action=review > > > Source/WebKit2/ChangeLog:9 > > + The destructor of BatteryProviderEfl should not call stopUpdating() > > + because it invokes e_ukit_shutdown() even though e_ukit_init() is not called. > > e_ukit_init() is called in BatteryProviderEFl::startUpdating(), so there seems to be some other problem here. Currently, startUpdating() is not called when the Minibrowser launches.
(In reply to comment #4) > Actually, the fix seems consistent with the WebKit1 implementation so OK. > > In addition to your fix, I would also add a check in BatteryProviderEfl::stopUpdating() / BatteryProviderEfl::startUpdating() to avoid such issue in the future. It should not cause any problem to call BatteryProviderEfl::stopUpdating() twice or BatteryProviderEfl::startUpdating() twice. I totally agree you that we need such code and would like you to add that. Thanks for your review.
(In reply to comment #4) > Actually, the fix seems consistent with the WebKit1 implementation so OK. > > In addition to your fix, I would also add a check in BatteryProviderEfl::stopUpdating() / BatteryProviderEfl::startUpdating() to avoid such issue in the future. It should not cause any problem to call BatteryProviderEfl::stopUpdating() twice or BatteryProviderEfl::startUpdating() twice. I agree with this. IMO, We need to add a flag to check for starting update.
Created attachment 161916 [details] patch
Comment on attachment 161916 [details] patch Attachment 161916 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13755029
(In reply to comment #7) > (In reply to comment #4) > > Actually, the fix seems consistent with the WebKit1 implementation so OK. > > > > In addition to your fix, I would also add a check in BatteryProviderEfl::stopUpdating() / BatteryProviderEfl::startUpdating() to avoid such issue in the future. It should not cause any problem to call BatteryProviderEfl::stopUpdating() twice or BatteryProviderEfl::startUpdating() twice. > > I agree with this. > IMO, We need to add a flag to check for starting update. I did not add a new flag to check the updating status but used the existing one, m_timer in the BatteryProviderEfl.h. By checking if the m_timer is active or not, stopUpdating() is executed only when we are updating. Also, I added stopUpdating() in the destructor of BatteryClientEfl.cpp as we can check if we are updating or not.
Comment on attachment 161916 [details] patch Attachment 161916 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13734463
Comment on attachment 161916 [details] patch LGTM. Thanks, this version is much better IMHO.
Why didn't you set review flag?
(In reply to comment #13) > Why didn't you set review flag? I changed the bug title to match the ChangeLog and set review flag.
Created attachment 161951 [details] patch
Comment on attachment 161951 [details] patch Looks fine.
Comment on attachment 161951 [details] patch Clearing flags on attachment: 161951 Committed r127436: <http://trac.webkit.org/changeset/127436>
All reviewed patches have been landed. Closing bug.