IsCommandIdEnabled should not fall though if the base did handle the command id

IsCommandIdEnabled should not fall though if the base did handle the command id.

I missed that RVContextMenu::IsCommandIdEnabled has NOTREACHED() and my refactoring was causing DCHECK failure.
I'll refactor a bit more to make each group modular when lazyboy@ is back.

BUG=401926
TEST=covered by RenderViewContextMenuTest.IsCustomCommandIdEnabled

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

[sky, 2014-08-08 16:36:23]

How about a test case too?

https://codereview.chromium.org/453993002/diff/1/components/renderer_context_...
File components/renderer_context_menu/render_view_context_menu_base.h (right):

https://codereview.chromium.org/453993002/diff/1/components/renderer_context_...
components/renderer_context_menu/render_view_context_menu_base.h:83: bool
GetCommandIdEnabled(int command_id, bool* enabled) const;
The description you have here and the name is mildly confusing. How about:

// Returns true if the specified command id is known and valid for this menu. If
// the command is known |enabled| is set to indicate if the command is enabled.
bool IsCommandIdKnown(int command_id, bool* enabled);

[oshima, 2014-08-08 17:14:17]

uploaded new patch. PTAL.

https://codereview.chromium.org/453993002/diff/1/components/renderer_context_...
File components/renderer_context_menu/render_view_context_menu_base.h (right):

https://codereview.chromium.org/453993002/diff/1/components/renderer_context_...
components/renderer_context_menu/render_view_context_menu_base.h:83: bool
GetCommandIdEnabled(int command_id, bool* enabled) const;
On 2014/08/08 16:36:23, sky wrote:
> The description you have here and the name is mildly confusing. How about:
> 
> // Returns true if the specified command id is known and valid for this menu.
If
> // the command is known |enabled| is set to indicate if the command is
enabled.
> bool IsCommandIdKnown(int command_id, bool* enabled);

Done.

[sky, 2014-08-08 19:15:25]

https://codereview.chromium.org/453993002/diff/20001/components/renderer_cont...
File components/renderer_context_menu/render_view_context_menu_base.h (right):

https://codereview.chromium.org/453993002/diff/20001/components/renderer_cont...
components/renderer_context_menu/render_view_context_menu_base.h:84: bool
GetCommandIdEnabled(int command_id, bool* enabled) const;
Were you going to rename this to IsCommandIdKnown as I suggested?

[oshima, 2014-08-08 20:42:40]

+avi for owner's review.

https://codereview.chromium.org/453993002/diff/20001/components/renderer_cont...
File components/renderer_context_menu/render_view_context_menu_base.h (right):

https://codereview.chromium.org/453993002/diff/20001/components/renderer_cont...
components/renderer_context_menu/render_view_context_menu_base.h:84: bool
GetCommandIdEnabled(int command_id, bool* enabled) const;
On 2014/08/08 19:15:25, sky wrote:
> Were you going to rename this to IsCommandIdKnown as I suggested?

oops, I missed that part. Done.

[Avi (use Gerrit), 2014-08-08 20:57:01]

lgtm

https://codereview.chromium.org/453993002/diff/40001/components/renderer_cont...
File components/renderer_context_menu/render_view_context_menu_base.h (right):

https://codereview.chromium.org/453993002/diff/40001/components/renderer_cont...
components/renderer_context_menu/render_view_context_menu_base.h:83: // if the
command isenabled.
s/isenabled/is enabled/

[oshima, 2014-08-08 21:03:20]

https://codereview.chromium.org/453993002/diff/40001/components/renderer_cont...
File components/renderer_context_menu/render_view_context_menu_base.h (right):

https://codereview.chromium.org/453993002/diff/40001/components/renderer_cont...
components/renderer_context_menu/render_view_context_menu_base.h:83: // if the
command isenabled.
On 2014/08/08 20:57:01, Avi wrote:
> s/isenabled/is enabled/

Done.

[sky, 2014-08-08 21:11:54]

LGTM

[oshima, 2014-08-08 21:21:10]

The CQ bit was checked by oshima@chromium.org

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

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

[oshima, 2014-08-08 22:03:44]

The CQ bit was checked by oshima@chromium.org

[oshima, 2014-08-08 22:03:48]

The CQ bit was unchecked by oshima@chromium.org

[oshima, 2014-08-09 04:41:08]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-08-09 04:43:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/453993002/60001

[commit-bot: I haz the power, 2014-08-09 10:52:58]

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

[commit-bot: I haz the power, 2014-08-09 10:55:31]

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

[commit-bot: I haz the power, 2014-08-09 10:55:32]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[oshima, 2014-08-09 15:30:14]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-08-09 15:31:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/453993002/60001

[commit-bot: I haz the power, 2014-08-09 15:34:02]

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

[commit-bot: I haz the power, 2014-08-09 15:36:53]

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

[commit-bot: I haz the power, 2014-08-09 15:36:54]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[oshima, 2014-08-12 02:02:09]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-08-12 02:10:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/453993002/60001

[commit-bot: I haz the power, 2014-08-12 02:16:49]

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

[commit-bot: I haz the power, 2014-08-12 02:23:29]

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

[commit-bot: I haz the power, 2014-08-12 02:23:30]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[oshima, 2014-08-12 15:39:23]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-08-12 15:41:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/453993002/60001

[commit-bot: I haz the power, 2014-08-12 15:50:41]

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

[commit-bot: I haz the power, 2014-08-12 15:59:30]

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

[commit-bot: I haz the power, 2014-08-12 15:59:31]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[oshima, 2014-08-12 18:26:10]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-08-12 18:27:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/453993002/80001

[commit-bot: I haz the power, 2014-08-12 23:57:07]

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

[commit-bot: I haz the power, 2014-08-13 01:17:07]

Change committed as 289142