Clean up RenderViewContextMenu a bit.

Clean up RenderViewContextMenu a bit.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291352

[lazyboy, 2014-08-17 15:23:28]

Sorry I'm OOO and slow to respond, you can pick another reviewer if I'm
unresponsive again.

Couple of comments/questions, otherwise looks great.

https://chromiumcodereview.appspot.com/468913003/diff/1/chrome/browser/render...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://chromiumcodereview.appspot.com/468913003/diff/1/chrome/browser/render...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:301: bool
g_custom_id_ranges_initialized = false;
I think we should keep it as is since this is not a global, rather a file level
var,
oshima@ made this w/o g_ prefix for this reason..

https://chromiumcodereview.appspot.com/468913003/diff/1/chrome/browser/render...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:347: const
MenuItem::ContextList& contexts,
FYI, ContextList provides a copy constructor, this probably was a copy (of a
uint32) for that reason.
I don't know the performance implication of either, can you provide a quick
explanation of this change?

[Lei Zhang, 2014-08-18 22:56:25]

No rush. Take your time.

https://chromiumcodereview.appspot.com/468913003/diff/1/chrome/browser/render...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://chromiumcodereview.appspot.com/468913003/diff/1/chrome/browser/render...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:301: bool
g_custom_id_ranges_initialized = false;
On 2014/08/17 15:23:28, lazyboy (OOO Aug1 - Aug19) wrote:
> I think we should keep it as is since this is not a global, rather a file
level
> var,
> oshima@ made this w/o g_ prefix for this reason..

File-level globals are still globals.

https://chromiumcodereview.appspot.com/468913003/diff/1/chrome/browser/render...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:347: const
MenuItem::ContextList& contexts,
On 2014/08/17 15:23:28, lazyboy (OOO Aug1 - Aug19) wrote:
> FYI, ContextList provides a copy constructor, this probably was a copy (of a
> uint32) for that reason.
> I don't know the performance implication of either, can you provide a quick
> explanation of this change?

I don't see a reason to make a copy. Also, when we pass by reference, it should
be const.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere...

[lazyboy, 2014-08-21 18:24:56]

lgtm

https://chromiumcodereview.appspot.com/468913003/diff/1/chrome/browser/render...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://chromiumcodereview.appspot.com/468913003/diff/1/chrome/browser/render...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:301: bool
g_custom_id_ranges_initialized = false;
On 2014/08/18 22:56:25, Lei Zhang wrote:
> On 2014/08/17 15:23:28, lazyboy (OOO Aug1 - Aug19) wrote:
> > I think we should keep it as is since this is not a global, rather a file
> level
> > var,
> > oshima@ made this w/o g_ prefix for this reason..
> 
> File-level globals are still globals.

OK.

[Lei Zhang, 2014-08-21 21:35:20]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-08-21 21:36:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thestig@chromium.org/468913003/1

[commit-bot: I haz the power, 2014-08-22 01:00:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-22 01:41:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-22 01:41:26]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Lei Zhang, 2014-08-22 02:42:04]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-08-22 02:43:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thestig@chromium.org/468913003/40001

[commit-bot: I haz the power, 2014-08-22 03:39:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-22 04:37:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-22 04:37:05]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Lei Zhang, 2014-08-22 04:52:55]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-08-22 04:53:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thestig@chromium.org/468913003/40001

[commit-bot: I haz the power, 2014-08-22 05:48:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Lei Zhang, 2014-08-22 07:22:49]

The CQ bit was unchecked by thestig@chromium.org

[Lei Zhang, 2014-08-22 07:22:55]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-08-22 07:26:09]

Committed patchset #2 (40001) as 291352