Initialize BlueZ in app_shell on Linux

Initialize BlueZ in app_shell on Linux

Whitelisted Linux apps can access chrome.bluetooth. Initialize Bluetooth so
we don't crash on those apps.

BUG=711142
R=rkc@chromium.org,rdevlin.cronin@chromium.org

Review-Url: https://codereview.chromium.org/2819593003
Cr-Commit-Position: refs/heads/master@{#469540}
Committed: https://chromium.googlesource.com/chromium/src/+/f221eabf049250f2aaad03c74ec660e5e12a7b45

[michaelpg, 2017-04-13 23:46:17]

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

[commit-bot: I haz the power, 2017-04-13 23:46:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 01:01:28]

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

[commit-bot: I haz the power, 2017-04-14 01:01:29]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2017-04-14 01:05:59]

Description was changed from

==========
Initialize BlueZ in app_shell on Linux

Whitelisted Linux apps can access chrome.bluetooth. Initialize Bluetooth so
we don't crash on those apps.

BUG=711142
R=rkc@chromium.org
==========

to

==========
Initialize BlueZ in app_shell on Linux

Whitelisted Linux apps can access chrome.bluetooth. Initialize Bluetooth so
we don't crash on those apps.

BUG=711142
R=rkc@chromium.org,rdevlin.cronin@chromium.org
==========

[michaelpg, 2017-04-14 01:05:59]

michaelpg@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[michaelpg, 2017-04-14 01:07:17]

PTAL. Devlin: What do you think about the test concept? It's just a defense
against super basic crashes for now.

[Devlin, 2017-04-14 01:43:44]

https://codereview.chromium.org/2819593003/diff/20001/extensions/shell/browse...
File extensions/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/2819593003/diff/20001/extensions/shell/browse...
extensions/shell/browser/shell_browser_main_parts.cc:62: #include
"device/bluetooth/dbus/bluez_dbus_manager.h"
Put this in the ifdef above for both CrOS and Linux?

