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

Issue 706658 link

Starred by 2 users
Status: Verified
Owner:
yhanada@chromium.org
Closed: May 2017
Cc:
█ yoshiki@chromium.org
peter@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Clicking settings button on Chrome notifications should open settings in message center

Project Member Reported by yoshiki@chromium.org, Mar 30 2017 
Currently, clicking settings button on Chrome (non-ARC) notifications open settings in chrome://settings. But the "settings" in the right click menu navigates the settings tab in the message center.

These are inconsistent. We should navigate to the settings tab in the message center by either way.

Step to repro:
- Open Chrome notification or HTML5 notification (not ARC notification)
- Move the mouse cursor on the notification
- Click the settings button on hover

Intended behavior:
- Open the settings tab in the message center

Actual behavior:
- Open the notification section in the settings window (chrome://settings)
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 7 2017 

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

commit f7d02d633ab7cb42632521709869212cdca3a835
Author: yhanada <yhanada@chromium.org>
Date: Fri Apr 07 09:12:38 2017

Open the settings tab in the message center when clicking the settings button on a notification.

Before this change, clicking the settings button on a notification
opens the notification section in the settings
window (chrome://settings)

BUG= 706658 

Review-Url: https://codereview.chromium.org/2799653004
Cr-Commit-Position: refs/heads/master@{#462815}

[modify] https://crrev.com/f7d02d633ab7cb42632521709869212cdca3a835/ui/message_center/views/message_center_view.cc
[modify] https://crrev.com/f7d02d633ab7cb42632521709869212cdca3a835/ui/message_center/views/message_popup_collection.cc

Comment 2 by peter@chromium.org, Apr 7 2017

Cc: peter@chromium.org
Hi! Sorry to randomly drop in, but I'm not very happy with the change in #1. It breaks the SettingsClick() delegate on all platforms except for Mac OS X. In turn, that means that the settings button doesn't do anything anymore on Windows and Linux, because they have no message center tray. Finally, we also stop counting the "Notifications.ShowSiteSettings" metric on all non-Mac platforms.

I've uploaded https://codereview.chromium.org/2807623002 with an alternative approach for your consideration.

Comment 3 by yhanada@chromium.org, Apr 10 2017 

I'm sorry for breaking the settings button on Windows and Linux. Thank you so much for making the fix!
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 10 2017 

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

commit 9b6e3c78898f12a0183edffb62d189cb716c6135
Author: peter <peter@chromium.org>
Date: Mon Apr 10 18:35:45 2017

Fix the settings button and UMA metrics for the message center

The settings button was unintentionally broken for Windows and
Linux users in 2799653004, as was gathering of some UMA. This
CL offers a slight refactoring that meets all goals.

BUG= 706658 

Review-Url: https://codereview.chromium.org/2807623002
Cr-Commit-Position: refs/heads/master@{#463339}

[modify] https://crrev.com/9b6e3c78898f12a0183edffb62d189cb716c6135/chrome/browser/notifications/message_center_stats_collector.cc
[modify] https://crrev.com/9b6e3c78898f12a0183edffb62d189cb716c6135/chrome/browser/notifications/message_center_stats_collector.h
[modify] https://crrev.com/9b6e3c78898f12a0183edffb62d189cb716c6135/chrome/browser/notifications/web_notification_delegate.cc
[modify] https://crrev.com/9b6e3c78898f12a0183edffb62d189cb716c6135/chrome/browser/notifications/web_notification_delegate.h
[modify] https://crrev.com/9b6e3c78898f12a0183edffb62d189cb716c6135/ui/message_center/message_center_impl.cc
[modify] https://crrev.com/9b6e3c78898f12a0183edffb62d189cb716c6135/ui/message_center/message_center_observer.h
[modify] https://crrev.com/9b6e3c78898f12a0183edffb62d189cb716c6135/ui/message_center/message_center_tray.cc
[modify] https://crrev.com/9b6e3c78898f12a0183edffb62d189cb716c6135/ui/message_center/message_center_tray.h
[modify] https://crrev.com/9b6e3c78898f12a0183edffb62d189cb716c6135/ui/message_center/notification_delegate.cc
[modify] https://crrev.com/9b6e3c78898f12a0183edffb62d189cb716c6135/ui/message_center/notification_delegate.h
[modify] https://crrev.com/9b6e3c78898f12a0183edffb62d189cb716c6135/ui/message_center/views/message_center_view.cc
[modify] https://crrev.com/9b6e3c78898f12a0183edffb62d189cb716c6135/ui/message_center/views/message_popup_collection.cc

Comment 5 by yhanada@chromium.org, Apr 13 2017

Labels: Merge-Request-58
Project Member

Comment 6 by sheriffbot@chromium.org, Apr 13 2017

Labels: -Merge-Request-58 Merge-Review-58 Hotlist-Merge-Review
This bug requires manual review: We are only 11 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 7 by bhthompson@google.com, Apr 13 2017

Labels: -Hotlist-Merge-Review -Merge-Review-58 Merge-Approved-58
If this has been validated on ToT, we can merge it, since this potentially touches stuff beyond Chrome OS we should be particularly careful here.

Comment 8 by yhanada@chromium.org, Apr 14 2017 

I built Linux Chromium ToT and confirmed it works fine.
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 14 2017

Labels: -merge-approved-58 merge-merged-3029
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/58b36a73de145bb4e1ae45f369dd43f7bbb2f252

commit 58b36a73de145bb4e1ae45f369dd43f7bbb2f252
Author: yhanada <yhanada@chromium.org>
Date: Fri Apr 14 04:23:54 2017

Open the settings tab in the message center when clicking the settings button on a notification.

Before this change, clicking the settings button on a notification
opens the notification section in the settings
window (chrome://settings)

BUG= 706658 

Review-Url: https://codereview.chromium.org/2799653004
Cr-Commit-Position: refs/heads/master@{#462815}
(cherry picked from commit f7d02d633ab7cb42632521709869212cdca3a835)

Review-Url: https://codereview.chromium.org/2820653002 .
Cr-Commit-Position: refs/branch-heads/3029@{#706}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/58b36a73de145bb4e1ae45f369dd43f7bbb2f252/ui/message_center/views/message_center_view.cc
[modify] https://crrev.com/58b36a73de145bb4e1ae45f369dd43f7bbb2f252/ui/message_center/views/message_popup_collection.cc
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 14 2017 

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

commit ebce3bd778c9770581cb11f2c90219fac48728f4
Author: yhanada <yhanada@chromium.org>
Date: Fri Apr 14 04:33:01 2017

Fix the settings button and UMA metrics for the message center

The settings button was unintentionally broken for Windows and
Linux users in 2799653004, as was gathering of some UMA. This
CL offers a slight refactoring that meets all goals.

BUG= 706658 

Review-Url: https://codereview.chromium.org/2807623002
Cr-Commit-Position: refs/heads/master@{#463339}
(cherry picked from commit 9b6e3c78898f12a0183edffb62d189cb716c6135)

Review-Url: https://codereview.chromium.org/2818063002 .
Cr-Commit-Position: refs/branch-heads/3029@{#707}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/ebce3bd778c9770581cb11f2c90219fac48728f4/chrome/browser/notifications/message_center_stats_collector.cc
[modify] https://crrev.com/ebce3bd778c9770581cb11f2c90219fac48728f4/chrome/browser/notifications/message_center_stats_collector.h
[modify] https://crrev.com/ebce3bd778c9770581cb11f2c90219fac48728f4/chrome/browser/notifications/web_notification_delegate.cc
[modify] https://crrev.com/ebce3bd778c9770581cb11f2c90219fac48728f4/chrome/browser/notifications/web_notification_delegate.h
[modify] https://crrev.com/ebce3bd778c9770581cb11f2c90219fac48728f4/ui/message_center/message_center_impl.cc
[modify] https://crrev.com/ebce3bd778c9770581cb11f2c90219fac48728f4/ui/message_center/message_center_observer.h
[modify] https://crrev.com/ebce3bd778c9770581cb11f2c90219fac48728f4/ui/message_center/message_center_tray.cc
[modify] https://crrev.com/ebce3bd778c9770581cb11f2c90219fac48728f4/ui/message_center/message_center_tray.h
[modify] https://crrev.com/ebce3bd778c9770581cb11f2c90219fac48728f4/ui/message_center/notification_delegate.cc
[modify] https://crrev.com/ebce3bd778c9770581cb11f2c90219fac48728f4/ui/message_center/notification_delegate.h
[modify] https://crrev.com/ebce3bd778c9770581cb11f2c90219fac48728f4/ui/message_center/views/message_center_view.cc
[modify] https://crrev.com/ebce3bd778c9770581cb11f2c90219fac48728f4/ui/message_center/views/message_popup_collection.cc

Comment 11 by yhanada@chromium.org, Apr 14 2017

Status: Fixed (was: Assigned)
Project Member

Comment 12 by bugdroid1@chromium.org, Apr 14 2017 

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

commit 7cf6a4e07de9deee6657a54423cf3e2e1aca9338
Author: alexmos <alexmos@chromium.org>
Date: Fri Apr 14 20:55:44 2017

Revert of Fix the settings button and UMA metrics for the message center (patchset #1 id:1 of https://codereview.chromium.org/2818063002/ )

Reason for revert:
Breaks compile on 'win Beta' for official.desktop.continuous, see https://crbug.com/711712

Original issue's description:
> Fix the settings button and UMA metrics for the message center
>
> The settings button was unintentionally broken for Windows and
> Linux users in 2799653004, as was gathering of some UMA. This
> CL offers a slight refactoring that meets all goals.
>
> BUG= 706658 
>
> Review-Url: https://codereview.chromium.org/2807623002
> Cr-Commit-Position: refs/heads/master@{#463339}
> (cherry picked from commit 9b6e3c78898f12a0183edffb62d189cb716c6135)
>
> Review-Url: https://codereview.chromium.org/2818063002 .
> Cr-Commit-Position: refs/branch-heads/3029@{#707}
> Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}
> Committed: https://chromium.googlesource.com/chromium/src/+/ebce3bd778c9770581cb11f2c90219fac48728f4

TBR=yhanada@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 706658 

Review-Url: https://codereview.chromium.org/2823673002
Cr-Commit-Position: refs/branch-heads/3029@{#718}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/7cf6a4e07de9deee6657a54423cf3e2e1aca9338/chrome/browser/notifications/message_center_stats_collector.cc
[modify] https://crrev.com/7cf6a4e07de9deee6657a54423cf3e2e1aca9338/chrome/browser/notifications/message_center_stats_collector.h
[modify] https://crrev.com/7cf6a4e07de9deee6657a54423cf3e2e1aca9338/chrome/browser/notifications/web_notification_delegate.cc
[modify] https://crrev.com/7cf6a4e07de9deee6657a54423cf3e2e1aca9338/chrome/browser/notifications/web_notification_delegate.h
[modify] https://crrev.com/7cf6a4e07de9deee6657a54423cf3e2e1aca9338/ui/message_center/message_center_impl.cc
[modify] https://crrev.com/7cf6a4e07de9deee6657a54423cf3e2e1aca9338/ui/message_center/message_center_observer.h
[modify] https://crrev.com/7cf6a4e07de9deee6657a54423cf3e2e1aca9338/ui/message_center/message_center_tray.cc
[modify] https://crrev.com/7cf6a4e07de9deee6657a54423cf3e2e1aca9338/ui/message_center/message_center_tray.h
[modify] https://crrev.com/7cf6a4e07de9deee6657a54423cf3e2e1aca9338/ui/message_center/notification_delegate.cc
[modify] https://crrev.com/7cf6a4e07de9deee6657a54423cf3e2e1aca9338/ui/message_center/notification_delegate.h
[modify] https://crrev.com/7cf6a4e07de9deee6657a54423cf3e2e1aca9338/ui/message_center/views/message_center_view.cc
[modify] https://crrev.com/7cf6a4e07de9deee6657a54423cf3e2e1aca9338/ui/message_center/views/message_popup_collection.cc
Project Member

Comment 13 by bugdroid1@chromium.org, Apr 17 2017 

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

commit 47650b27b22ddf35a1abed716c82c4d8248ce1f8
Author: yhanada <yhanada@chromium.org>
Date: Mon Apr 17 02:20:48 2017

Fix the settings button and UMA metrics for the message center

The settings button was unintentionally broken for Windows and
Linux users in 2799653004, as was gathering of some UMA. This
CL offers a slight refactoring that meets all goals.

BUG= 706658 

Review-Url: https://codereview.chromium.org/2807623002
Cr-Commit-Position: refs/heads/master@{#463339}
(cherry picked from commit 9b6e3c78898f12a0183edffb62d189cb716c6135)

Review-Url: https://codereview.chromium.org/2822933002 .
Cr-Commit-Position: refs/branch-heads/3029@{#729}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/47650b27b22ddf35a1abed716c82c4d8248ce1f8/chrome/browser/notifications/message_center_stats_collector.cc
[modify] https://crrev.com/47650b27b22ddf35a1abed716c82c4d8248ce1f8/chrome/browser/notifications/message_center_stats_collector.h
[modify] https://crrev.com/47650b27b22ddf35a1abed716c82c4d8248ce1f8/chrome/browser/notifications/web_notification_delegate.cc
[modify] https://crrev.com/47650b27b22ddf35a1abed716c82c4d8248ce1f8/chrome/browser/notifications/web_notification_delegate.h
[modify] https://crrev.com/47650b27b22ddf35a1abed716c82c4d8248ce1f8/ui/message_center/message_center_impl.cc
[modify] https://crrev.com/47650b27b22ddf35a1abed716c82c4d8248ce1f8/ui/message_center/message_center_observer.h
[modify] https://crrev.com/47650b27b22ddf35a1abed716c82c4d8248ce1f8/ui/message_center/message_center_tray.cc
[modify] https://crrev.com/47650b27b22ddf35a1abed716c82c4d8248ce1f8/ui/message_center/message_center_tray.h
[modify] https://crrev.com/47650b27b22ddf35a1abed716c82c4d8248ce1f8/ui/message_center/notification_delegate.cc
[modify] https://crrev.com/47650b27b22ddf35a1abed716c82c4d8248ce1f8/ui/message_center/notification_delegate.h
[modify] https://crrev.com/47650b27b22ddf35a1abed716c82c4d8248ce1f8/ui/message_center/views/message_center_view.cc
[modify] https://crrev.com/47650b27b22ddf35a1abed716c82c4d8248ce1f8/ui/message_center/views/message_popup_collection.cc

Comment 14 by sdantul...@chromium.org, May 11 2017

Labels: Needs-Feedback
Status: Assigned (was: Fixed)
Hi, is this complete and ready for verification?

I am on TOT ChromeOS 9534.0.0, 60.0.3092.0 and I don't see any settings button on non-ARC notifications. I tested on screenshot and image download notifications.

Comment 15 by abod...@chromium.org, May 12 2017 

 Yes,I don't see any settings button on non-ARC notifications on M58 too.
Google Chrome	58.0.3029.112 (Official Build) (64-bit)
Platform	9334.69.0 (Official Build) stable-channel
Screenshot 2017-05-11 at 16.59.26.png
183 KB View Download

Comment 16 by yhanada@chromium.org, May 12 2017 

Settings button on non-ARC notifications shows only for notifications from Web page. Can you try "Notify me!" button in https://developer.mozilla.org/en-US/docs/Web/API/Notifications_API/Using_the_Notifications_API ?

Comment 17 by sdantul...@chromium.org, May 12 2017

Status: Verified (was: Assigned)
Thanks yhanada@

Works fine. Verified on ChromeOS 9534.0.0, 60.0.3092.0
►

Sign in to add a comment