Migrates battery_status from content/renderer/ to WebKit/platform/

Migrates battery_status from content/renderer/ to WebKit/platform/

As an effort to simplify the codebase after the repogitory
merge of Blink into Chromium, we no longer need to put
battery_status in content/renderer/.  Thus, migrates it to
WebKit/Source/platform/.

* Files under WebKit/Source/platform/battery/ are
intentionally written in Chromium's coding style so that
it's easy to follow the diff.

* There is a plan to rewrite the mock service of battery
status for layout tests based on JS-bindings of Mojo.
(c.f. http://crrev.com/1472693004 )

BUG=561879
NOPRESUBMIT=true

Committed: https://crrev.com/0d152ad4f21edf77ac5b585fcd8e6d05f1aae162
Cr-Commit-Position: refs/heads/master@{#377586}

[Yuki, 2015-12-18 06:36:49]

Description was changed from

==========
onion: Moves battery_status from content/ to WebKit/

BUG=
==========

to

==========
onion: Moves battery_status from content/ to WebKit/

BUG=
==========

[Yuki, 2015-12-18 07:48:20]

Patchset #1 (id:1) has been deleted

[Yuki, 2015-12-18 08:14:58]

Patchset #1 (id:20001) has been deleted

[haraken, 2016-01-25 02:00:28]

haraken@chromium.org changed reviewers:
+ amistry@chromium.org, esprehn@chromium.org, haraken@chromium.org,
jam@chromium.org, sammc@chromium.org

[haraken, 2016-01-25 02:00:28]

+jam +esprehn +sammc +amistry

This CL looks not ready in a couple of senses but I'm ccing you for early
feedback :)

https://codereview.chromium.org/1538803002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/battery/BatteryStatusDispatcher.cpp
(right):

https://codereview.chromium.org/1538803002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/battery/BatteryStatusDispatcher.cpp:31:
bindInstanceToMethod(&BatteryStatusDispatcher::onDidChange, this));

Why can't we just use WTF::bind?

[Anand Mistry (off Chromium), 2016-01-25 06:00:24]

https://codereview.chromium.org/1538803002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/battery/BatteryStatusDispatcher.cpp
(right):

https://codereview.chromium.org/1538803002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/battery/BatteryStatusDispatcher.cpp:31:
bindInstanceToMethod(&BatteryStatusDispatcher::onDidChange, this));
On 2016/01/25 02:00:28, haraken wrote:
> 
> Why can't we just use WTF::bind?

Why use bind at all? This doesn't need to be thread-safe (mojo callbacks are
explicitly not thread-safe) and |this| is unretained, so just pass in a lambda.
Don't know how OK that is with the style though.

[jam, 2016-01-25 18:28:48]

I just looked at content and the DEPS files changes in WebKit. Things look fine
to me.

The only minor comment I'll make is that this should have the master bug (or a
child one) of the refactorings listed in the cl description.

Also, I'd prefer if we don't have project names like "onion" in cl descriptions.
Years later when people are looking at changes, many people won't know project
code names. Can we have something descriptive listed in the cl description
instead?

[Yuki, 2016-01-26 07:36:24]

Thanks for the feedback, will address them.

Kinuko-san, could you take a quick look at
https://codereview.chromium.org/1538803002/diff/260001/third_party/WebKit/Sou...
and
https://codereview.chromium.org/1538803002/diff/260001/third_party/WebKit/Sou...
where base::Bind will be used?

I'm personally okay to use either of lambda or anything else.  I'd like to hear
your opinions.  Probably we'd like to avoid abuse of bare lambda because it
would become hard to track thread-safety bugs.

[haraken, 2016-01-26 07:38:55]

On 2016/01/26 07:36:24, Yuki wrote:
> Thanks for the feedback, will address them.
> 
> Kinuko-san, could you take a quick look at
>
https://codereview.chromium.org/1538803002/diff/260001/third_party/WebKit/Sou...
> and
>
https://codereview.chromium.org/1538803002/diff/260001/third_party/WebKit/Sou...
> where base::Bind will be used?
> 
> I'm personally okay to use either of lambda or anything else.  I'd like to
hear
> your opinions.  Probably we'd like to avoid abuse of bare lambda because it
> would become hard to track thread-safety bugs.

Drive-by: I don't think we have a coding style rule about lambda, but I
personally want to avoid introducing advanced C++ features to the code base.

[Yuki, 2016-01-26 07:50:32]

https://codereview.chromium.org/1538803002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/battery/BatteryStatusDispatcher.cpp
(right):

https://codereview.chromium.org/1538803002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/battery/BatteryStatusDispatcher.cpp:31:
bindInstanceToMethod(&BatteryStatusDispatcher::onDidChange, this));
On 2016/01/25 06:00:24, Anand Mistry wrote:
> On 2016/01/25 02:00:28, haraken wrote:
> > 
> > Why can't we just use WTF::bind?
> 
> Why use bind at all? This doesn't need to be thread-safe (mojo callbacks are
> explicitly not thread-safe) and |this| is unretained, so just pass in a
lambda.
> Don't know how OK that is with the style though.

Note that base::Bind and WTF::bind are NOT compatible.  We cannot use WTF::bind
in this case because Mojo requires base::Callback.

[haraken, 2016-01-26 07:53:51]

On 2016/01/26 07:50:32, Yuki wrote:
>
https://codereview.chromium.org/1538803002/diff/260001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/battery/BatteryStatusDispatcher.cpp
> (right):
> 
>
https://codereview.chromium.org/1538803002/diff/260001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/battery/BatteryStatusDispatcher.cpp:31:
> bindInstanceToMethod(&BatteryStatusDispatcher::onDidChange, this));
> On 2016/01/25 06:00:24, Anand Mistry wrote:
> > On 2016/01/25 02:00:28, haraken wrote:
> > > 
> > > Why can't we just use WTF::bind?
> > 
> > Why use bind at all? This doesn't need to be thread-safe (mojo callbacks are
> > explicitly not thread-safe) and |this| is unretained, so just pass in a
> lambda.
> > Don't know how OK that is with the style though.
> 
> Note that base::Bind and WTF::bind are NOT compatible.  We cannot use
WTF::bind
> in this case because Mojo requires base::Callback.

Ah, I now understand the complexity. It may be better to start a separate thread
in platform-architecture-dev@ ccing kinuko@, hiroshige@, tzik@ and yutak@.

[esprehn, 2016-01-26 08:08:41]

This seems like a mojo bug, jam@ said they were going to fix the bindings
for blink. :)

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2016-01-26 08:16:16]

This seems like a mojo bug, jam@ said they were going to fix the bindings
for blink. :)

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kinuko, 2016-01-26 08:19:05]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2016-01-26 08:19:06]

As far as all mojo binding code would be doing is same-thread method invocation
let it just support wtf closures would probably make sense.

(I prefer not using lambda here either)

[Sam McNally, 2016-01-26 23:12:54]

On 2016/01/26 08:19:06, kinuko wrote:
> As far as all mojo binding code would be doing is same-thread method
invocation
> let it just support wtf closures would probably make sense.
> 
> (I prefer not using lambda here either)

http://chromium-cpp.appspot.com/ doesn't allow lambdas to be used this way.
Apart from an adapter from WTF::Function to mojo::Callback, we'd also need to
change WTF::Function to correctly forward move-only types.

https://codereview.chromium.org/1367853002/ is what I ended up with from trying
the same sort of thing for geolocation.

[esprehn, 2016-01-26 23:14:15]

Description was changed from

==========
onion: Moves battery_status from content/ to WebKit/

BUG=
==========

to

==========
[Onion Soup]: Moves battery_status from content/ to WebKit/

BUG=
==========

[jam, 2016-01-26 23:29:07]

Description was changed from

==========
[Onion Soup]: Moves battery_status from content/ to WebKit/

BUG=
==========

to

==========
[Onion Soup]: Moves battery_status from content/ to WebKit/

BUG=
==========

[jam, 2016-01-26 23:29:08]

jam@chromium.org changed reviewers:
+ rockot@chromium.org, yzshen@chromium.org

[jam, 2016-01-26 23:31:23]

+rockot,yzshen who are more familiar with me about bindings

On 2016/01/26 08:16:16, esprehn wrote:
> This seems like a mojo bug, jam@ said they were going to fix the bindings
> for blink. :)

What we discussed is that the bindings would support blink types for parameters.
I believe this thread is about using WTF::bind for the mojo callbacks? I don't
know if anyone has thought about that specific issue. Is base::bind not allowed
in blink?

[Yuki, 2016-01-27 00:43:42]

On 2016/01/26 23:31:23, jam wrote:
> +rockot,yzshen who are more familiar with me about bindings
> 
> On 2016/01/26 08:16:16, esprehn wrote:
> > This seems like a mojo bug, jam@ said they were going to fix the bindings
> > for blink. :)
> 
> What we discussed is that the bindings would support blink types for
parameters.
> I believe this thread is about using WTF::bind for the mojo callbacks? I don't
> know if anyone has thought about that specific issue. Is base::bind not
allowed
> in blink?

base::Bind has already been used in Blink.  We'll discuss this point with Kinuko
and Hiroshige separately how we should use base::Bind or not.

[Yuki, 2016-01-27 00:57:42]

On 2016/01/27 00:43:42, Yuki wrote:
> On 2016/01/26 23:31:23, jam wrote:
> > +rockot,yzshen who are more familiar with me about bindings
> > 
> > On 2016/01/26 08:16:16, esprehn wrote:
> > > This seems like a mojo bug, jam@ said they were going to fix the bindings
> > > for blink. :)
> > 
> > What we discussed is that the bindings would support blink types for
> parameters.
> > I believe this thread is about using WTF::bind for the mojo callbacks? I
don't
> > know if anyone has thought about that specific issue. Is base::bind not
> allowed
> > in blink?
> 
> base::Bind has already been used in Blink.  We'll discuss this point with
Kinuko
> and Hiroshige separately how we should use base::Bind or not.

