Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
432003007:
Separate chrome independent part from RVContextMenu (Closed)
|
Created:
6 years, 4 months ago by oshima Modified:
6 years, 4 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, jennb, tfarina, Dmitry Titov, jianli, dcheng, chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionSeparate chrome independent part from RVContextMenu
and move it to components/renderer_context_menu/renderer_view_context_menu_base.{h|cc}
BUG=397320
TBR=blundell@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287971
Patch Set 1 : #Patch Set 2 : #
Total comments: 20
Patch Set 3 : #
Total comments: 6
Patch Set 4 : addressed comments #Patch Set 5 : exclude android webview #
Messages
Total messages: 29 (0 generated)
https://chromiumcodereview.appspot.com/432003007/diff/140001/chrome/browser/r... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://chromiumcodereview.appspot.com/432003007/diff/140001/chrome/browser/r... chrome/browser/renderer_context_menu/render_view_context_menu.cc:281: bool custom_id_ranges_initialized = false; rename to g_custom_id_ranges_initialized https://chromiumcodereview.appspot.com/432003007/diff/140001/chrome/browser/r... File chrome/browser/renderer_context_menu/render_view_context_menu.h (right): https://chromiumcodereview.appspot.com/432003007/diff/140001/chrome/browser/r... chrome/browser/renderer_context_menu/render_view_context_menu.h:52: static void InitCustomIdRnages(); Ranges typo. https://chromiumcodereview.appspot.com/432003007/diff/140001/chrome/browser/r... chrome/browser/renderer_context_menu/render_view_context_menu.h:83: // RenderViewContextMenu: RenderViewContextMenuBase nit: also sort them? https://chromiumcodereview.appspot.com/432003007/diff/140001/components/rende... File components/renderer_context_menu.gypi (right): https://chromiumcodereview.appspot.com/432003007/diff/140001/components/rende... components/renderer_context_menu.gypi:26: 'renderer_context_menu/render_view_context_menu_base.cc', Sort. https://chromiumcodereview.appspot.com/432003007/diff/140001/components/rende... File components/renderer_context_menu/render_view_context_menu_base.cc (right): https://chromiumcodereview.appspot.com/432003007/diff/140001/components/rende... components/renderer_context_menu/render_view_context_menu_base.cc:35: // The range of command IDs reserved for content's custom menus. The (inclusive) range of ... https://chromiumcodereview.appspot.com/432003007/diff/140001/components/rende... components/renderer_context_menu/render_view_context_menu_base.cc:36: // TODO(oshima): These values will be injected by embedders. This TODO has been taken care of? https://chromiumcodereview.appspot.com/432003007/diff/140001/components/rende... components/renderer_context_menu/render_view_context_menu_base.cc:37: int content_context_custom_first = -1; These should also have g_ prefix as they are global. https://chromiumcodereview.appspot.com/432003007/diff/140001/components/rende... components/renderer_context_menu/render_view_context_menu_base.cc:137: void RenderViewContextMenuBase::SetContentCustomCommandIdRange( Add a note that this range is inclusive. https://chromiumcodereview.appspot.com/432003007/diff/140001/components/rende... File components/renderer_context_menu/render_view_context_menu_base.h (right): https://chromiumcodereview.appspot.com/432003007/diff/140001/components/rende... components/renderer_context_menu/render_view_context_menu_base.h:129: virtual void HandleAuthorizeAllPlugins() = 0; This needs to be guarded by defined(ENABLE_PLUGINS). https://chromiumcodereview.appspot.com/432003007/diff/140001/components/rende... components/renderer_context_menu/render_view_context_menu_base.h:162: // extensions::ContextMenuMatcher extension_items_; remove
https://codereview.chromium.org/432003007/diff/140001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/432003007/diff/140001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:281: bool custom_id_ranges_initialized = false; On 2014/08/01 07:59:34, lazyboy wrote: > rename to g_custom_id_ranges_initialized this is file scoped, not global. https://codereview.chromium.org/432003007/diff/140001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.h (right): https://codereview.chromium.org/432003007/diff/140001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.h:52: static void InitCustomIdRnages(); On 2014/08/01 07:59:34, lazyboy wrote: > Ranges typo. This was no longer necessary. removed. https://codereview.chromium.org/432003007/diff/140001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.h:83: // RenderViewContextMenu: On 2014/08/01 07:59:34, lazyboy wrote: > RenderViewContextMenuBase Done > nit: also sort them? We don't sort methods. It should be in the same order as in the super class/interface. I changed the order a bit though. I kept the order of method body in this CL to make it easy to see the diff. I'll reorder them in a separate CL so that they'll match. https://codereview.chromium.org/432003007/diff/140001/components/renderer_con... File components/renderer_context_menu.gypi (right): https://codereview.chromium.org/432003007/diff/140001/components/renderer_con... components/renderer_context_menu.gypi:26: 'renderer_context_menu/render_view_context_menu_base.cc', On 2014/08/01 07:59:34, lazyboy wrote: > Sort. Done. https://codereview.chromium.org/432003007/diff/140001/components/renderer_con... File components/renderer_context_menu/render_view_context_menu_base.cc (right): https://codereview.chromium.org/432003007/diff/140001/components/renderer_con... components/renderer_context_menu/render_view_context_menu_base.cc:36: // TODO(oshima): These values will be injected by embedders. On 2014/08/01 07:59:34, lazyboy wrote: > This TODO has been taken care of? Done. Thanks https://codereview.chromium.org/432003007/diff/140001/components/renderer_con... components/renderer_context_menu/render_view_context_menu_base.cc:37: int content_context_custom_first = -1; On 2014/08/01 07:59:34, lazyboy wrote: > These should also have g_ prefix as they are global. same here. they're file scoped, not global. https://codereview.chromium.org/432003007/diff/140001/components/renderer_con... components/renderer_context_menu/render_view_context_menu_base.cc:137: void RenderViewContextMenuBase::SetContentCustomCommandIdRange( On 2014/08/01 07:59:34, lazyboy wrote: > Add a note that this range is inclusive. Done. https://codereview.chromium.org/432003007/diff/140001/components/renderer_con... File components/renderer_context_menu/render_view_context_menu_base.h (right): https://codereview.chromium.org/432003007/diff/140001/components/renderer_con... components/renderer_context_menu/render_view_context_menu_base.h:129: virtual void HandleAuthorizeAllPlugins() = 0; On 2014/08/01 07:59:34, lazyboy wrote: > This needs to be guarded by defined(ENABLE_PLUGINS). Thank you for the catch. Fixed. https://codereview.chromium.org/432003007/diff/140001/components/renderer_con... components/renderer_context_menu/render_view_context_menu_base.h:162: // extensions::ContextMenuMatcher extension_items_; On 2014/08/01 07:59:34, lazyboy wrote: > remove Done.
LGTM with nits. This CL should give you the most you need to use context menu for athena, right? https://codereview.chromium.org/432003007/diff/140001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/432003007/diff/140001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:281: bool custom_id_ranges_initialized = false; On 2014/08/01 10:38:44, oshima wrote: > On 2014/08/01 07:59:34, lazyboy wrote: > > rename to g_custom_id_ranges_initialized > > this is file scoped, not global. OK. I have seen a lot g_ identifiers for file level variables. https://codereview.chromium.org/432003007/diff/240001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/432003007/diff/240001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:281: bool custom_id_ranges_initialized = false; nit: prefer declaring variables at the beginning of the namespace. https://codereview.chromium.org/432003007/diff/240001/components/renderer_con... File components/renderer_context_menu/render_view_context_menu_base.h (right): https://codereview.chromium.org/432003007/diff/240001/components/renderer_con... components/renderer_context_menu/render_view_context_menu_base.h:141: // Subclasses should send notification. Just noting as I don't have any concrete suggestions here: These two methods seem too specific to be in RenderViewContextMenuBase interface, but well. https://codereview.chromium.org/432003007/diff/240001/components/renderer_con... components/renderer_context_menu/render_view_context_menu_base.h:146: // TODO(oshima): Remove this. Add a bit more context to this todo.
On 2014/08/01 20:15:54, lazyboy wrote: > LGTM with nits. > > This CL should give you the most you need to use context menu for athena, right? Yes, thank you for reviewing this today! > > https://codereview.chromium.org/432003007/diff/140001/chrome/browser/renderer... > File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): > > https://codereview.chromium.org/432003007/diff/140001/chrome/browser/renderer... > chrome/browser/renderer_context_menu/render_view_context_menu.cc:281: bool > custom_id_ranges_initialized = false; > On 2014/08/01 10:38:44, oshima wrote: > > On 2014/08/01 07:59:34, lazyboy wrote: > > > rename to g_custom_id_ranges_initialized > > > > this is file scoped, not global. > > OK. > I have seen a lot g_ identifiers for file level variables. Yes, that's misuse. Global variables are discouraged an must be used with caution (may be changed in somewhere else even in another read). File scoped variables are yours :) I'll address your commands and will land today. Thanks again! > > https://codereview.chromium.org/432003007/diff/240001/chrome/browser/renderer... > File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): > > https://codereview.chromium.org/432003007/diff/240001/chrome/browser/renderer... > chrome/browser/renderer_context_menu/render_view_context_menu.cc:281: bool > custom_id_ranges_initialized = false; > nit: prefer declaring variables at the beginning of the namespace. > > https://codereview.chromium.org/432003007/diff/240001/components/renderer_con... > File components/renderer_context_menu/render_view_context_menu_base.h (right): > > https://codereview.chromium.org/432003007/diff/240001/components/renderer_con... > components/renderer_context_menu/render_view_context_menu_base.h:141: // > Subclasses should send notification. > Just noting as I don't have any concrete suggestions here: > These two methods seem too specific to be in RenderViewContextMenuBase > interface, but well. > > https://codereview.chromium.org/432003007/diff/240001/components/renderer_con... > components/renderer_context_menu/render_view_context_menu_base.h:146: // > TODO(oshima): Remove this. > Add a bit more context to this todo.
https://codereview.chromium.org/432003007/diff/240001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/432003007/diff/240001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:281: bool custom_id_ranges_initialized = false; On 2014/08/01 20:15:54, lazyboy wrote: > nit: prefer declaring variables at the beginning of the namespace. Done. https://codereview.chromium.org/432003007/diff/240001/components/renderer_con... File components/renderer_context_menu/render_view_context_menu_base.h (right): https://codereview.chromium.org/432003007/diff/240001/components/renderer_con... components/renderer_context_menu/render_view_context_menu_base.h:141: // Subclasses should send notification. On 2014/08/01 20:15:54, lazyboy wrote: > Just noting as I don't have any concrete suggestions here: > These two methods seem too specific to be in RenderViewContextMenuBase > interface, but well. Notification should be replaced observer interface, but it'd require a bit more work. I will work on in a separate CL. https://codereview.chromium.org/432003007/diff/240001/components/renderer_con... components/renderer_context_menu/render_view_context_menu_base.h:146: // TODO(oshima): Remove this. On 2014/08/01 20:15:54, lazyboy wrote: > Add a bit more context to this todo. Done.
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/432003007/260001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/432003007/260001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/432003007/260001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
rbr'ing blundell@ for gyp change in comopnents.gyp (moving gypi)
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/432003007/300001
The CQ bit was unchecked by oshima@chromium.org
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/432003007/300001
lgtm
Message was sent while issue was closed.
Change committed as 287971 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
