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

Issue 739386 link

Starred by 2 users
Status: Started
Owner:
peter@chromium.org
Cc:
█ picksi@chromium.org
dewittj@chromium.org
rsesek@chromium.org
█ yoshiki@chromium.org
peter@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug



Sign in to add a comment

Stop building the message center for Mac

Project Member Reported by peter@chromium.org, Jul 5 2017 
We now use native notifications on Mac OS X, so we can stop building the message center. This will yield a lot of clean-up opportunities, such as removing the cocoa code, simplifying code and clarifying the dependencies.
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 10 2017 

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

commit b8115a49ef17d57475a7c45ad6ab3f6d59a3eeb3
Author: Peter Beverloo <peter@chromium.org>
Date: Mon Jul 10 15:50:25 2017

Stop running message_center_unittests on Mac bots

BUG=739386

Change-Id: I81d4802615ef8b02aaa48db99bddbdd8ce8f8868
Reviewed-on: https://chromium-review.googlesource.com/559536
Reviewed-by: John Budorick <jbudorick@chromium.org>
Commit-Queue: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485280}
[modify] https://crrev.com/b8115a49ef17d57475a7c45ad6ab3f6d59a3eeb3/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/b8115a49ef17d57475a7c45ad6ab3f6d59a3eeb3/testing/buildbot/chromium.mac.json
Project Member

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

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

commit 6994832389908c025485d6dbd6e88f44c3a611e1
Author: Peter Beverloo <peter@chromium.org>
Date: Fri Jul 28 19:01:57 2017

Unify the checks on whether the message center should be used

There's a bunch of checks in a few places now: let's have a single flag
in ui_features.gni to toggle control.

This CL also explicitly drops support for iOS. In practice iOS wasn't
using the message center anyway, so the few checks can just be deleted.

BUG=739386

Change-Id: I783bc9732f35f5a400410628fb90341141e2e03a
Reviewed-on: https://chromium-review.googlesource.com/559539
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Commit-Queue: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490481}
[modify] https://crrev.com/6994832389908c025485d6dbd6e88f44c3a611e1/BUILD.gn
[modify] https://crrev.com/6994832389908c025485d6dbd6e88f44c3a611e1/chrome/test/BUILD.gn
[modify] https://crrev.com/6994832389908c025485d6dbd6e88f44c3a611e1/ui/base/ui_features.gni
[modify] https://crrev.com/6994832389908c025485d6dbd6e88f44c3a611e1/ui/message_center/BUILD.gn
[modify] https://crrev.com/6994832389908c025485d6dbd6e88f44c3a611e1/ui/message_center/dummy_message_center.cc
[modify] https://crrev.com/6994832389908c025485d6dbd6e88f44c3a611e1/ui/message_center/notification.h
[modify] https://crrev.com/6994832389908c025485d6dbd6e88f44c3a611e1/ui/message_center/test/run_all_unittests.cc

Comment 4 by rsesek@chromium.org, Oct 17 2017 

What's the status of the third CL?

Comment 5 by rsesek@chromium.org, Feb 6 2018 

I just tried doing this here https://chromium-review.googlesource.com/c/chromium/src/+/904265 and it doesn't work because of a TRANSIENT notification type that was added(?). It's kinda bad that we've been shipping effectively dead code for 10 months and we can't really remove it...
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 19 

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

commit 8f34427c02d36bc72c92a16f4caf87573a924c03
Author: Robert Sesek <rsesek@chromium.org>
Date: Mon Nov 19 23:56:40 2018

Fixes to get Views versions of message_center_unittests passing on Mac.

This enables the views version of the message_center, in preparation
for removing the Cocoa implementation.

Fixes:
- Use an EventGenerator in tests, rather than manually synthesizing
  events.
- NotificationInputContainerMD::HandleKeyEvent() now explicitly clears
  the text input field when handling VKEY_RETURN, as the test expects
  this.
- Disabling notifications is no longer handled in
  NotificationViewMD::ButtonPressed(), since it wasn't the button
  Listener for the block_all_button_ (the
  NotificationControlButtonsView is). Disabling is now handled in
  ToggleInlineSettings() instead.