rockot@, jam@, sammc@, amistry@ and haraken@,
What I'm doing for tests in this CL is to allow browsertests to use a mock
battery status service.
https://codereview.chromium.org/1538803002/diff/320001/third_party/WebKit/pub...
https://codereview.chromium.org/1538803002/diff/320001/components/test_runner...
I've added blink::WebView::updateBatteryStatusForTesting() so that TestRunner in
components/ can control the battery status for testing. 
TestRunner::DidChangeBatteryStatus() in C++ is exposed to JS as
didChangeBatteryStatus function.
https://code.google.com/p/chromium/codesearch#chromium/src/components/test_ru...

I think we need this kind of public APIs in Blink to allow browsertests to
control some of device services.

[dcheng, 2016-01-27 02:50:01]

Drive-by.

https://codereview.chromium.org/1538803002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/base/Bind.h (right):

https://codereview.chromium.org/1538803002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/base/Bind.h:20: auto
bindInstanceToMethod(ReturnType (Class::*method)(Args...), Class* instance)
Out of curiosity, why not base::Callback<ReturnType(Args...)>
bindInstanceToMethod(...)?

[Yuki, 2016-01-27 04:29:45]

https://codereview.chromium.org/1538803002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/base/Bind.h (right):

https://codereview.chromium.org/1538803002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/base/Bind.h:20: auto
bindInstanceToMethod(ReturnType (Class::*method)(Args...), Class* instance)
On 2016/01/27 02:50:01, dcheng wrote:
> Out of curiosity, why not base::Callback<ReturnType(Args...)>
> bindInstanceToMethod(...)?

The return type of base::Bind is

template <typename Functor, typename... Args>
base::Callback<
    typename internal::BindState<
        typename internal::FunctorTraits<Functor>::RunnableType,
        typename internal::FunctorTraits<Functor>::RunType,
        typename internal::CallbackParamTraits<Args>::StorageType...>
            ::UnboundRunType>

and it's really hard to write the completely same type manually here (note that
bindInstanceToMethod and base::Bind take different template parameters), and I
think we shouldn't depend on too much of details of the internal implementation
of Bind.

What we'd like to return is what base::Bind returns.  That's
"decltype(base::Bind(...))".

[haraken, 2016-01-27 04:52:23]

On 2016/01/27 04:29:45, Yuki wrote:
>
https://codereview.chromium.org/1538803002/diff/320001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/base/Bind.h (right):
> 
>
https://codereview.chromium.org/1538803002/diff/320001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/base/Bind.h:20: auto
> bindInstanceToMethod(ReturnType (Class::*method)(Args...), Class* instance)
> On 2016/01/27 02:50:01, dcheng wrote:
> > Out of curiosity, why not base::Callback<ReturnType(Args...)>
> > bindInstanceToMethod(...)?
> 
> The return type of base::Bind is
> 
> template <typename Functor, typename... Args>
> base::Callback<
>     typename internal::BindState<
>         typename internal::FunctorTraits<Functor>::RunnableType,
>         typename internal::FunctorTraits<Functor>::RunType,
>         typename internal::CallbackParamTraits<Args>::StorageType...>
>             ::UnboundRunType>
> 
> and it's really hard to write the completely same type manually here (note
that
> bindInstanceToMethod and base::Bind take different template parameters), and I
> think we shouldn't depend on too much of details of the internal
implementation
> of Bind.
> 
> What we'd like to return is what base::Bind returns.  That's
> "decltype(base::Bind(...))".

Regarding the function binding, I started a separate thread in
platform-architecture-dev@
(https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/xI...).

Regarding testing, I'd recommend you start a separate thread as well in order
not to mess up the code review with various discussions.

[tzik, 2016-01-27 05:17:25]

tzik@chromium.org changed reviewers:
+ tzik@chromium.org

[tzik, 2016-01-27 05:17:26]

https://codereview.chromium.org/1538803002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/base/Bind.h (right):

https://codereview.chromium.org/1538803002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/base/Bind.h:20: auto
bindInstanceToMethod(ReturnType (Class::*method)(Args...), Class* instance)
On 2016/01/27 04:29:45, Yuki wrote:
> On 2016/01/27 02:50:01, dcheng wrote:
> > Out of curiosity, why not base::Callback<ReturnType(Args...)>
> > bindInstanceToMethod(...)?
> 
> The return type of base::Bind is
> 
> template <typename Functor, typename... Args>
> base::Callback<
>     typename internal::BindState<
>         typename internal::FunctorTraits<Functor>::RunnableType,
>         typename internal::FunctorTraits<Functor>::RunType,
>         typename internal::CallbackParamTraits<Args>::StorageType...>
>             ::UnboundRunType>
> 
> and it's really hard to write the completely same type manually here (note
that
> bindInstanceToMethod and base::Bind take different template parameters), and I
> think we shouldn't depend on too much of details of the internal
implementation
> of Bind.
> 
> What we'd like to return is what base::Bind returns.  That's
> "decltype(base::Bind(...))".

The result of the complex template is just to drop some items from Args.
base::Callback<ReturnType(Args...)> seems to work here.

[Yuki, 2016-01-27 05:31:08]

https://codereview.chromium.org/1538803002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/base/Bind.h (right):

https://codereview.chromium.org/1538803002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/base/Bind.h:20: auto
bindInstanceToMethod(ReturnType (Class::*method)(Args...), Class* instance)
On 2016/01/27 05:17:26, tzik wrote:
> On 2016/01/27 04:29:45, Yuki wrote:
> > On 2016/01/27 02:50:01, dcheng wrote:
> > > Out of curiosity, why not base::Callback<ReturnType(Args...)>
> > > bindInstanceToMethod(...)?
> > 
> > The return type of base::Bind is
> > 
> > template <typename Functor, typename... Args>
> > base::Callback<
> >     typename internal::BindState<
> >         typename internal::FunctorTraits<Functor>::RunnableType,
> >         typename internal::FunctorTraits<Functor>::RunType,
> >         typename internal::CallbackParamTraits<Args>::StorageType...>
> >             ::UnboundRunType>
> > 
> > and it's really hard to write the completely same type manually here (note
> that
> > bindInstanceToMethod and base::Bind take different template parameters), and
I
> > think we shouldn't depend on too much of details of the internal
> implementation
> > of Bind.
> > 
> > What we'd like to return is what base::Bind returns.  That's
> > "decltype(base::Bind(...))".
> 
> The result of the complex template is just to drop some items from Args.
> base::Callback<ReturnType(Args...)> seems to work here.

Ah, I see.  Good to know.  Thanks.

[Yuki, 2016-01-29 09:54:24]

Description was changed from

==========
[Onion Soup]: Moves battery_status from content/ to WebKit/

BUG=
==========

to

==========
[Onion Soup]: Moves battery_status from content/ to WebKit/

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[darin (slow to review), 2016-02-02 17:13:37]

darin@chromium.org changed reviewers:
+ darin@chromium.org

[darin (slow to review), 2016-02-02 17:13:38]

drive-by question: why even bother with BatteryStatusDispatcher? why not just
have the code in modules/ talk directly to the mojom-defined BatteryMonitor
interface?

[Sam McNally, 2016-02-03 01:28:27]

On 2016/02/02 17:13:38, darin (slow to review) wrote:
> drive-by question: why even bother with BatteryStatusDispatcher? why not just
> have the code in modules/ talk directly to the mojom-defined BatteryMonitor
> interface?

https://docs.google.com/document/d/13XsbaBz7A2H0PZIdFcytHf5-fVOlAfkLlIUKhxKzs...
restricts the use of mojo to the platform layer.

[haraken, 2016-02-03 02:20:23]

On 2016/02/03 01:28:27, Sam McNally wrote:
> On 2016/02/02 17:13:38, darin (slow to review) wrote:
> > drive-by question: why even bother with BatteryStatusDispatcher? why not
just
> > have the code in modules/ talk directly to the mojom-defined BatteryMonitor
> > interface?
> 
>
https://docs.google.com/document/d/13XsbaBz7A2H0PZIdFcytHf5-fVOlAfkLlIUKhxKzs...
> restricts the use of mojo to the platform layer.

platform/ => mojo is allowed, although modules/ => mojo is not allowed. That's
why we have BatteryStatusDispatcher.

As darin pointed out, it may be worth considering to allow modules/ => mojo. It
will be helpful to remove redundant abstraction layers. Elliott?

[shalamov, 2016-02-05 13:32:01]

https://codereview.chromium.org/1538803002/diff/620001/content/renderer/rende...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1538803002/diff/620001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.cc:1193: // In the layout test
mode, mock services should be used instead.
If I understand correctly, with this "if(layout_test_mode)", it would be
impossible to connect to any mojo service in "layout_test_mode".
Is it possible to remove this check? 

In crrev.com/1543823002 for layout tests I substitute real mojo service with
mock service at runtime, and this check would force me to rewrite
RendererBlinkPlatformImpl::connectToRemoteService in dispatcher.

[Yuki, 2016-02-05 14:44:12]

https://codereview.chromium.org/1538803002/diff/620001/content/renderer/rende...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1538803002/diff/620001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.cc:1193: // In the layout test
mode, mock services should be used instead.
On 2016/02/05 13:32:01, shalamov wrote:
> If I understand correctly, with this "if(layout_test_mode)", it would be
> impossible to connect to any mojo service in "layout_test_mode".
> Is it possible to remove this check? 
> 
> In crrev.com/1543823002 for layout tests I substitute real mojo service with
> mock service at runtime, and this check would force me to rewrite
> RendererBlinkPlatformImpl::connectToRemoteService in dispatcher.

This is a dirty hack to make the layout test run and pass, and should be
implemented in another way.

We should support a way to inject mock services and/or a way to disable services
selectively.  I'll revisit this point in this CL, otherwise in a follow-up CL.

Having said that, this method |connectToRemoteService| is a new method which is
going to be introduced by this CL, and only the battery status service in this
CL uses this method.  So your CL shouldn't be affected.

[Ken Rockot(use gerrit already), 2016-02-05 16:45:53]

