bluetooth: Make device/bluetooth a shared library component.

bluetooth: Make device/bluetooth a shared library component.

device/bluetooth will soon be accessed by content/browser. This
change converts it from a static library to a shared component.

BUG=438305
Committed: https://crrev.com/ff6eaf5adf4d6dc5a748e2ee4e8de080655d1975
Cr-Commit-Position: refs/heads/master@{#307049}

Committed: https://crrev.com/7c0d27723f1a89c546493103ce01157f92b27d12
Cr-Commit-Position: refs/heads/master@{#307180}

[scheib, 2014-12-02 23:50:12]

Patchset #1 (id:1) has been deleted

[scheib, 2014-12-02 23:50:18]

Patchset #1 (id:20001) has been deleted

[scheib, 2014-12-02 23:51:39]

scheib@chromium.org changed reviewers:
+ armansito@chromium.org

[armansito, 2014-12-03 00:10:04]

I'll do a first round review later today if that's OK with you? CC'd keybuk@ and
isherman@ as FYI, since isherman was also doing something similar in another CL
that never got merged (https://codereview.chromium.org/655093002/).

[scheib, 2014-12-03 00:11:15]

Thanks, sounds good.

On Tue, Dec 2, 2014 at 4:10 PM, <armansito@chromium.org> wrote:

> I'll do a first round review later today if that's OK with you? CC'd
> keybuk@ and
> isherman@ as FYI, since isherman was also doing something similar in
> another CL
> that never got merged (https://codereview.chromium.org/655093002/).
>
> https://codereview.chromium.org/778443002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[armansito, 2014-12-03 00:20:34]

https://codereview.chromium.org/778443002/diff/60001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_chromeos.h (right):

https://codereview.chromium.org/778443002/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.h:37: class DEVICE_BLUETOOTH_EXPORT
BluetoothAdapterChromeOS
One of the discussions that I was having with isherman@ on his CL was whether or
not we want to export the symbols for platform-specific implementations, though
not doing so caused problems with device_unittests.

[Ilya Sherman, 2014-12-03 01:10:46]

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

[Ilya Sherman, 2014-12-03 01:10:47]

Ah, yes, I've been meaning to come back to this.  The main issue that I ran into
was in finding the syntax to allow the tests to access the platform-specific
classes, but disallowing any other code from accessing them.  Part of the
challenge is that the bluetooth tests run as part of a broader device_unittests
target; so there's a question of whether all tests in that target can access
these classes (probably fine, but a little odd conceptually, and possibly
syntactically); or whether to define a new target for just the bluetooth tests.

https://codereview.chromium.org/778443002/diff/60001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_chromeos.h (right):

https://codereview.chromium.org/778443002/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.h:37: class DEVICE_BLUETOOTH_EXPORT
BluetoothAdapterChromeOS
On 2014/12/03 00:20:34, armansito wrote:
> One of the discussions that I was having with isherman@ on his CL was whether
or
> not we want to export the symbols for platform-specific implementations,
though
> not doing so caused problems with device_unittests.

Yeah, I think the explicit goal of adding export macros was to *not* export the
platform-specific implementations.

[armansito, 2014-12-03 03:22:14]

On 2014/12/03 01:10:47, Ilya Sherman wrote:
> Ah, yes, I've been meaning to come back to this.  The main issue that I ran
into
> was in finding the syntax to allow the tests to access the platform-specific
> classes, but disallowing any other code from accessing them.  Part of the
> challenge is that the bluetooth tests run as part of a broader
device_unittests
> target; so there's a question of whether all tests in that target can access
> these classes (probably fine, but a little odd conceptually, and possibly
> syntactically); or whether to define a new target for just the bluetooth
tests.
> 

Actually, this might make most sense; basically just have a bluetooth_tests
target. Another thing I've had in mind for some time was to move bluetooth/ out
of device/ into components/, which might go along with adding a bluetooth-only
test target.

>
https://codereview.chromium.org/778443002/diff/60001/device/bluetooth/bluetoo...
> File device/bluetooth/bluetooth_adapter_chromeos.h (right):
> 
>
https://codereview.chromium.org/778443002/diff/60001/device/bluetooth/bluetoo...
> device/bluetooth/bluetooth_adapter_chromeos.h:37: class
DEVICE_BLUETOOTH_EXPORT
> BluetoothAdapterChromeOS
> On 2014/12/03 00:20:34, armansito wrote:
> > One of the discussions that I was having with isherman@ on his CL was
whether
> or
> > not we want to export the symbols for platform-specific implementations,
> though
> > not doing so caused problems with device_unittests.
> 
> Yeah, I think the explicit goal of adding export macros was to *not* export
the
> platform-specific implementations.

[scheib, 2014-12-03 04:20:55]

On 2014/12/03 03:22:14, armansito wrote:
> On 2014/12/03 01:10:47, Ilya Sherman wrote:
> > Ah, yes, I've been meaning to come back to this.  The main issue that I ran
> into
> > was in finding the syntax to allow the tests to access the platform-specific
> > classes, but disallowing any other code from accessing them.  Part of the
> > challenge is that the bluetooth tests run as part of a broader
> device_unittests
> > target; so there's a question of whether all tests in that target can access
> > these classes (probably fine, but a little odd conceptually, and possibly
> > syntactically); or whether to define a new target for just the bluetooth
> tests.
> > 
> 
> Actually, this might make most sense; basically just have a bluetooth_tests
> target. Another thing I've had in mind for some time was to move bluetooth/
out
> of device/ into components/, which might go along with adding a bluetooth-only
> test target.

device and components seem fairly similar, except that:
a) device/ has been setup build wise better, more of the components there are
actually componentized. src/components/components.gyp is a mess.
b) device/ clearly indicates that the sub folders are related to device access,
they're not generalized components. This isn't particularly high value, but I
think there is still value there and there's already momentum.

A separate refactor for a bluetooth_tests target sounds fine, but I don't think
it should be conflated with solving the library type issue. The Web Bluetooth
work is blocked on the component build not working, and I'd like to fix that
specifically.

[scheib, 2014-12-03 04:21:04]

https://codereview.chromium.org/778443002/diff/60001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_chromeos.h (right):

https://codereview.chromium.org/778443002/diff/60001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_chromeos.h:37: class DEVICE_BLUETOOTH_EXPORT
BluetoothAdapterChromeOS
On 2014/12/03 01:10:46, Ilya Sherman wrote:
> On 2014/12/03 00:20:34, armansito wrote:
> > One of the discussions that I was having with isherman@ on his CL was
whether
> or
> > not we want to export the symbols for platform-specific implementations,
> though
> > not doing so caused problems with device_unittests.
> 
> Yeah, I think the explicit goal of adding export macros was to *not* export
the
> platform-specific implementations.

I think this is orthogonal to the code being a static lib or a shared lib. We
shouldn't conflate the two.

[armansito, 2014-12-03 20:32:30]

armansito@chromium.org changed reviewers:
+ benchan@chromium.org

[armansito, 2014-12-03 20:32:31]

lgtm with one nit. Added benchan@ as a reviewer since ensemble pulls in the
bluetooth code, to make sure that this won't break anything for them.

https://codereview.chromium.org/778443002/diff/80001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_export.h (right):

https://codereview.chromium.org/778443002/diff/80001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_export.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
nit: Remove the "(c)"

[scheib, 2014-12-05 06:47:47]

benchan?
On Dec 3, 2014 12:32 PM, <armansito@chromium.org> wrote:

> lgtm with one nit. Added benchan@ as a reviewer since ensemble pulls in
> the
> bluetooth code, to make sure that this won't break anything for them.
>
>
> https://codereview.chromium.org/778443002/diff/80001/
> device/bluetooth/bluetooth_export.h
> File device/bluetooth/bluetooth_export.h (right):
>
> https://codereview.chromium.org/778443002/diff/80001/
> device/bluetooth/bluetooth_export.h#newcode1
> device/bluetooth/bluetooth_export.h:1: // Copyright (c) 2014 The
> Chromium Authors. All rights reserved.
> nit: Remove the "(c)"
>
> https://codereview.chromium.org/778443002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ben Chan, 2014-12-05 07:18:11]

lgtm
On Dec 4, 2014 10:47 PM, "Vincent Scheib" <scheib@chromium.org> wrote:

> benchan?
> On Dec 3, 2014 12:32 PM, <armansito@chromium.org> wrote:
>
>> lgtm with one nit. Added benchan@ as a reviewer since ensemble pulls in
>> the
>> bluetooth code, to make sure that this won't break anything for them.
>>
>>
>> https://codereview.chromium.org/778443002/diff/80001/
>> device/bluetooth/bluetooth_export.h
>> File device/bluetooth/bluetooth_export.h (right):
>>
>> https://codereview.chromium.org/778443002/diff/80001/
>> device/bluetooth/bluetooth_export.h#newcode1
>> device/bluetooth/bluetooth_export.h:1: // Copyright (c) 2014 The
>> Chromium Authors. All rights reserved.
>> nit: Remove the "(c)"
>>
>> https://codereview.chromium.org/778443002/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[scheib, 2014-12-05 16:51:33]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2014-12-05 16:52:11]

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

[commit-bot: I haz the power, 2014-12-05 18:32:41]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2014-12-05 18:33:20]

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

[scheib, 2014-12-05 22:30:43]

A revert of this CL (patchset #4 id:100001) has been created in
https://codereview.chromium.org/748873004/ by scheib@chromium.org.

The reason for reverting is: Chrome target is failing to load due to
BluetoothApis.dll not being found on Win7. Likely the DelayLoadDLLs for
BluetoothApis.dll is necessary on more than just device_bluetooth. .

[scheib, 2014-12-05 23:51:24]

Solution found. The DelayLoadDll section of the device_bluetooth component only
applied to dependents. When it converted to a shared library it no longer
applied, and was a hard dependency.

I won't land the fix until I'm available to monitor it.

[scheib, 2014-12-05 23:55:12]

Note: Reasonable bot to monitor for issues is:
http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%28...

Previous failure looked like this
http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%28...

[scheib, 2014-12-06 14:55:03]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2014-12-06 14:55:47]

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

[commit-bot: I haz the power, 2014-12-06 16:02:13]

Committed patchset #5 (id:120001)

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

Patchset 5 (id:??) landed as
https://crrev.com/7c0d27723f1a89c546493103ce01157f92b27d12
Cr-Commit-Position: refs/heads/master@{#307180}