- The SlideOut tests are disabled on Mac because the test framework
  doesn't support synthesizing gesture scroll events.

Bug: 739386
Change-Id: Id841779233662111f52d491ba78e5315422003b4
Reviewed-on: https://chromium-review.googlesource.com/c/1341091
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609514}
[modify] https://crrev.com/8f34427c02d36bc72c92a16f4caf87573a924c03/ui/message_center/BUILD.gn
[modify] https://crrev.com/8f34427c02d36bc72c92a16f4caf87573a924c03/ui/message_center/test/run_all_unittests.cc
[modify] https://crrev.com/8f34427c02d36bc72c92a16f4caf87573a924c03/ui/message_center/views/message_popup_view.cc
[add] https://crrev.com/8f34427c02d36bc72c92a16f4caf87573a924c03/ui/message_center/views/message_popup_view_mac.mm
[modify] https://crrev.com/8f34427c02d36bc72c92a16f4caf87573a924c03/ui/message_center/views/notification_view_md.cc
[modify] https://crrev.com/8f34427c02d36bc72c92a16f4caf87573a924c03/ui/message_center/views/notification_view_md_unittest.cc
[modify] https://crrev.com/8f34427c02d36bc72c92a16f4caf87573a924c03/ui/message_center/views/notification_view_unittest.cc
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 20 

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

commit 975e1773a51762be98e6073489a93596daa2036b
Author: Maxim Kolosovskiy <kolos@chromium.org>
Date: Tue Nov 20 09:44:39 2018

Revert "Fixes to get Views versions of message_center_unittests passing on Mac."

This reverts commit 8f34427c02d36bc72c92a16f4caf87573a924c03.

Reason for revert: BoundedLabelTest.GetWrappedTextTest failures.
Builder: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.11%20Tests

First failed build: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.11%20Tests/30863

Original change's description:
> Fixes to get Views versions of message_center_unittests passing on Mac.
> 
> This enables the views version of the message_center, in preparation
> for removing the Cocoa implementation.
> 
> Fixes:
> - Use an EventGenerator in tests, rather than manually synthesizing
>   events.
> - NotificationInputContainerMD::HandleKeyEvent() now explicitly clears
>   the text input field when handling VKEY_RETURN, as the test expects
>   this.
> - Disabling notifications is no longer handled in
>   NotificationViewMD::ButtonPressed(), since it wasn't the button
>   Listener for the block_all_button_ (the
>   NotificationControlButtonsView is). Disabling is now handled in
>   ToggleInlineSettings() instead.
> - The SlideOut tests are disabled on Mac because the test framework
>   doesn't support synthesizing gesture scroll events.
> 
> Bug: 739386
> Change-Id: Id841779233662111f52d491ba78e5315422003b4
> Reviewed-on: https://chromium-review.googlesource.com/c/1341091
> Commit-Queue: Robert Sesek <rsesek@chromium.org>
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Reviewed-by: Evan Stade <estade@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#609514}

TBR=stevenjb@chromium.org,estade@chromium.org,rsesek@chromium.org

Change-Id: I16b0173147f81502bd0a1fa83ffb895db6420f13
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 739386
Reviewed-on: https://chromium-review.googlesource.com/c/1343262
Reviewed-by: Maxim Kolosovskiy <kolos@chromium.org>
Commit-Queue: Maxim Kolosovskiy <kolos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609648}
[modify] https://crrev.com/975e1773a51762be98e6073489a93596daa2036b/ui/message_center/BUILD.gn
[modify] https://crrev.com/975e1773a51762be98e6073489a93596daa2036b/ui/message_center/test/run_all_unittests.cc
[modify] https://crrev.com/975e1773a51762be98e6073489a93596daa2036b/ui/message_center/views/message_popup_view.cc
[delete] https://crrev.com/a948722765a92cb9899176ecfb87e1211889b784/ui/message_center/views/message_popup_view_mac.mm
[modify] https://crrev.com/975e1773a51762be98e6073489a93596daa2036b/ui/message_center/views/notification_view_md.cc
[modify] https://crrev.com/975e1773a51762be98e6073489a93596daa2036b/ui/message_center/views/notification_view_md_unittest.cc
[modify] https://crrev.com/975e1773a51762be98e6073489a93596daa2036b/ui/message_center/views/notification_view_unittest.cc
Project Member

