bluetooth: mac: Reject requestDevice promise if Mac version is <= 10.9

bluetooth: mac: Reject requestDevice promise if Mac version is <= 10.9

BUG=651350
Committed: https://crrev.com/30846487078f8a61467d1b39b12e41cfe3bed524
Cr-Commit-Position: refs/heads/master@{#423155}

[ortuno, 2016-10-04 03:20:23]

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

[ortuno, 2016-10-04 03:20:24]

In the latest iteration, the tests that are failing are tests that call
requestDevice() multiple times. When we first call
WebBluetoothServiceImpl::RequestDevice() we retrieve an adapter. The second
time, we call IsLowEnergySupported() which returns false.

The problem is that we don't set the adapter on the BluetoothAdapterFactory but
rather on the BluetoothAdapterFactoryWrapper. So even after we set the fake
adapter, BluetoothAdapterFactory::IsLowEnergySupported() will return false. You
need to change the logic of BluetoothAdapterFactoryWrapper::IsLowEnergySupported
to first check if there is an adapter present and if so return true.

https://codereview.chromium.org/2384463002/diff/100001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/2384463002/diff/100001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:343: if
(!BluetoothAdapterFactoryWrapper::Get().IsLowEnergyAvailable()) {
Replace BluetoothAdapterFactoryWrapper::Get().IsBluetoothAdapterAvailable() with
this.

https://codereview.chromium.org/2384463002/diff/100001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:345:
UMARequestDeviceOutcome::BLUETOOTH_LOW_ENERGY_NOT_AVAILABLE);
Use this errors rather than the NO_BLUETOOTH_ADAPTER one. This one is more
appropriate because IsBluetoothAdapterAvailable returns whether or not bluetooth
is supported.

https://codereview.chromium.org/2384463002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_factory.cc (right):

https://codereview.chromium.org/2384463002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_factory.cc:75: #if defined(OS_ANDROID) ||
defined(OS_CHROMEOS) || defined(OS_WIN) || \
We have a couple of issues to remove devices the user can't interact with from
the chooser. I think Windows falls under this category as well. WDYT?

https://codereview.chromium.org/2384463002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_factory.cc:79: #if defined(OS_MACOSX)
nit: Could you combine this with the previous line with #elif
defined(OS_MACOSX)?

https://codereview.chromium.org/2384463002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_factory.cc:81: #endif
nit: comment missing:
// defined(OS_ANDROID) || defined(OS_CHROMEOS) ...

https://codereview.chromium.org/2384463002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_factory_wrapper.cc (right):

https://codereview.chromium.org/2384463002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_factory_wrapper.cc:45: return
BluetoothAdapterFactory::IsLowEnergyAvailable();
I think you would want a similar logic to the adapter factory:

if(adapter_ != nullptr) {
  return true;
}

https://codereview.chromium.org/2384463002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_factory_wrapper.h (right):

https://codereview.chromium.org/2384463002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_factory_wrapper.h:38: bool
IsLowEnergyAvailable();
We no longer need IsBluetoothAdapterAvailable; replace it with this.

[François Beaufort, 2016-10-04 09:54:10]

Patchset #1 (id:1) has been deleted

[François Beaufort, 2016-10-04 09:54:18]

Patchset #1 (id:20001) has been deleted

[François Beaufort, 2016-10-04 09:54:24]

Patchset #1 (id:40001) has been deleted

[François Beaufort, 2016-10-04 09:54:32]

Patchset #1 (id:60001) has been deleted

[François Beaufort, 2016-10-04 09:54:38]

Patchset #1 (id:80001) has been deleted

[François Beaufort, 2016-10-04 10:00:22]

Thanks Giovanni!

https://codereview.chromium.org/2384463002/diff/100001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.cc (right):

https://codereview.chromium.org/2384463002/diff/100001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:343: if
(!BluetoothAdapterFactoryWrapper::Get().IsLowEnergyAvailable()) {
On 2016/10/04 03:20:24, ortuno wrote:
> Replace BluetoothAdapterFactoryWrapper::Get().IsBluetoothAdapterAvailable()
with
> this.

Done.

https://codereview.chromium.org/2384463002/diff/100001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.cc:345:
UMARequestDeviceOutcome::BLUETOOTH_LOW_ENERGY_NOT_AVAILABLE);
On 2016/10/04 03:20:24, ortuno wrote:
> Use this errors rather than the NO_BLUETOOTH_ADAPTER one. This one is more
> appropriate because IsBluetoothAdapterAvailable returns whether or not
bluetooth
> is supported.

Done.

https://codereview.chromium.org/2384463002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_factory.cc (right):

https://codereview.chromium.org/2384463002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_factory.cc:75: #if defined(OS_ANDROID) ||
defined(OS_CHROMEOS) || defined(OS_WIN) || \
On 2016/10/04 03:20:24, ortuno wrote:
> We have a couple of issues to remove devices the user can't interact with from
> the chooser. I think Windows falls under this category as well. WDYT?

So you want Web Bluetooth chooser to not show up on Windows no matter what for
now?

https://codereview.chromium.org/2384463002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_factory.cc:79: #if defined(OS_MACOSX)
On 2016/10/04 03:20:24, ortuno wrote:
> nit: Could you combine this with the previous line with #elif
> defined(OS_MACOSX)?

Done.

https://codereview.chromium.org/2384463002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_factory.cc:81: #endif
On 2016/10/04 03:20:24, ortuno wrote:
> nit: comment missing:
> // defined(OS_ANDROID) || defined(OS_CHROMEOS) ...

Done.

https://codereview.chromium.org/2384463002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_factory_wrapper.cc (right):

https://codereview.chromium.org/2384463002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_factory_wrapper.cc:45: return
BluetoothAdapterFactory::IsLowEnergyAvailable();
On 2016/10/04 03:20:24, ortuno wrote:
> I think you would want a similar logic to the adapter factory:
> 
> if(adapter_ != nullptr) {
>   return true;
> }

Done.

https://codereview.chromium.org/2384463002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_factory_wrapper.h (right):

https://codereview.chromium.org/2384463002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_factory_wrapper.h:38: bool
IsLowEnergyAvailable();
On 2016/10/04 03:20:24, ortuno wrote:
> We no longer need IsBluetoothAdapterAvailable; replace it with this.

Done.

[ortuno, 2016-10-04 21:23:16]

lgtm

https://codereview.chromium.org/2384463002/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_factory.cc (right):

https://codereview.chromium.org/2384463002/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_factory.cc:75: #if defined(OS_ANDROID) ||
defined(OS_CHROMEOS) || defined(OS_WIN) || \
On 2016/10/04 at 10:00:22, François Beaufort wrote:
> On 2016/10/04 03:20:24, ortuno wrote:
> > We have a couple of issues to remove devices the user can't interact with
from
> > the chooser. I think Windows falls under this category as well. WDYT?
> 
> So you want Web Bluetooth chooser to not show up on Windows no matter what for
now?

Now that I think about it the only way to use Web Bluetooth on Windows right now
is if you used the Web Bluetooth flag. So I think it's fine to leave as is for
now, we might want to revisit this once we ship on windows.

[François Beaufort, 2016-10-05 05:04:22]

The CQ bit was checked by beaufort.francois@gmail.com

[commit-bot: I haz the power, 2016-10-05 05:04:40]

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, 2016-10-05 05:14:17]

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

[commit-bot: I haz the power, 2016-10-05 05:14:18]

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

[François Beaufort, 2016-10-05 06:41:12]

beaufort.francois@gmail.com changed reviewers:
+ dcheng@chromium.org, isherman@chromium.org

[François Beaufort, 2016-10-05 06:41:12]

Hello,

Ilya, please review changes in tools/metrics/histograms/histograms.xml


Daniel, please review changes in 
third_party/WebKit/public/platform/modules/bluetooth/web_bluetooth.mojom

Thank you in advance,
Francois

[dcheng, 2016-10-05 06:47:42]

mojo lgtm, but i think the CL might need #if defined(OS_MACOSX) around one of
the #include headers in bluetooth_adaptor_factory.cc:

FAILED: obj/device/bluetooth/bluetooth/bluetooth_adapter_factory.obj 
ninja -t msvc -e environment.x86 -- E:\b\c\cipd\goma/gomacc.exe
"E:\b\depot_tools\win_toolchain\vs_files\d5dc33b15d1b2c086f2f6632e2fd15882f80dbd3\VC\bin\amd64_x86/cl.exe"
/nologo /showIncludes /FC
@obj/device/bluetooth/bluetooth/bluetooth_adapter_factory.obj.rsp /c
../../device/bluetooth/bluetooth_adapter_factory.cc
/Foobj/device/bluetooth/bluetooth/bluetooth_adapter_factory.obj
/Fd"obj/device/bluetooth/bluetooth_cc.pdb"
e:\b\c\b\win\src\base\mac\mac_util.h(8): fatal error C1083: Cannot open include
file: 'Carbon/Carbon.h': No such file or directory

[Ilya Sherman, 2016-10-05 06:49:35]

histograms.xml lgtm

[François Beaufort, 2016-10-05 08:49:28]

On 2016/10/05 06:47:42, dcheng wrote:
> mojo lgtm, but i think the CL might need #if defined(OS_MACOSX) around one of
> the #include headers in bluetooth_adaptor_factory.cc:
> 
> FAILED: obj/device/bluetooth/bluetooth/bluetooth_adapter_factory.obj 
> ninja -t msvc -e environment.x86 -- E:\b\c\cipd\goma/gomacc.exe
>
"E:\b\depot_tools\win_toolchain\vs_files\d5dc33b15d1b2c086f2f6632e2fd15882f80dbd3\VC\bin\amd64_x86/cl.exe"
> /nologo /showIncludes /FC
> @obj/device/bluetooth/bluetooth/bluetooth_adapter_factory.obj.rsp /c
> ../../device/bluetooth/bluetooth_adapter_factory.cc
> /Foobj/device/bluetooth/bluetooth/bluetooth_adapter_factory.obj
> /Fd"obj/device/bluetooth/bluetooth_cc.pdb"
> e:\b\c\b\win\src\base\mac\mac_util.h(8): fatal error C1083: Cannot open
include
> file: 'Carbon/Carbon.h': No such file or directory

Done

[fbeaufortchromium, 2016-10-05 08:49:39]

The CQ bit was checked by fbeaufort@chromium.org

[fbeaufortchromium, 2016-10-05 08:49:40]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
isherman@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2384463002/#ps140001
(title: "Add missing #if defined(OS_MACOSX)")

[commit-bot: I haz the power, 2016-10-05 08:49:53]

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

[dcheng, 2016-10-05 08:56:48]

(still lgtm though)

https://codereview.chromium.org/2384463002/diff/140001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_factory.cc (right):

https://codereview.chromium.org/2384463002/diff/140001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_factory.cc:11: #if defined(OS_MACOSX)
Nit: platform-specific includes guarded by conditions usually go at the end
(https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes)

[fbeaufortchromium, 2016-10-05 08:58:50]

The CQ bit was unchecked by fbeaufort@chromium.org

[François Beaufort, 2016-10-05 09:01:40]

On 2016/10/05 08:56:48, dcheng wrote:
> (still lgtm though)
> 
>
https://codereview.chromium.org/2384463002/diff/140001/device/bluetooth/bluet...
> File device/bluetooth/bluetooth_adapter_factory.cc (right):
> 
>
https://codereview.chromium.org/2384463002/diff/140001/device/bluetooth/bluet...
> device/bluetooth/bluetooth_adapter_factory.cc:11: #if defined(OS_MACOSX)
> Nit: platform-specific includes guarded by conditions usually go at the end
>
(https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes)

Thanks for reminding me this.
I've fixed it now.

[fbeaufortchromium, 2016-10-05 09:01:45]

The CQ bit was checked by fbeaufort@chromium.org

[fbeaufortchromium, 2016-10-05 09:01:46]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
isherman@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2384463002/#ps160001
(title: "Fix nit")

[commit-bot: I haz the power, 2016-10-05 09:01:59]

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, 2016-10-05 09:46:14]

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

[commit-bot: I haz the power, 2016-10-05 09:46:15]

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

[fbeaufortchromium, 2016-10-05 11:07:23]

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

[commit-bot: I haz the power, 2016-10-05 11:07:41]

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

[sdefresne, 2016-10-05 11:08:30]

https://codereview.chromium.org/2384463002/diff/160001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_factory.cc (right):

