Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
212643010:
Add Dispatcher class to the Battery Status API module. (Closed)
|
Created:
6 years, 9 months ago by Srini Modified:
6 years, 8 months ago CC:
blink-reviews, jamesr, dglazkov+blink, abarth-chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAdd Dispatcher class to the Battery Status API module.
BUG=122593
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172021
Patch Set 1 #
Total comments: 19
Patch Set 2 : Store status only in Dispatcher and fixes other comments. #
Total comments: 21
Patch Set 3 : Fixed all comments and waiting for dispatchDeviceEvent impl. #
Total comments: 9
Patch Set 4 : Fix comments and add m_hasEventListener appropriately. #
Total comments: 2
Patch Set 5 : Code cleanup. #
Total comments: 3
Patch Set 6 : Remove comments and stopUpdating in the destructor. #
Messages
Total messages: 36 (0 generated)
Hi ojan Can you review this patch? I will be happy to rework if you have any comments on this. Thanks, -Srini.
+Tim Volodine
Thanks for the patch Srini! I left some comments below. https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... File Source/modules/battery/BatteryDispatcher.cpp (right): https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryDispatcher.cpp:45: else if (status->charging() && m_batteryStatus->chargingTime() != status->chargingTime()) do you need an 'else if' here? looks like this should be just an 'if'. https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... File Source/modules/battery/BatteryDispatcher.h (right): https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryDispatcher.h:27: void didChangeBatteryStatus(const AtomicString& eventType, PassRefPtr<BatteryStatus>); does this method need to be public? https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... File Source/modules/battery/BatteryManager.cpp (right): https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryManager.cpp:24: { we probably want to unregister here by calling stopUpdating() https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryManager.cpp:34: startUpdating(); yeah this is a bit tricky. Think it's generally ok to start updating here, but note that BatteryManager::level() will not necessarily have m_batteryStatus updated, because the startUpdating is async. The current implementation might be ok for as a short term solution, but we need some mechanism to have a sync fetch. Taking Battery::level() as example, I think we could have something like BatteryDispatcher::instance.sampleData(WebBatteryStatus&) in case no local m_batteryStatus is available. wdyt? https://codereview.chromium.org/212643010/diff/1/public/platform/WebBatterySt... File public/platform/WebBatteryStatus.h (right): https://codereview.chromium.org/212643010/diff/1/public/platform/WebBatterySt... public/platform/WebBatteryStatus.h:12: WebBatteryStatus() we'll probably need a corresponding .cpp file, but can add this later. https://codereview.chromium.org/212643010/diff/1/public/platform/WebBatterySt... public/platform/WebBatteryStatus.h:13: : charging(true) if the idea is to set the values to 'undefined; it' probably better to set level=1, chargingTime=dischargingTime=+inf, which would be more in line with the spec.
Thx a lot Tim Volodine for your review. https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... File Source/modules/battery/BatteryDispatcher.cpp (right): https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryDispatcher.cpp:45: else if (status->charging() && m_batteryStatus->chargingTime() != status->chargingTime()) On 2014/04/02 14:24:51, timvolodine wrote: > do you need an 'else if' here? looks like this should be just an 'if'. Yes, I think I over thought this. Ill fix this. https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... File Source/modules/battery/BatteryDispatcher.h (right): https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryDispatcher.h:27: void didChangeBatteryStatus(const AtomicString& eventType, PassRefPtr<BatteryStatus>); On 2014/04/02 14:24:51, timvolodine wrote: > does this method need to be public? Nope. I'll fix this. https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... File Source/modules/battery/BatteryManager.cpp (right): https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryManager.cpp:34: startUpdating(); On 2014/04/02 14:24:51, timvolodine wrote: > yeah this is a bit tricky. Think it's generally ok to start updating here, but > note that BatteryManager::level() will not necessarily have m_batteryStatus > updated, because the startUpdating is async. The current implementation might be > ok for as a short term solution, but we need some mechanism to have a sync > fetch. > Taking Battery::level() as example, I think we could have something like > BatteryDispatcher::instance.sampleData(WebBatteryStatus&) in case no local > m_batteryStatus is available. wdyt? The spec mentions a default value when the battery status isn't available. I have made them the default values here, just before they are fetched/updated. IMO this should be sufficient in that small interval. https://codereview.chromium.org/212643010/diff/1/public/platform/WebBatterySt... File public/platform/WebBatteryStatus.h (right): https://codereview.chromium.org/212643010/diff/1/public/platform/WebBatterySt... public/platform/WebBatteryStatus.h:12: WebBatteryStatus() On 2014/04/02 14:24:51, timvolodine wrote: > we'll probably need a corresponding .cpp file, but can add this later. I don't understand completly, but Im still catching up with a lot of code, may be I'll read it up. Thx for pointing this. https://codereview.chromium.org/212643010/diff/1/public/platform/WebBatterySt... public/platform/WebBatteryStatus.h:13: : charging(true) On 2014/04/02 14:24:51, timvolodine wrote: > if the idea is to set the values to 'undefined; it' probably better to set > level=1, chargingTime=dischargingTime=+inf, which would be more in line with the > spec. Yes, that is what I remembered to have set everywhere. I'll fix this.
some more comments in addition to those from yesterday. https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... File Source/modules/battery/BatteryManager.cpp (right): https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryManager.cpp:60: return m_batteryStatus ? m_batteryStatus->level() : 1; Also batteryStatus appears to be stored twice in both the dispatcher and the manager. I think we should avoid this by removing m_batteryStatus from the BatteryManager and simply delegate calls like BatteryManagers::level() to BatteryDispatcher: { if (BatteryStatus* lastData = BatteryDispatcher::instance::getLatestData()){ return lastData->level(); } return 1; } this would likely simplify the code a bit. Also we need a one-shot callback upon first registration if last data is available. Simply providing hasLastData() and getLastEvent() in BatteryManager would solve that issue. You can have a look at DeviceOrientationDispatcher/Controller for reference.
On 2014/04/03 13:48:15, timvolodine wrote: > some more comments in addition to those from yesterday. > > https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... > File Source/modules/battery/BatteryManager.cpp (right): > > https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... > Source/modules/battery/BatteryManager.cpp:60: return m_batteryStatus ? > m_batteryStatus->level() : 1; > Also batteryStatus appears to be stored twice in both the dispatcher and the > manager. I think we should avoid this by removing m_batteryStatus from the > BatteryManager and simply delegate calls like BatteryManagers::level() to > BatteryDispatcher: > { > if (BatteryStatus* lastData = BatteryDispatcher::instance::getLatestData()){ > return lastData->level(); > } > return 1; > } > this would likely simplify the code a bit. > Also we need a one-shot callback upon first registration if last data is > available. Simply providing hasLastData() and getLastEvent() in BatteryManager > would solve that issue. You can have a look at > DeviceOrientationDispatcher/Controller for reference. Thanks for your valuable comments. Ill see the DeviceOrientation code and fix this
On 2014/04/03 13:48:15, timvolodine wrote: > some more comments in addition to those from yesterday. > > https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... > File Source/modules/battery/BatteryManager.cpp (right): > > https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... > Source/modules/battery/BatteryManager.cpp:60: return m_batteryStatus ? > m_batteryStatus->level() : 1; > Also batteryStatus appears to be stored twice in both the dispatcher and the > manager. I think we should avoid this by removing m_batteryStatus from the > BatteryManager and simply delegate calls like BatteryManagers::level() to > BatteryDispatcher: > { > if (BatteryStatus* lastData = BatteryDispatcher::instance::getLatestData()){ > return lastData->level(); > } > return 1; > } > this would likely simplify the code a bit. I'll fix this. > Also we need a one-shot callback upon first registration if last data is > available. Simply providing hasLastData() and getLastEvent() in BatteryManager > would solve that issue. You can have a look at > DeviceOrientationDispatcher/Controller for reference. <sorry for late response, I was away all last week, with access to only email> I didn't provide hasLastData() in BatteryManager, because the DeviceSensorEventController dispatches the event on the window where as the battery manager dispatches on itself. I kept the code here similar to Gamepad here. But I get your point. I need to fire an event, just after the controller is added. May be I can fire event at BatteryDispatcher::addClient after the controller is added. Thanks for pointing this out.
On 2014/04/07 07:35:07, Srini wrote: > On 2014/04/03 13:48:15, timvolodine wrote: > > some more comments in addition to those from yesterday. > > > > > https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... > > File Source/modules/battery/BatteryManager.cpp (right): > > > > > https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... > > Source/modules/battery/BatteryManager.cpp:60: return m_batteryStatus ? > > m_batteryStatus->level() : 1; > > Also batteryStatus appears to be stored twice in both the dispatcher and the > > manager. I think we should avoid this by removing m_batteryStatus from the > > BatteryManager and simply delegate calls like BatteryManagers::level() to > > BatteryDispatcher: > > { > > if (BatteryStatus* lastData = BatteryDispatcher::instance::getLatestData()){ > > return lastData->level(); > > } > > return 1; > > } > > this would likely simplify the code a bit. > > I'll fix this. > > > Also we need a one-shot callback upon first registration if last data is > > available. Simply providing hasLastData() and getLastEvent() in BatteryManager > > would solve that issue. You can have a look at > > DeviceOrientationDispatcher/Controller for reference. > > <sorry for late response, I was away all last week, with access to only email> > > I didn't provide hasLastData() in BatteryManager, because the > DeviceSensorEventController dispatches the event on the window where as the > battery manager dispatches on itself. I kept the code here similar to Gamepad > here. > > But I get your point. I need to fire an event, just after the controller is > added. May be I can fire event at BatteryDispatcher::addClient after the > controller is added. Thanks for pointing this out. I see, in that case I think we can refactor the DeviceSensorEventController class a bit to allow for generic dispatches. Instead of calling dispatchDeviceEvent(getLastEvent) in DeviceSensorEventController::fireDeviceEvent, it would call a pure virtual method e.g. dispatchDeviceSensorEvent(..), which you could define inside BatteryManager and dispatch however you want. It looks like that's the only difference with events that dispatch on window. If that helps I can prepare a patch for this. wdyt?
Some feedback on the use of Oilpan transition types. https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... File Source/modules/battery/BatteryDispatcher.cpp (right): https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryDispatcher.cpp:63: RefPtr<Event> event = Event::create(eventType); Change RefPtr<Event> to the Oilpan transition type RefPtrWillBeRawPtr<Event> https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... File Source/modules/battery/BatteryManager.cpp (right): https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryManager.cpp:18: RefPtr<BatteryManager> batteryManager(adoptRefWillBeRefCountedGarbageCollected(new BatteryManager(context))); Use RefPtrWillBeRawPtr<> instead of RefPtr<>. https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryManager.cpp:63: void BatteryManager::didChangeBatteryStatus(PassRefPtr<Event> event, PassRefPtr<BatteryStatus> batteryStatus) Change PassRefPtr<Event> to PassRefPtrWillBeRawPtr<Event> here also. https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryManager.cpp:87: PassRefPtr<Event> BatteryManager::getLastEvent() Change this to PassRefPtrWillBeRawPtr<Event> https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... File Source/modules/battery/BatteryManager.h (right): https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryManager.h:39: void didChangeBatteryStatus(PassRefPtr<Event>, PassRefPtr<BatteryStatus>); Change PassRefPtr<Event> to PassRefPtrWillBeRawPtr<Event> https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryManager.h:48: virtual void registerWithDispatcher() OVERRIDE FINAL; Given that the class is decorated with FINAL, this shouldn't be needed. https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... Source/modules/battery/BatteryManager.h:51: virtual PassRefPtr<Event> getLastEvent() OVERRIDE FINAL; Change the return type to PassRefPtrWillBeRawPtr<Event> to make this compile with Oilpan enabled (and override the method on DeviceSensorEventController.)
On 2014/04/07 12:34:43, timvolodine wrote: > On 2014/04/07 07:35:07, Srini wrote: > > On 2014/04/03 13:48:15, timvolodine wrote: > > > some more comments in addition to those from yesterday. > > > > > > > > > https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... > > > File Source/modules/battery/BatteryManager.cpp (right): > > > > > > > > > https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... > > > Source/modules/battery/BatteryManager.cpp:60: return m_batteryStatus ? > > > m_batteryStatus->level() : 1; > > > Also batteryStatus appears to be stored twice in both the dispatcher and the > > > manager. I think we should avoid this by removing m_batteryStatus from the > > > BatteryManager and simply delegate calls like BatteryManagers::level() to > > > BatteryDispatcher: > > > { > > > if (BatteryStatus* lastData = > BatteryDispatcher::instance::getLatestData()){ > > > return lastData->level(); > > > } > > > return 1; > > > } > > > this would likely simplify the code a bit. > > > > I'll fix this. > > > > > Also we need a one-shot callback upon first registration if last data is > > > available. Simply providing hasLastData() and getLastEvent() in > BatteryManager > > > would solve that issue. You can have a look at > > > DeviceOrientationDispatcher/Controller for reference. > > > > <sorry for late response, I was away all last week, with access to only email> > > > > I didn't provide hasLastData() in BatteryManager, because the > > DeviceSensorEventController dispatches the event on the window where as the > > battery manager dispatches on itself. I kept the code here similar to Gamepad > > here. > > > > But I get your point. I need to fire an event, just after the controller is > > added. May be I can fire event at BatteryDispatcher::addClient after the > > controller is added. Thanks for pointing this out. > > I see, in that case I think we can refactor the DeviceSensorEventController > class a bit to allow for generic dispatches. > Instead of calling dispatchDeviceEvent(getLastEvent) in > DeviceSensorEventController::fireDeviceEvent, it would call a pure virtual > method e.g. dispatchDeviceSensorEvent(..), which you could define inside > BatteryManager and dispatch however you want. It looks like that's the only > difference with events that dispatch on window. If that helps I can prepare a > patch for this. wdyt? That would be awesome. Gamepad would also benefit from this iiuc.
On 2014/04/07 12:52:44, sof wrote: > Some feedback on the use of Oilpan transition types. > > https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... > File Source/modules/battery/BatteryDispatcher.cpp (right): > > https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... > Source/modules/battery/BatteryDispatcher.cpp:63: RefPtr<Event> event = > Event::create(eventType); > Change RefPtr<Event> to the Oilpan transition type RefPtrWillBeRawPtr<Event> > > https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... > File Source/modules/battery/BatteryManager.cpp (right): > > https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... > Source/modules/battery/BatteryManager.cpp:18: RefPtr<BatteryManager> > batteryManager(adoptRefWillBeRefCountedGarbageCollected(new > BatteryManager(context))); > Use RefPtrWillBeRawPtr<> instead of RefPtr<>. > > https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... > Source/modules/battery/BatteryManager.cpp:63: void > BatteryManager::didChangeBatteryStatus(PassRefPtr<Event> event, > PassRefPtr<BatteryStatus> batteryStatus) > Change PassRefPtr<Event> to PassRefPtrWillBeRawPtr<Event> here also. > > https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... > Source/modules/battery/BatteryManager.cpp:87: PassRefPtr<Event> > BatteryManager::getLastEvent() > Change this to PassRefPtrWillBeRawPtr<Event> > > https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... > File Source/modules/battery/BatteryManager.h (right): > > https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... > Source/modules/battery/BatteryManager.h:39: void > didChangeBatteryStatus(PassRefPtr<Event>, PassRefPtr<BatteryStatus>); > Change PassRefPtr<Event> to PassRefPtrWillBeRawPtr<Event> > > https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... > Source/modules/battery/BatteryManager.h:48: virtual void > registerWithDispatcher() OVERRIDE FINAL; > Given that the class is decorated with FINAL, this shouldn't be needed. > > https://codereview.chromium.org/212643010/diff/1/Source/modules/battery/Batte... > Source/modules/battery/BatteryManager.h:51: virtual PassRefPtr<Event> > getLastEvent() OVERRIDE FINAL; > Change the return type to PassRefPtrWillBeRawPtr<Event> to make this compile > with Oilpan enabled (and override the method on DeviceSensorEventController.) Thanks for your review. I will incorporate this tonight and update the patch along with Tim's comments.
> I see, in that case I think we can refactor the DeviceSensorEventController > class a bit to allow for generic dispatches. > Instead of calling dispatchDeviceEvent(getLastEvent) in > DeviceSensorEventController::fireDeviceEvent, it would call a pure virtual > method e.g. dispatchDeviceSensorEvent(..), which you could define inside > BatteryManager and dispatch however you want. It looks like that's the only > difference with events that dispatch on window. If that helps I can prepare a > patch for this. wdyt? I updated other comments except for this.
Looks fine wrt Oilpan types; thanks.
Thanks Srini! Looks much better, just a few more comments below. https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... File Source/modules/battery/BatteryDispatcher.cpp (right): https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryDispatcher.cpp:40: RefPtr<BatteryStatus> status = BatteryStatus::create(batteryStatus.charging, batteryStatus.chargingTime, batteryStatus.dischargingTime, batteryStatus.level); could you just do: RefPtr<BatteryStatus> oldStatus = m_batteryStatus; m_batteryStatus = BatteryStatus::create(...); https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryDispatcher.cpp:57: didChangeBatteryStatus(EventTypeNames::chargingchange); also fire dischargingchange? https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryDispatcher.cpp:65: if (m_batteryStatus) no need for if, just return m_batteryStatus.get(); https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... File Source/modules/battery/BatteryManager.cpp (right): https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:50: ASSERT(lastData->level() != 1.0 && lastData->chargingTime() == 0.0); please remove this assert, as you mention in the comment it is the task of the 'backend' to provide proper data, the task here is just to propagate. https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:55: return 0; should this be infinity as below? https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:93: return false; call to dispatcher. I've uploaded https://codereview.chromium.org/231003002/ to allow overrides of dispatchDeviceEvent(), please override it in this class to call dispatchEvent (instead of the DOMWindow as by default). https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:99: return nullptr; call to dispatcher + return create event https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:104: return true; return false; I don't think we have the concept of null events for battery.
Hi Tim, Thanks for your comments. I will incorporate them. I have a few new doubts, can you go through my comments and see if they are of concern? Thanks, -Srini. https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... File Source/modules/battery/BatteryDispatcher.cpp (right): https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryDispatcher.cpp:40: RefPtr<BatteryStatus> status = BatteryStatus::create(batteryStatus.charging, batteryStatus.chargingTime, batteryStatus.dischargingTime, batteryStatus.level); On 2014/04/09 15:19:18, timvolodine wrote: > could you just do: > RefPtr<BatteryStatus> oldStatus = m_batteryStatus; > m_batteryStatus = BatteryStatus::create(...); I thought of this, but then I wanted to find the case of no previous status, i.e no m_batteryStatus and getting it fresh from the platform. May be I can find that save in a flag before I do this. I'll fix this. https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryDispatcher.cpp:57: didChangeBatteryStatus(EventTypeNames::chargingchange); On 2014/04/09 15:19:18, timvolodine wrote: > also fire dischargingchange? This is the first time event fire. I wasn't sure, should I emit all or just send few. May be I should check values between default and actual and fire accordingly? https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryDispatcher.cpp:65: if (m_batteryStatus) On 2014/04/09 15:19:18, timvolodine wrote: > no need for if, just return m_batteryStatus.get(); ok. https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... File Source/modules/battery/BatteryManager.cpp (right): https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:50: ASSERT(lastData->level() != 1.0 && lastData->chargingTime() == 0.0); On 2014/04/09 15:19:18, timvolodine wrote: > please remove this assert, as you mention in the comment it is the task of the > 'backend' to provide proper data, the task here is just to propagate. Ok. https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:55: return 0; On 2014/04/09 15:19:18, timvolodine wrote: > should this be infinity as below? I think that default is 0 when the charging default is true. I filled in the default values from the spec. This is what the assert checks above. https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:93: return false; On 2014/04/09 15:19:18, timvolodine wrote: > call to dispatcher. > I've uploaded https://codereview.chromium.org/231003002/ to allow overrides of > dispatchDeviceEvent(), please override it in this class to call dispatchEvent > (instead of the DOMWindow as by default). Ill add this once your patch lands. Thanks for this. https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:99: return nullptr; On 2014/04/09 15:19:18, timvolodine wrote: > call to dispatcher + return create event I think I need some help here. Now that I was trying to incorporate this, this will be called startUpdating runs. But this will send only one event. I may be required to send more than one. I was thinking that I should return null here, and I will send 4 events for all the 4 properties in the overridden function (there is no way to figure out what changed since we last stopped updating). Or should I store something (which I removed). Or am I confusing it? What do you think? https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:104: return true; On 2014/04/09 15:19:18, timvolodine wrote: > return false; > I don't think we have the concept of null events for battery. Ok.
https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... File Source/modules/battery/BatteryDispatcher.cpp (right): https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryDispatcher.cpp:40: RefPtr<BatteryStatus> status = BatteryStatus::create(batteryStatus.charging, batteryStatus.chargingTime, batteryStatus.dischargingTime, batteryStatus.level); On 2014/04/09 16:09:57, Srini wrote: > On 2014/04/09 15:19:18, timvolodine wrote: > > could you just do: > > RefPtr<BatteryStatus> oldStatus = m_batteryStatus; > > m_batteryStatus = BatteryStatus::create(...); > > I thought of this, but then I wanted to find the case of no previous status, i.e > no m_batteryStatus and getting it fresh from the platform. May be I can find > that save in a flag before I do this. I'll fix this. I am not exactly sure what you mean, can you just use if(oldStatus) to see if there is anything previously. https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryDispatcher.cpp:57: didChangeBatteryStatus(EventTypeNames::chargingchange); On 2014/04/09 16:09:57, Srini wrote: > On 2014/04/09 15:19:18, timvolodine wrote: > > also fire dischargingchange? > > This is the first time event fire. I wasn't sure, should I emit all or just send > few. May be I should check values between default and actual and fire > accordingly? I think you should fire all, if chrome cannot provide data for some reason it can still fire with default values. https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... File Source/modules/battery/BatteryManager.cpp (right): https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:32: // Need to figure out a way to startUpdating only afters EventListeners are added: didAddEventListener? also, we need to think about this as it might be a problem. events could get lost if no listener is registered at the time of callback.. https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:55: return 0; On 2014/04/09 16:09:57, Srini wrote: > On 2014/04/09 15:19:18, timvolodine wrote: > > should this be infinity as below? > > I think that default is 0 when the charging default is true. I filled in the > default values from the spec. This is what the assert checks above. I thought the default was +inf, but looking at the spec now you are probably right. https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:99: return nullptr; On 2014/04/09 16:09:57, Srini wrote: > On 2014/04/09 15:19:18, timvolodine wrote: > > call to dispatcher + return create event > > I think I need some help here. Now that I was trying to incorporate this, this > will be called startUpdating runs. But this will send only one event. I may be > required to send more than one. I was thinking that I should return null here, > and I will send 4 events for all the 4 properties in the overridden function > (there is no way to figure out what changed since we last stopped updating). Or > should I store something (which I removed). Or am I confusing it? What do you > think? yeah, I see the problem. The DeviceSensorEventController was designed to fire only one event. I guess you could circumvent this by having nullptr here and making sure the overriden dispatchDeviceEvent() method dispatches all events when called with null. Alternatively we could consider extending getLastEvent to getLastEvents and return multiple events there, but that would require some refactoring.
On 2014/04/09 18:59:01, timvolodine wrote: > https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... > File Source/modules/battery/BatteryDispatcher.cpp (right): > > https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... > Source/modules/battery/BatteryDispatcher.cpp:40: RefPtr<BatteryStatus> status = > BatteryStatus::create(batteryStatus.charging, batteryStatus.chargingTime, > batteryStatus.dischargingTime, batteryStatus.level); > On 2014/04/09 16:09:57, Srini wrote: > > On 2014/04/09 15:19:18, timvolodine wrote: > > > could you just do: > > > RefPtr<BatteryStatus> oldStatus = m_batteryStatus; > > > m_batteryStatus = BatteryStatus::create(...); > > > > I thought of this, but then I wanted to find the case of no previous status, > i.e > > no m_batteryStatus and getting it fresh from the platform. May be I can find > > that save in a flag before I do this. I'll fix this. > > I am not exactly sure what you mean, can you just use if(oldStatus) to see if > there is anything previously. Ignore it, I was confusing myself. Ill fix this. > > https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... > Source/modules/battery/BatteryDispatcher.cpp:57: > didChangeBatteryStatus(EventTypeNames::chargingchange); > On 2014/04/09 16:09:57, Srini wrote: > > On 2014/04/09 15:19:18, timvolodine wrote: > > > also fire dischargingchange? > > > > This is the first time event fire. I wasn't sure, should I emit all or just > send > > few. May be I should check values between default and actual and fire > > accordingly? > > I think you should fire all, if chrome cannot provide data for some reason it > can still fire with default values. Ok. > > https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... > File Source/modules/battery/BatteryManager.cpp (right): > > https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... > Source/modules/battery/BatteryManager.cpp:32: // Need to figure out a way to > startUpdating only afters EventListeners are added: didAddEventListener? > > also, we need to think about this as it might be a problem. events could get > lost if no listener is registered at the time of callback.. As long as we do startUpdating, I dont think we'd miss events. That comment is to optimize to start listening only&after event listeners are added. > > https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... > Source/modules/battery/BatteryManager.cpp:55: return 0; > On 2014/04/09 16:09:57, Srini wrote: > > On 2014/04/09 15:19:18, timvolodine wrote: > > > should this be infinity as below? > > > > I think that default is 0 when the charging default is true. I filled in the > > default values from the spec. This is what the assert checks above. > > I thought the default was +inf, but looking at the spec now you are probably > right. > > https://codereview.chromium.org/212643010/diff/20001/Source/modules/battery/B... > Source/modules/battery/BatteryManager.cpp:99: return nullptr; > On 2014/04/09 16:09:57, Srini wrote: > > On 2014/04/09 15:19:18, timvolodine wrote: > > > call to dispatcher + return create event > > > > I think I need some help here. Now that I was trying to incorporate this, this > > will be called startUpdating runs. But this will send only one event. I may be > > required to send more than one. I was thinking that I should return null here, > > and I will send 4 events for all the 4 properties in the overridden function > > (there is no way to figure out what changed since we last stopped updating). > Or > > should I store something (which I removed). Or am I confusing it? What do you > > think? > > yeah, I see the problem. The DeviceSensorEventController was designed to fire > only one event. I guess you could circumvent this by having nullptr here and > making sure the overriden dispatchDeviceEvent() method dispatches all events > when called with null. Alternatively we could consider extending getLastEvent to > getLastEvents and return multiple events there, but that would require some > refactoring. For now, Ill do it in the dispatchDeviceEvent. It is probably simple & clean atm.
> > > > yeah, I see the problem. The DeviceSensorEventController was designed to fire > > only one event. I guess you could circumvent this by having nullptr here and > > making sure the overriden dispatchDeviceEvent() method dispatches all events > > when called with null. Alternatively we could consider extending getLastEvent > to > > getLastEvents and return multiple events there, but that would require some > > refactoring. > > For now, Ill do it in the dispatchDeviceEvent. It is probably simple & clean > atm. I meant in the overridden function.
On 2014/04/09 20:22:52, Srini wrote: > > > > > > yeah, I see the problem. The DeviceSensorEventController was designed to > fire > > > only one event. I guess you could circumvent this by having nullptr here and > > > making sure the overriden dispatchDeviceEvent() method dispatches all events > > > when called with null. Alternatively we could consider extending > getLastEvent > > to > > > getLastEvents and return multiple events there, but that would require some > > > refactoring. > > > > For now, Ill do it in the dispatchDeviceEvent. It is probably simple & clean > > atm. > > I meant in the overridden function. I fixed everything else pending overriding dispatchDeviceEvent. Ill add this once your patch gets in. Thanks Tim.
On 2014/04/10 08:08:00, Srini wrote: > On 2014/04/09 20:22:52, Srini wrote: > > > > > > > > yeah, I see the problem. The DeviceSensorEventController was designed to > > fire > > > > only one event. I guess you could circumvent this by having nullptr here > and > > > > making sure the overriden dispatchDeviceEvent() method dispatches all > events > > > > when called with null. Alternatively we could consider extending > > getLastEvent > > > to > > > > getLastEvents and return multiple events there, but that would require > some > > > > refactoring. > > > > > > For now, Ill do it in the dispatchDeviceEvent. It is probably simple & clean > > > atm. > > > > I meant in the overridden function. > > I fixed everything else pending overriding dispatchDeviceEvent. Ill add this > once your patch gets in. Thanks Tim. Thanks Srini! I've added a comment re overriding in https://codereview.chromium.org/212643010/.
On 2014/04/10 12:14:10, timvolodine wrote: > On 2014/04/10 08:08:00, Srini wrote: > > On 2014/04/09 20:22:52, Srini wrote: > > > > > > > > > > yeah, I see the problem. The DeviceSensorEventController was designed to > > > fire > > > > > only one event. I guess you could circumvent this by having nullptr here > > and > > > > > making sure the overriden dispatchDeviceEvent() method dispatches all > > events > > > > > when called with null. Alternatively we could consider extending > > > getLastEvent > > > > to > > > > > getLastEvents and return multiple events there, but that would require > > some > > > > > refactoring. > > > > > > > > For now, Ill do it in the dispatchDeviceEvent. It is probably simple & > clean > > > > atm. > > > > > > I meant in the overridden function. > > > > I fixed everything else pending overriding dispatchDeviceEvent. Ill add this > > once your patch gets in. Thanks Tim. > > Thanks Srini! I've added a comment re overriding in > https://codereview.chromium.org/212643010/. With this overriding not required, all the review comments are fixed, let me know if anything else needs is missed/to be looked at. Thanks! -Srini.
just a few questions below https://codereview.chromium.org/212643010/diff/40001/Source/modules/battery/B... File Source/modules/battery/BatteryDispatcher.cpp (right): https://codereview.chromium.org/212643010/diff/40001/Source/modules/battery/B... Source/modules/battery/BatteryDispatcher.cpp:86: { notImplemented(); https://codereview.chromium.org/212643010/diff/40001/Source/modules/battery/B... Source/modules/battery/BatteryDispatcher.cpp:91: { notImplemented(); https://codereview.chromium.org/212643010/diff/40001/Source/modules/battery/B... File Source/modules/battery/BatteryManager.cpp (right): https://codereview.chromium.org/212643010/diff/40001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:32: // Need to figure out a way to startUpdating only afters EventListeners are added: didAddEventListener? remove this line, we actually need to start updating here in order to satisfy potential direct calls like level() in the future. https://codereview.chromium.org/212643010/diff/40001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:93: // We would fire events directly. could you add that this happens through BatteryManager::didChangeBatteryStatus only. https://codereview.chromium.org/212643010/diff/40001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:103: { how does this work with page visibility? should the m_hasEventListener=false here and true on resume()? https://codereview.chromium.org/212643010/diff/40001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:113: { is it possible to resume() after stop()? do we need m_hasEventListener = false here?
Just fixed those and updated the patch. Thanks! -Srini. https://codereview.chromium.org/212643010/diff/40001/Source/modules/battery/B... File Source/modules/battery/BatteryDispatcher.cpp (right): https://codereview.chromium.org/212643010/diff/40001/Source/modules/battery/B... Source/modules/battery/BatteryDispatcher.cpp:86: { On 2014/04/14 17:51:57, timvolodine wrote: > notImplemented(); Ill fix both. https://codereview.chromium.org/212643010/diff/40001/Source/modules/battery/B... File Source/modules/battery/BatteryManager.cpp (right): https://codereview.chromium.org/212643010/diff/40001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:93: // We would fire events directly. On 2014/04/14 17:51:57, timvolodine wrote: > could you add that this happens through BatteryManager::didChangeBatteryStatus > only. Sure. https://codereview.chromium.org/212643010/diff/40001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:103: { On 2014/04/14 17:51:57, timvolodine wrote: > how does this work with page visibility? > should the m_hasEventListener=false here and true on resume()? Yes, we should have it false here and true on resume() and the same on stop(). Ill change it accordingly.
looks ok to me at least functionally ;) https://codereview.chromium.org/212643010/diff/60001/Source/modules/battery/B... File Source/modules/battery/BatteryDispatcher.cpp (right): https://codereview.chromium.org/212643010/diff/60001/Source/modules/battery/B... Source/modules/battery/BatteryDispatcher.cpp:55: didChangeBatteryStatus(EventTypeNames::chargingchange); I guess if there is no listener this will be a noop at EventTarget which seems ok. https://codereview.chromium.org/212643010/diff/60001/Source/modules/battery/B... File Source/modules/battery/BatteryDispatcher.h (right): https://codereview.chromium.org/212643010/diff/60001/Source/modules/battery/B... Source/modules/battery/BatteryDispatcher.h:32: RefPtr<BatteryStatus> m_batteryStatus; I think the convention is to put this after constructors and functions.
On 2014/04/15 14:26:53, timvolodine wrote: > looks ok to me at least functionally ;) > > https://codereview.chromium.org/212643010/diff/60001/Source/modules/battery/B... > File Source/modules/battery/BatteryDispatcher.cpp (right): > > https://codereview.chromium.org/212643010/diff/60001/Source/modules/battery/B... > Source/modules/battery/BatteryDispatcher.cpp:55: > didChangeBatteryStatus(EventTypeNames::chargingchange); > I guess if there is no listener this will be a noop at EventTarget which seems > ok. > > https://codereview.chromium.org/212643010/diff/60001/Source/modules/battery/B... > File Source/modules/battery/BatteryDispatcher.h (right): > > https://codereview.chromium.org/212643010/diff/60001/Source/modules/battery/B... > Source/modules/battery/BatteryDispatcher.h:32: RefPtr<BatteryStatus> > m_batteryStatus; > I think the convention is to put this after constructors and functions. Fixed it. Thanks Tim.
lgtm you'll need approval from blink owners to land this.
On 2014/04/16 17:24:06, timvolodine wrote: > lgtm > > > you'll need approval from blink owners to land this. Thanks Tim. I'll wait for an approval from blink owners.
On 2014/04/16 18:02:29, Srini wrote: > On 2014/04/16 17:24:06, timvolodine wrote: > > lgtm > > > > > > you'll need approval from blink owners to land this. > > Thanks Tim. I'll wait for an approval from blink owners. ojan peter adamk abarth : can one of you review and give me an approval? Thanks!
I believe you'll need abarth for this one, as he's the only one of us who's an OWNER in public/
lgtm https://codereview.chromium.org/212643010/diff/80001/Source/modules/battery/B... File Source/modules/battery/BatteryDispatcher.cpp (right): https://codereview.chromium.org/212643010/diff/80001/Source/modules/battery/B... Source/modules/battery/BatteryDispatcher.cpp:89: // blink::Platform::current()->setBatteryListener(this); Can you remove this commented out code? We can add it in the future when it's needed. :) https://codereview.chromium.org/212643010/diff/80001/Source/modules/battery/B... Source/modules/battery/BatteryDispatcher.cpp:95: // blink::Platform::current()->setBatteryListener(0); ditto https://codereview.chromium.org/212643010/diff/80001/Source/modules/battery/B... File Source/modules/battery/BatteryManager.cpp (right): https://codereview.chromium.org/212643010/diff/80001/Source/modules/battery/B... Source/modules/battery/BatteryManager.cpp:25: stopUpdating(); Does a test fail if you remove this line?
On 2014/04/17 23:02:15, abarth wrote: > lgtm > > https://codereview.chromium.org/212643010/diff/80001/Source/modules/battery/B... > File Source/modules/battery/BatteryDispatcher.cpp (right): > > https://codereview.chromium.org/212643010/diff/80001/Source/modules/battery/B... > Source/modules/battery/BatteryDispatcher.cpp:89: // > blink::Platform::current()->setBatteryListener(this); > Can you remove this commented out code? We can add it in the future when it's > needed. :) Sure. > > https://codereview.chromium.org/212643010/diff/80001/Source/modules/battery/B... > Source/modules/battery/BatteryDispatcher.cpp:95: // > blink::Platform::current()->setBatteryListener(0); > ditto Sure. > > https://codereview.chromium.org/212643010/diff/80001/Source/modules/battery/B... > File Source/modules/battery/BatteryManager.cpp (right): > > https://codereview.chromium.org/212643010/diff/80001/Source/modules/battery/B... > Source/modules/battery/BatteryManager.cpp:25: stopUpdating(); > Does a test fail if you remove this line? I'll run locally and check it up and update here. (Sorry the last 2 days are local holidays and Im slow on work). Thanks, -Srini.
On 2014/04/17 23:02:15, abarth wrote: > lgtm > > https://codereview.chromium.org/212643010/diff/80001/Source/modules/battery/B... > File Source/modules/battery/BatteryDispatcher.cpp (right): > > https://codereview.chromium.org/212643010/diff/80001/Source/modules/battery/B... > Source/modules/battery/BatteryDispatcher.cpp:89: // > blink::Platform::current()->setBatteryListener(this); > Can you remove this commented out code? We can add it in the future when it's > needed. :) > > https://codereview.chromium.org/212643010/diff/80001/Source/modules/battery/B... > Source/modules/battery/BatteryDispatcher.cpp:95: // > blink::Platform::current()->setBatteryListener(0); > ditto > > https://codereview.chromium.org/212643010/diff/80001/Source/modules/battery/B... > File Source/modules/battery/BatteryManager.cpp (right): > > https://codereview.chromium.org/212643010/diff/80001/Source/modules/battery/B... > Source/modules/battery/BatteryManager.cpp:25: stopUpdating(); > Does a test fail if you remove this line? I checked completely. Not a test a fails. Also I checked manually and displayed it as a counter and when I ran multiple tests in the same session. The controller gets unregistered at the dispatcher despite the removal of this. Im pushing the patch with this change also.
The CQ bit was checked by srinivasa.ragavan.venkateswaran@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....
Message was sent while issue was closed.
Change committed as 172021 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