On 2016/02/05 at 14:44:12, yukishiino wrote:
>
https://codereview.chromium.org/1538803002/diff/620001/content/renderer/rende...
> File content/renderer/renderer_blink_platform_impl.cc (right):
> 
>
https://codereview.chromium.org/1538803002/diff/620001/content/renderer/rende...
> content/renderer/renderer_blink_platform_impl.cc:1193: // In the layout test
mode, mock services should be used instead.
> On 2016/02/05 13:32:01, shalamov wrote:
> > If I understand correctly, with this "if(layout_test_mode)", it would be
> > impossible to connect to any mojo service in "layout_test_mode".
> > Is it possible to remove this check? 
> > 
> > In crrev.com/1543823002 for layout tests I substitute real mojo service with
> > mock service at runtime, and this check would force me to rewrite
> > RendererBlinkPlatformImpl::connectToRemoteService in dispatcher.
> 
> This is a dirty hack to make the layout test run and pass, and should be
implemented in another way.
> 
> We should support a way to inject mock services and/or a way to disable
services selectively.  I'll revisit this point in this CL, otherwise in a
follow-up CL.
> 
> Having said that, this method |connectToRemoteService| is a new method which
is going to be introduced by this CL, and only the battery status service in
this CL uses this method.  So your CL shouldn't be affected.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
demonstrates a general-purpose solution for Mojo service mocking in layout
tests.

[Yuki, 2016-02-08 07:33:52]

Description was changed from

==========
[Onion Soup]: Moves battery_status from content/ to WebKit/

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Migrates battery_status from content/renderer/ to WebKit/platform/

As an effort to simplify the codebase after
the repogitory merge of Blink into Chromium,
we no longer need to put battery_status in
content/renderer/.  Thus, migrates it to
WebKit/Source/platform/.

BUG=561879
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Yuki, 2016-02-08 13:00:59]

On 2016/02/05 16:45:53, Ken Rockot wrote:
> On 2016/02/05 at 14:44:12, yukishiino wrote:
> >
>
https://codereview.chromium.org/1538803002/diff/620001/content/renderer/rende...
> > File content/renderer/renderer_blink_platform_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1538803002/diff/620001/content/renderer/rende...
> > content/renderer/renderer_blink_platform_impl.cc:1193: // In the layout test
> mode, mock services should be used instead.
> > On 2016/02/05 13:32:01, shalamov wrote:
> > > If I understand correctly, with this "if(layout_test_mode)", it would be
> > > impossible to connect to any mojo service in "layout_test_mode".
> > > Is it possible to remove this check? 
> > > 
> > > In crrev.com/1543823002 for layout tests I substitute real mojo service
with
> > > mock service at runtime, and this check would force me to rewrite
> > > RendererBlinkPlatformImpl::connectToRemoteService in dispatcher.
> > 
> > This is a dirty hack to make the layout test run and pass, and should be
> implemented in another way.
> > 
> > We should support a way to inject mock services and/or a way to disable
> services selectively.  I'll revisit this point in this CL, otherwise in a
> follow-up CL.
> > 
> > Having said that, this method |connectToRemoteService| is a new method which
> is going to be introduced by this CL, and only the battery status service in
> this CL uses this method.  So your CL shouldn't be affected.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> demonstrates a general-purpose solution for Mojo service mocking in layout
> tests.

Thanks!  I'd like to follow the way of http://crrev.com/1472693004 , i.e. create
battery-helpers.js as same as usb-helpers.js.  Once your CL gets landed, I'll
update this hack and follow your way.  For the time being, I leave a TODO
comment here.

[Yuki, 2016-02-08 13:08:17]

Description was changed from

==========
Migrates battery_status from content/renderer/ to WebKit/platform/

As an effort to simplify the codebase after
the repogitory merge of Blink into Chromium,
we no longer need to put battery_status in
content/renderer/.  Thus, migrates it to
WebKit/Source/platform/.

BUG=561879
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Migrates battery_status from content/renderer/ to WebKit/platform/

As an effort to simplify the codebase after the repogitory
merge of Blink into Chromium, we no longer need to put
battery_status in content/renderer/.  Thus, migrates it to
WebKit/Source/platform/.

* Files under WebKit/Source/platform/battery/ are
intentionally written in Chromium's coding style so that
it's easy to follow the diff.

* There is a plan to rewrite the mock service of battery
status for layout tests based on JS-bindings of Mojo.
(c.f. http://crrev.com/1472693004 )

BUG=561879
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Yuki, 2016-02-08 13:09:26]

Thank you guys for a lot of suggestions.

I think now that this CL is finally ready for a code review.
Could you guys take a closer look?

If I'm missing any of your comments / suggestions, please let me know.

[haraken, 2016-02-09 12:01:22]

Thanks for being persistent on this! Mostly looks good.

My main comment is about the BatteryStatusDispatcher. Honestly speaking, I want
to avoid introducing the indirection class by letting modules/ directly talk
with Mojo, but I'm okay with having the indirection class until we have a good
conversion system between Mojo and Blink. However, even in that case, I want to
make it clear that the BatteryStatusDispatcher is just a "proxy" class. See my
comment on battery_status_dispatcher.cc.

https://codereview.chromium.org/1538803002/diff/700001/components/test_runner...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1538803002/diff/700001/components/test_runner...
components/test_runner/test_runner.cc:2554: void
TestRunner::DidChangeBatteryStatus(bool charging,

Not directly related to this CL, but what's a difference between TestRunner and
window.internals?

Currently a layout test updates the battery status in the following path:

  layout test => TestRunner => WebView => BatteryDispatcher => BatteryStatus

Another (simpler) way to do that would be:

  layout test => window.internals => BatteryDispatcher => BatteryStatus

In this case, we don't need to involve WebView or go back and forth between
Blink <=> content/. So I'm curious why we don't use window.internals here.

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/base/Bind.h (right):

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/base/Bind.h:14: // See also
"platform/ThreadSafeFunctional.h".

I'd move the contents of this file to ThreadSafeFunctional.h.

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/base/Bind.h:22:
bindInstanceToMethodForMojo(ReturnType (Class::*method)(Args...), Class*
instance)

tzik@: In your refactoring plan, this method is going to be WTF::sameThreadBind,
right? If so, we can rename this to WTF::sameThreadBind.

Also it would be great if we could insert an assert to check that the bind
function is not used across threads. You don't need to address it in this CL.

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/base/DEPS (right):

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/base/DEPS:2: "+base/bind.h",

Add a comment:

# To whitelist base/ stuff Blink is allowed to include, we list up all
# directories and files instead of writing 'base/'.

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/battery/battery_status_dispatcher.cc
(right):

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/battery/battery_status_dispatcher.cc:15:
BatteryStatusDispatcher::BatteryStatusDispatcher(BatteryStatusListener*
listener)

Honestly speaking, I want to let modules/ directly talk with Mojo and remove the
BatteryStatusDispatcher. However, if we don't want to go that way for now, at
least I want to make it clearer that the BatteryStatusDispatcher is a "proxy"
for the BatteryDispatcher. Specifically:

- Rename BatteryStatusDispatcher to BatteryDispatcherProxy.

- Make the lifetime of BatteryDispatcher and BatteryDispatcherProxy identical.
The constructor should be renamed to startListening and the destructor should be
renamed to stopListening.

- BatteryDispatcherProxy should take the BatteryDispatcher as an argument,
rather than BatteryStatusListener. The role of the BatteryDispatcherProxy should
be *just proxying* methods between BatteryDispatcher and content/. Make that
logic clearer.

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/RunAllTests.cpp (right):

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/RunAllTests.cpp:97: adoptPtr(new
mojo::test::ScopedIPCSupport(testIoThread.task_runner())));

Shall we rename InitializeMojo to InitializePlatform and move the TestIOThread
initialization into InitializePlatform?

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp
(right):

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp:120: #if
!OS(ANDROID)

I'm just curious: Why do we need !OS(ANDROID)?

[Yuki, 2016-02-12 12:36:15]

https://codereview.chromium.org/1538803002/diff/700001/components/test_runner...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1538803002/diff/700001/components/test_runner...
components/test_runner/test_runner.cc:2554: void
TestRunner::DidChangeBatteryStatus(bool charging,
On 2016/02/09 12:01:21, haraken wrote:
> 
> Not directly related to this CL, but what's a difference between TestRunner
and
> window.internals?
> 
> Currently a layout test updates the battery status in the following path:
> 
>   layout test => TestRunner => WebView => BatteryDispatcher => BatteryStatus
> 
> Another (simpler) way to do that would be:
> 
>   layout test => window.internals => BatteryDispatcher => BatteryStatus
> 
> In this case, we don't need to involve WebView or go back and forth between
> Blink <=> content/. So I'm curious why we don't use window.internals here.

I'm not sure, but I guess window.internals is implemented in Blink while
window.testRunner is implemented in content/.  Another guess is that
window.testRunner is dedicated to testing purpose while window.internals is
dedicated to more general purpose?

Anyway, as a long term solution, we'd like to follow the way of
http://crrev.com/1472693004 .

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/base/Bind.h (right):

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/base/Bind.h:14: // See also
"platform/ThreadSafeFunctional.h".
On 2016/02/09 12:01:21, haraken wrote:
> 
> I'd move the contents of this file to ThreadSafeFunctional.h.

We'd like to limit the access to base/bind.h.  Note that ThreadSafeFunctional.h
is placed at platform/, and we don't want to allow all the code in platform/ to
use base/bind.h.  So I dug this subdirectory and placed a new DEPS.

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/base/Bind.h:22:
bindInstanceToMethodForMojo(ReturnType (Class::*method)(Args...), Class*
instance)
On 2016/02/09 12:01:21, haraken wrote:
> 
> tzik@: In your refactoring plan, this method is going to be
WTF::sameThreadBind,
> right? If so, we can rename this to WTF::sameThreadBind.
> 
> Also it would be great if we could insert an assert to check that the bind
> function is not used across threads. You don't need to address it in this CL.

tzik@, is there an easy way to check whether we're running in the same thread or
not?

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/base/DEPS (right):

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/base/DEPS:2: "+base/bind.h",
On 2016/02/09 12:01:21, haraken wrote:
> 
> Add a comment:
> 
> # To whitelist base/ stuff Blink is allowed to include, we list up all
> # directories and files instead of writing 'base/'.

Done.

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/battery/battery_status_dispatcher.cc
(right):

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/battery/battery_status_dispatcher.cc:15:
BatteryStatusDispatcher::BatteryStatusDispatcher(BatteryStatusListener*
listener)
On 2016/02/09 12:01:21, haraken wrote:
> Honestly speaking, I want to let modules/ directly talk with Mojo and remove
the
> BatteryStatusDispatcher. However, if we don't want to go that way for now, at
> least I want to make it clearer that the BatteryStatusDispatcher is a "proxy"

OK

> for the BatteryDispatcher. Specifically:
> 
> - Rename BatteryStatusDispatcher to BatteryDispatcherProxy.

OK

> - Make the lifetime of BatteryDispatcher and BatteryDispatcherProxy identical.
> The constructor should be renamed to startListening and the destructor should
be
> renamed to stopListening.

OK

> - BatteryDispatcherProxy should take the BatteryDispatcher as an argument,
> rather than BatteryStatusListener. The role of the BatteryDispatcherProxy
should
> be *just proxying* methods between BatteryDispatcher and content/. Make that
> logic clearer.

No, platform/battery/BatteryDispatcherProxy cannot depend on
modules/battery/BatteryDispatcher.  The proxy class cannot directly take
BatteryDispatcher as an argument.

Is it okay to keep using BatteryStatusListener for the time being?
If you're okay, I'll address the other two comments above.

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/RunAllTests.cpp (right):

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/RunAllTests.cpp:97: adoptPtr(new
mojo::test::ScopedIPCSupport(testIoThread.task_runner())));
On 2016/02/09 12:01:21, haraken wrote:
> 
> Shall we rename InitializeMojo to InitializePlatform and move the TestIOThread
> initialization into InitializePlatform?

