New issue
Advanced search Search tips

Issue 885198 link

Starred by 1 user
Status: Fixed
Owner:
rdevlin....@chromium.org
Closed: Sep 24
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 885201



Sign in to add a comment

SimpleMenuModel::Delegate and MenuModel both declare virtual MenuWillShow() methods

Project Member Reported by rdevlin....@chromium.org, Sep 18 
ui::SimpleMenuModel inherits from ui::MenuModel.
ui::MenuModel declares `virtual void MenuWillShow()`
ui::SimpleMenuModel::Delegate declares `virtual void MenuWillShow(ui::SimpleMenuModel*)`

Because of this, a class that inherits both ui::SimpleMenuModel and ui::SimpleMenuModel::Delegate cannot override MenuWillShow(), since overriding either will hide the other.  However, deriving both ui::SimpleMenuModel and ui::SimpleMenuModel::Delegate is reasonably common practice (even being done in the example [1]).

As a simple workaround, I say we just rename one of the methods.

[1] https://chromium.googlesource.com/chromium/src/+/73b31c297c48b99f2d352625b74f159ef33e0f91/ui/views/examples/menu_example.cc#26

Comment 1 by rdevlin....@chromium.org, Sep 18

Blocking: 885201
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 20 

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

commit f4ab46c9b1e917bd616d09d9b97e6388c1ffad39
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Thu Sep 20 06:14:36 2018

[UI] Resolve method conflict between MenuModel, SimpleMenuModel::Delegate

ui::MenuModel (which ui::SimpleMenuModel inherits from) declares a
virtual method, `void MenuWillShow()`. SimpleMenuModel::Delegate
declares a similar method, `void MenuWillShow(SimpleMenuModel*)`. This
results in a conflict for any class that inherits from both
SimpleMenuModel and SimpleMenuModel::Delegate (which is not uncommon).

Resolve this by renaming the SimpleMenuModel::Delegate version to
`void SimpleMenuWillShow(SimpleMenuModel*)`.

Bug:  885198 
Change-Id: I0581991fa7a12368007f6c082c461acb4833750c
Reviewed-on: https://chromium-review.googlesource.com/1230581
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592694}
[modify] https://crrev.com/f4ab46c9b1e917bd616d09d9b97e6388c1ffad39/chrome/browser/renderer_context_menu/mock_render_view_context_menu.cc
[modify] https://crrev.com/f4ab46c9b1e917bd616d09d9b97e6388c1ffad39/chrome/browser/renderer_context_menu/mock_render_view_context_menu.h
[modify] https://crrev.com/f4ab46c9b1e917bd616d09d9b97e6388c1ffad39/chrome/browser/ui/toolbar/media_router_contextual_menu.cc
[modify] https://crrev.com/f4ab46c9b1e917bd616d09d9b97e6388c1ffad39/chrome/browser/ui/toolbar/media_router_contextual_menu.h
[modify] https://crrev.com/f4ab46c9b1e917bd616d09d9b97e6388c1ffad39/chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc
[modify] https://crrev.com/f4ab46c9b1e917bd616d09d9b97e6388c1ffad39/components/renderer_context_menu/render_view_context_menu_base.cc
[modify] https://crrev.com/f4ab46c9b1e917bd616d09d9b97e6388c1ffad39/components/renderer_context_menu/render_view_context_menu_base.h
[modify] https://crrev.com/f4ab46c9b1e917bd616d09d9b97e6388c1ffad39/ui/base/cocoa/menu_controller_unittest.mm
[modify] https://crrev.com/f4ab46c9b1e917bd616d09d9b97e6388c1ffad39/ui/base/models/simple_menu_model.cc
[modify] https://crrev.com/f4ab46c9b1e917bd616d09d9b97e6388c1ffad39/ui/base/models/simple_menu_model.h
[modify] https://crrev.com/f4ab46c9b1e917bd616d09d9b97e6388c1ffad39/ui/views/controls/menu/menu_runner_cocoa_unittest.mm

Comment 3 by rdevlin....@chromium.org, Sep 24

Status: Fixed (was: Started)
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 26

Labels: merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/01e57246b9df7ad5bc179d440eac9c5b1b98c740

commit 01e57246b9df7ad5bc179d440eac9c5b1b98c740
Author: Karandeep Bhatia <karandeepb@chromium.org>
Date: Wed Sep 26 18:30:33 2018

[Merge M70] Click to script: Record metrics for action chosen through extension icon context menu.

This CL adds metrics for the action taken by the user through the extension icon
context menu. This also requires commits
94aa4fbb1bf03db09b0b191cb1f4f1ccc3f094dc and
f4ab46c9b1e917bd616d09d9b97e6388c1ffad39 as pre-requisities, which don't
introduce any behavior changes.

