Suppress WebUSB notifications when appropriate

Suppress WebUSB notifications when appropriate

Currently, a WebUSB notification appears regardless of whether the
landing page is already open.  This change takes a more refined
approach to WebUSB notifications, as outlined below.

Managing the notification:
- If the active tab is the landing page, don't fire a notification.
- If the the landing page becomes featured in the active tab and a
  notification exists, remove it.

When clicking a notification:
- If a tab exists with the landing page, activate it.
- If a tab does not exist with the landing page, create a new tab and
  navigate to the landing page.

BUG=689128
Review-Url: https://codereview.chromium.org/2824923002
Cr-Commit-Position: refs/heads/master@{#473273}
Committed: https://chromium.googlesource.com/chromium/src/+/dca47e28ab2e0f43b8b44874b3905676a3d961ce

[cco3, 2017-04-17 23:39:07]

cco3@chromium.org changed reviewers:
+ reillyg@chromium.org

[cco3, 2017-04-17 23:39:08]

I have an issue with my checkout, so I haven't been able to test this yet.

[cco3, 2017-04-17 23:39:20]

Description was changed from

==========
Suppress WebUSB notifications when appropriate

Currently, a WebUSB notification appears regardless of whether the
landing page is already open.  This change takes a more refined
approach to WebUSB notifications, as defined below.

Managing the notification:
- If the active tab is the landing page, don't fire a notification.
- If the active tab navigates to the landing page and a notification
  exists, remove it.

When clicking a notification
- If a tab exists with the landing page, activate it.
- If a tab does not exist with the landing page, create a new tab and
  navigate to the landing page.

BUG=689128
==========

to

==========
Suppress WebUSB notifications when appropriate

Currently, a WebUSB notification appears regardless of whether the
landing page is already open.  This change takes a more refined
approach to WebUSB notifications, as defined below.

Managing the notification:
- If the active tab is the landing page, don't fire a notification.
- If the active tab navigates to the landing page and a notification
  exists, remove it.

When clicking a notification:
- If a tab exists with the landing page, activate it.
- If a tab does not exist with the landing page, create a new tab and
  navigate to the landing page.

BUG=689128
==========

[cco3, 2017-04-17 23:40:56]

Description was changed from

==========
Suppress WebUSB notifications when appropriate

Currently, a WebUSB notification appears regardless of whether the
landing page is already open.  This change takes a more refined
approach to WebUSB notifications, as defined below.

Managing the notification:
- If the active tab is the landing page, don't fire a notification.
- If the active tab navigates to the landing page and a notification
  exists, remove it.

When clicking a notification:
- If a tab exists with the landing page, activate it.
- If a tab does not exist with the landing page, create a new tab and
  navigate to the landing page.

BUG=689128
==========

to

==========
Suppress WebUSB notifications when appropriate

Currently, a WebUSB notification appears regardless of whether the
landing page is already open.  This change takes a more refined
approach to WebUSB notifications, as outlined below.

Managing the notification:
- If the active tab is the landing page, don't fire a notification.
- If the active tab navigates to the landing page and a notification
  exists, remove it.

When clicking a notification:
- If a tab exists with the landing page, activate it.
- If a tab does not exist with the landing page, create a new tab and
  navigate to the landing page.

BUG=689128
==========

[Reilly Grant (use Gerrit), 2017-04-18 00:44:02]

For the second case I believe you mean,

"If the user switches to a tab displaying the landing page and a notification
exists, remove it."

[cco3, 2017-04-18 16:32:09]

Description was changed from

==========
Suppress WebUSB notifications when appropriate

Currently, a WebUSB notification appears regardless of whether the
landing page is already open.  This change takes a more refined
approach to WebUSB notifications, as outlined below.

Managing the notification:
- If the active tab is the landing page, don't fire a notification.
- If the active tab navigates to the landing page and a notification
  exists, remove it.

When clicking a notification:
- If a tab exists with the landing page, activate it.
- If a tab does not exist with the landing page, create a new tab and
  navigate to the landing page.

BUG=689128
==========

to

==========
Suppress WebUSB notifications when appropriate

Currently, a WebUSB notification appears regardless of whether the
landing page is already open.  This change takes a more refined
approach to WebUSB notifications, as outlined below.

Managing the notification:
- If the active tab is the landing page, don't fire a notification.
- If the the landing page becomes featured in the active tab and a
  notification exists, remove it.

When clicking a notification:
- If a tab exists with the landing page, activate it.
- If a tab does not exist with the landing page, create a new tab and
  navigate to the landing page.

BUG=689128
==========

[cco3, 2017-04-18 16:32:50]

On 2017/04/18 00:44:02, Reilly Grant wrote:
> For the second case I believe you mean,
> 
> "If the user switches to a tab displaying the landing page and a notification
> exists, remove it."

Hm, I mean both...I'll try to clarify.

[cco3, 2017-04-18 17:17:37]

OK, I got my tree fixed, but now I get a segfault from master (without my
change) when I plug in the device at
device::UsbDeviceHandleUsbfs::FileThreadHelper::Start().  I'm assuming this
isn't expected.

[Reilly Grant (use Gerrit), 2017-04-18 18:23:47]

On 2017/04/18 17:17:37, cco3 wrote:
> OK, I got my tree fixed, but now I get a segfault from master (without my
> change) when I plug in the device at
> device::UsbDeviceHandleUsbfs::FileThreadHelper::Start().  I'm assuming this
> isn't expected.

That crash should have been fixed by
https://codereview.chromium.org/2797433005/. Have you synced recently?

[cco3, 2017-04-19 01:26:09]

OK, I've played around with it and everything works as expected.  I'll try to
write some tests.

[cco3, 2017-04-26 22:40:22]

OK Reilly, I'm ready for you to take another look at this.

[Reilly Grant (use Gerrit), 2017-04-27 20:25:43]

lgtm with nits

Thanks for doing this. It turned out to be rather complex but you've built a
really easy-to-understand solution to the problem.

https://codereview.chromium.org/2824923002/diff/80001/chrome/browser/usb/web_...
File chrome/browser/usb/web_usb_detector.cc (right):

https://codereview.chromium.org/2824923002/diff/80001/chrome/browser/usb/web_...
chrome/browser/usb/web_usb_detector.cc:68:
WEBUSB_NOTIFICATION_CLOSED_MANUAL_NAVIGATION,
Update tools/metrics/histograms/histograms.xml to include this new value.

https://codereview.chromium.org/2824923002/diff/80001/chrome/browser/usb/web_...
chrome/browser/usb/web_usb_detector.cc:90: }
nit: No braces around single-line ifs.

