Add support to Battery Status API in blink.

Add support to Battery Status API in blink.

This patch adds basic Battery Manager to Navigator with default battery
status values and the test enables to test if those values are passed
correctly.

Intent-to-Implement link: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/fzoG6Phr09k/dnAEzkgMA1YJ

BUG=122593
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168632

[Srini, 2014-02-27 05:01:25]

Hi,

Can you review this patch. It breaks down the earlier patch and just submitting
the most basic patch and a test. I'll be happy to rework if you have any
comments.

Thanks,
-Srini.

[abarth-chromium, 2014-02-27 05:42:08]

Thanks for breaking this CL up into smaller pieces.

This API appears to provide synchronous access to battery information.  For
example, navigator.battery.level seems like it needs to change over time.  I
understand that's not in this CL, but I'd like to understand how that will work.
 Presumably we're not going to issue a synchronous IPC call to retrieve this
information....

Wouldn't this API make more sense if it was based on Promises?  That way you'd
ask for the battery level, and the browser can fulfill the promise once it
knows.

Finally, this CL is missing a runtime feature flag.  If we landed this CL as
written, we'd expose this API to the web immediately.  Please take a look at
RuntimeEnabledFeatures.in to see how to add a runtime flag for this feature. 
The status should be set to "test" initially.

https://codereview.chromium.org/182613002/diff/1/Source/modules/battery/Batte...
File Source/modules/battery/BatteryManager.cpp (right):

https://codereview.chromium.org/182613002/diff/1/Source/modules/battery/Batte...
Source/modules/battery/BatteryManager.cpp:105: }
Given that you don't implement these functions, there's no reason to subclass
ActiveDOMObject.

https://codereview.chromium.org/182613002/diff/1/Source/modules/battery/Batte...
File Source/modules/battery/BatteryManager.h (right):

https://codereview.chromium.org/182613002/diff/1/Source/modules/battery/Batte...
Source/modules/battery/BatteryManager.h:19: */
Please use this license for new files:

http://dev.chromium.org/blink/coding-style#TOC-License

https://codereview.chromium.org/182613002/diff/1/Source/modules/battery/Batte...
Source/modules/battery/BatteryManager.h:38: static PassRefPtr<BatteryManager>
create(Frame*);
Why are there two create functions?  It seems like one create function should be
enough.

https://codereview.chromium.org/182613002/diff/1/Source/modules/battery/Batte...
File Source/modules/battery/BatteryStatus.h (right):

