Turn off Task Manager should not break the build on Linux

Turn off Task Manager should not break the build on Linux

BUG=355781
R=

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

[miket_OOO, 2014-03-24 23:23:26]

https://codereview.chromium.org/209353016/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/209353016/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/processes/processes_api.cc:675: #endif  //
defined(ENABLE_TASK_MANAGER)
Should there be a SendResponse() here as well?

https://codereview.chromium.org/209353016/diff/1/chrome/browser/ui/browser_co...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/209353016/diff/1/chrome/browser/ui/browser_co...
chrome/browser/ui/browser_commands.cc:1028: #endif  //
defined(ENABLE_TASK_MANAGER)
I can't tell for sure from here, but is this better as an #else NOT_REACHED()?
You should be able to inspect the code and determine that if
CanOpenTaskManager() is hard-coded false, then no code should ever call
OpenTaskManager().

[yi, 2014-03-24 23:38:11]

Thanks for the review, mlket. I have updated the patch based on your suggestion.

[miket_OOO, 2014-03-24 23:48:08]

lgtm

[yi, 2014-03-25 00:01:11]

The CQ bit was checked by yi.shen@samsung.com

[commit-bot: I haz the power, 2014-03-25 00:03:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yi.shen@samsung.com/209353016/20001

[commit-bot: I haz the power, 2014-03-25 00:50:12]

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

[commit-bot: I haz the power, 2014-03-25 00:50:12]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[yi, 2014-03-25 16:45:11]

On 2014/03/24 23:48:08, miket wrote:
> lgtm

mlket, I got following errors from Trybot. Could you please help to check with
it?

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/ui/browser_commands.cc

http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...

[miket_OOO, 2014-03-25 16:56:52]

I can't help you further than the error message. You need to get an approval
from a person listed as an owner. Apparently I'm not one of those people.

[yi, 2014-03-25 17:00:11]

On 2014/03/25 16:56:52, miket wrote:
> I can't help you further than the error message. You need to get an approval
> from a person listed as an owner. Apparently I'm not one of those people.

Thank you, Mlket!

Peter, could you help me review this patch? Thanks in advanced!

[Peter Kasting, 2014-03-25 20:48:43]

https://codereview.chromium.org/209353016/diff/20001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/209353016/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands.cc:1014: #endif  //
defined(ENABLE_TASK_MANAGER)
You can't do this; this will trigger an unreachable code warning in MSVC if
ENABLE_TASK_MANAGER is not defined, once that warning is turned on.

Instead of the #endif you have, you need to wrap the subsequent code in an
#else.

Nit: Don't bother with the EOL comments here and below, these #ifs are short and
the comments just add noise.

[yi, 2014-03-25 21:29:04]

On 2014/03/25 20:48:43, Peter Kasting wrote:
>
https://codereview.chromium.org/209353016/diff/20001/chrome/browser/ui/browse...
> File chrome/browser/ui/browser_commands.cc (right):
> 
>
https://codereview.chromium.org/209353016/diff/20001/chrome/browser/ui/browse...
> chrome/browser/ui/browser_commands.cc:1014: #endif  //
> defined(ENABLE_TASK_MANAGER)
> You can't do this; this will trigger an unreachable code warning in MSVC if
> ENABLE_TASK_MANAGER is not defined, once that warning is turned on.
> 
> Instead of the #endif you have, you need to wrap the subsequent code in an
> #else.
> 
> Nit: Don't bother with the EOL comments here and below, these #ifs are short
and
> the comments just add noise.

Thanks for review. I have updated the patch.

[Peter Kasting, 2014-03-25 21:30:11]

LGTM

https://codereview.chromium.org/209353016/diff/90001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/209353016/diff/90001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands.cc:1030: #endif  //
defined(ENABLE_TASK_MANAGER)
Nit: No need for this EOL comment

[yi, 2014-03-25 21:35:41]

On 2014/03/25 21:30:11, Peter Kasting wrote:
> LGTM
> 
>
https://codereview.chromium.org/209353016/diff/90001/chrome/browser/ui/browse...
> File chrome/browser/ui/browser_commands.cc (right):
> 
>
https://codereview.chromium.org/209353016/diff/90001/chrome/browser/ui/browse...
> chrome/browser/ui/browser_commands.cc:1030: #endif  //
> defined(ENABLE_TASK_MANAGER)
> Nit: No need for this EOL comment

Thank you!

[yi, 2014-03-25 21:35:53]

The CQ bit was checked by yi.shen@samsung.com

[commit-bot: I haz the power, 2014-03-25 21:38:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yi.shen@samsung.com/209353016/120001

[commit-bot: I haz the power, 2014-03-25 21:57:23]

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

[commit-bot: I haz the power, 2014-03-25 21:57:23]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[yi, 2014-03-26 00:52:53]

The CQ bit was checked by yi.shen@samsung.com

[yi, 2014-03-26 00:52:54]

The CQ bit was unchecked by yi.shen@samsung.com

[yi, 2014-03-26 00:53:01]

The CQ bit was checked by yi.shen@samsung.com

[commit-bot: I haz the power, 2014-03-26 00:55:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yi.shen@samsung.com/209353016/120001

[commit-bot: I haz the power, 2014-03-26 04:09:06]

Change committed as 259468