https://codereview.chromium.org/2819593003/diff/20001/extensions/shell/browse...
extensions/shell/browser/shell_browser_main_parts.cc:138:
ui::InitializeInputMethodForTesting();
ForTesting methods used in non test environments typically mean something should
change :) (even if it's just the method name). Do the same assumptions held in
bug 381852 still hold true?

https://codereview.chromium.org/2819593003/diff/20001/extensions/test/data/ap...
File extensions/test/data/api_test/app_apis/runtest.js (right):

https://codereview.chromium.org/2819593003/diff/20001/extensions/test/data/ap...
extensions/test/data/api_test/app_apis/runtest.js:41: testBluetooth,
It's going to be hard to scale this. Is there any way we can iterate over APIs?
Or do we really need to have end-to-end tests of a method?

[michaelpg, 2017-04-14 03:52:58]

https://codereview.chromium.org/2819593003/diff/20001/extensions/shell/browse...
File extensions/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/2819593003/diff/20001/extensions/shell/browse...
extensions/shell/browser/shell_browser_main_parts.cc:62: #include
"device/bluetooth/dbus/bluez_dbus_manager.h"
On 2017/04/14 01:43:44, Devlin wrote:
> Put this in the ifdef above for both CrOS and Linux?

Done.

https://codereview.chromium.org/2819593003/diff/20001/extensions/shell/browse...
extensions/shell/browser/shell_browser_main_parts.cc:138:
ui::InitializeInputMethodForTesting();
On 2017/04/14 01:43:44, Devlin wrote:
> ForTesting methods used in non test environments typically mean something
should
> change :) (even if it's just the method name). Do the same assumptions held in
> bug 381852 still hold true?

I added the TODO because I need to investigate this. (There isn't a functional
change here besides the Bluetooth stuff.)

I do know that keyboard input *works* on Linux because of this line... i'm just
not sure why, or how robust IME support is or would need to be. Agree that we
should refactor as necessary to avoid calling ForTesting() methods.

https://codereview.chromium.org/2819593003/diff/20001/extensions/test/data/ap...
File extensions/test/data/api_test/app_apis/runtest.js (right):

https://codereview.chromium.org/2819593003/diff/20001/extensions/test/data/ap...
extensions/test/data/api_test/app_apis/runtest.js:41: testBluetooth,
On 2017/04/14 01:43:44, Devlin wrote:
> It's going to be hard to scale this. Is there any way we can iterate over
APIs?
> Or do we really need to have end-to-end tests of a method?

I'm not sure what "really" is modifying here...

"Do we *really* need end-to-end tests of a method?"

Yes, because without them we don't notice that they crash app_shell and/or
chrome.

"Do we really need *end-to-end* tests of a method?"

Yes, but that's not ready yet.
https://docs.google.com/document/d/12a1k9BT5-1FjLyNhUA9wKFgcO1rWsbuu0h7S-lrH7yQ

[Devlin, 2017-04-14 16:10:30]

https://codereview.chromium.org/2819593003/diff/20001/extensions/test/data/ap...
File extensions/test/data/api_test/app_apis/runtest.js (right):

https://codereview.chromium.org/2819593003/diff/20001/extensions/test/data/ap...
extensions/test/data/api_test/app_apis/runtest.js:41: testBluetooth,
On 2017/04/14 03:52:58, michaelpg wrote:
> On 2017/04/14 01:43:44, Devlin wrote:
> > It's going to be hard to scale this. Is there any way we can iterate over
> APIs?
> > Or do we really need to have end-to-end tests of a method?
> 
> I'm not sure what "really" is modifying here...
> 
> "Do we *really* need end-to-end tests of a method?"
> 
> Yes, because without them we don't notice that they crash app_shell and/or
> chrome.
> 
> "Do we really need *end-to-end* tests of a method?"
> 
> Yes, but that's not ready yet.
>
https://docs.google.com/document/d/12a1k9BT5-1FjLyNhUA9wKFgcO1rWsbuu0h7S-lrH7yQ

It will be a losing battle to try and re-test every API in both appshell and
chrome.  I think longer term, we want to look into a way of testing both while
only writing a single test (which I think is what the doc is targeting,
ideally).  In the near-term, I guess I'm not sure if I see the benefit in trying
to write a thorough test for a single API in a one-off manner, rather than
either a) writing basic sanity checks or b) testing properties of the shell
system.

If we have to write one-offs for some things in the short term for other reasons
(if, for instance, bluetooth was a p1 api to get working on app shell and we
wanted to make sure nothing broke), that's fine, but then I wouldn't put it
under the guise of a generic "test all the apis!"-type test.

Does that make sense?

[michaelpg, 2017-04-14 22:54:43]

https://codereview.chromium.org/2819593003/diff/20001/extensions/test/data/ap...
File extensions/test/data/api_test/app_apis/runtest.js (right):

https://codereview.chromium.org/2819593003/diff/20001/extensions/test/data/ap...
extensions/test/data/api_test/app_apis/runtest.js:14: !!chrome.bluetooth,
'chrome.bluetooth should be available');
oops, will fix this indent

https://codereview.chromium.org/2819593003/diff/20001/extensions/test/data/ap...
extensions/test/data/api_test/app_apis/runtest.js:41: testBluetooth,
On 2017/04/14 16:10:30, Devlin wrote:
> On 2017/04/14 03:52:58, michaelpg wrote:
> > On 2017/04/14 01:43:44, Devlin wrote:
> > > It's going to be hard to scale this. Is there any way we can iterate over
> > APIs?
> > > Or do we really need to have end-to-end tests of a method?
> > 
> > I'm not sure what "really" is modifying here...
> > 
> > "Do we *really* need end-to-end tests of a method?"
> > 
> > Yes, because without them we don't notice that they crash app_shell and/or
> > chrome.
> > 
> > "Do we really need *end-to-end* tests of a method?"
> > 
> > Yes, but that's not ready yet.
> >
>
https://docs.google.com/document/d/12a1k9BT5-1FjLyNhUA9wKFgcO1rWsbuu0h7S-lrH7yQ
> 
> It will be a losing battle to try and re-test every API in both appshell and
> chrome.  I think longer term, we want to look into a way of testing both while
> only writing a single test (which I think is what the doc is targeting,
> ideally).