https://codereview.chromium.org/2824923002/diff/80001/chrome/browser/usb/web_...
chrome/browser/usb/web_usb_detector.cc:97: }
nit: No braces around single-line ifs.

https://codereview.chromium.org/2824923002/diff/80001/chrome/browser/usb/web_...
chrome/browser/usb/web_usb_detector.cc:129: }
nit: No braces around single-line ifs.

https://codereview.chromium.org/2824923002/diff/80001/chrome/browser/usb/web_...
chrome/browser/usb/web_usb_detector.cc:164: }
nit: No braces around single-line ifs.

https://codereview.chromium.org/2824923002/diff/80001/chrome/browser/usb/web_...
chrome/browser/usb/web_usb_detector.cc:218: }
nit: No braces around single-line ifs.

https://codereview.chromium.org/2824923002/diff/80001/chrome/browser/usb/web_...
File chrome/browser/usb/web_usb_detector_unittest.cc (right):

https://codereview.chromium.org/2824923002/diff/80001/chrome/browser/usb/web_...
chrome/browser/usb/web_usb_detector_unittest.cc:441: ASSERT_EQ(nullptr,
message_center_->FindVisibleNotificationById(guid_1));
Can you use ASSERT_FALSE(...) and ASSERT_TRUE(...) instead of ASSERT_EQ(nullptr,
...) and ASSERT_NE(nullptr, ...) because it's a bit more readable?

[cco3, 2017-04-27 21:31:16]

https://codereview.chromium.org/2824923002/diff/80001/chrome/browser/usb/web_...
File chrome/browser/usb/web_usb_detector.cc (right):

https://codereview.chromium.org/2824923002/diff/80001/chrome/browser/usb/web_...
chrome/browser/usb/web_usb_detector.cc:68:
WEBUSB_NOTIFICATION_CLOSED_MANUAL_NAVIGATION,
On 2017/04/27 20:25:43, Reilly Grant wrote:
> Update tools/metrics/histograms/histograms.xml to include this new value.

Done. Ah, forgot to do this.

https://codereview.chromium.org/2824923002/diff/80001/chrome/browser/usb/web_...
chrome/browser/usb/web_usb_detector.cc:90: }
On 2017/04/27 20:25:43, Reilly Grant wrote:
> nit: No braces around single-line ifs.

Done.

https://codereview.chromium.org/2824923002/diff/80001/chrome/browser/usb/web_...
chrome/browser/usb/web_usb_detector.cc:97: }
On 2017/04/27 20:25:43, Reilly Grant wrote:
> nit: No braces around single-line ifs.

Done.

https://codereview.chromium.org/2824923002/diff/80001/chrome/browser/usb/web_...
chrome/browser/usb/web_usb_detector.cc:164: }
On 2017/04/27 20:25:43, Reilly Grant wrote:
> nit: No braces around single-line ifs.

Done.

https://codereview.chromium.org/2824923002/diff/80001/chrome/browser/usb/web_...
chrome/browser/usb/web_usb_detector.cc:218: }
On 2017/04/27 20:25:43, Reilly Grant wrote:
> nit: No braces around single-line ifs.

Done.

https://codereview.chromium.org/2824923002/diff/80001/chrome/browser/usb/web_...
File chrome/browser/usb/web_usb_detector_unittest.cc (right):