https://codereview.chromium.org/2384463002/diff/160001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_factory.cc:85: return false;
The above will expand to the following when OS_WIN is defined:

  return true;
  return false;

The Windows compiler warns because the "return false" cannot be reached.

I recommend changing this to the following:

#if defined(OS_ANDROID) || defined(OS_CHROMEOS) || defined(OS_WIN) || \
     defined(OS_LINUX)
  return true;
#elif defined(OS_MACOSX)
  return base::mac::IsAtLeastOS10_10();
#else
  return false;
#endif  // defined(OS_ANDROID) || defined(OS_CHROMEOS) || defined(OS_WIN) ||
        // defined(OS_LINUX)

Or if you do want to have an #else, use a local variable (but note that this
will cause a assignment has no effect when building with clang static analyser):

bool is_low_energy_available = false;
#if defined(OS_ANDROID) || defined(OS_CHROMEOS) || defined(OS_WIN) || \
     defined(OS_LINUX)
  is_low_energy_available = true;
#elif defined(OS_MACOSX)
  is_low_energy_available = base::mac::IsAtLeastOS10_10();
#endif  // defined(OS_ANDROID) || defined(OS_CHROMEOS) || defined(OS_WIN) ||
        // defined(OS_LINUX)
  return is_low_energy_available;

[commit-bot: I haz the power, 2016-10-05 12:26:08]

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

[commit-bot: I haz the power, 2016-10-05 12:26:09]

Dry run: This issue passed the CQ dry run.

[François Beaufort, 2016-10-05 12:27:59]

The CQ bit was checked by beaufort.francois@gmail.com

[François Beaufort, 2016-10-05 12:28:00]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
isherman@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2384463002/#ps180001
(title: "Fix unreachable code")

[commit-bot: I haz the power, 2016-10-05 12:28:17]

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, 2016-10-05 12:34:37]

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

[commit-bot: I haz the power, 2016-10-05 12:34:39]

Failed to apply patch for
third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:
While running git apply --index -3 -p1;
  error: patch failed:
third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:60
  Falling back to three-way merge...
  Applied patch to
'third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp' with conflicts.
  U third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp

Patch:       third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp
Index: third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp
diff --git a/third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp
b/third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp
index
31a77d16d00919bd660a32c832263613e2d47c33..40e926de2a51b256c24dcda55e5aaf025f5d20c3
100644
--- a/third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp
+++ b/third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp
@@ -60,6 +60,7 @@ DOMException* BluetoothError::take(ScriptPromiseResolver*,
int32_t webError /* C
         MAP_ERROR(NO_SERVICES_FOUND, NotFoundError, "No Services found in
device.");
         MAP_ERROR(CHARACTERISTIC_NOT_FOUND, NotFoundError, "No Characteristics
with specified UUID found in Service.");
         MAP_ERROR(NO_CHARACTERISTICS_FOUND, NotFoundError, "No Characteristics
found in service.");
+        MAP_ERROR(BLUETOOTH_LOW_ENERGY_NOT_AVAILABLE, NotFoundError, "Bluetooth
Low Energy not available.");
 
         // NotSupportedErrors:
         MAP_ERROR(GATT_UNKNOWN_ERROR, NotSupportedError, "GATT Error
Unknown.");

[François Beaufort, 2016-10-05 13:00:51]

The CQ bit was checked by beaufort.francois@gmail.com

[François Beaufort, 2016-10-05 13:00:51]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
isherman@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2384463002/#ps200001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-05 13:01:05]

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, 2016-10-05 14:35:59]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2016-10-05 14:38:27]

Description was changed from

==========
bluetooth: mac: Reject requestDevice promise if Mac version is <= 10.9

BUG=651350
==========

to

==========
bluetooth: mac: Reject requestDevice promise if Mac version is <= 10.9

BUG=651350

Committed: https://crrev.com/30846487078f8a61467d1b39b12e41cfe3bed524
Cr-Commit-Position: refs/heads/master@{#423155}
==========

[commit-bot: I haz the power, 2016-10-05 14:38:28]

Patchset 6 (id:??) landed as
https://crrev.com/30846487078f8a61467d1b39b12e41cfe3bed524
Cr-Commit-Position: refs/heads/master@{#423155}