bluetooth: Show tab indicator when BT device is connected

bluetooth: Show tab indicator when BT device is connected

1. Adds a setter and getter to WebContents to get/set whether a tab is connected to
a BluetoothDevice. The setter will notify that the tab changed.
2. Sets the Device connected state in BluetoothDispatcherHost whenever a tab connects
to a device.
3. Gets the Bluetooth Device connected state in TabUtils.
4. Adds resources for the Bluetooth indicator.

The .icon resource was generated by passing the svg of the issue attached through https://rawgit.com/evanstade/skiafy/gh-pages/index.html

BUG=469019
Committed: https://crrev.com/32e7db3c7cf14dbeaf5f14d683fae7fe38bb6201
Cr-Commit-Position: refs/heads/master@{#383736}

[ortuno, 2016-02-29 23:10:10]

Description was changed from

==========
bluetooth: Show tab indicator when BT device is connected

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

to

==========
bluetooth: Show tab indicator when BT device is connected

1. Adds a setter and getter to WebContents to get/set whether a tab is connected
to
a BluetoothDevice. The setter will notify that the tab changed.
2. Sets the Device connected state in BluetoothDispatcherHost whenever a tab
connects
to a device.
3. Gets the Bluetooth Device connected state in TabUtils.
4. Adds resources for the Bluetooth indicator.

The .icon resource was generated by passing the svg of the issue attached
through https://rawgit.com/evanstade/skiafy/gh-pages/index.html

BUG=469019
==========

[ortuno, 2016-02-29 23:35:23]

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

[ortuno, 2016-02-29 23:35:24]

Hi Adrienne,

This is the patch that implements the Tab Indicator for Web Bluetooth. This
patch will add BLUETOOTH_CONNECTED to the MediaState related enums. Jeffrey
Yasskin mentioned in the past that
by adding Bluetooth to the list of possible indicators, it no longer makes sense
for these
enums and related functions to include "Media" in their name and mentioned I
should contact you
for name suggestions. I thought about just removing "Media" from the names but
it leaves some
functions with very vague names:

TabMediaState -> TabState
ShouldTabShowMediaIndicator -> ShouldTabShowIndicator
GetTabMediaStateForContents -> GetTabStateForContents
GetTabMediaInidicatorImage -> GetTabInidicatorImage
GetTabMediaIndicatorAffordanceImage -> GetTabInidicatorAffordanceImage
CreateTabMediaIndicatorFadeAnimation -> CreateTabIndicatorFadeAnimation

Do you have any suggestions as to what to replace "Media" with?

Thanks,

Gio

[fbeaufortchromium, 2016-03-02 09:22:40]

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

[fbeaufortchromium, 2016-03-02 09:22:40]

https://codereview.chromium.org/1746333002/diff/1/chrome/app/theme/theme_reso...
File chrome/app/theme/theme_resources.grd (right):

https://codereview.chromium.org/1746333002/diff/1/chrome/app/theme/theme_reso...
chrome/app/theme/theme_resources.grd:819: <structure type="chrome_scaled_image"
name="IDR_TAB_BLUETOOTH_INDICATOR" file="legacy/bluetooth.png" />
May I suggest to rename bluetooth.png file name to tab_bluetooth_indicator.png?

[miu, 2016-03-02 19:21:09]

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

[miu, 2016-03-02 19:21:10]

Adding myself as a reviewer (as owner of the tab media indicator).

ortuno: Could you explain the user experience story here?  What does it mean
when a tab connects via Bluetooth?  Is it remoting content?  Should a tab
automatically unmute when using BT, just like it does for tab capture?

In the code, I see you have the Bluetooth indicator taking priority over the
others.  Can you justify why that is?

Also, have you discussed this with chrome-ui-review@?  Changes to the tabstrip
UI need approval for launch.  Otherwise, you'll need to enable this only via a
command-line flag.

[miu, 2016-03-02 19:23:45]

On 2016/03/02 19:21:10, miu wrote:
> Adding myself as a reviewer (as owner of the tab media indicator).
> 
> ortuno: Could you explain the user experience story here?  What does it mean
> when a tab connects via Bluetooth?  Is it remoting content?  Should a tab
> automatically unmute when using BT, just like it does for tab capture?
> 
> In the code, I see you have the Bluetooth indicator taking priority over the
> others.  Can you justify why that is?
> 
> Also, have you discussed this with chrome-ui-review@?  Changes to the tabstrip
> UI need approval for launch.  Otherwise, you'll need to enable this only via a
> command-line flag.

OIC, some of my questions are answered in the crbug.  But, did everyone come to
a consensus about the priority order?

[miu, 2016-03-02 19:25:15]

On 2016/02/29 23:35:24, ortuno wrote:
> Hi Adrienne,
> 
> This is the patch that implements the Tab Indicator for Web Bluetooth. This
> patch will add BLUETOOTH_CONNECTED to the MediaState related enums. Jeffrey
> Yasskin mentioned in the past that
> by adding Bluetooth to the list of possible indicators, it no longer makes
sense
> for these
> enums and related functions to include "Media" in their name and mentioned I
> should contact you
> for name suggestions. I thought about just removing "Media" from the names but
> it leaves some
> functions with very vague names:
> 
> TabMediaState -> TabState
> ShouldTabShowMediaIndicator -> ShouldTabShowIndicator
> GetTabMediaStateForContents -> GetTabStateForContents
> GetTabMediaInidicatorImage -> GetTabInidicatorImage
> GetTabMediaIndicatorAffordanceImage -> GetTabInidicatorAffordanceImage
> CreateTabMediaIndicatorFadeAnimation -> CreateTabIndicatorFadeAnimation
> 
> Do you have any suggestions as to what to replace "Media" with?
> 
> Thanks,
> 
> Gio

BTW, personally I don't see a reason to remove "media" from the enums or
function names.  Isn't Bluetooth a media transport mechansim, after all?  ;)

[felt, 2016-03-02 19:28:43]

hey fyi i'm out sick but will be back tomorrow (i hope). forgot to put up an
auto response.

[ortuno, 2016-03-02 20:25:35]

@miu: Thanks for looking that the patch. Let me know if there are any questions
not answered in the crbug linked. Just to be clear, this is not about classic BT
which is usually used for headphones and such. This is for Bluetooth Low Energy
devices which range from heart rate monitors to toys to hardware dev kits; I
don't think all of these fall under the "media" category.

Re: priority, I went with @felt's proposal on the issue and I think it's a good
way to start the conversation, feel free to suggest any other priority order.

Re: naming, as other APIs like Bluetooth start to be implemented, the name
"MediaIndicator" will become less and less accurate. Web USB and Web Serial will
probably use the tab indicator. I'm OK with leaving this as is, but we will need
to have a discussion about this soon.

@felt: No problem. Hope you feel better soon!

[miu, 2016-03-04 23:42:37]

lgtm.  A nice, clean implementation.  :)

https://codereview.chromium.org/1746333002/diff/1/content/browser/bluetooth/b...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1746333002/diff/1/content/browser/bluetooth/b...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:729: if
(render_frame_host && web_contents) {
nit: Not necessary to test render_frame_host, since web_contents will be null if
render_frame_host was null when the WC was looked-up.

https://codereview.chromium.org/1746333002/diff/1/content/browser/bluetooth/b...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1463: if
(render_frame_host && web_contents) {
nit: ditto--no need for render_frame_host

[ortuno, 2016-03-14 20:52:50]

https://codereview.chromium.org/1746333002/diff/1/chrome/app/theme/theme_reso...
File chrome/app/theme/theme_resources.grd (right):

https://codereview.chromium.org/1746333002/diff/1/chrome/app/theme/theme_reso...
chrome/app/theme/theme_resources.grd:819: <structure type="chrome_scaled_image"
name="IDR_TAB_BLUETOOTH_INDICATOR" file="legacy/bluetooth.png" />
On 2016/03/02 at 09:22:40, fbeaufortchromium wrote:
> May I suggest to rename bluetooth.png file name to
tab_bluetooth_indicator.png?

Done.

https://codereview.chromium.org/1746333002/diff/1/content/browser/bluetooth/b...
File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right):

https://codereview.chromium.org/1746333002/diff/1/content/browser/bluetooth/b...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:729: if
(render_frame_host && web_contents) {
On 2016/03/04 at 23:42:37, miu_NO_REVIEWS_UNTIL_MAR_21 wrote:
> nit: Not necessary to test render_frame_host, since web_contents will be null
if render_frame_host was null when the WC was looked-up.

Done.

https://codereview.chromium.org/1746333002/diff/1/content/browser/bluetooth/b...
content/browser/bluetooth/bluetooth_dispatcher_host.cc:1463: if
(render_frame_host && web_contents) {
On 2016/03/04 at 23:42:37, miu_NO_REVIEWS_UNTIL_MAR_21 wrote:
> nit: ditto--no need for render_frame_host

Done.

[ortuno, 2016-03-14 20:53:50]

@miu: Do you have any thoughts on the priority?
@felt: Do you think the rename is necessary?

[miu, 2016-03-14 21:13:21]

On 2016/03/14 20:53:50, ortuno wrote:
> @miu: Do you have any thoughts on the priority?

Seems good, as you have it in PS2.  I agree that tab capture and
webcam/microphone recording have higher precedence, and audio mute/playback have
lower precedence.  We can always adjust later.  If there are other indicators
added in the future, though, we may need to completely re-think this UI.

[felt, 2016-03-23 21:25:42]

sorry about the delay, but lgtm % naming

perhaps something like Permission instead of MediaState?

[ortuno, 2016-03-25 22:29:29]

Thanks. Since the refactoring is quite big I made a follow up patch:
http://crrev.com/1827083004 PTAL. Since this is blocking other web bluetooth
work I'll land this and continue work on the refactoring in the follow up patch.

[ortuno, 2016-03-28 20:57:52]

ortuno@chromium.org changed reviewers:
+ estade@chromium.org, nick@chromium.org, oshima@chromium.org

[ortuno, 2016-03-28 20:57:52]

oshima@: Please review changes in chrome/app/theme

nick@: Please review changes in web_contents.*

estade@: Please review changes in ui/gfx

[ncarter (slow), 2016-03-28 21:43:19]

lgtm after one fix

https://codereview.chromium.org/1746333002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1746333002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:281: void
SetBluetoothDeviceConnected(bool mute) override;
"bool mute" -> "bool connected"

[Evan Stade, 2016-03-28 21:49:22]

vector icon stuff lgtm

[ortuno, 2016-03-28 23:24:37]

Thanks!

https://codereview.chromium.org/1746333002/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1746333002/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:281: void
SetBluetoothDeviceConnected(bool mute) override;
On 2016/03/28 at 21:43:19, ncarter wrote:
> "bool mute" -> "bool connected"

Done.

[oshima, 2016-03-29 13:32:19]

c/a/theme lgtm

[ortuno, 2016-03-29 15:04:43]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-03-29 15:04:44]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org,
felt@chromium.org, nick@chromium.org, estade@chromium.org
Link to the patchset: https://codereview.chromium.org/1746333002/#ps80001
(title: "mute -> connected")

[commit-bot: I haz the power, 2016-03-29 15:04:59]

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

[commit-bot: I haz the power, 2016-03-29 16:14:32]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-29 16:16:04]

Description was changed from

==========
bluetooth: Show tab indicator when BT device is connected

1. Adds a setter and getter to WebContents to get/set whether a tab is connected
to
a BluetoothDevice. The setter will notify that the tab changed.
2. Sets the Device connected state in BluetoothDispatcherHost whenever a tab
connects
to a device.
3. Gets the Bluetooth Device connected state in TabUtils.
4. Adds resources for the Bluetooth indicator.

The .icon resource was generated by passing the svg of the issue attached
through https://rawgit.com/evanstade/skiafy/gh-pages/index.html

BUG=469019
==========

to

==========
bluetooth: Show tab indicator when BT device is connected

1. Adds a setter and getter to WebContents to get/set whether a tab is connected
to
a BluetoothDevice. The setter will notify that the tab changed.
2. Sets the Device connected state in BluetoothDispatcherHost whenever a tab
connects
to a device.
3. Gets the Bluetooth Device connected state in TabUtils.
4. Adds resources for the Bluetooth indicator.

The .icon resource was generated by passing the svg of the issue attached
through https://rawgit.com/evanstade/skiafy/gh-pages/index.html

BUG=469019

Committed: https://crrev.com/32e7db3c7cf14dbeaf5f14d683fae7fe38bb6201
Cr-Commit-Position: refs/heads/master@{#383736}
==========

[commit-bot: I haz the power, 2016-03-29 16:16:05]

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