Wire up process launch error codes.

Wire up process launch error codes.

Return error code from StartSandboxedProcess.

Pass error code through to metrics.

BUG=526198
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng

Committed: https://crrev.com/3adf87d1226c74d593205946ff499d9fcffa3dfc
Cr-Commit-Position: refs/heads/master@{#391392}

[Will Harris, 2016-04-27 21:47:18]

Description was changed from

==========
Return error code from StartSandboxedProcess

BUG=526198
==========

to

==========
Return error code from StartSandboxedProcess

BUG=526198
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[Will Harris, 2016-04-28 02:51:23]

Description was changed from

==========
Return error code from StartSandboxedProcess

BUG=526198
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Wire up process launch error codes.

Return error code from StartSandboxedProcess.

Pass error code through to metrics.

BUG=526198
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[Will Harris, 2016-04-28 02:51:45]

wfh@chromium.org changed reviewers:
+ jam@chromium.org

[jam, 2016-04-28 15:41:14]

https://codereview.chromium.org/1923653002/diff/120001/content/browser/child_...
File content/browser/child_process_launcher.h (right):

https://codereview.chromium.org/1923653002/diff/120001/content/browser/child_...
content/browser/child_process_launcher.h:29: // For Windows, these are
sandbox::ResultCode starting at offset 1000 located in
normally this is done by having the lower layer set an enum for max value it
needs (with some room to leave for further growth if it's in UMA). then content
etc can start its values there. i understand sandbox' enum is only on windows,
but it seems fine to have this enum here have an ifdef to start it at that value
(1000) on windows, otherwise it starts at 0. that way sandbox doesn't have to
keep the assumption that content etc won't use more than a thousand values.

https://codereview.chromium.org/1923653002/diff/120001/content/public/browser...
File content/public/browser/utility_process_host_client.h (right):

https://codereview.chromium.org/1923653002/diff/120001/content/public/browser...
content/public/browser/utility_process_host_client.h:27: // specific error code
for why the process failed to launch.
it's confusing looking at this api what "platform specific" means. in the
context of this cl, it's obvious, but someone looking later won't know.

why not have this be a typedef?

[Will Harris, 2016-05-02 16:45:24]

I left the error_code as an int so it can consume both typedefs.

PTAL

https://codereview.chromium.org/1923653002/diff/120001/content/browser/child_...
File content/browser/child_process_launcher.h (right):

https://codereview.chromium.org/1923653002/diff/120001/content/browser/child_...
content/browser/child_process_launcher.h:29: // For Windows, these are
sandbox::ResultCode starting at offset 1000 located in
On 2016/04/28 15:41:14, jam wrote:
> normally this is done by having the lower layer set an enum for max value it
> needs (with some room to leave for further growth if it's in UMA). then
content
> etc can start its values there. i understand sandbox' enum is only on windows,
> but it seems fine to have this enum here have an ifdef to start it at that
value
> (1000) on windows, otherwise it starts at 0. that way sandbox doesn't have to
> keep the assumption that content etc won't use more than a thousand values.

I now start the enum in the lower layer, and then continue it up in content.

I added a static assert to verify that the ranges can never overlap, since I
can't just start at the end of sandbox (since that's a Win only header file).

https://codereview.chromium.org/1923653002/diff/120001/content/public/browser...
File content/public/browser/utility_process_host_client.h (right):

https://codereview.chromium.org/1923653002/diff/120001/content/public/browser...
content/public/browser/utility_process_host_client.h:27: // specific error code
for why the process failed to launch.
On 2016/04/28 15:41:14, jam wrote:
> it's confusing looking at this api what "platform specific" means. in the
> context of this cl, it's obvious, but someone looking later won't know.
> 
> why not have this be a typedef?

updated this comment to specify the typedefs used. I can't use the typedef as
there's two different ones so using int.

[jam, 2016-05-02 20:54:59]

lgtm

[Will Harris, 2016-05-02 21:00:31]

wfh@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Will Harris, 2016-05-02 21:01:09]

asvitkine@ can you review histograms.xml and components/metrics please?

[Alexei Svitkine (slow), 2016-05-03 17:06:57]

lgtm % comment

https://codereview.chromium.org/1923653002/diff/200001/sandbox/win/src/sandbo...
File sandbox/win/src/sandbox_types.h (left):

https://codereview.chromium.org/1923653002/diff/200001/sandbox/win/src/sandbo...
sandbox/win/src/sandbox_types.h:49: SBOX_ERROR_CREATE_PROCESS = 18,
Nit: For a long list like this, I actually find the explicit numbers helpful to
verify that histograms.xml and these definitions match.

It also makes it harder for someone to accidentally stick a new value in the
middle and renumber everything. So please restore them.

[Will Harris, 2016-05-03 17:26:52]

Thanks for review.

https://codereview.chromium.org/1923653002/diff/200001/sandbox/win/src/sandbo...
File sandbox/win/src/sandbox_types.h (left):

https://codereview.chromium.org/1923653002/diff/200001/sandbox/win/src/sandbo...
sandbox/win/src/sandbox_types.h:49: SBOX_ERROR_CREATE_PROCESS = 18,
On 2016/05/03 17:06:57, Alexei Svitkine wrote:
> Nit: For a long list like this, I actually find the explicit numbers helpful
to
> verify that histograms.xml and these definitions match.
> 
> It also makes it harder for someone to accidentally stick a new value in the
> middle and renumber everything. So please restore them.

Done.

[Will Harris, 2016-05-03 17:26:58]

The CQ bit was checked by wfh@chromium.org

[Will Harris, 2016-05-03 17:26:59]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1923653002/#ps220001
(title: "code review comments")

[commit-bot: I haz the power, 2016-05-03 17:27:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1923653002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1923653002/220001

[Will Harris, 2016-05-03 18:11:53]

The CQ bit was unchecked by wfh@chromium.org

[Will Harris, 2016-05-03 20:01:06]

Patchset #14 (id:260001) has been deleted

[Will Harris, 2016-05-03 20:01:11]

Patchset #13 (id:240001) has been deleted

[Will Harris, 2016-05-03 21:03:58]

The CQ bit was checked by wfh@chromium.org

[Will Harris, 2016-05-03 21:03:59]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1923653002/#ps280001
(title: "fix debug and clang")

[commit-bot: I haz the power, 2016-05-03 21:04:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1923653002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1923653002/280001

[commit-bot: I haz the power, 2016-05-03 23:26:17]

Description was changed from

==========
Wire up process launch error codes.

Return error code from StartSandboxedProcess.

Pass error code through to metrics.

BUG=526198
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Wire up process launch error codes.

Return error code from StartSandboxedProcess.

Pass error code through to metrics.

BUG=526198
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[commit-bot: I haz the power, 2016-05-03 23:26:18]

Committed patchset #13 (id:280001)

[commit-bot: I haz the power, 2016-05-03 23:27:37]

Description was changed from

==========
Wire up process launch error codes.

Return error code from StartSandboxedProcess.

Pass error code through to metrics.

BUG=526198
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Wire up process launch error codes.

Return error code from StartSandboxedProcess.

Pass error code through to metrics.

BUG=526198
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng

Committed: https://crrev.com/3adf87d1226c74d593205946ff499d9fcffa3dfc
Cr-Commit-Position: refs/heads/master@{#391392}
==========

[commit-bot: I haz the power, 2016-05-03 23:27:39]

Patchset 13 (id:??) landed as
https://crrev.com/3adf87d1226c74d593205946ff499d9fcffa3dfc
Cr-Commit-Position: refs/heads/master@{#391392}