New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 738225 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Mobile data settings opens every time i click *any* notification.

Project Member Reported by jlklein@chromium.org, Jun 29 2017

Issue description

After enabling instant tethering, clicking any notification opens mobile data settings :-p...

 
Cc: jonmann@chromium.org jlklein@chromium.org jhawkins@chromium.org hansberry@chromium.org lesliewatkins@chromium.org
Status: Started (was: Assigned)
Labels: -Pri-3 M-61 Pri-1
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1dc1a252722fb977415f27df87e6fb8cfd0cf768

commit 1dc1a252722fb977415f27df87e6fb8cfd0cf768
Author: Kyle Horimoto <khorimoto@google.com>
Date: Wed Jul 19 01:56:35 2017

[CrOS Tether] Fix issue which caused the mobile data settings to be opened on
any notification click.

The issue was that we opened settings on any OnNotificationClicked() callback.
However, this callback is invoked for *any* notification click, even when
notifications from another app/feature are clicked. To fix this issue, we now
explicitly check to make sure that the ID of the clicked notification matches
one of the IDs we care about.

Bug: 672263,  738225 
Change-Id: I02a187dae03e8a553680302b4154aa33cafd7431
Reviewed-on: https://chromium-review.googlesource.com/571856
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487717}
[modify] https://crrev.com/1dc1a252722fb977415f27df87e6fb8cfd0cf768/chrome/browser/chromeos/net/tether_notification_presenter.cc
[modify] https://crrev.com/1dc1a252722fb977415f27df87e6fb8cfd0cf768/chrome/browser/chromeos/net/tether_notification_presenter.h
[modify] https://crrev.com/1dc1a252722fb977415f27df87e6fb8cfd0cf768/chrome/browser/chromeos/net/tether_notification_presenter_unittest.cc

Status: Fixed (was: Started)
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7660aa7c83daef48d20824bdd3b433c4767095b1

commit 7660aa7c83daef48d20824bdd3b433c4767095b1
Author: Alice Boxhall <aboxhall@chromium.org>
Date: Wed Jul 19 04:08:13 2017

Revert "[CrOS Tether] Fix issue which caused the mobile data settings to be opened on"

This reverts commit 1dc1a252722fb977415f27df87e6fb8cfd0cf768.

Reason for revert: Caused compile failure on ChromiumOS MSan https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20ChromiumOS%20MSan%20Builder/builds/1998

Original change's description:
> [CrOS Tether] Fix issue which caused the mobile data settings to be opened on
> any notification click.
> 
> The issue was that we opened settings on any OnNotificationClicked() callback.
> However, this callback is invoked for *any* notification click, even when
> notifications from another app/feature are clicked. To fix this issue, we now
> explicitly check to make sure that the ID of the clicked notification matches
> one of the IDs we care about.
> 
> Bug: 672263,  738225 
> Change-Id: I02a187dae03e8a553680302b4154aa33cafd7431
> Reviewed-on: https://chromium-review.googlesource.com/571856
> Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#487717}

TBR=stevenjb@chromium.org,khorimoto@chromium.org

Change-Id: I0cc12d643d1775cb2b44c976c4841a41e4d530d9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 672263,  738225 
Reviewed-on: https://chromium-review.googlesource.com/576013
Reviewed-by: Alice Boxhall <aboxhall@chromium.org>
Commit-Queue: Alice Boxhall <aboxhall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487742}
[modify] https://crrev.com/7660aa7c83daef48d20824bdd3b433c4767095b1/chrome/browser/chromeos/net/tether_notification_presenter.cc
[modify] https://crrev.com/7660aa7c83daef48d20824bdd3b433c4767095b1/chrome/browser/chromeos/net/tether_notification_presenter.h
[modify] https://crrev.com/7660aa7c83daef48d20824bdd3b433c4767095b1/chrome/browser/chromeos/net/tether_notification_presenter_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/65dd85fdeb9af037eda82907e737a3448748db23

commit 65dd85fdeb9af037eda82907e737a3448748db23
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Wed Jul 19 04:09:41 2017

Revert "[CrOS Tether] Fix issue which caused the mobile data settings to be opened on"

This reverts commit 1dc1a252722fb977415f27df87e6fb8cfd0cf768.

Reason for revert: This fails Linux ChromiumOS MSan Builder:
https://build.chromium.org/p/chromium.memory/builders/Linux%20ChromiumOS%20MSan%20Builder/builds/1998
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.memory%2FLinux_ChromiumOS_MSan_Builder%2F1998%2F%2B%2Frecipes%2Fsteps%2Fcompile%2F0%2Fstdout

Original change's description:
> [CrOS Tether] Fix issue which caused the mobile data settings to be opened on
> any notification click.
> 
> The issue was that we opened settings on any OnNotificationClicked() callback.
> However, this callback is invoked for *any* notification click, even when
> notifications from another app/feature are clicked. To fix this issue, we now
> explicitly check to make sure that the ID of the clicked notification matches
> one of the IDs we care about.
> 
> Bug: 672263,  738225 
> Change-Id: I02a187dae03e8a553680302b4154aa33cafd7431
> Reviewed-on: https://chromium-review.googlesource.com/571856
> Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#487717}

TBR=stevenjb@chromium.org,khorimoto@chromium.org

Change-Id: Ia20a3a665058f90a78c24ca8221cfeeda621d6b0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 672263,  738225 
Reviewed-on: https://chromium-review.googlesource.com/576014
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487743}

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d82fed18c6e30b22009d86ec5cb9030ff3ae10c6

commit d82fed18c6e30b22009d86ec5cb9030ff3ae10c6
Author: Kyle Horimoto <khorimoto@google.com>
Date: Wed Jul 19 18:26:02 2017

[CrOS Tether] Fix issue which caused the mobile data settings to be opened on
any notification click.

The issue was that we opened settings on any OnNotificationClicked() callback.
However, this callback is invoked for *any* notification click, even when
notifications from another app/feature are clicked. To fix this issue, we now
explicitly check to make sure that the ID of the clicked notification matches
one of the IDs we care about.

This CL was originally:
https://chromium-review.googlesource.com/c/571856/

But was reverted as part of:
https://chromium-review.googlesource.com/c/576013/

My original CL is the initial upload on this CL, and my changes to fix the
breakage are in the subsequent patch version.

Bug: 672263,  738225 
Change-Id: Ieae494d786f5fd3b50535d3d34f4a17ce30b0152
Reviewed-on: https://chromium-review.googlesource.com/577762
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487924}
[modify] https://crrev.com/d82fed18c6e30b22009d86ec5cb9030ff3ae10c6/chrome/browser/chromeos/net/tether_notification_presenter.cc
[modify] https://crrev.com/d82fed18c6e30b22009d86ec5cb9030ff3ae10c6/chrome/browser/chromeos/net/tether_notification_presenter.h
[modify] https://crrev.com/d82fed18c6e30b22009d86ec5cb9030ff3ae10c6/chrome/browser/chromeos/net/tether_notification_presenter_unittest.cc

Comment 8 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment