Enable the mojo-based serial API in the renderer behind a flag.

Enable the mojo-based serial API in the renderer behind a flag.

This enables the serial custom bindings that forward to the mojo serial
service behind the --enable-mojo-serial-service flag. This adds service
factory overrides to ServiceRegistrationManager for testing and updates
the serial api browsertest to test both serial implementations.

BUG=389016
Committed: https://crrev.com/fb8875c804337e452e7f799bc3960b912100e5a9
Cr-Commit-Position: refs/heads/master@{#301594}

[Sam McNally, 2014-10-14 18:13:38]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2014-10-14 18:21:08]

sammc@chromium.org changed reviewers:
+ raymes@chromium.org

[raymes, 2014-10-15 21:07:09]

Sorry, there's a few things in the test I'm not sure about!

Also there's probably someone better to review the changes in
chrome_content_browser_client_extensions_part.cc and module_system.cc. I don't
know much about those but the changes look simple.

https://codereview.chromium.org/652313002/diff/20001/extensions/browser/api/s...
File extensions/browser/api/serial/serial_apitest.cc (left):

https://codereview.chromium.org/652313002/diff/20001/extensions/browser/api/s...
extensions/browser/api/serial/serial_apitest.cc:76: Return(true));
Why did this line get moved?

https://codereview.chromium.org/652313002/diff/20001/extensions/browser/api/s...
File extensions/browser/api/serial/serial_apitest.cc (right):

https://codereview.chromium.org/652313002/diff/20001/extensions/browser/api/s...
extensions/browser/api/serial/serial_apitest.cc:72: static
scoped_refptr<device::SerialIoHandler> Create() {
Where is this called from?

https://codereview.chromium.org/652313002/diff/20001/extensions/browser/api/s...
extensions/browser/api/serial/serial_apitest.cc:109: if (GetParam())
I don't really understand these GetParam related changes. I guess this is for
the mojo implementation, but where is the flag set? Does the test get run twice,
once with the flag and once without?

https://codereview.chromium.org/652313002/diff/20001/extensions/browser/api/s...
extensions/browser/api/serial/serial_apitest.cc:137: &request);
Could you split this up into a few statements? :)

https://codereview.chromium.org/652313002/diff/20001/extensions/browser/servi...
File extensions/browser/service_registration_manager.h (right):

https://codereview.chromium.org/652313002/diff/20001/extensions/browser/servi...
extensions/browser/service_registration_manager.h:89: void
OverrideServiceFactoryForTest(
It seems a bit nicer to me to avoid having this test code in the implementation.
Instead you could override the ServiceRegistrationManager for test with your own
one that is constructed manually (perhaps with a separate constructor that
doesn't add the services automatically).

[Sam McNally, 2014-10-20 07:09:05]

Patchset #2 (id:40001) has been deleted

[Sam McNally, 2014-10-20 07:17:08]

Patchset #3 (id:80001) has been deleted

[Sam McNally, 2014-10-20 07:20:45]

Patchset #3 (id:100001) has been deleted

[Sam McNally, 2014-10-20 07:21:07]

https://codereview.chromium.org/652313002/diff/20001/extensions/browser/api/s...
File extensions/browser/api/serial/serial_apitest.cc (left):

https://codereview.chromium.org/652313002/diff/20001/extensions/browser/api/s...
extensions/browser/api/serial/serial_apitest.cc:76: Return(true));
On 2014/10/15 21:07:09, raymes wrote:
> Why did this line get moved?

So FakeEchoSerialIoHandlers created elsewhere have the same mock expectation.

https://codereview.chromium.org/652313002/diff/20001/extensions/browser/api/s...
File extensions/browser/api/serial/serial_apitest.cc (right):

https://codereview.chromium.org/652313002/diff/20001/extensions/browser/api/s...
extensions/browser/api/serial/serial_apitest.cc:72: static
scoped_refptr<device::SerialIoHandler> Create() {
On 2014/10/15 21:07:09, raymes wrote:
> Where is this called from?

CreateTestSerialServiceOnFileThread.

https://codereview.chromium.org/652313002/diff/20001/extensions/browser/api/s...
extensions/browser/api/serial/serial_apitest.cc:109: if (GetParam())
On 2014/10/15 21:07:09, raymes wrote:
> I don't really understand these GetParam related changes. I guess this is for
> the mojo implementation, but where is the flag set? Does the test get run
twice,
> once with the flag and once without?

Yes. INSTANTIATE_TEST_CASE_P(SerialApiTest, SerialApiTest, testing::Bool()) does
it.

https://codereview.chromium.org/652313002/diff/20001/extensions/browser/api/s...
extensions/browser/api/serial/serial_apitest.cc:137: &request);
On 2014/10/15 21:07:09, raymes wrote:
> Could you split this up into a few statements? :)

Done.

https://codereview.chromium.org/652313002/diff/20001/extensions/browser/servi...
File extensions/browser/service_registration_manager.h (right):