No.  We'd like to keep a ScopedIPCSupport alive while running the tests.  If we
moved it into a function, it's destroyed when the function returns.

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp
(right):

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp:120: #if
!OS(ANDROID)
On 2016/02/09 12:01:21, haraken wrote:
> 
> I'm just curious: Why do we need !OS(ANDROID)?

On Android, BatteryMonitor is implemented in Java.  There is no C++
implementation.

See
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
and
https://code.google.com/p/chromium/codesearch#chromium/src/device/battery/BUI...

[haraken, 2016-02-12 12:48:45]

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/battery/battery_status_dispatcher.cc
(right):

https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/battery/battery_status_dispatcher.cc:15:
BatteryStatusDispatcher::BatteryStatusDispatcher(BatteryStatusListener*
listener)
On 2016/02/12 12:36:14, Yuki wrote:
> On 2016/02/09 12:01:21, haraken wrote:
> > Honestly speaking, I want to let modules/ directly talk with Mojo and remove
> the
> > BatteryStatusDispatcher. However, if we don't want to go that way for now,
at
> > least I want to make it clearer that the BatteryStatusDispatcher is a
"proxy"
> 
> OK
> 
> > for the BatteryDispatcher. Specifically:
> > 
> > - Rename BatteryStatusDispatcher to BatteryDispatcherProxy.
> 
> OK
> 
> > - Make the lifetime of BatteryDispatcher and BatteryDispatcherProxy
identical.
> > The constructor should be renamed to startListening and the destructor
should
> be
> > renamed to stopListening.
> 
> OK
> 
> > - BatteryDispatcherProxy should take the BatteryDispatcher as an argument,
> > rather than BatteryStatusListener. The role of the BatteryDispatcherProxy
> should
> > be *just proxying* methods between BatteryDispatcher and content/. Make that
> > logic clearer.
> 
> No, platform/battery/BatteryDispatcherProxy cannot depend on
> modules/battery/BatteryDispatcher.  The proxy class cannot directly take
> BatteryDispatcher as an argument.
> 
> Is it okay to keep using BatteryStatusListener for the time being?
> If you're okay, I'll address the other two comments above.

Ah, got it. Then how about passing a function pointer that invokes
BatteryDispatcher::OnDidChage()? Then the BatteryDispatcher::OnDidChage()
invokes listener_->OnUpdateBatteryStatus(status).

Avoiding the dependency problem by passing BatteryStatusListener sounds like a
kind of hack to me.

[haraken, 2016-02-12 12:56:29]

On 2016/02/12 12:48:45, haraken wrote:
>
https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/battery/battery_status_dispatcher.cc
> (right):
> 
>
https://codereview.chromium.org/1538803002/diff/700001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/battery/battery_status_dispatcher.cc:15:
> BatteryStatusDispatcher::BatteryStatusDispatcher(BatteryStatusListener*
> listener)
> On 2016/02/12 12:36:14, Yuki wrote:
> > On 2016/02/09 12:01:21, haraken wrote:
> > > Honestly speaking, I want to let modules/ directly talk with Mojo and
remove
> > the
> > > BatteryStatusDispatcher. However, if we don't want to go that way for now,
> at
> > > least I want to make it clearer that the BatteryStatusDispatcher is a
> "proxy"
> > 
> > OK
> > 
> > > for the BatteryDispatcher. Specifically:
> > > 
> > > - Rename BatteryStatusDispatcher to BatteryDispatcherProxy.
> > 
> > OK
> > 
> > > - Make the lifetime of BatteryDispatcher and BatteryDispatcherProxy
> identical.
> > > The constructor should be renamed to startListening and the destructor
> should
> > be
> > > renamed to stopListening.
> > 
> > OK
> > 
> > > - BatteryDispatcherProxy should take the BatteryDispatcher as an argument,
> > > rather than BatteryStatusListener. The role of the BatteryDispatcherProxy
> > should
> > > be *just proxying* methods between BatteryDispatcher and content/. Make
that
> > > logic clearer.
> > 
> > No, platform/battery/BatteryDispatcherProxy cannot depend on
> > modules/battery/BatteryDispatcher.  The proxy class cannot directly take
> > BatteryDispatcher as an argument.
> > 
> > Is it okay to keep using BatteryStatusListener for the time being?
> > If you're okay, I'll address the other two comments above.
> 
> Ah, got it. Then how about passing a function pointer that invokes
> BatteryDispatcher::OnDidChage()? Then the BatteryDispatcher::OnDidChage()
> invokes listener_->OnUpdateBatteryStatus(status).
> 
> Avoiding the dependency problem by passing BatteryStatusListener sounds like a
> kind of hack to me.

Discussed offline. Given that the function pointer will add more complexity and
that this code will be gone once the Mojo-WTF converter is available, let's take
a simpler approach; i.e., I'm fine with using listener_->.

[Yuki, 2016-02-15 08:03:05]

Addressed the rest of haraken's comments.
Could you guys take another look?

[haraken, 2016-02-15 13:18:39]