https://codereview.chromium.org/182613002/diff/1/Source/modules/battery/Batte...
Source/modules/battery/BatteryStatus.h:29: class BatteryStatus : public
RefCounted<BatteryStatus> {
Why is this object RefCounted?  It looks like there's only ever one reference. 
We should use OwnPtr rather than RefPtr.

https://codereview.chromium.org/182613002/diff/1/Source/modules/battery/Navig...
File Source/modules/battery/NavigatorBattery.cpp (right):

https://codereview.chromium.org/182613002/diff/1/Source/modules/battery/Navig...
Source/modules/battery/NavigatorBattery.cpp:30:
NavigatorBattery::NavigatorBattery(Frame *frame)
"Frame *"  ->  "Frame*"

[Srini, 2014-02-27 06:42:30]

On 2014/02/27 05:42:08, abarth wrote:
> Thanks for breaking this CL up into smaller pieces.
> 
> This API appears to provide synchronous access to battery information.  For
> example, navigator.battery.level seems like it needs to change over time.  I
> understand that's not in this CL, but I'd like to understand how that will
work.
>  Presumably we're not going to issue a synchronous IPC call to retrieve this
> information....

The navigator.battery has a default value. The battery manager registers for
updates to the controller and the controller notifies the new values to update
from default. When the controller gets update from the system the values at the
battery manager are updated. navigator.battery.level return from the last
updated/notified value. I hope Im explaining it as the code/design is.

> 
> Wouldn't this API make more sense if it was based on Promises?  That way you'd
> ask for the battery level, and the browser can fulfill the promise once it
> knows.

This was discussed on the intent-to-implement thread, but it was discussed that
these arent one time events and not suitable for Promises.
https://groups.google.com/a/chromium.org/d/msg/blink-dev/fzoG6Phr09k/i4uuhfec...

> 
> Finally, this CL is missing a runtime feature flag.  If we landed this CL as
> written, we'd expose this API to the web immediately.  Please take a look at
> RuntimeEnabledFeatures.in to see how to add a runtime flag for this feature. 
> The status should be set to "test" initially.

I'll fix all the comments below and submit again asap. Thanks a lot for the
review.

> 
>
https://codereview.chromium.org/182613002/diff/1/Source/modules/battery/Batte...
> File Source/modules/battery/BatteryManager.cpp (right):
> 
>
https://codereview.chromium.org/182613002/diff/1/Source/modules/battery/Batte...
> Source/modules/battery/BatteryManager.cpp:105: }
> Given that you don't implement these functions, there's no reason to subclass
> ActiveDOMObject.
> 
>
https://codereview.chromium.org/182613002/diff/1/Source/modules/battery/Batte...
> File Source/modules/battery/BatteryManager.h (right):
> 
>
https://codereview.chromium.org/182613002/diff/1/Source/modules/battery/Batte...
> Source/modules/battery/BatteryManager.h:19: */
> Please use this license for new files:
> 
> http://dev.chromium.org/blink/coding-style#TOC-License
> 
>
https://codereview.chromium.org/182613002/diff/1/Source/modules/battery/Batte...
> Source/modules/battery/BatteryManager.h:38: static PassRefPtr<BatteryManager>
> create(Frame*);
> Why are there two create functions?  It seems like one create function should
be
> enough.
> 
>
https://codereview.chromium.org/182613002/diff/1/Source/modules/battery/Batte...
> File Source/modules/battery/BatteryStatus.h (right):
> 
>
https://codereview.chromium.org/182613002/diff/1/Source/modules/battery/Batte...
> Source/modules/battery/BatteryStatus.h:29: class BatteryStatus : public
> RefCounted<BatteryStatus> {
> Why is this object RefCounted?  It looks like there's only ever one reference.

> We should use OwnPtr rather than RefPtr.
> 
>
https://codereview.chromium.org/182613002/diff/1/Source/modules/battery/Navig...
> File Source/modules/battery/NavigatorBattery.cpp (right):
> 
>
https://codereview.chromium.org/182613002/diff/1/Source/modules/battery/Navig...
> Source/modules/battery/NavigatorBattery.cpp:30:
> NavigatorBattery::NavigatorBattery(Frame *frame)
> "Frame *"  ->  "Frame*"

[abarth-chromium, 2014-02-27 06:49:35]

On 2014/02/27 06:42:30, Srini wrote:
> The navigator.battery has a default value. The battery manager registers for
> updates to the controller and the controller notifies the new values to update
> from default. When the controller gets update from the system the values at
the
> battery manager are updated. navigator.battery.level return from the last
> updated/notified value. I hope Im explaining it as the code/design is.

I see.  It seems like a better approach would be to match what device
orientation does.  Specifically, there aren't any getters for rotationRate. 
Instead, that data is delivered as a property of the event.  For this API, there
wouldn't be a navigator.battery.level.  Instead, the level would be delivered
with the onlevelchange event.  We could then use the addEventListener /
removeEventListener calls to know when we should be pushing information about
the battery level to a renderer.

Put another way, suppose a script reads navigator.battery.level.  How do we know
to update that value as the battery discharges?  When to we stop updating the
value?

[Srini, 2014-02-27 07:42:17]

On 2014/02/27 06:49:35, abarth wrote:
> On 2014/02/27 06:42:30, Srini wrote:
> > The navigator.battery has a default value. The battery manager registers for
> > updates to the controller and the controller notifies the new values to
update
> > from default. When the controller gets update from the system the values at
> the
> > battery manager are updated. navigator.battery.level return from the last
> > updated/notified value. I hope Im explaining it as the code/design is.
> 
> I see.  It seems like a better approach would be to match what device
> orientation does.  Specifically, there aren't any getters for rotationRate. 
> Instead, that data is delivered as a property of the event.  For this API,
there
> wouldn't be a navigator.battery.level.  Instead, the level would be delivered
> with the onlevelchange event.  We could then use the addEventListener /
> removeEventListener calls to know when we should be pushing information about
> the battery level to a renderer.

I kept this to be in sync with the spec -
https://dvcs.w3.org/hg/dap/raw-file/default/battery/Overview.html#idl-def-Bat...

These are delivered as part events also. (onlevelchange, onchargingchange,
onchargingtimechange, ondischargingtimechange). My test couldn't cover this, as
this needs me to have more code (BatteryClient, Controller etc) all to be added
and this will be again the same size. I thought I'll add it in the next
iteration. Sorry, I should have been clearer.

> 
> Put another way, suppose a script reads navigator.battery.level.  How do we
know
> to update that value as the battery discharges?  When to we stop updating the
> value?

[abarth-chromium, 2014-02-27 07:44:16]

On 2014/02/27 07:42:17, Srini wrote:
> I kept this to be in sync with the spec -
>
https://dvcs.w3.org/hg/dap/raw-file/default/battery/Overview.html#idl-def-Bat...

We should provide feedback to the working group on this topic.

> These are delivered as part events also. (onlevelchange, onchargingchange,
> onchargingtimechange, ondischargingtimechange). My test couldn't cover this,
as
> this needs me to have more code (BatteryClient, Controller etc) all to be
added
> and this will be again the same size. I thought I'll add it in the next
> iteration. Sorry, I should have been clearer.

Oh, that's good.  For now, let's start with the properties on the events and
leave off the properties on BatteryManager.  We can then discuss those
properties in the working group.

[Srini, 2014-02-27 07:53:41]

On 2014/02/27 07:44:16, abarth wrote:
> On 2014/02/27 07:42:17, Srini wrote:
> > I kept this to be in sync with the spec -
> >
>
https://dvcs.w3.org/hg/dap/raw-file/default/battery/Overview.html#idl-def-Bat...
> 
> We should provide feedback to the working group on this topic.
> 
> > These are delivered as part events also. (onlevelchange, onchargingchange,
> > onchargingtimechange, ondischargingtimechange). My test couldn't cover this,
> as
> > this needs me to have more code (BatteryClient, Controller etc) all to be
> added
> > and this will be again the same size. I thought I'll add it in the next
> > iteration. Sorry, I should have been clearer.
> 
> Oh, that's good.  For now, let's start with the properties on the events and
> leave off the properties on BatteryManager.  We can then discuss those
> properties in the working group.

Sorry, I couldn't understand. Do you mean that I leave this as it is for now and
proceed with fixing the other review comments and on the next iteration I add up
the events/tests/etc?

[abarth-chromium, 2014-02-27 07:57:37]

On 2014/02/27 07:53:41, Srini wrote:
> Sorry, I couldn't understand. Do you mean that I leave this as it is for now
and
> proceed with fixing the other review comments and on the next iteration I add
up
> the events/tests/etc?

Oh, this discussion doesn't need to block any of your work.  I was just giving
you feedback.  Let's get the technical issues addressed and this code landed. 
We can iterate on the API with the working group separately.

[Srini, 2014-02-27 08:00:05]

On 2014/02/27 07:57:37, abarth wrote:
> On 2014/02/27 07:53:41, Srini wrote:
> > Sorry, I couldn't understand. Do you mean that I leave this as it is for now
> and
> > proceed with fixing the other review comments and on the next iteration I
add
> up
> > the events/tests/etc?
> 
> Oh, this discussion doesn't need to block any of your work.  I was just giving
> you feedback.  Let's get the technical issues addressed and this code landed. 
> We can iterate on the API with the working group separately.

Thanks abarth!

[Srini, 2014-02-27 08:48:00]

I just fixed the review comments except, 'subclassing ActiveDOMObject' as this
is needed in the next iteration. I just left comments in it for now. Hope it is
fine.

[Srini, 2014-02-27 19:24:42]

On 2014/02/27 08:48:00, Srini wrote:
> I just fixed the review comments except, 'subclassing ActiveDOMObject' as this
> is needed in the next iteration. I just left comments in it for now. Hope it
is
> fine.

Just added RunTimeEnabledFeatures support, missed it completely earlier.

[jam, 2014-02-27 21:56:10]

removing myself as a reviewer (i'm not in any blink owners files)

[abarth-chromium, 2014-02-28 06:45:14]

https://codereview.chromium.org/182613002/diff/40001/LayoutTests/battery-stat...
File LayoutTests/battery-status/test-battery-status-basic.html (right):

https://codereview.chromium.org/182613002/diff/40001/LayoutTests/battery-stat...
LayoutTests/battery-status/test-battery-status-basic.html:18:
document.getElementById("p4").innerHTML	= "Percentage: " + battery.level*100;
Can you clean up the style a bit here?  Generally, we have one space before and
one space after the = sign.

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/B...
File Source/modules/battery/BatteryManager.cpp (right):

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.cpp:36: :
ActiveDOMObject(frame->document())
Notice that this base class (and ContextDestructionObserver) want a Document as
the context object.  Rather than fighting with them, you should take a Document
as an argument.

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.cpp:69: if
(RuntimeEnabledFeatures::batteryStatusEnabled()) {
This check isn't correct.  Instead, you should just

ASSERT(RuntimeEnabledFeatures::batteryStatusEnabled());

It ought to be an error to call this function if this feature isn't enabled.

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.cpp:88: }
Please remove these functions.  We can add them in a future CL when they do
something.

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/B...
File Source/modules/battery/BatteryManager.h (right):

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.h:18: class BatteryManager : public
ActiveDOMObject, public RefCounted<BatteryManager>, public EventTarget {
Please remove the ActiveDOMObject base class.  It's not needed in this CL.  We
can add it once its needed.  For now, you just need to be a
ContextDestructionObserver.

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.h:21: static PassRefPtr<BatteryManager>
create(Frame*);
This should probably be constructed from a Document rather than a Frame

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.h:25: virtual ExecutionContext*
executionContext() const OVERRIDE FINAL { return
ActiveDOMObject::executionContext(); }
The reason you should use a Document as the context object is because this
function makes you specific to a given Document.

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.h:54: explicit BatteryManager(Frame*);
You've still got two constructors.  You only need one.

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/B...
File Source/modules/battery/BatteryManager.idl (right):

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.idl:14: readonly attribute double level;
I suspect we'll want to remove these properties as discussed earlier, but we can
include them for now.

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/N...
File Source/modules/battery/NavigatorBattery.cpp (right):

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/N...
Source/modules/battery/NavigatorBattery.cpp:8: #include
"core/dom/ExecutionContext.h"
This header does not appear to be used by this file.  Please remove it.

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/N...
Source/modules/battery/NavigatorBattery.cpp:9: #include "core/frame/Navigator.h"
You've already included this header in the NavigatorBattery.h.  No need to
include it again.

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/N...
Source/modules/battery/NavigatorBattery.cpp:25: return
NavigatorBattery::from(navigator).battery();
Notice that you can pass navigator as the argument to battery() here and avoid
having to be a DOMWindowProperty.

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/N...
Source/modules/battery/NavigatorBattery.cpp:53: }
This function does not appear to be necessary.  Please remove it.

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/N...
File Source/modules/battery/NavigatorBattery.h (right):

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/N...
Source/modules/battery/NavigatorBattery.h:15: class ScriptExecutionContext;
This forward declaration doesn't appear to be used in this file.  Please remove
it.

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/N...
Source/modules/battery/NavigatorBattery.h:17: class NavigatorBattery : public
Supplement<Navigator>, DOMWindowProperty {
Why does this class need to be a DOMWindowProperty?  That base class shouldn't
be needed.

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/N...
Source/modules/battery/NavigatorBattery.h:26: BatteryManager* batteryManager();
What's the difference between battery() and batteryManager() ?

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/N...
Source/modules/battery/NavigatorBattery.h:29: NavigatorBattery(Frame*);
Please mark one-argument constructors explicit.

https://codereview.chromium.org/182613002/diff/40001/Source/modules/battery/N...
Source/modules/battery/NavigatorBattery.h:39: 
Please remove these extra blank lines at the end of this file.

[Srini, 2014-02-28 11:26:32]

Just fixed all the comments you mentioned. Hopefully I didn't miss any. Thanks!

[Srini, 2014-03-04 03:17:57]

On 2014/02/28 11:26:32, Srini wrote:
> Just fixed all the comments you mentioned. Hopefully I didn't miss any.
Thanks!

Hi abarth, can you check this up again? If fine, I can start with the next
iteration. Thanks!

[abarth-chromium, 2014-03-04 06:42:11]

This is close.  Just a couple more nits.

https://codereview.chromium.org/182613002/diff/60001/LayoutTests/battery-stat...
File LayoutTests/battery-status/test-battery-status-basic.html (right):

https://codereview.chromium.org/182613002/diff/60001/LayoutTests/battery-stat...
LayoutTests/battery-status/test-battery-status-basic.html:18:
document.getElementById("p4").innerHTML	= "Percentage: " + battery.level*100;
You've still got some odd whitespace on this line.

https://codereview.chromium.org/182613002/diff/60001/Source/modules/battery/B...
File Source/modules/battery/BatteryManager.cpp (right):

https://codereview.chromium.org/182613002/diff/60001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.cpp:19: return batteryManager.release();
There's no need for this local variable.  You can just write:

return adoptRef(new BatteryManager(context));

https://codereview.chromium.org/182613002/diff/60001/Source/modules/battery/B...
Source/modules/battery/BatteryManager.cpp:35: }
There's no need for this override.  You're just forwarding to the base class.

[abarth-chromium, 2014-03-04 06:42:29]

LGTM assuming the nits above are addressed.

[Srini, 2014-03-04 07:42:30]

The CQ bit was checked by srinivasa.ragavan.venkateswaran@intel.com

[commit-bot: I haz the power, 2014-03-04 07:42:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....

[commit-bot: I haz the power, 2014-03-04 07:42:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-04 07:42:53]

Failed to apply patch for Source/modules/modules.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/modules.gypi
  Hunk #1 FAILED at 10.
  Hunk #2 succeeded at 185 (offset 7 lines).
  Hunk #3 FAILED at 216.
  2 out of 3 hunks FAILED -- saving rejects to file
Source/modules/modules.gypi.rej

Patch:       Source/modules/modules.gypi
Index: Source/modules/modules.gypi
diff --git a/Source/modules/modules.gypi b/Source/modules/modules.gypi
index
b20542fa199403d93b6766f4c0325423543a05c5..dce8dea5cbbb4171a77db2aa23f9c73fe94c2873
100644
--- a/Source/modules/modules.gypi
+++ b/Source/modules/modules.gypi
@@ -10,6 +10,7 @@
     # Files for which bindings (.cpp and .h files) will be generated
     'modules_idl_files': [
       '<@(extra_blink_module_idl_files)',
+      'battery/BatteryManager.idl',
       'crypto/Algorithm.idl',
       'crypto/Crypto.idl',
       'crypto/Key.idl',
@@ -178,6 +179,7 @@
     ],
     # 'partial interface' or target (right side of) 'implements'
     'modules_dependency_idl_files': [
+      'battery/NavigatorBattery.idl',
       'crypto/WindowCrypto.idl',
       'crypto/WorkerGlobalScopeCrypto.idl',
       'donottrack/NavigatorDoNotTrack.idl',
@@ -215,6 +217,12 @@
     ],
     'modules_files': [
       '<@(extra_blink_module_files)',
+      'battery/BatteryManager.cpp',
+      'battery/BatteryManager.h',
+      'battery/BatteryStatus.cpp',
+      'battery/BatteryStatus.h',
+      'battery/NavigatorBattery.cpp',
+      'battery/NavigatorBattery.h',
       'crypto/Algorithm.cpp',
       'crypto/Algorithm.h',
       'crypto/Crypto.cpp',

[Srini, 2014-03-04 08:48:36]

The CQ bit was checked by srinivasa.ragavan.venkateswaran@intel.com

[commit-bot: I haz the power, 2014-03-04 08:48:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....

[Srini, 2014-03-04 09:12:20]

The CQ bit was checked by srinivasa.ragavan.venkateswaran@intel.com

[commit-bot: I haz the power, 2014-03-04 09:12:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....

[commit-bot: I haz the power, 2014-03-04 11:26:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-04 11:26:18]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[Srini, 2014-03-04 12:54:55]

The CQ bit was checked by srinivasa.ragavan.venkateswaran@intel.com

[commit-bot: I haz the power, 2014-03-04 12:55:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....

[Srini, 2014-03-04 14:24:04]

On 2014/03/04 12:55:01, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>
https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran%40inte...

This is the crash why the four tests are failing. In my setup, even without my
patch, the tests crash for me :/. Wondering if this totally unrelated.
ASSERTION FAILED: isTextControl()
../../third_party/WebKit/Source/core/accessibility/AXRenderObject.cpp(1548) :
virtual WebCore::AXObject::PlainTextRange
WebCore::AXRenderObject::selectedTextRange() const
1   0x7fc607da69c3
2   0x7fc60679a007 blink::WebAXObject::selectionStart() const

[commit-bot: I haz the power, 2014-03-04 14:52:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-04 14:52:09]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[Srini, 2014-03-04 16:06:58]

On 2014/03/04 14:52:09, I haz the power (commit-bot) wrote:
> Retried try job too often on mac_blink_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

abarth: http://pastebin.com/KZZ40Hki is the trace of this crash. One patch need
me to rebase the results.
(LayoutTests/fast/dom/navigator-detached-no-crash.html). I also see similar
failures at:
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds...


Just wondering if Im overlooking something or is it a genuine issue with my
patch. I couldn't find the relation honestly.

[Srini, 2014-03-04 16:24:38]

The CQ bit was checked by srinivasa.ragavan.venkateswaran@intel.com

[commit-bot: I haz the power, 2014-03-04 16:24:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....

[commit-bot: I haz the power, 2014-03-04 18:21:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-04 18:21:27]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[abarth-chromium, 2014-03-04 19:04:18]

The CQ bit was checked by abarth@chromium.org

[abarth-chromium, 2014-03-04 19:04:46]

On 2014/03/04 16:06:58, Srini wrote:
> Just wondering if Im overlooking something or is it a genuine issue with my
> patch. I couldn't find the relation honestly.

It seems very unlikely to be a problem with you CL.  Let's try the CQ again.

[commit-bot: I haz the power, 2014-03-04 19:05:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....

[commit-bot: I haz the power, 2014-03-04 20:58:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-04 20:58:57]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[Srini, 2014-03-05 03:09:45]

The CQ bit was checked by srinivasa.ragavan.venkateswaran@intel.com

[commit-bot: I haz the power, 2014-03-05 03:10:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....

[commit-bot: I haz the power, 2014-03-05 05:54:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-05 05:54:05]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[Srini, 2014-03-05 14:18:55]

The CQ bit was checked by srinivasa.ragavan.venkateswaran@intel.com

[commit-bot: I haz the power, 2014-03-05 14:19:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....

[commit-bot: I haz the power, 2014-03-05 16:04:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-05 16:04:05]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[Srini, 2014-03-06 03:46:40]

The CQ bit was checked by srinivasa.ragavan.venkateswaran@intel.com

[commit-bot: I haz the power, 2014-03-06 03:47:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....

[commit-bot: I haz the power, 2014-03-06 04:11:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 04:11:49]

Try jobs failed on following builders: linux_blink_rel

[Srini, 2014-03-06 06:13:27]

The CQ bit was checked by srinivasa.ragavan.venkateswaran@intel.com

[commit-bot: I haz the power, 2014-03-06 06:13:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....

[commit-bot: I haz the power, 2014-03-06 06:52:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 06:52:35]

Try jobs failed on following builders: mac_blink_rel

[Srini, 2014-03-06 10:51:21]

The CQ bit was checked by srinivasa.ragavan.venkateswaran@intel.com

[commit-bot: I haz the power, 2014-03-06 10:51:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....

[commit-bot: I haz the power, 2014-03-06 10:52:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 10:52:41]

Try jobs failed on following builders: mac_blink_compile

[Srini, 2014-03-06 11:01:06]

The CQ bit was checked by srinivasa.ragavan.venkateswaran@intel.com

[commit-bot: I haz the power, 2014-03-06 11:01:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....

[commit-bot: I haz the power, 2014-03-06 11:07:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 11:07:06]

Try jobs failed on following builders: linux_blink

[Srini, 2014-03-06 11:27:49]

The CQ bit was checked by srinivasa.ragavan.venkateswaran@intel.com

[commit-bot: I haz the power, 2014-03-06 11:27:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....

[commit-bot: I haz the power, 2014-03-06 11:39:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 11:39:27]

Try jobs failed on following builders: mac_blink_rel

[Srini, 2014-03-06 12:03:31]

The CQ bit was checked by srinivasa.ragavan.venkateswaran@intel.com

[commit-bot: I haz the power, 2014-03-06 12:03:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....

[commit-bot: I haz the power, 2014-03-06 12:58:18]

Change committed as 168632

[Inactive, 2014-03-06 13:45:38]

Sorry for being late. I think the code can be simplified a little bit.

https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
File Source/modules/battery/BatteryManager.h (right):

https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
Source/modules/battery/BatteryManager.h:17: class BatteryManager : public
ContextLifecycleObserver, public RefCounted<BatteryManager>, public EventTarget
{
I believe you can subclass EventTargetWithInlineData instead of EventTarget.

Also, if BatteryManager is not subclassed, it should be marked as FINAL.

https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
Source/modules/battery/BatteryManager.h:18: public:
REFCOUNTED_EVENT_TARGET(BatteryManager);

https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
Source/modules/battery/BatteryManager.h:24: virtual ExecutionContext*
executionContext() const OVERRIDE FINAL { return
ContextLifecycleObserver::executionContext(); }
Don't need the FINAL here if you mark the class as FINAL.

https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
Source/modules/battery/BatteryManager.h:38: using
RefCounted<BatteryManager>::ref;
Not needed if you use the macro I suggested.

https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
Source/modules/battery/BatteryManager.h:39: using
RefCounted<BatteryManager>::deref;
Not needed if you use the macro I suggested.

https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
Source/modules/battery/BatteryManager.h:42: virtual EventTargetData*
eventTargetData() { return &m_eventTargetData; }
Not needed if you subclass EventTargetWithInlineData.

https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
Source/modules/battery/BatteryManager.h:43: virtual EventTargetData&
ensureEventTargetData() { return m_eventTargetData; }
Not needed if you subclass EventTargetWithInlineData.

https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
Source/modules/battery/BatteryManager.h:48: // EventTarget implementation.
Those 2 methods are not needed if you use the macro I suggested.

https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
Source/modules/battery/BatteryManager.h:52: EventTargetData m_eventTargetData;
Not needed if you subclass EventTargetWithInlineData.

https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
File Source/modules/battery/NavigatorBattery.h (right):

https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
Source/modules/battery/NavigatorBattery.h:26: explicit NavigatorBattery();
A bit odd to make this constructor explicit since it has no argument

[Srini, 2014-03-06 13:50:32]

On 2014/03/06 13:45:38, Chris Dumez wrote:
> Sorry for being late. I think the code can be simplified a little bit.

Heya btw!. I'll fix this and submit as a new CL in a bit.

Thanks,
-Srini.

> 
>
https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
> File Source/modules/battery/BatteryManager.h (right):
> 
>
https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
> Source/modules/battery/BatteryManager.h:17: class BatteryManager : public
> ContextLifecycleObserver, public RefCounted<BatteryManager>, public
EventTarget
> {
> I believe you can subclass EventTargetWithInlineData instead of EventTarget.
> 
> Also, if BatteryManager is not subclassed, it should be marked as FINAL.
> 
>
https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
> Source/modules/battery/BatteryManager.h:18: public:
> REFCOUNTED_EVENT_TARGET(BatteryManager);
> 
>
https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
> Source/modules/battery/BatteryManager.h:24: virtual ExecutionContext*
> executionContext() const OVERRIDE FINAL { return
> ContextLifecycleObserver::executionContext(); }
> Don't need the FINAL here if you mark the class as FINAL.
> 
>
https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
> Source/modules/battery/BatteryManager.h:38: using
> RefCounted<BatteryManager>::ref;
> Not needed if you use the macro I suggested.
> 
>
https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
> Source/modules/battery/BatteryManager.h:39: using
> RefCounted<BatteryManager>::deref;
> Not needed if you use the macro I suggested.
> 
>
https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
> Source/modules/battery/BatteryManager.h:42: virtual EventTargetData*
> eventTargetData() { return &m_eventTargetData; }
> Not needed if you subclass EventTargetWithInlineData.
> 
>
https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
> Source/modules/battery/BatteryManager.h:43: virtual EventTargetData&
> ensureEventTargetData() { return m_eventTargetData; }
> Not needed if you subclass EventTargetWithInlineData.
> 
>
https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
> Source/modules/battery/BatteryManager.h:48: // EventTarget implementation.
> Those 2 methods are not needed if you use the macro I suggested.
> 
>
https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
> Source/modules/battery/BatteryManager.h:52: EventTargetData m_eventTargetData;
> Not needed if you subclass EventTargetWithInlineData.
> 
>
https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
> File Source/modules/battery/NavigatorBattery.h (right):
> 
>
https://codereview.chromium.org/182613002/diff/190001/Source/modules/battery/...
> Source/modules/battery/NavigatorBattery.h:26: explicit NavigatorBattery();
> A bit odd to make this constructor explicit since it has no argument