https://codereview.chromium.org/652313002/diff/20001/extensions/browser/servi...
extensions/browser/service_registration_manager.h:89: void
OverrideServiceFactoryForTest(
On 2014/10/15 21:07:09, raymes wrote:
> It seems a bit nicer to me to avoid having this test code in the
implementation.
> Instead you could override the ServiceRegistrationManager for test with your
own
> one that is constructed manually (perhaps with a separate constructor that
> doesn't add the services automatically).

Done.

[raymes, 2014-10-24 00:09:30]

https://codereview.chromium.org/652313002/diff/20001/extensions/browser/api/s...
File extensions/browser/api/serial/serial_apitest.cc (right):

https://codereview.chromium.org/652313002/diff/20001/extensions/browser/api/s...
extensions/browser/api/serial/serial_apitest.cc:109: if (GetParam())
nit: Cool. It's probably worth making a comment about that here.

https://codereview.chromium.org/652313002/diff/140001/extensions/browser/serv...
File extensions/browser/service_registration_manager.cc (right):

https://codereview.chromium.org/652313002/diff/140001/extensions/browser/serv...
extensions/browser/service_registration_manager.cc:98: void
ServiceRegistrationManager::ClearServiceRegistrationManagerForTest() {
nit: this isn't really needed right, you could just call
SetServiceRegistrationManagerForTest(NULL)

https://codereview.chromium.org/652313002/diff/140001/extensions/test/service...
File extensions/test/service_registration_manager_test_api.h (right):

https://codereview.chromium.org/652313002/diff/140001/extensions/test/service...
extensions/test/service_registration_manager_test_api.h:32: factory)))).second;
Please break this up a bit to reduce the indenting.

https://codereview.chromium.org/652313002/diff/140001/extensions/test/service...
extensions/test/service_registration_manager_test_api.h:50: class
ServiceRegistrationManagerTestApi {
Why not just expose TestServiceRegistrationManager directly and call
SetServiceRegstrationManagerForTest from inside the test?

[Sam McNally, 2014-10-24 03:48:47]

Patchset #5 (id:160001) has been deleted

[Sam McNally, 2014-10-24 03:55:57]

https://codereview.chromium.org/652313002/diff/140001/extensions/browser/serv...
File extensions/browser/service_registration_manager.cc (right):

https://codereview.chromium.org/652313002/diff/140001/extensions/browser/serv...
extensions/browser/service_registration_manager.cc:98: void
ServiceRegistrationManager::ClearServiceRegistrationManagerForTest() {
On 2014/10/24 00:09:30, raymes wrote:
> nit: this isn't really needed right, you could just call
> SetServiceRegistrationManagerForTest(NULL)

Done.

https://codereview.chromium.org/652313002/diff/140001/extensions/test/service...
File extensions/test/service_registration_manager_test_api.h (right):

https://codereview.chromium.org/652313002/diff/140001/extensions/test/service...
extensions/test/service_registration_manager_test_api.h:32: factory)))).second;
On 2014/10/24 00:09:30, raymes wrote:
> Please break this up a bit to reduce the indenting.

Done.

https://codereview.chromium.org/652313002/diff/140001/extensions/test/service...
extensions/test/service_registration_manager_test_api.h:50: class
ServiceRegistrationManagerTestApi {
On 2014/10/24 00:09:30, raymes wrote:
> Why not just expose TestServiceRegistrationManager directly and call
> SetServiceRegstrationManagerForTest from inside the test?

ServiceRegistrationManager isn't really part of the testing interface - only
OverrideServiceFactoryForTest is.

[raymes, 2014-10-27 00:11:47]

https://codereview.chromium.org/652313002/diff/140001/extensions/test/service...
File extensions/test/service_registration_manager_test_api.h (right):

https://codereview.chromium.org/652313002/diff/140001/extensions/test/service...
extensions/test/service_registration_manager_test_api.h:50: class
ServiceRegistrationManagerTestApi {
As discussed, let's get rid of this class and just expose the above directly :)

[Sam McNally, 2014-10-27 06:52:13]

Patchset #7 (id:220001) has been deleted

[Sam McNally, 2014-10-27 06:56:25]

https://codereview.chromium.org/652313002/diff/140001/extensions/test/service...
File extensions/test/service_registration_manager_test_api.h (right):

https://codereview.chromium.org/652313002/diff/140001/extensions/test/service...
extensions/test/service_registration_manager_test_api.h:50: class
ServiceRegistrationManagerTestApi {
On 2014/10/27 00:11:46, raymes wrote:
> As discussed, let's get rid of this class and just expose the above directly
:)

Done.

[raymes, 2014-10-27 22:44:18]

lgtm

[Sam McNally, 2014-10-28 03:20:36]

sammc@chromium.org changed reviewers:
+ rockot@chromium.org

[Sam McNally, 2014-10-28 03:20:36]

+rockot

[Ken Rockot(use gerrit already), 2014-10-28 05:15:22]

lgtm

[Sam McNally, 2014-10-28 07:13:56]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-10-28 07:15:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/652313002/260001

[commit-bot: I haz the power, 2014-10-28 07:19:11]

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

[commit-bot: I haz the power, 2014-10-28 07:19:12]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/74453)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Sam McNally, 2014-10-28 07:21:22]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-10-28 07:22:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/652313002/280001

[commit-bot: I haz the power, 2014-10-28 07:57:57]

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

[commit-bot: I haz the power, 2014-10-28 07:57:58]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/79505)

[Sam McNally, 2014-10-28 09:56:42]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-10-28 09:57:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/652313002/300001

[commit-bot: I haz the power, 2014-10-28 11:51:14]

Committed patchset #10 (id:300001)

[commit-bot: I haz the power, 2014-10-28 11:52:19]

Patchset 10 (id:??) landed as
https://crrev.com/fb8875c804337e452e7f799bc3960b912100e5a9
Cr-Commit-Position: refs/heads/master@{#301594}