LGTM!

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/DEPS (right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/DEPS:13: "+base/values.h",

I wonder why you don't need to add base/message_loop.h here while you're
including base/message_loop.h in battery_dispatcher_proxy_unittest.cc. If you
don't hit a presubmit error, is it a bug of DEPS analysis?

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/base/Bind.h (right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/base/Bind.h:6: #define Bind_h

(Nit: I'm not really happy that Blink's threading primitives are scattered in
many files in wtf/ and platform/, but this would be out of the scope of this CL.
I'm fine with the Bind.h in this CL.)

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/base/Bind.h:22:
bindInstanceToMethodForMojo(ReturnType (Class::*method)(Args...), Class*
instance)

sameThreadBindForMojo ? (Assuming that tzik@ is introducing
sameThreadBind/crossThreadBind.)

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/battery/battery_dispatcher_proxy.h
(right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/battery/battery_dispatcher_proxy.h:19: //
Mojo, and then, there will be no need to use this proxy class.

// TODO(yukishiino): Remove this class.

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/battery/battery_dispatcher_proxy_unittest.cc
(right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/battery/battery_dispatcher_proxy_unittest.cc:52:
base::MessageLoop message_loop_;  // Mojo needs the message loop.

Hmm. If possible, I don't want to expose base::MessageLoop to platform/. Would
it be possible to create some testing helper in platform/testing/ so that
base::MessageLoop is allowed only in platform/testing/?

[Yuki, 2016-02-16 07:28:55]

Patchset #41 (id:830001) has been deleted

[Yuki, 2016-02-16 07:31:58]

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/DEPS (right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/DEPS:13: "+base/values.h",
On 2016/02/15 13:18:39, haraken wrote:
> 
> I wonder why you don't need to add base/message_loop.h here while you're
> including base/message_loop.h in battery_dispatcher_proxy_unittest.cc. If you
> don't hit a presubmit error, is it a bug of DEPS analysis?

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...

I put "+base/message_loop.h" here.
(and removed it according to another review comment from you.)

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/base/Bind.h (right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/base/Bind.h:6: #define Bind_h
On 2016/02/15 13:18:39, haraken wrote:
> 
> (Nit: I'm not really happy that Blink's threading primitives are scattered in
> many files in wtf/ and platform/, but this would be out of the scope of this
CL.
> I'm fine with the Bind.h in this CL.)

Acknowledged.

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/base/Bind.h:22:
bindInstanceToMethodForMojo(ReturnType (Class::*method)(Args...), Class*
instance)
On 2016/02/15 13:18:39, haraken wrote:
> 
> sameThreadBindForMojo ? (Assuming that tzik@ is introducing
> sameThreadBind/crossThreadBind.)

Done.

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/battery/battery_dispatcher_proxy.h
(right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/battery/battery_dispatcher_proxy.h:19: //
Mojo, and then, there will be no need to use this proxy class.
On 2016/02/15 13:18:39, haraken wrote:
> 
> // TODO(yukishiino): Remove this class.

Done.

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/battery/battery_dispatcher_proxy_unittest.cc
(right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/battery/battery_dispatcher_proxy_unittest.cc:52:
base::MessageLoop message_loop_;  // Mojo needs the message loop.
On 2016/02/15 13:18:39, haraken wrote:
> 
> Hmm. If possible, I don't want to expose base::MessageLoop to platform/. Would
> it be possible to create some testing helper in platform/testing/ so that
> base::MessageLoop is allowed only in platform/testing/?

Done.

[kinuko, 2016-02-16 10:09:18]

For the chromium-styled files to be placed under platform are we going to have
some common (temporary) mechanism in presubmit like
https://codereview.chromium.org/1660383002/diff/140001/third_party/WebKit/PRE...
?

https://codereview.chromium.org/1538803002/diff/810001/components/test_runner...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1538803002/diff/810001/components/test_runner...
components/test_runner/test_runner.cc:2569: charging, chargingTime,
dischargingTime, level);
Now that we have most code in blink do we need to go through test runner code?

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/battery/BatteryDispatcher.h (right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/battery/BatteryDispatcher.h:11: #include
"platform/battery/battery_dispatcher_proxy.h"
nit: this could be forward-declared

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/base/DEPS (right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/base/DEPS:5:
"+mojo/public/cpp/bindings/callback.h",
nit: What other files are we going to put in this new directory? 'base/' feels
very generic to imagine what to expect, maybe 'mojo/' could work?  Or could we
have the new (and tentative) bind.h under platform/threading?

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/battery/battery_status.h (right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/battery/battery_status.h:24:
BatteryStatus(bool charging, double charging_time, double discharging_time,
double level)
nit: wrap line if this file follows chromium style

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/battery/battery_status_listener.h
(right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/battery/battery_status_listener.h:14: class
PLATFORM_EXPORT BatteryStatusListener {
I think it'd be probably make the structure clearer to name this
BatteryDispatcherProxyClient or BatteryDispatcherProxy::Listener (as an inner
class)?

(For the final naming decision I defer to haraken/main reviewers)

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebView.h (right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebView.h:492: virtual void
resetBatteryStatusForTesting() = 0;
Hmm, these don't look they belong to WebView...

[Yuki, 2016-02-16 13:21:59]

Edited third_party/WebKit/PRESUBMIT.py so that it skips files that follow
Chromium's coding style.

https://codereview.chromium.org/1538803002/diff/810001/components/test_runner...
File components/test_runner/test_runner.cc (right):

https://codereview.chromium.org/1538803002/diff/810001/components/test_runner...
components/test_runner/test_runner.cc:2569: charging, chargingTime,
dischargingTime, level);
On 2016/02/16 10:09:18, kinuko wrote:
> Now that we have most code in blink do we need to go through test runner code?

Our plan is to follow the way of http://crrev.com/1472693004 once it gets
avaiable.  When it's available, we will no longer need window.testRunner nor
window.internals.

For the time being, I'd like to minimize the effort to support the tests.

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/battery/BatteryDispatcher.h (right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/battery/BatteryDispatcher.h:11: #include
"platform/battery/battery_dispatcher_proxy.h"
On 2016/02/16 10:09:18, kinuko wrote:
> nit: this could be forward-declared

Done.

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/base/DEPS (right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/base/DEPS:5:
"+mojo/public/cpp/bindings/callback.h",
On 2016/02/16 10:09:18, kinuko wrote:
> nit: What other files are we going to put in this new directory? 'base/' feels
> very generic to imagine what to expect, maybe 'mojo/' could work?  Or could we
> have the new (and tentative) bind.h under platform/threading?

I moved platform/base/Bind.h to platform/threading/BindForMojo.h .  Does this
look better?

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/battery/battery_status.h (right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/battery/battery_status.h:24:
BatteryStatus(bool charging, double charging_time, double discharging_time,
double level)
On 2016/02/16 10:09:18, kinuko wrote:
> nit: wrap line if this file follows chromium style

Done.

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/battery/battery_status_listener.h
(right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/battery/battery_status_listener.h:14: class
PLATFORM_EXPORT BatteryStatusListener {
On 2016/02/16 10:09:18, kinuko wrote:
> I think it'd be probably make the structure clearer to name this
> BatteryDispatcherProxyClient or BatteryDispatcherProxy::Listener (as an inner
> class)?
> 
> (For the final naming decision I defer to haraken/main reviewers)

Took an idea of BatteryDispatcherProxy::Listener.

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebView.h (right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebView.h:492: virtual void
resetBatteryStatusForTesting() = 0;
On 2016/02/16 10:09:18, kinuko wrote:
> Hmm, these don't look they belong to WebView...

I think we can remove these functions when http://crrev.com/1472693004 gets
available.  Can we live with this way for the time being?

[kinuko, 2016-02-16 22:53:39]

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebView.h (right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebView.h:492: virtual void
resetBatteryStatusForTesting() = 0;
On 2016/02/16 13:21:59, Yuki wrote:
> On 2016/02/16 10:09:18, kinuko wrote:
> > Hmm, these don't look they belong to WebView...
> 
> I think we can remove these functions when http://crrev.com/1472693004 gets
> available.  Can we live with this way for the time being?

Could you please add TODO to remove these then, I don't like to see people
blindly follow this pattern.

[Yuki, 2016-02-17 09:12:54]

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebView.h (right):

https://codereview.chromium.org/1538803002/diff/810001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebView.h:492: virtual void
resetBatteryStatusForTesting() = 0;
On 2016/02/16 22:53:39, kinuko wrote:
> On 2016/02/16 13:21:59, Yuki wrote:
> > On 2016/02/16 10:09:18, kinuko wrote:
> > > Hmm, these don't look they belong to WebView...
> > 
> > I think we can remove these functions when http://crrev.com/1472693004 gets
> > available.  Can we live with this way for the time being?
> 
> Could you please add TODO to remove these then, I don't like to see people
> blindly follow this pattern.

I see.  Added a TODO comment.

[jam, 2016-02-17 16:53:23]

lgtm

btw is there anything holding up landing this? other changes could benefit from
being able to make mojo calls from blink. if there are other things blocking,
could maybe just that part land now by itself? example:
https://codereview.chromium.org/1615133002/

[Sam McNally, 2016-02-18 00:39:35]

LGTM

https://codereview.chromium.org/1538803002/diff/890001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/1538803002/diff/890001/third_party/WebKit/pub...
third_party/WebKit/public/platform/Platform.h:56: #include
"mojo/public/cpp/bindings/interface_ptr.h"
I don't think this is needed.

[kinuko, 2016-02-18 00:50:54]

lgtm, thanks.

https://codereview.chromium.org/1538803002/diff/890001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/battery/BatteryDispatcher.cpp (right):

https://codereview.chromium.org/1538803002/diff/890001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/battery/BatteryDispatcher.cpp:7: #include
"platform/battery/battery_dispatcher_proxy.h"
nit: not needed (it's in header file)

https://codereview.chromium.org/1538803002/diff/890001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/battery/BatteryDispatcher.h (right):

https://codereview.chromium.org/1538803002/diff/890001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/battery/BatteryDispatcher.h:16: class
BatteryDispatcherProxy;
nit: now we include the header for Listener class we can remove this line :)

https://codereview.chromium.org/1538803002/diff/890001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/MojoTestHelper.h (right):

https://codereview.chromium.org/1538803002/diff/890001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/MojoTestHelper.h:13: class
MojoTestHelper final {
nit: lightweight comment would be helpful

[Yuki, 2016-02-18 06:05:12]

yukishiino@chromium.org changed reviewers:
+ tkent@chromium.org

[Yuki, 2016-02-18 06:05:14]

+tkent@, could you review this CL as an owner of third_party/WebKit/public/ ?

https://codereview.chromium.org/1538803002/diff/890001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/battery/BatteryDispatcher.cpp (right):

https://codereview.chromium.org/1538803002/diff/890001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/battery/BatteryDispatcher.cpp:7: #include
"platform/battery/battery_dispatcher_proxy.h"
On 2016/02/18 00:50:53, kinuko wrote:
> nit: not needed (it's in header file)

Done.

https://codereview.chromium.org/1538803002/diff/890001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/battery/BatteryDispatcher.h (right):

https://codereview.chromium.org/1538803002/diff/890001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/battery/BatteryDispatcher.h:16: class
BatteryDispatcherProxy;
On 2016/02/18 00:50:53, kinuko wrote:
> nit: now we include the header for Listener class we can remove this line :)

Done.

https://codereview.chromium.org/1538803002/diff/890001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/MojoTestHelper.h (right):

https://codereview.chromium.org/1538803002/diff/890001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/MojoTestHelper.h:13: class
MojoTestHelper final {
On 2016/02/18 00:50:54, kinuko wrote:
> nit: lightweight comment would be helpful

Done.

https://codereview.chromium.org/1538803002/diff/890001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/1538803002/diff/890001/third_party/WebKit/pub...
third_party/WebKit/public/platform/Platform.h:56: #include
"mojo/public/cpp/bindings/interface_ptr.h"
On 2016/02/18 00:39:35, Sam McNally wrote:
> I don't think this is needed.

Done.

[esprehn, 2016-02-18 06:48:13]

Don't add new APIs to web/ here.

https://codereview.chromium.org/1538803002/diff/910001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/MojoTestHelper.h (right):

https://codereview.chromium.org/1538803002/diff/910001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/MojoTestHelper.h:18: //    
MojoTestHelper mojo_test_helper_;
This doesn't seem good, it just adds a message loop. Maybe name this
TestMojoMessageLoop?

This isn't a Helper (and that name doesn't add value).

https://codereview.chromium.org/1538803002/diff/910001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/RunAllTests.cpp (right):

https://codereview.chromium.org/1538803002/diff/910001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/RunAllTests.cpp:31: #include
"base/command_line.h"
Why?

https://codereview.chromium.org/1538803002/diff/910001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1538803002/diff/910001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:4532:
BatteryDispatcher::instance().OnUpdateBatteryStatus(
This should be exposed in the platform API, not through the web view.

https://codereview.chromium.org/1538803002/diff/910001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:4536: void
WebViewImpl::resetBatteryStatusForTesting()
Ditto

https://codereview.chromium.org/1538803002/diff/910001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/1538803002/diff/910001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.h:519: void
updateBatteryStatusForTesting(bool charging, double chargingTime, double
dischargingTime, double level) override;
Move this to platform.

https://codereview.chromium.org/1538803002/diff/910001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebView.h (right):

https://codereview.chromium.org/1538803002/diff/910001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebView.h:494: virtual void
updateBatteryStatusForTesting(bool charging, double chargingTime, double
dischargingTime, double level) = 0;
This should be in platform.

[Yuki, 2016-02-18 07:20:35]

https://codereview.chromium.org/1538803002/diff/910001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/MojoTestHelper.h (right):

https://codereview.chromium.org/1538803002/diff/910001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/MojoTestHelper.h:18: //    
MojoTestHelper mojo_test_helper_;
On 2016/02/18 06:48:13, esprehn wrote:
> This doesn't seem good, it just adds a message loop. Maybe name this
> TestMojoMessageLoop?
> 
> This isn't a Helper (and that name doesn't add value).

Then, we're going to introduce this header file just in order to avoid putting a
line of "+base/message_loop" in platform/battery/DEPS, right?

In that way, we don't need TestMojoMessageLoop, we can use base::MessageLoop and
that's what we want.

platform/testing/DEPS:
  "+base/message_loop"  # already exists
platform/testing/MessageLoopForMojo.h:
  #include "base/message_loop/message_loop.h"

platform/battery/battery_dispatcher_proxy_unittest.cc:
  #include "platform/testing/MessageLoopForMojo.h"
  class BatteryDispatcherProxyTest : publi testing::Test {
   private:
    base::MessageLoop message_loop_;  // Why don't we directly use
base::MesssageLoop instead of TestMojoMessageLoop?
  };

Does this look okay?  Or do you really want TestMojoMessageLoop like following?

platform/testing/TestMojoMessageLoop.h:
  #include "base/message_loop/message_loop.h"
  typedef base::MessageLoop TestMojoMessageLoop;

[esprehn, 2016-02-18 07:29:48]

I'm fine with using MessageLoop directly, it's just not clear that you always
need one when creating a mojo test, maybe we just need a MojoTest class you
extend instead of testing::Test so every test doesn't need this magic
incantation.

I also have to say I'm sad that mojo tests need a message loop, why can't we
mock out the IPC send/response layer and control precisely when incoming and
outgoing messages happen? The tests should be perfectly predictable, see for
example Sim tests where the network layer is mocked out to control response
order precisely.

[Ken Rockot(use gerrit already), 2016-02-18 08:45:31]

On Wed, Feb 17, 2016 at 11:29 PM, <esprehn@chromium.org> wrote:

> I'm fine with using MessageLoop directly, it's just not clear that you always
> need one when creating a mojo test, maybe we just need a MojoTest class you
> extend instead of testing::Test so every test doesn't need this magic
> incantation.
>
> I also have to say I'm sad that mojo tests need a message loop, why can't we
> mock out the IPC send/response layer and control precisely when incoming and
> outgoing messages happen? The tests should be perfectly predictable, see for
>
> So we could do this, but I'm not convinced we should.

It'd mean building a mock implementation of the mojo system API layer as
well as a few other bits at the bindings layer. This isn't a huge problem
in itself, but we'll also need a way of letting code in the same process
talk to two different system implementations. I suppose we could support
something like a thread-local function table of API overrides...

In any case it's a non-trivial effort and the practical value of eschewing
all asynchronous behavior in these tests is not clear to me.

example Sim tests where the network layer is mocked out to control response
> order precisely.
> https://codereview.chromium.org/1538803002/
>
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-02-18 08:45:31]

On Wed, Feb 17, 2016 at 11:29 PM, <esprehn@chromium.org> wrote:

> I'm fine with using MessageLoop directly, it's just not clear that you always
> need one when creating a mojo test, maybe we just need a MojoTest class you
> extend instead of testing::Test so every test doesn't need this magic
> incantation.
>
> I also have to say I'm sad that mojo tests need a message loop, why can't we
> mock out the IPC send/response layer and control precisely when incoming and
> outgoing messages happen? The tests should be perfectly predictable, see for
>
> So we could do this, but I'm not convinced we should.

It'd mean building a mock implementation of the mojo system API layer as
well as a few other bits at the bindings layer. This isn't a huge problem
in itself, but we'll also need a way of letting code in the same process
talk to two different system implementations. I suppose we could support
something like a thread-local function table of API overrides...

In any case it's a non-trivial effort and the practical value of eschewing
all asynchronous behavior in these tests is not clear to me.

example Sim tests where the network layer is mocked out to control response
> order precisely.
> https://codereview.chromium.org/1538803002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[esprehn, 2016-02-18 09:10:48]

On 2016/02/18 at 08:45:31, rockot wrote:
> On Wed, Feb 17, 2016 at 11:29 PM, <esprehn@chromium.org> wrote:
> 
> > I'm fine with using MessageLoop directly, it's just not clear that you
always
> > need one when creating a mojo test, maybe we just need a MojoTest class you
> > extend instead of testing::Test so every test doesn't need this magic
> > incantation.
> >
> > I also have to say I'm sad that mojo tests need a message loop, why can't we
> > mock out the IPC send/response layer and control precisely when incoming and
> > outgoing messages happen? The tests should be perfectly predictable, see for
> >
> > So we could do this, but I'm not convinced we should.
> 
> It'd mean building a mock implementation of the mojo system API layer as
> well as a few other bits at the bindings layer. This isn't a huge problem
> in itself, but we'll also need a way of letting code in the same process
> talk to two different system implementations. I suppose we could support
> something like a thread-local function table of API overrides...
> 
> In any case it's a non-trivial effort and the practical value of eschewing
> all asynchronous behavior in these tests is not clear to me.
> 

Deterministic tests are fast and never flaky.  They're easier to debug and
simpler to write.

IPC in blink should work this way, you should be able to say
expect(outgoingIpc).thenReplyWith(incomingIpc) and mock out the browser side
entirely. Blink platform tests shouldn't need to yield to the event loop and
potentially let lots of other code interfere.

I'm hoping to mock out the entire scheduler and event loop for unit tests with
Sim.

If mojo isnt going to provide this we'll need to hide mojo behind an API layer
in platform and not use it directly.

[haraken, 2016-02-18 11:38:04]

https://codereview.chromium.org/1538803002/diff/910001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1538803002/diff/910001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:4532:
BatteryDispatcher::instance().OnUpdateBatteryStatus(
On 2016/02/18 06:48:13, esprehn wrote:
> This should be exposed in the platform API, not through the web view.

Just to confirm: Do you mean public/web/? This is an API from content/ to Blink,
it should be in public/web/, not in public/platform/.

However, a better option would be moving testRunner.updateBatteryStatus to
window.internals.updateBatteryStatus. Then no public APIs will be needed.

(I'm not sure how much we want to stick to this point because once we build up
the Mojo-JS bindings, this API will be gone anyway. But if you spend time on
moving the API to public/web/, it would be better to spend time to move it to
window.internals.)

[Yuki, 2016-02-18 13:33:44]

Addressed Elliott's comments except for how to expose test APIs.

If possible, I'd like to go as is expecting that we can soon support JS-binding
of Mojo and build the tests on it.

What do you guys think?

https://codereview.chromium.org/1538803002/diff/910001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/RunAllTests.cpp (right):

https://codereview.chromium.org/1538803002/diff/910001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/RunAllTests.cpp:31: #include
"base/command_line.h"
On 2016/02/18 06:48:13, esprehn wrote:
> Why?

I overlooked
  #include <base/command_line.h>
below.  Good catch.

[haraken, 2016-02-18 13:36:31]

On 2016/02/18 13:33:44, Yuki wrote:
> Addressed Elliott's comments except for how to expose test APIs.
> 
> If possible, I'd like to go as is expecting that we can soon support
JS-binding
> of Mojo and build the tests on it.
> 
> What do you guys think?

I'm fine with it.

My point is just that if you spend time moving the API to public/web/, I'd
prefer moving it to window.internals.

[Ken Rockot(use gerrit already), 2016-02-18 15:36:36]

On 2016/02/18 at 09:10:48, esprehn wrote:
> On 2016/02/18 at 08:45:31, rockot wrote:
> > On Wed, Feb 17, 2016 at 11:29 PM, <esprehn@chromium.org> wrote:
> > 
> > > I'm fine with using MessageLoop directly, it's just not clear that you
always
> > > need one when creating a mojo test, maybe we just need a MojoTest class
you
> > > extend instead of testing::Test so every test doesn't need this magic
> > > incantation.
> > >
> > > I also have to say I'm sad that mojo tests need a message loop, why can't
we
> > > mock out the IPC send/response layer and control precisely when incoming
and
> > > outgoing messages happen? The tests should be perfectly predictable, see
for
> > >
> > > So we could do this, but I'm not convinced we should.
> > 
> > It'd mean building a mock implementation of the mojo system API layer as
> > well as a few other bits at the bindings layer. This isn't a huge problem
> > in itself, but we'll also need a way of letting code in the same process
> > talk to two different system implementations. I suppose we could support
> > something like a thread-local function table of API overrides...
> > 
> > In any case it's a non-trivial effort and the practical value of eschewing
> > all asynchronous behavior in these tests is not clear to me.
> > 
> 
> Deterministic tests are fast and never flaky.  They're easier to debug and
simpler to write.
> 

I agree that determinism is a good constraint to enforce.

A single MessageLoop which is only used to defer sequenced tasks on a single
thread
for consumers on that same thread does not introduce any non-determinism.
Presumably
in test cases that's what we have: a mock service and its client on the same
thread
with pipes between them. In this case I don't see how it's worth avoiding the
asynchronous behavior.

What you're proposing would require either that:

  A. Services support re-entrancy so if a client calls Foo::DoStuff and calls it
     again upon receiving the response, Foo::DoStuff needs to be safely
re-entered.
     Or

  B. The test code manually pumps a special task queue exposed by the
hypothetical
     mock mojo implementation.

Option A is bad and not a real option, and I fail to see how option B is enough
of
an improvement over automatic task pumping that it's worth building all this
other
crap to support it.

Am I missing a source of non-determinism here?

> IPC in blink should work this way, you should be able to say
expect(outgoingIpc).thenReplyWith(incomingIpc) and mock out the browser side
entirely. Blink platform tests shouldn't need to yield to the event loop and
potentially let lots of other code interfere.
> 
> I'm hoping to mock out the entire scheduler and event loop for unit tests with
Sim.
> 
> If mojo isnt going to provide this we'll need to hide mojo behind an API layer
in platform and not use it directly.

[jam, 2016-02-18 20:24:07]

On 2016/02/18 15:36:36, Ken Rockot wrote:
> On 2016/02/18 at 09:10:48, esprehn wrote:
> > On 2016/02/18 at 08:45:31, rockot wrote:
> > > On Wed, Feb 17, 2016 at 11:29 PM, <mailto:esprehn@chromium.org> wrote:
> > > 
> > > > I'm fine with using MessageLoop directly, it's just not clear that you
> always
> > > > need one when creating a mojo test, maybe we just need a MojoTest class
> you
> > > > extend instead of testing::Test so every test doesn't need this magic
> > > > incantation.
> > > >
> > > > I also have to say I'm sad that mojo tests need a message loop, why
can't
> we
> > > > mock out the IPC send/response layer and control precisely when incoming
> and
> > > > outgoing messages happen? The tests should be perfectly predictable, see
> for
> > > >
> > > > So we could do this, but I'm not convinced we should.
> > > 
> > > It'd mean building a mock implementation of the mojo system API layer as
> > > well as a few other bits at the bindings layer. This isn't a huge problem
> > > in itself, but we'll also need a way of letting code in the same process
> > > talk to two different system implementations. I suppose we could support
> > > something like a thread-local function table of API overrides...
> > > 
> > > In any case it's a non-trivial effort and the practical value of eschewing
> > > all asynchronous behavior in these tests is not clear to me.
> > > 
> > 
> > Deterministic tests are fast and never flaky.  They're easier to debug and
> simpler to write.
> > 
> 
> I agree that determinism is a good constraint to enforce.
> 
> A single MessageLoop which is only used to defer sequenced tasks on a single
> thread
> for consumers on that same thread does not introduce any non-determinism.
> Presumably
> in test cases that's what we have: a mock service and its client on the same
> thread
> with pipes between them. In this case I don't see how it's worth avoiding the
> asynchronous behavior.
> 
> What you're proposing would require either that:
> 
>   A. Services support re-entrancy so if a client calls Foo::DoStuff and calls
it
>      again upon receiving the response, Foo::DoStuff needs to be safely
> re-entered.
>      Or
> 
>   B. The test code manually pumps a special task queue exposed by the
> hypothetical
>      mock mojo implementation.
> 
> Option A is bad and not a real option, and I fail to see how option B is
enough
> of
> an improvement over automatic task pumping that it's worth building all this
> other
> crap to support it.
> 
> Am I missing a source of non-determinism here?

Another option could be to run the mock service on a separate thread. Then the
client can call the service and block (without pumping on its thread) for the
mock service to receive the ipc. that would address Elliott's concern about
other tasks on the main blink thread running in the meantime.

> 
> > IPC in blink should work this way, you should be able to say
> expect(outgoingIpc).thenReplyWith(incomingIpc) and mock out the browser side
> entirely. Blink platform tests shouldn't need to yield to the event loop and
> potentially let lots of other code interfere.
> > 
> > I'm hoping to mock out the entire scheduler and event loop for unit tests
with
> Sim.
> > 
> > If mojo isnt going to provide this we'll need to hide mojo behind an API
layer
> in platform and not use it directly.

adding another layer seems counter to the efforts to reduce layering. perhaps we
can sync up in person when you're in mtv? like Ken, I'd like to understand your
concerns a bit more, and how exactly you think the IPC replies should work.

[tkent, 2016-02-19 00:20:25]

On 2016/02/18 at 13:36:31, haraken wrote:
> My point is just that if you spend time moving the API to public/web/, I'd
prefer moving it to window.internals.

I agree with it.  It seems that moving the WebView functions to window.internals
is easy.
We'd like to avoid to expose unnecessary stuff via public/.

[esprehn, 2016-02-19 00:21:48]

On 2016/02/19 at 00:20:25, tkent wrote:
> On 2016/02/18 at 13:36:31, haraken wrote:
> > My point is just that if you spend time moving the API to public/web/, I'd
prefer moving it to window.internals.
> 
> I agree with it.  It seems that moving the WebView functions to
window.internals is easy.
> We'd like to avoid to expose unnecessary stuff via public/.

Yeah, adding more public apis should really be avoided if possible. :)

[Yuki, 2016-02-19 07:37:20]

Okay, I moved testing support from window.testRunner to window.internals.  No
change in public/web/.  Could you guys take another look?

[haraken, 2016-02-19 07:42:00]

LGTM

[tkent, 2016-02-19 07:46:51]

components/test_runner and third_party/WebKit lgtm

[Ken Rockot(use gerrit already), 2016-02-19 21:55:01]

On 2016/02/19 at 07:46:51, tkent wrote:
> components/test_runner and third_party/WebKit lgtm

FWIW (keeping the discussion in this thread) the vision for testing Mojo clients
in Blink is to never interact with the browser process, but to instead intercept
service requests and fulfill them with mock implementations on the same thread
as the client. There's already been work done to support this, with an example
in LayoutTests/harness-tests/mojo-helpers.html. And though it's a bit dated and
incomplete there's an example of mocking the USB services consumed by Web USB
here: https://codereview.chromium.org/1472693004. It's waiting on a solution for
how to load generated bindings in layout tests since we don't want to commit
generated code.

[esprehn, 2016-02-19 22:04:58]

On 2016/02/19 at 21:55:01, rockot wrote:
> On 2016/02/19 at 07:46:51, tkent wrote:
> > components/test_runner and third_party/WebKit lgtm
> 
> FWIW (keeping the discussion in this thread) the vision for testing Mojo
clients in Blink is to never interact with the browser process, but to instead
intercept service requests and fulfill them with mock implementations on the
same thread as the client. There's already been work done to support this, with
an example in LayoutTests/harness-tests/mojo-helpers.html. And though it's a bit
dated and incomplete there's an example of mocking the USB services consumed by
Web USB here: https://codereview.chromium.org/1472693004. It's waiting on a
solution for how to load generated bindings in layout tests since we don't want
to commit generated code.

Sounds like an excellent plan. :)

lgtm

[jam, 2016-02-19 23:07:40]

On 2016/02/19 22:04:58, esprehn wrote:
> On 2016/02/19 at 21:55:01, rockot wrote:
> > On 2016/02/19 at 07:46:51, tkent wrote:
> > > components/test_runner and third_party/WebKit lgtm
> > 
> > FWIW (keeping the discussion in this thread) the vision for testing Mojo
> clients in Blink is to never interact with the browser process, but to instead
> intercept service requests and fulfill them with mock implementations on the
> same thread as the client. There's already been work done to support this,
with
> an example in LayoutTests/harness-tests/mojo-helpers.html. And though it's a
bit
> dated and incomplete there's an example of mocking the USB services consumed
by
> Web USB here: https://codereview.chromium.org/1472693004. It's waiting on a
> solution for how to load generated bindings in layout tests since we don't
want
> to commit generated code.
> 
> Sounds like an excellent plan. :)
> 
> lgtm

just to be clear, are we talking about JS or C++ tests? the previous comments
discussing testing::Test etc (i.e. comment 63) made it seem like C++ unit tests
was what's being discussed. The message above is for the layout tests.

For C++ unit tests, there'll be no browser process. There's still the issue
where in a unit test one could call a mojo method and want to either verify that
a given callback is received. a messageloop would still need to be run if the
callback is a parameter of the method invocation. this is common practice with
our C++ unit tests that use mojo.

[Yuki, 2016-02-24 07:35:07]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2016-02-24 07:35:08]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
kinuko@chromium.org, sammc@chromium.org, tkent@chromium.org,
esprehn@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1538803002/#ps990001
(title: "Synced.")

[commit-bot: I haz the power, 2016-02-24 07:36:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1538803002/990001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1538803002/990001

[commit-bot: I haz the power, 2016-02-24 07:49:11]

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

[commit-bot: I haz the power, 2016-02-24 07:49:14]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Yuki, 2016-02-24 10:43:44]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2016-02-24 10:45:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1538803002/990001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1538803002/990001

[commit-bot: I haz the power, 2016-02-24 11:01:29]

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

[commit-bot: I haz the power, 2016-02-24 11:01:31]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Yuki, 2016-02-24 12:59:15]

Description was changed from

==========
Migrates battery_status from content/renderer/ to WebKit/platform/

As an effort to simplify the codebase after the repogitory
merge of Blink into Chromium, we no longer need to put
battery_status in content/renderer/.  Thus, migrates it to
WebKit/Source/platform/.

* Files under WebKit/Source/platform/battery/ are
intentionally written in Chromium's coding style so that
it's easy to follow the diff.

* There is a plan to rewrite the mock service of battery
status for layout tests based on JS-bindings of Mojo.
(c.f. http://crrev.com/1472693004 )

BUG=561879
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Migrates battery_status from content/renderer/ to WebKit/platform/

As an effort to simplify the codebase after the repogitory
merge of Blink into Chromium, we no longer need to put
battery_status in content/renderer/.  Thus, migrates it to
WebKit/Source/platform/.

* Files under WebKit/Source/platform/battery/ are
intentionally written in Chromium's coding style so that
it's easy to follow the diff.

* There is a plan to rewrite the mock service of battery
status for layout tests based on JS-bindings of Mojo.
(c.f. http://crrev.com/1472693004 )

BUG=561879
==========

[Yuki, 2016-02-24 14:37:43]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-24 14:38:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1538803002/1030001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1538803002/1030001

[commit-bot: I haz the power, 2016-02-24 14:51:04]

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

[commit-bot: I haz the power, 2016-02-24 14:51:06]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Sam McNally, 2016-02-24 23:49:11]

On 2016/02/24 14:51:06, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

I've tracked this down to a bug in git cl format. It's getting confused because
this CL only changes non-C++ files in media. I'm working on a fix.

[Yuki, 2016-02-25 05:37:46]

On 2016/02/24 23:49:11, Sam McNally wrote:
> On 2016/02/24 14:51:06, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> >   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> I've tracked this down to a bug in git cl format. It's getting confused
because
> this CL only changes non-C++ files in media. I'm working on a fix.

Good to know!  Thanks for letting me know and working on the issue.

[Yuki, 2016-02-25 07:09:12]

Description was changed from

==========
Migrates battery_status from content/renderer/ to WebKit/platform/

As an effort to simplify the codebase after the repogitory
merge of Blink into Chromium, we no longer need to put
battery_status in content/renderer/.  Thus, migrates it to
WebKit/Source/platform/.

* Files under WebKit/Source/platform/battery/ are
intentionally written in Chromium's coding style so that
it's easy to follow the diff.

* There is a plan to rewrite the mock service of battery
status for layout tests based on JS-bindings of Mojo.
(c.f. http://crrev.com/1472693004 )

BUG=561879
==========

to

==========
Migrates battery_status from content/renderer/ to WebKit/platform/

As an effort to simplify the codebase after the repogitory
merge of Blink into Chromium, we no longer need to put
battery_status in content/renderer/.  Thus, migrates it to
WebKit/Source/platform/.

* Files under WebKit/Source/platform/battery/ are
intentionally written in Chromium's coding style so that
it's easy to follow the diff.

* There is a plan to rewrite the mock service of battery
status for layout tests based on JS-bindings of Mojo.
(c.f. http://crrev.com/1472693004 )

BUG=561879
NOPRESUBMIT=true
==========

[Yuki, 2016-02-25 08:41:24]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2016-02-25 08:41:25]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
esprehn@chromium.org, jam@chromium.org, kinuko@chromium.org, sammc@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1538803002/#ps1050001
(title: "Synced.")

[commit-bot: I haz the power, 2016-02-25 08:42:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1538803002/1050001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1538803002/1050001

[commit-bot: I haz the power, 2016-02-25 10:10:50]

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

[commit-bot: I haz the power, 2016-02-25 10:10:52]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Yuki, 2016-02-25 12:36:42]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2016-02-25 12:36:43]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
esprehn@chromium.org, jam@chromium.org, kinuko@chromium.org, sammc@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1538803002/#ps1070001
(title: "Synced.")

[commit-bot: I haz the power, 2016-02-25 12:38:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1538803002/1070001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1538803002/1070001

[commit-bot: I haz the power, 2016-02-25 14:28:22]

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

[commit-bot: I haz the power, 2016-02-25 14:28:25]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yuki, 2016-02-25 14:30:10]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2016-02-25 14:30:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1538803002/1070001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1538803002/1070001

[commit-bot: I haz the power, 2016-02-25 16:28:51]

Description was changed from

==========
Migrates battery_status from content/renderer/ to WebKit/platform/

As an effort to simplify the codebase after the repogitory
merge of Blink into Chromium, we no longer need to put
battery_status in content/renderer/.  Thus, migrates it to
WebKit/Source/platform/.

* Files under WebKit/Source/platform/battery/ are
intentionally written in Chromium's coding style so that
it's easy to follow the diff.

* There is a plan to rewrite the mock service of battery
status for layout tests based on JS-bindings of Mojo.
(c.f. http://crrev.com/1472693004 )

BUG=561879
NOPRESUBMIT=true
==========

to

==========
Migrates battery_status from content/renderer/ to WebKit/platform/

As an effort to simplify the codebase after the repogitory
merge of Blink into Chromium, we no longer need to put
battery_status in content/renderer/.  Thus, migrates it to
WebKit/Source/platform/.

* Files under WebKit/Source/platform/battery/ are
intentionally written in Chromium's coding style so that
it's easy to follow the diff.

* There is a plan to rewrite the mock service of battery
status for layout tests based on JS-bindings of Mojo.
(c.f. http://crrev.com/1472693004 )

BUG=561879
NOPRESUBMIT=true
==========

[commit-bot: I haz the power, 2016-02-25 16:28:54]

Committed patchset #52 (id:1070001)

[commit-bot: I haz the power, 2016-02-25 16:30:30]

Description was changed from

==========
Migrates battery_status from content/renderer/ to WebKit/platform/

As an effort to simplify the codebase after the repogitory
merge of Blink into Chromium, we no longer need to put
battery_status in content/renderer/.  Thus, migrates it to
WebKit/Source/platform/.

* Files under WebKit/Source/platform/battery/ are
intentionally written in Chromium's coding style so that
it's easy to follow the diff.

* There is a plan to rewrite the mock service of battery
status for layout tests based on JS-bindings of Mojo.
(c.f. http://crrev.com/1472693004 )

BUG=561879
NOPRESUBMIT=true
==========

to

==========
Migrates battery_status from content/renderer/ to WebKit/platform/

As an effort to simplify the codebase after the repogitory
merge of Blink into Chromium, we no longer need to put
battery_status in content/renderer/.  Thus, migrates it to
WebKit/Source/platform/.

* Files under WebKit/Source/platform/battery/ are
intentionally written in Chromium's coding style so that
it's easy to follow the diff.

* There is a plan to rewrite the mock service of battery
status for layout tests based on JS-bindings of Mojo.
(c.f. http://crrev.com/1472693004 )

BUG=561879
NOPRESUBMIT=true

Committed: https://crrev.com/0d152ad4f21edf77ac5b585fcd8e6d05f1aae162
Cr-Commit-Position: refs/heads/master@{#377586}
==========

[commit-bot: I haz the power, 2016-02-25 16:30:33]

Patchset 52 (id:??) landed as
https://crrev.com/0d152ad4f21edf77ac5b585fcd8e6d05f1aae162
Cr-Commit-Position: refs/heads/master@{#377586}

[agrieve, 2016-02-25 20:24:16]

On 2016/02/25 16:30:33, commit-bot: I haz the power wrote:
> Patchset 52 (id:??) landed as
> https://crrev.com/0d152ad4f21edf77ac5b585fcd8e6d05f1aae162
> Cr-Commit-Position: refs/heads/master@{#377586}

Sadly, I think this is breaking Android's Intel bots: 
https://build.chromium.org/p/chromium.android/builders/Android%20x64%20Builde...
https://build.chromium.org/p/chromium.android/builders/Android%20x86%20Builde...

Filed to track: https://bugs.chromium.org/p/chromium/issues/detail?id=589949

Shall I revert?

[Ken Rockot(use gerrit already), 2016-02-25 20:26:20]

Yep, let's revert.
On Feb 25, 2016 12:24 PM, <agrieve@chromium.org> wrote:

> On 2016/02/25 16:30:33, commit-bot: I haz the power wrote:
> > Patchset 52 (id:??) landed as
> > https://crrev.com/0d152ad4f21edf77ac5b585fcd8e6d05f1aae162
> > Cr-Commit-Position: refs/heads/master@{#377586}
>
> Sadly, I think this is breaking Android's Intel bots:
>
>
https://build.chromium.org/p/chromium.android/builders/Android%20x64%20Builde...
>
>
https://build.chromium.org/p/chromium.android/builders/Android%20x86%20Builde...
>
> Filed to track:
> https://bugs.chromium.org/p/chromium/issues/detail?id=589949
>
> Shall I revert?
>
> https://codereview.chromium.org/1538803002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-02-25 20:26:20]

Yep, let's revert.
On Feb 25, 2016 12:24 PM, <agrieve@chromium.org> wrote:

> On 2016/02/25 16:30:33, commit-bot: I haz the power wrote:
> > Patchset 52 (id:??) landed as
> > https://crrev.com/0d152ad4f21edf77ac5b585fcd8e6d05f1aae162
> > Cr-Commit-Position: refs/heads/master@{#377586}
>
> Sadly, I think this is breaking Android's Intel bots:
>
>
https://build.chromium.org/p/chromium.android/builders/Android%20x64%20Builde...
>
>
https://build.chromium.org/p/chromium.android/builders/Android%20x86%20Builde...
>
> Filed to track:
> https://bugs.chromium.org/p/chromium/issues/detail?id=589949
>
> Shall I revert?
>
> https://codereview.chromium.org/1538803002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[agrieve, 2016-02-25 20:34:14]

A revert of this CL (patchset #52 id:1070001) has been created in
https://codereview.chromium.org/1740843002/ by agrieve@chromium.org.

The reason for reverting is: Breaking bots.
https://bugs.chromium.org/p/chromium/issues/detail?id=589949.

[jam, 2016-02-25 20:42:17]

These aren't bots on the CQ, so we can try to fix it asynchronously. Yuki is in
a different timezone, but I can spend a little of time on it

On 2016/02/25 20:26:20, Ken Rockot wrote:
> Yep, let's revert.
> On Feb 25, 2016 12:24 PM, <mailto:agrieve@chromium.org> wrote:
> 
> > On 2016/02/25 16:30:33, commit-bot: I haz the power wrote:
> > > Patchset 52 (id:??) landed as
> > > https://crrev.com/0d152ad4f21edf77ac5b585fcd8e6d05f1aae162
> > > Cr-Commit-Position: refs/heads/master@{#377586}
> >
> > Sadly, I think this is breaking Android's Intel bots:
> >
> >
>
https://build.chromium.org/p/chromium.android/builders/Android%20x64%20Builde...
> >
> >
>
https://build.chromium.org/p/chromium.android/builders/Android%20x86%20Builde...
> >
> > Filed to track:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=589949
> >
> > Shall I revert?
> >
> > https://codereview.chromium.org/1538803002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Blink
> Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

[jam, 2016-02-25 21:06:05]

fix on the way https://codereview.chromium.org/1737873002