Comment 8 by bugdroid1@chromium.org, Nov 20 

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

commit 85954d42d4252eb721aa0fe2e074aa219ae62ec4
Author: Robert Sesek <rsesek@chromium.org>
Date: Tue Nov 20 17:26:11 2018

Reland "Fixes to get Views versions of message_center_unittests passing on Mac."

This is a reland of 8f34427c02d36bc72c92a16f4caf87573a924c03

Additional fix:
- Increase a pixel expectation in BoundedLabelTest, which was
  affected by some macOS 10.11-specific font variation.

Original change's description:
> Fixes to get Views versions of message_center_unittests passing on Mac.
>
> This enables the views version of the message_center, in preparation
> for removing the Cocoa implementation.
>
> Fixes:
> - Use an EventGenerator in tests, rather than manually synthesizing
>   events.
> - NotificationInputContainerMD::HandleKeyEvent() now explicitly clears
>   the text input field when handling VKEY_RETURN, as the test expects
>   this.
> - Disabling notifications is no longer handled in
>   NotificationViewMD::ButtonPressed(), since it wasn't the button
>   Listener for the block_all_button_ (the
>   NotificationControlButtonsView is). Disabling is now handled in
>   ToggleInlineSettings() instead.
> - The SlideOut tests are disabled on Mac because the test framework
>   doesn't support synthesizing gesture scroll events.
>
> Bug: 739386
> Change-Id: Id841779233662111f52d491ba78e5315422003b4
> Reviewed-on: https://chromium-review.googlesource.com/c/1341091
> Commit-Queue: Robert Sesek <rsesek@chromium.org>
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Reviewed-by: Evan Stade <estade@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#609514}

Tbr: estade@chromium.org
Bug: 739386
Change-Id: Id7b6b05f5fa487ebb9caaff18ee8570cf5d4b99d
Reviewed-on: https://chromium-review.googlesource.com/c/1343665
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609735}
[modify] https://crrev.com/85954d42d4252eb721aa0fe2e074aa219ae62ec4/ui/message_center/BUILD.gn
[modify] https://crrev.com/85954d42d4252eb721aa0fe2e074aa219ae62ec4/ui/message_center/test/run_all_unittests.cc
[modify] https://crrev.com/85954d42d4252eb721aa0fe2e074aa219ae62ec4/ui/message_center/views/bounded_label_unittest.cc
[modify] https://crrev.com/85954d42d4252eb721aa0fe2e074aa219ae62ec4/ui/message_center/views/message_popup_view.cc
[add] https://crrev.com/85954d42d4252eb721aa0fe2e074aa219ae62ec4/ui/message_center/views/message_popup_view_mac.mm
[modify] https://crrev.com/85954d42d4252eb721aa0fe2e074aa219ae62ec4/ui/message_center/views/notification_view_md.cc
[modify] https://crrev.com/85954d42d4252eb721aa0fe2e074aa219ae62ec4/ui/message_center/views/notification_view_md_unittest.cc
[modify] https://crrev.com/85954d42d4252eb721aa0fe2e074aa219ae62ec4/ui/message_center/views/notification_view_unittest.cc
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 20 

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

commit 90940f0e9438ad443c6519c47b6c96a1e5c68fd0
Author: Robert Sesek <rsesek@chromium.org>
Date: Tue Nov 20 19:14:37 2018

Disable BoundedLabelTest.GetWrappedTextTest on macOS 10.10

There are slightly different font metrics between macOS versions, and
either macOS 10.10 or 10.11 will fail with a single hard-coded constant
value for pixel widths. Just disable this test on 10.10, which is the
oldest version that is supported, and let the test pass on 10.11+.

