New issue
Advanced search Search tips

Issue 702823 link

Starred by 1 user

Issue metadata

Status: Archived
Owner: ----
Closed: Mar 2018
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Task



Sign in to add a comment

Remove PlatformAccelerator and Accelerator::

Project Member Reported by sky@chromium.org, Mar 17 2017

Issue description

PlatformAccelerator only has one concrete class, PlatformAcceleratorCocoa. Because there is only one subclass there is no point in having the generic cross-platform class. Accelerator has a PlatformAccelerator, but it hasn't been wired up correctly for a while. Cocoa code should instead maintain the PlatformAcceleratorCocoa separately from Accelerator.
 
Labels: Hotlist-Polish
Status: Available (was: Untriaged)
Project Member

Comment 2 by bugdroid1@chromium.org, Mar 20 2017

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

commit 12d428d5d4f63b4e39f7eb8aacc66b8444419982
Author: sky <sky@chromium.org>
Date: Mon Mar 20 16:15:18 2017

Remove comparing PlatformAccelerator from operator== for Accelerator

It wasn't wired up correctly, so presumably we aren't relying on it.

BUG= 702823 
TEST=none

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

[modify] https://crrev.com/12d428d5d4f63b4e39f7eb8aacc66b8444419982/ui/base/accelerators/accelerator.cc
[modify] https://crrev.com/12d428d5d4f63b4e39f7eb8aacc66b8444419982/ui/base/accelerators/accelerator.h

Project Member

Comment 3 by sheriffbot@chromium.org, Mar 21 2018

Status: Archived (was: Available)
This issue has been available for more than 365 days with no owner or cc list, so archiving this bug. Please re-open or file a new bug if this is still an issue.

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

Comment 4 by bugdroid1@chromium.org, Jul 16

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

commit 2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b
Author: Erik Chen <erikchen@chromium.org>
Date: Mon Jul 16 21:30:54 2018

Remove PlatformAccelerator.

ui::Accelerator had a member platform_accelerator() which was only used on
macOS, and provided redundant information. This CL removes the member.

Change-Id: Ib3954a2c8ff7197606c946e207dd5d3bd75af66d
Bug:  846893 ,  702823 
Reviewed-on: https://chromium-review.googlesource.com/1135644
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575429}
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/global_keyboard_shortcuts_mac.mm
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/accelerators_cocoa.h
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/accelerators_cocoa.mm
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm
[delete] https://crrev.com/0131dcca85a720678ded88d843088de9e0e263c8/chrome/browser/ui/cocoa/accelerators_cocoa_unittest.mm
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/app_menu/app_menu_controller.h
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/app_menu/app_menu_controller.mm
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/app_menu/app_menu_controller_unittest.mm
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/confirm_quit_panel_controller.h
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/confirm_quit_panel_controller.mm
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/confirm_quit_panel_controller_unittest.mm
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/share_menu_controller.mm
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/browser/ui/cocoa/toolbar/reload_button_cocoa.mm
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/chrome/test/BUILD.gn
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/ui/base/BUILD.gn
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/ui/base/accelerators/accelerator.cc
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/ui/base/accelerators/accelerator.h
[delete] https://crrev.com/0131dcca85a720678ded88d843088de9e0e263c8/ui/base/accelerators/platform_accelerator.h
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/ui/base/accelerators/platform_accelerator_cocoa.h
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/ui/base/accelerators/platform_accelerator_cocoa.mm
[modify] https://crrev.com/2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b/ui/base/cocoa/menu_controller.mm

Project Member

Comment 5 by bugdroid1@chromium.org, Jul 16

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

commit 7b8b4f583548218b816cff7bb4dbfef27ed81a56
Author: Xiaocheng Hu <xiaochengh@chromium.org>
Date: Mon Jul 16 23:46:51 2018

Revert "Remove PlatformAccelerator."

This reverts commit 2f0772ee38f51f66d307338eb7fbd4d7d9a7e76b.

Reason for revert: Suspected to cause failure in AcceleratorsCocoaBrowserTest.MainMenuAcceleratorsInMapping