Yep! It should be as simple and automatic as possible.

>  In the near-term, I guess I'm not sure if I see the benefit in trying
> to write a thorough test for a single API in a one-off manner, rather than
> either a) writing basic sanity checks or b) testing properties of the shell
> system.

Basic sanity checks are what I'm after in the near term. I don't see this test
as being thorough; it merely verifies that calling API functions does not throw
an error in JS or crash the process. We have real Bluetooth tests (that mostly
run in Chrome) which test the API implementation more thoroughly.

> 
> If we have to write one-offs for some things in the short term for other
reasons
> (if, for instance, bluetooth was a p1 api to get working on app shell and we
> wanted to make sure nothing broke), that's fine, but then I wouldn't put it
> under the guise of a generic "test all the apis!"-type test.

A crash when calling an API that's allowed on app_shell is a P1 bug. Tests that
identify (thus prevent) these bugs are also P1; LMK if you disagree and we can
discuss.

I'm fine with renaming this something like bluetooth_sanity_test if you like.

[Devlin, 2017-04-17 15:04:48]

lgtm

> I'm fine with renaming this something like bluetooth_sanity_test if you like.

Yeah, I think that helps clarify the point of these tests as making sure we have
no catastrophic failures at different points, rather than testing the API
implementation itself.

https://codereview.chromium.org/2819593003/diff/40001/extensions/browser/api/...
File extensions/browser/api/app_apis_test.cc (right):