TBR=rdevlin.cronin@chromium.org, avi@chromium.org, sky@chromium.org
BUG= 885201 

Change-Id: I5eb2261f493298667ec978d24e554b15814e5f84
Reviewed-on: https://chromium-review.googlesource.com/1239217
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593782}
(cherry picked from commit 6b9706d935f92b2e90824ca37c16b0fddb13e4df)

[Extensions] Update ExtensionContextMenuModel::MenuEntries names

A few of the name values for the menu entries in the
ExtensionContextMenuModel were a little misleading. Remap the
following (<old> -> <new>):
- NAME -> HOME_PAGE (opens the extension's home page, or the webstore
  if a separate home page was not specified).
- CONFIGURE - OPTIONS (opens the extension's options page)
- MANAGE -> MANAGE_EXTENSIONS (opens the chrome://extensions page)

No functional change is intended; this is purely a renaming of code
values.

Bug: None

Change-Id: Iabac73bf3bd7d85ea02b23379a9a19eeef02cf21
Reviewed-on: https://chromium-review.googlesource.com/1229774
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593082}
(cherry picked from commit 94aa4fbb1bf03db09b0b191cb1f4f1ccc3f094dc)

[UI] Resolve method conflict between MenuModel, SimpleMenuModel::Delegate

ui::MenuModel (which ui::SimpleMenuModel inherits from) declares a
virtual method, `void MenuWillShow()`. SimpleMenuModel::Delegate
declares a similar method, `void MenuWillShow(SimpleMenuModel*)`. This
results in a conflict for any class that inherits from both
SimpleMenuModel and SimpleMenuModel::Delegate (which is not uncommon).

Resolve this by renaming the SimpleMenuModel::Delegate version to
`void SimpleMenuWillShow(SimpleMenuModel*)`.

(cherry picked from commit f4ab46c9b1e917bd616d09d9b97e6388c1ffad39)

Bug:  885198 
Change-Id: I0581991fa7a12368007f6c082c461acb4833750c
Reviewed-on: https://chromium-review.googlesource.com/1230581
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#592694}
Reviewed-on: https://chromium-review.googlesource.com/1246683
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#688}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/01e57246b9df7ad5bc179d440eac9c5b1b98c740/chrome/browser/extensions/extension_context_menu_model.cc
[modify] https://crrev.com/01e57246b9df7ad5bc179d440eac9c5b1b98c740/chrome/browser/extensions/extension_context_menu_model.h
[modify] https://crrev.com/01e57246b9df7ad5bc179d440eac9c5b1b98c740/chrome/browser/extensions/extension_context_menu_model_unittest.cc
[modify] https://crrev.com/01e57246b9df7ad5bc179d440eac9c5b1b98c740/chrome/browser/renderer_context_menu/mock_render_view_context_menu.cc
[modify] https://crrev.com/01e57246b9df7ad5bc179d440eac9c5b1b98c740/chrome/browser/renderer_context_menu/mock_render_view_context_menu.h
[modify] https://crrev.com/01e57246b9df7ad5bc179d440eac9c5b1b98c740/chrome/browser/ui/toolbar/media_router_contextual_menu.cc
[modify] https://crrev.com/01e57246b9df7ad5bc179d440eac9c5b1b98c740/chrome/browser/ui/toolbar/media_router_contextual_menu.h
[modify] https://crrev.com/01e57246b9df7ad5bc179d440eac9c5b1b98c740/chrome/browser/ui/toolbar/media_router_contextual_menu_unittest.cc
[modify] https://crrev.com/01e57246b9df7ad5bc179d440eac9c5b1b98c740/components/renderer_context_menu/render_view_context_menu_base.cc
[modify] https://crrev.com/01e57246b9df7ad5bc179d440eac9c5b1b98c740/components/renderer_context_menu/render_view_context_menu_base.h
[modify] https://crrev.com/01e57246b9df7ad5bc179d440eac9c5b1b98c740/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/01e57246b9df7ad5bc179d440eac9c5b1b98c740/tools/metrics/histograms/histograms.xml
[modify] https://crrev.com/01e57246b9df7ad5bc179d440eac9c5b1b98c740/ui/base/cocoa/menu_controller_unittest.mm
[modify] https://crrev.com/01e57246b9df7ad5bc179d440eac9c5b1b98c740/ui/base/models/simple_menu_model.cc
[modify] https://crrev.com/01e57246b9df7ad5bc179d440eac9c5b1b98c740/ui/base/models/simple_menu_model.h
[modify] https://crrev.com/01e57246b9df7ad5bc179d440eac9c5b1b98c740/ui/views/controls/menu/menu_runner_cocoa_unittest.mm
►

Sign in to add a comment