Original change's description:
> Remove PlatformAccelerator.
> 
> ui::Accelerator had a member platform_accelerator() which was only used on
> macOS, and provided redundant information. This CL removes the member.
> 
> Change-Id: Ib3954a2c8ff7197606c946e207dd5d3bd75af66d
> Bug:  846893 ,  702823 
> Reviewed-on: https://chromium-review.googlesource.com/1135644
> Commit-Queue: Erik Chen <erikchen@chromium.org>
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#575429}

TBR=thakis@chromium.org,erikchen@chromium.org

Change-Id: I3f142628a1ab406028a5f64db7d6acdf13c4d41b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  846893 ,  702823 ,  864272 
Reviewed-on: https://chromium-review.googlesource.com/1139153
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575484}
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/global_keyboard_shortcuts_mac.mm
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/accelerators_cocoa.h
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/accelerators_cocoa.mm
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm
[add] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/accelerators_cocoa_unittest.mm
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/app_menu/app_menu_controller.h
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/app_menu/app_menu_controller.mm
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/app_menu/app_menu_controller_unittest.mm
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/confirm_quit_panel_controller.h
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/confirm_quit_panel_controller.mm
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/confirm_quit_panel_controller_unittest.mm
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/share_menu_controller.mm
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/browser/ui/cocoa/toolbar/reload_button_cocoa.mm
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/chrome/test/BUILD.gn
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/ui/base/BUILD.gn
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/ui/base/accelerators/accelerator.cc
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/ui/base/accelerators/accelerator.h
[add] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/ui/base/accelerators/platform_accelerator.h
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/ui/base/accelerators/platform_accelerator_cocoa.h
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/ui/base/accelerators/platform_accelerator_cocoa.mm
[modify] https://crrev.com/7b8b4f583548218b816cff7bb4dbfef27ed81a56/ui/base/cocoa/menu_controller.mm

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 18

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

commit e262e854ea05600f16a13f771bcf62bac25c6337
Author: erikchen <erikchen@chromium.org>
Date: Wed Jul 18 23:12:05 2018

[Reland #1] Remove PlatformAccelerator.

The test AcceleratorsCocoaBrowserTest.MainMenuAcceleratorsInMapping had an
incorrect early return, where it intended to use a continue. The first attempt
to land this CL changed the early return to a continue. This broke the test.

This CL fixes the test by:
  * Not performing any checks on menu items without tags, as they may just be
    macOS menu items added by the OS that we don't care about.
  * Adding missing accelerator combinations to AcceleratorsCocoa.

This CL also removes some dead code in accelerators_cocoa.mm

> ui::Accelerator had a member platform_accelerator() which was only used on
> macOS, and provided redundant information. This CL removes the member.
>
> Bug:  846893 ,  702823 
> Reviewed-on: https://chromium-review.googlesource.com/1135644
> Commit-Queue: Erik Chen <erikchen@chromium.org>
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#575429}

Change-Id: I195bd174a96da58bff96e22495c5ea5b5bfe8549
Bug:  846893 ,  702823 
Reviewed-on: https://chromium-review.googlesource.com/1140211
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576244}
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/global_keyboard_shortcuts_mac.mm
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/accelerators_cocoa.h
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/accelerators_cocoa.mm
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm
[delete] https://crrev.com/e923a58c970eebeeba7d71f268346377e83efe1f/chrome/browser/ui/cocoa/accelerators_cocoa_unittest.mm
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/app_menu/app_menu_controller.h
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/app_menu/app_menu_controller.mm
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/app_menu/app_menu_controller_unittest.mm
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/confirm_quit_panel_controller.h
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/confirm_quit_panel_controller.mm
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/confirm_quit_panel_controller_unittest.mm
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/share_menu_controller.mm
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/browser/ui/cocoa/toolbar/reload_button_cocoa.mm
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/chrome/test/BUILD.gn
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/ui/base/BUILD.gn
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/ui/base/accelerators/accelerator.cc
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/ui/base/accelerators/accelerator.h
[delete] https://crrev.com/e923a58c970eebeeba7d71f268346377e83efe1f/ui/base/accelerators/platform_accelerator.h
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/ui/base/accelerators/platform_accelerator_cocoa.h
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/ui/base/accelerators/platform_accelerator_cocoa.mm
[modify] https://crrev.com/e262e854ea05600f16a13f771bcf62bac25c6337/ui/base/cocoa/menu_controller.mm

Sign in to add a comment