https://codereview.chromium.org/2819593003/diff/40001/extensions/browser/api/...
extensions/browser/api/app_apis_test.cc:9: IN_PROC_BROWSER_TEST_F(AppApisTest,
Load) {
nit: s/Load/ApiSanityCheck or similar

https://codereview.chromium.org/2819593003/diff/40001/extensions/test/data/ap...
File extensions/test/data/api_test/app_apis/runtest.js (right):

https://codereview.chromium.org/2819593003/diff/40001/extensions/test/data/ap...
extensions/test/data/api_test/app_apis/runtest.js:12: function testBluetooth() {
nit: bluetoothSanityCheck, blueZSanityCheck, etc.

https://codereview.chromium.org/2819593003/diff/40001/extensions/test/data/ap...
extensions/test/data/api_test/app_apis/runtest.js:18: 'AB:CD:EF:01:23:45',
chrome.test.callback(() => {}, 'Invalid device'));
nit: prefer using chrome.test.callbackFail('Invalid device')

[rkc, 2017-04-17 18:42:09]

https://codereview.chromium.org/2819593003/diff/40001/extensions/browser/BUIL...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/2819593003/diff/40001/extensions/browser/BUIL...
extensions/browser/BUILD.gn:315: "api/app_apis_test.cc",
I presume this is only needed till crbug.com/650835 is fixed? If so, add a TODO
here to remove.

https://codereview.chromium.org/2819593003/diff/40001/extensions/shell/browse...
File extensions/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/2819593003/diff/40001/extensions/shell/browse...
extensions/shell/browser/shell_browser_main_parts.cc:137:
ui::InitializeInputMethodForTesting();
Should this be part of another CL?

[michaelpg, 2017-04-21 20:26:52]

Patchset #5 (id:80001) has been deleted

[michaelpg, 2017-04-21 20:27:01]

https://codereview.chromium.org/2819593003/diff/40001/extensions/browser/BUIL...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/2819593003/diff/40001/extensions/browser/BUIL...
extensions/browser/BUILD.gn:315: "api/app_apis_test.cc",
On 2017/04/17 18:42:09, rkc wrote:
> I presume this is only needed till crbug.com/650835 is fixed? If so, add a
TODO
> here to remove.

Done.

https://codereview.chromium.org/2819593003/diff/40001/extensions/browser/api/...
File extensions/browser/api/app_apis_test.cc (right):

https://codereview.chromium.org/2819593003/diff/40001/extensions/browser/api/...
extensions/browser/api/app_apis_test.cc:9: IN_PROC_BROWSER_TEST_F(AppApisTest,
Load) {
On 2017/04/17 15:04:47, Devlin wrote:
> nit: s/Load/ApiSanityCheck or similar

Done (1)

https://codereview.chromium.org/2819593003/diff/40001/extensions/shell/browse...
File extensions/shell/browser/shell_browser_main_parts.cc (right):

https://codereview.chromium.org/2819593003/diff/40001/extensions/shell/browse...
extensions/shell/browser/shell_browser_main_parts.cc:137:
ui::InitializeInputMethodForTesting();
On 2017/04/17 18:42:09, rkc wrote:
> Should this be part of another CL?

I think I would write it like this anyway, I think duplicating a small amount of
work in an #elif is the right thing to do as the initialization orders for
different platforms may matter. I added the comment because this CL is touching
the code around it.

Note that there's no functional change with this line.

https://codereview.chromium.org/2819593003/diff/40001/extensions/test/data/ap...
File extensions/test/data/api_test/app_apis/runtest.js (right):

https://codereview.chromium.org/2819593003/diff/40001/extensions/test/data/ap...
extensions/test/data/api_test/app_apis/runtest.js:12: function testBluetooth() {
On 2017/04/17 15:04:48, Devlin wrote:
> nit: bluetoothSanityCheck, blueZSanityCheck, etc.

Done (2)

https://codereview.chromium.org/2819593003/diff/40001/extensions/test/data/ap...
extensions/test/data/api_test/app_apis/runtest.js:18: 'AB:CD:EF:01:23:45',
chrome.test.callback(() => {}, 'Invalid device'));
On 2017/04/17 15:04:48, Devlin wrote:
> nit: prefer using chrome.test.callbackFail('Invalid device')

Done (3)

https://codereview.chromium.org/2819593003/diff/60001/extensions/browser/api/...
File extensions/browser/api/bluetooth/bluetooth_appshell_test.cc (right):

https://codereview.chromium.org/2819593003/diff/60001/extensions/browser/api/...
extensions/browser/api/bluetooth/bluetooth_appshell_test.cc:9:
IN_PROC_BROWSER_TEST_F(BluetoothShellApiTest, ApiSanityCheck) {
here 1

https://codereview.chromium.org/2819593003/diff/60001/extensions/test/data/ap...
File extensions/test/data/api_test/bluetooth/runtest.js (right):

https://codereview.chromium.org/2819593003/diff/60001/extensions/test/data/ap...
extensions/test/data/api_test/bluetooth/runtest.js:12: function
bluetoothSanityCheck() {
here 2

https://codereview.chromium.org/2819593003/diff/60001/extensions/test/data/ap...
extensions/test/data/api_test/bluetooth/runtest.js:18: 'AB:CD:EF:01:23:45',
chrome.test.callbackFail('Invalid device'));
here 3

[rkc, 2017-04-21 20:31:25]

lgtm

[michaelpg, 2017-05-04 23:09:52]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2017-05-04 23:09:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, rkc@chromium.org
Link to the patchset: https://codereview.chromium.org/2819593003/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-04 23:11:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 00:17:43]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493939392662530,
"parent_rev": "0f4c67a6a0ab2e4fc4b57d888ff5a007cee93700", "commit_rev":
"f221eabf049250f2aaad03c74ec660e5e12a7b45"}

[commit-bot: I haz the power, 2017-05-05 00:17:56]

Description was changed from

==========
Initialize BlueZ in app_shell on Linux

Whitelisted Linux apps can access chrome.bluetooth. Initialize Bluetooth so
we don't crash on those apps.

BUG=711142
R=rkc@chromium.org,rdevlin.cronin@chromium.org
==========

to

==========
Initialize BlueZ in app_shell on Linux

Whitelisted Linux apps can access chrome.bluetooth. Initialize Bluetooth so
we don't crash on those apps.

BUG=711142
R=rkc@chromium.org,rdevlin.cronin@chromium.org

Review-Url: https://codereview.chromium.org/2819593003
Cr-Commit-Position: refs/heads/master@{#469540}
Committed:
https://chromium.googlesource.com/chromium/src/+/f221eabf049250f2aaad03c74ec6...
==========

[commit-bot: I haz the power, 2017-05-05 00:17:57]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f221eabf049250f2aaad03c74ec6...