Bug: 739386
Change-Id: I82ea6a3276ebe5de4906c33a906ebf76cf2c85b9
Reviewed-on: https://chromium-review.googlesource.com/c/1344769
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609764}
[modify] https://crrev.com/90940f0e9438ad443c6519c47b6c96a1e5c68fd0/ui/message_center/views/bounded_label_unittest.cc
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 20 

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

commit 85cd2a967787690bf93c9159734b9e94ad25e852
Author: Robert Sesek <rsesek@chromium.org>
Date: Tue Nov 20 21:27:15 2018

Delete Cocoa message_center and switch to the Views version.

This also removes some now-unused Cocoa control classes in //ui/base and
several unneeded resources.

Bug: 739386,  832676 
Change-Id: I9289cc5613c4303521aaed9271ddc7574e2611a0
Reviewed-on: https://chromium-review.googlesource.com/c/1343055
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609810}
[modify] https://crrev.com/85cd2a967787690bf93c9159734b9e94ad25e852/chrome/browser/ui/BUILD.gn
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/chrome/browser/ui/cocoa/notifications/message_center_bridge.h
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/chrome/browser/ui/cocoa/notifications/message_center_bridge.mm
[modify] https://crrev.com/85cd2a967787690bf93c9159734b9e94ad25e852/ui/base/BUILD.gn
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/base/cocoa/hover_button.h
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/base/cocoa/hover_button.mm
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/base/cocoa/hover_button_unittest.mm
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/base/cocoa/hover_image_button.h
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/base/cocoa/hover_image_button.mm
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/base/cocoa/hover_image_button_unittest.mm
[modify] https://crrev.com/85cd2a967787690bf93c9159734b9e94ad25e852/ui/message_center/BUILD.gn
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/message_center/cocoa/notification_controller.h
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/message_center/cocoa/notification_controller.mm
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/message_center/cocoa/notification_controller_unittest.mm
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/message_center/cocoa/opaque_views.h
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/message_center/cocoa/opaque_views.mm
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/message_center/cocoa/popup_collection.h
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/message_center/cocoa/popup_collection.mm
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/message_center/cocoa/popup_collection_unittest.mm
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/message_center/cocoa/popup_controller.h
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/message_center/cocoa/popup_controller.mm
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/message_center/cocoa/popup_controller_unittest.mm
[modify] https://crrev.com/85cd2a967787690bf93c9159734b9e94ad25e852/ui/message_center/test/run_all_unittests.cc
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/resources/default_100_percent/common/notification_close.png
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/resources/default_100_percent/common/notification_close_hover.png
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/resources/default_100_percent/common/notification_close_pressed.png
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/resources/default_100_percent/common/notification_settings_button.png
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/resources/default_100_percent/common/notification_settings_button_hover.png
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/resources/default_100_percent/common/notification_settings_button_pressed.png
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/resources/default_100_percent/win/notification_close.png
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/resources/default_100_percent/win/notification_close_hover.png
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/resources/default_100_percent/win/notification_close_pressed.png
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/resources/default_200_percent/common/notification_close.png
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/resources/default_200_percent/common/notification_close_hover.png
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/resources/default_200_percent/common/notification_close_pressed.png
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/resources/default_200_percent/common/notification_settings_button.png
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/resources/default_200_percent/common/notification_settings_button_hover.png
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/resources/default_200_percent/common/notification_settings_button_pressed.png
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/resources/default_200_percent/win/notification_close.png
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/resources/default_200_percent/win/notification_close_hover.png
[delete] https://crrev.com/0fc5f9d3864137ca0063ab9665f59644f93427fa/ui/resources/default_200_percent/win/notification_close_pressed.png
[modify] https://crrev.com/85cd2a967787690bf93c9159734b9e94ad25e852/ui/resources/ui_resources.grd

Comment 11 by mmanchala@chromium.org, Nov 22

Labels: Hotlist-DesktopUIValid Hotlist-DesktopUIChecked
***UI Mass Triage***

Since work is in progress, adding appropriate labels
►

Sign in to add a comment