https://codereview.chromium.org/2824923002/diff/80001/chrome/browser/usb/web_...
chrome/browser/usb/web_usb_detector_unittest.cc:441: ASSERT_EQ(nullptr,
message_center_->FindVisibleNotificationById(guid_1));
On 2017/04/27 20:25:43, Reilly Grant wrote:
> Can you use ASSERT_FALSE(...) and ASSERT_TRUE(...) instead of
ASSERT_EQ(nullptr,
> ...) and ASSERT_NE(nullptr, ...) because it's a bit more readable?

Done.

[cco3, 2017-04-27 21:34:12]

cco3@chromium.org changed reviewers:
+ mpearson@chromium.org

[cco3, 2017-04-27 21:34:13]

Hi Mark, could you take a look at our histograms file?  Thanks!

[Mark P, 2017-04-27 23:17:50]

histograms.xml lgtm modulo one comment

--mark

https://codereview.chromium.org/2824923002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2824923002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:118271: +  <int value="3" label="Manual
navigation"/>
nit: ... to the landing page
(otherwise it could be a lot more general)

[cco3, 2017-04-28 02:39:22]

https://codereview.chromium.org/2824923002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2824923002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:118271: +  <int value="3" label="Manual
navigation"/>
On 2017/04/27 23:17:50, Mark P wrote:
> nit: ... to the landing page
> (otherwise it could be a lot more general)

Done.

[cco3, 2017-04-28 16:44:20]

The CQ bit was checked by cco3@chromium.org

[cco3, 2017-04-28 16:44:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
reillyg@chromium.org, mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2824923002/#ps140001
(title: "Update metrics description")

[commit-bot: I haz the power, 2017-04-28 16:44:44]

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-28 18:26:54]

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

[commit-bot: I haz the power, 2017-04-28 18:26:55]

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

[cco3, 2017-04-28 18:43:38]

On 2017/04/28 18:26:55, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

MessageCenter isn't initializing?  Any idea what could be going on here Reilly?

[cco3, 2017-05-02 00:19:56]

cco3@chromium.org changed reviewers:
+ nkostylev@chromium.org

[cco3, 2017-05-02 00:19:57]

Hi Nikita, would you be able to take a look at the chromeos/profiles/ files?

[cco3, 2017-05-19 16:56:46]

cco3@chromium.org changed reviewers:
+ xiyuan@chromium.org

[cco3, 2017-05-19 16:56:47]

Hi xiyuan@, would you be able to look at the files in
	chrome/browser/chromeos/profiles/?

[xiyuan, 2017-05-19 17:45:49]

c/b/chromeos/profiles lgtm

[cco3, 2017-05-19 18:10:28]

Thanks!

[cco3, 2017-05-19 18:10:34]

The CQ bit was checked by cco3@chromium.org

[cco3, 2017-05-19 18:10:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
reillyg@chromium.org, mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2824923002/#ps160001
(title: "Get cros tests working")

[commit-bot: I haz the power, 2017-05-19 18:11:24]

CQ is trying da patch.

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

[cco3, 2017-05-19 18:14:03]

The CQ bit was unchecked by cco3@chromium.org

[cco3, 2017-05-19 18:25:12]

The CQ bit was checked by cco3@chromium.org

[cco3, 2017-05-19 18:25:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
reillyg@chromium.org, mpearson@chromium.org, xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2824923002/#ps180001
(title: "Use new enums.xml file")

[commit-bot: I haz the power, 2017-05-19 18:25: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, 2017-05-19 19:23:05]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495218312064490,
"parent_rev": "c62bebdff1b9d9f8aa6d5e5339f14586e7ec1b04", "commit_rev":
"dca47e28ab2e0f43b8b44874b3905676a3d961ce"}

[commit-bot: I haz the power, 2017-05-19 19:23:20]

Description was changed from

==========
Suppress WebUSB notifications when appropriate

Currently, a WebUSB notification appears regardless of whether the
landing page is already open.  This change takes a more refined
approach to WebUSB notifications, as outlined below.

Managing the notification:
- If the active tab is the landing page, don't fire a notification.
- If the the landing page becomes featured in the active tab and a
  notification exists, remove it.

When clicking a notification:
- If a tab exists with the landing page, activate it.
- If a tab does not exist with the landing page, create a new tab and
  navigate to the landing page.

BUG=689128
==========

to

==========
Suppress WebUSB notifications when appropriate

Currently, a WebUSB notification appears regardless of whether the
landing page is already open.  This change takes a more refined
approach to WebUSB notifications, as outlined below.

Managing the notification:
- If the active tab is the landing page, don't fire a notification.
- If the the landing page becomes featured in the active tab and a
  notification exists, remove it.

When clicking a notification:
- If a tab exists with the landing page, activate it.
- If a tab does not exist with the landing page, create a new tab and
  navigate to the landing page.

BUG=689128

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

[commit-bot: I haz the power, 2017-05-19 19:23:22]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/dca47e28ab2e0f43b8b44874b390...