Report text output and exit code for command-line operations.

Report text output and exit code for command-line operations.

Allows a client of chrome.exe to observe the text output and numerical exit code of command-line invocations even if the launched process outlives the operation or vice-versa (i.e., the launched process exits immediately but delegates the operation to a previously running process via ProcessSingleton).

app_host.exe uses this functionality to ensure the results of operations can be reported to its clients (via typical console output and process exit code).

BUG=170726, 170734

[erikwright (departed), 2013-03-27 02:34:12]

gab, robert: PTAL.

Note that I still have work to do to implement a good test of OperationLauncher
(using test_operation) as well as tests of the functionality in chrome/browser,
but  I believe the code is otherwise correct and complete.

I don't intend to commit '--greetings' support :)

[erikwright (departed), 2013-03-27 02:34:45]

On 2013/03/27 02:34:12, erikwright wrote:
> gab, robert: PTAL.
> 
> Note that I still have work to do to implement a good test of
OperationLauncher
> (using test_operation) as well as tests of the functionality in
chrome/browser,
> but  I believe the code is otherwise correct and complete.
> 
> I don't intend to commit '--greetings' support :)

(This is CL 3/3).

[robertshield, 2013-03-28 02:38:03]

Working my way through, couple of questions, streaming comments as I go.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/app_host_ma...
File apps/app_host/app_host_main.cc (right):

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/app_host_ma...
apps/app_host/app_host_main.cc:44: // Launch Chrome before checking for update,
for faster user experience.
grammar nit: for -> for a

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
File apps/app_host/operation_launcher.cc (right):

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:74: NULL, 0) == 0) {
I'm curious: how does this work? I had understood that in order for the returned
handles to be inheritable, the security attributes in the 3rd param had to be
non-null. Specifically, should not the bInheritHandle in the SECURITY_ATTRIBUTES
structure be set to true so that the child process gets the handle to
exit_code_write?

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:119: exit_code_write.Close();
Why do we Close() here, rather than doing so when the ScopedHandle goes out of
scope?

[erikwright (departed), 2013-03-28 02:49:52]

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
File apps/app_host/operation_launcher.cc (right):

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:74: NULL, 0) == 0) {
On 2013/03/28 02:38:03, robertshield wrote:
> I'm curious: how does this work? I had understood that in order for the
returned
> handles to be inheritable, the security attributes in the 3rd param had to be
> non-null. Specifically, should not the bInheritHandle in the
SECURITY_ATTRIBUTES
> structure be set to true so that the child process gets the handle to
> exit_code_write?

They are not inherited. Making them inherited would be dangerous because Chrome
could quite feasibly start any number of child processes that would also inherit
them. Then they would never close, which would make it hard to detect the end of
the stream.

There would still be the question, furthermore, of how the new chrome.exe would
pass them into the pre-existing chrome.exe (ProcessSingleton rendezvous case).
Presumably process singleton would need to duplicate them into the pre-existing
chrome process and then change the command line passed to that process.

Furthermore, if they were inherited, I'm not sure we would even know the right
HANDLE to pass to the child process. Is it the same as the HANDLE in the parent
process?

Anyway, what we _do_ do is pass the parent process ID and handles valid in the
parent process. The child can use that info to duplicate the handles into its
own process.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:119: exit_code_write.Close();
On 2013/03/28 02:38:03, robertshield wrote:
> Why do we Close() here, rather than doing so when the ScopedHandle goes out of
> scope?

We will detect the termination of the operation when we can no longer ReadFile()
on the read-end of the pipe.

As long as there is an open HANDLE to the write-end of the pipe, ReadFile will
block. By closing our HANDLE to the write-end, we ensure that we will be able to
detect when the operation handler closes its HANDLE to the write-end.

[gab, 2013-03-28 03:06:05]

Also streaming what I have so far (stopped at the top of chrome_browser_main.cc
for tonight).

Cheers!
Gab

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/DEPS
File apps/app_host/DEPS (right):

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/DEPS#newcode2
apps/app_host/DEPS:2: "+google_update",
Shouldn't google_update be alphabetically ordered after chrome/*?

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/app_host_ma...
File apps/app_host/app_host_main.cc (right):

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/app_host_ma...
apps/app_host/app_host_main.cc:16: 
nit: Remove extra empty line.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/app_host_ma...
apps/app_host/app_host_main.cc:49: LOG(ERROR) << "Failed to delegate to
Chrome.";
Does this need to make it in Release builds or is a NOTREACHED() sufficient
here?

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/app_host_ma...
apps/app_host/app_host_main.cc:50: return 1;
This should be a constant return code, no?

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
File apps/app_host/operation_launcher.cc (right):

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:30: class PipeReader : public
base::PlatformThread::Delegate {
**I have no idea what I'm doing** (never wrote a background thread in
Chromium...), but according to platform_thread.h, you should use Thread from
base/threading/thread.h instead:

"WARNING: You should *NOT* be using this class directly."

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:30: class PipeReader : public
base::PlatformThread::Delegate {
PipeReader sounds a bit too generic for a class with such a specific function
imo

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:32: PipeReader(base::win::ScopedHandle
exit_code_read)
"read" feels like the past-tense of "read" (aaah English!) which doesn't sound
like a handle at all to me.

I understood why you choose that name when making it to line 70, but please
consider my naming suggestion there.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:34: exit_code_read_ = exit_code_read.Pass();
Why can't this also be done in the initializer list?

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:51: return exit_code_;
Such simple getters are usually written on a single-line.

In fact I feel like lines 49-56 could be shrunk to two lines:

DWORD exit_code() { return exit_code_; }
bool success() { return success_; }

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:71: base::win::ScopedHandle exit_code_write;
How about exit_code_in/exit_code_out (see comment on line 32 for justification).

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:74: NULL, 0) == 0) {
How about using '!' instead of '== 0' check? Feels more natural to me.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:75: PLOG(ERROR) << "CreatePipe failed.";
DPLOG (avoids shipping a ton of log strings in the binary; unless you feel
strongly this should make it in user's logs...?)

Same goes for LOG->DLOG all over this CL.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:129: break;
Remove break;, already preceded by a return statement.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
File apps/app_host/operation_launcher.h (right):

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.h:23: // write the operation exit code (as a
binary DWORD).
nit: Indent 2nd line of comment 2 more spaces

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.h:23: // write the operation exit code (as a
binary DWORD).
s/(as a binary DWORD)/(as a DWORD)
?
How can a DWORD be non-binary?!

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.h:40: // Runs the provided command, passing
anonymous pipes to be used for operation
s/command/|command_line/

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.h:41: // output and exit code. To prevent
deadlocks, |output_write| must be drained
Can you add an example of why this could cause a deadlock?

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/test_operat...
File apps/app_host/test_operation.cc (right):

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/test_operat...
apps/app_host/test_operation.cc:15: const char* kDelegateWait = "delegate-wait";
Use char[] instead of char* for literal constants.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/test_operat...
apps/app_host/test_operation.cc:23: ::CreateEvent(NULL, TRUE, FALSE,
L"AppHostTestOperationChildReady"));
Add "Local\" prefix to the event name to make sure the event is created in the
session namespace
(http://msdn.microsoft.com/en-ca/library/windows/desktop/ms682396(v=vs.85).aspx).

I'm also not sure why, but all event names I've seen in Chrome are postfixed
with '!' (ref. process_singleton_win.cc).

(same for other event names below).

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/test_operat...
apps/app_host/test_operation.cc:25: return 1;
Define some error code constant to 1 instead of using "return 1" all over the
code?

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/test_operat...
apps/app_host/test_operation.cc:28: CommandLine::Init(0, NULL);
Feels a bit weird that CommandLine::Init() takes argc+argv, but ignores them on
Windows; the behavior of that method is completely different on POSIX :(.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/test_operat...
apps/app_host/test_operation.cc:60: if (::WaitForSingleObject(child_wait_event,
10000) != WAIT_OBJECT_0)
Constantify 10000 here and above into something like kMaxWait.

https://codereview.chromium.org/12674028/diff/80001/apps/apps.gypi
File apps/apps.gypi (right):

https://codereview.chromium.org/12674028/diff/80001/apps/apps.gypi#newcode115
apps/apps.gypi:115: 'SubSystem': '2',  # Set /SUBSYSTEM:WINDOWS
Curious: what does this do?

https://codereview.chromium.org/12674028/diff/80001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/12674028/diff/80001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:11: #include <limits.h>
C-headers come after C++ headers and are usually in their own "paragraph" I
think.

[robertshield, 2013-03-28 03:39:34]

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
File apps/app_host/operation_launcher.cc (right):

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:74: NULL, 0) == 0) {
On 2013/03/28 02:49:53, erikwright wrote:
> On 2013/03/28 02:38:03, robertshield wrote:
> > I'm curious: how does this work? I had understood that in order for the
> returned
> > handles to be inheritable, the security attributes in the 3rd param had to
be
> > non-null. Specifically, should not the bInheritHandle in the
> SECURITY_ATTRIBUTES
> > structure be set to true so that the child process gets the handle to
> > exit_code_write?
> 
> They are not inherited. Making them inherited would be dangerous because
Chrome
> could quite feasibly start any number of child processes that would also
inherit
> them. Then they would never close, which would make it hard to detect the end
of
> the stream.
> 
> There would still be the question, furthermore, of how the new chrome.exe
would
> pass them into the pre-existing chrome.exe (ProcessSingleton rendezvous case).
> Presumably process singleton would need to duplicate them into the
pre-existing
> chrome process and then change the command line passed to that process.
> 
> Furthermore, if they were inherited, I'm not sure we would even know the right
> HANDLE to pass to the child process. Is it the same as the HANDLE in the
parent
> process?
> 
> Anyway, what we _do_ do is pass the parent process ID and handles valid in the
> parent process. The child can use that info to duplicate the handles into its
> own process.

Ah, sorry should have read the other end first ;-) I saw passing a handle on the
command line to the child process and assumed you were trying inheritance.
Anyhow, carry on. 

Since I wondered about it, perhaps also add a comment that the exit_code_write
handles is intentionally not being inherited.

https://codereview.chromium.org/12674028/diff/80001/chrome/browser/operation_...
File chrome/browser/operation_output.h (right):

https://codereview.chromium.org/12674028/diff/80001/chrome/browser/operation_...
chrome/browser/operation_output.h:6: // definition of an operation.
I don't have a strong preference (a chrome/browser OWNER probably will), but did
you consider sticking the browser part of the operation stuff under
src/chrome/browser/operation?

[robertshield, 2013-03-28 14:31:48]

Few more nits and comments.

https://codereview.chromium.org/12674028/diff/80001/chrome/browser/process_si...
File chrome/browser/process_singleton_lock.cc (right):

https://codereview.chromium.org/12674028/diff/80001/chrome/browser/process_si...
chrome/browser/process_singleton_lock.cc:64: saved_startup_messages_.push_back(
minor nit: since these are only going to be played back once each, consider
deduping at insertion time, rather than when replaying the list.

https://codereview.chromium.org/12674028/diff/80001/chrome/browser/ui/startup...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/12674028/diff/80001/chrome/browser/ui/startup...
chrome/browser/ui/startup/startup_browser_creator.cc:457: }
Is the greetings flag still useful?

https://codereview.chromium.org/12674028/diff/80001/chrome/browser/ui/startup...
File chrome/browser/ui/startup/startup_browser_creator.h (right):

https://codereview.chromium.org/12674028/diff/80001/chrome/browser/ui/startup...
chrome/browser/ui/startup/startup_browser_creator.h:40: // called during actual
process startup.
Nit: ProcessCommandLine -> ProcessCommandLineImpl
...
which leads me to think this sentence is an implementation detail, and shouldn't
be a comment on the interface.

https://codereview.chromium.org/12674028/diff/80001/chrome/common/chrome_swit...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/12674028/diff/80001/chrome/common/chrome_swit...
chrome/common/chrome_switches.cc:1651: const char kTaskResultHandle[]           
  = "task-result-handle";
All other flags here have comments on them, please do the same.  Also, please
add them in alphabetic order as per the comment below (at least within the
OS_WIN section).

[gab, 2013-03-28 14:52:41]

https://codereview.chromium.org/12674028/diff/80001/chrome/browser/process_si...
File chrome/browser/process_singleton_lock.cc (right):

https://codereview.chromium.org/12674028/diff/80001/chrome/browser/process_si...
chrome/browser/process_singleton_lock.cc:64: saved_startup_messages_.push_back(
On 2013/03/28 14:31:48, robertshield wrote:
> minor nit: since these are only going to be played back once each, consider
> deduping at insertion time, rather than when replaying the list.

I thought about this too, the theory being that insert would be O(n) and replay
O(n) as well instead of O(1) and O(n^2) as it is now, but I don't think it has
any benefits in practice (i.e. the choice is basically between deduping against
the full_list here or against the replayed_list on replay; seems to me that the
size of replayed_list is always <= the size of the full_list for any given item;
making dedupping at replay time more efficient (or equal) in terms of overall
cost (inserts+replay).

At least these were my thoughts on patch 2/3 and I don't think things have
changed here (haven't fully grokked this CL yet).

[erikwright (departed), 2013-04-18 17:43:04]

Updated a bit more. I'm going to wait for the upstream patch to land before
fine-tuning and completing the test suite.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/DEPS
File apps/app_host/DEPS (right):

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/DEPS#newcode2
apps/app_host/DEPS:2: "+google_update",
On 2013/03/28 03:06:06, gab wrote:
> Shouldn't google_update be alphabetically ordered after chrome/*?

Done.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/app_host_ma...
File apps/app_host/app_host_main.cc (right):

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/app_host_ma...
apps/app_host/app_host_main.cc:49: LOG(ERROR) << "Failed to delegate to
Chrome.";
On 2013/03/28 03:06:06, gab wrote:
> Does this need to make it in Release builds or is a NOTREACHED() sufficient
> here?

This is not a NOTREACHED scenario. Things out of our control could cause this to
happen.

I think the log message is useful in that case.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
File apps/app_host/operation_launcher.cc (right):

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:30: class PipeReader : public
base::PlatformThread::Delegate {
On 2013/03/28 03:06:06, gab wrote:
> **I have no idea what I'm doing** (never wrote a background thread in
> Chromium...), but according to platform_thread.h, you should use Thread from
> base/threading/thread.h instead:
> 
> "WARNING: You should *NOT* be using this class directly."

That's a good warning, but all classes exist to be used and I think this is an
appropriate place to use that class.

This code runs outside of Chrome and the world of message loops. Furthermore,
this code is intimately tied to the platform-specific behaviour of threads and
anonymous pipes. And finally, this code needs to do something that is only
possible with this closer-to-the-metal abstraction, which is to
WaitOnMultipleObjects.

The alternatives to using PlatformThread would be:
1) Break encapsulation of a higher-level construct such as SimpleThread or
Thread and expose the PlatformThreadHandle
2) Use the raw Win32 ::CreateThread API and friends.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:34: exit_code_read_ = exit_code_read.Pass();
On 2013/03/28 03:06:06, gab wrote:
> Why can't this also be done in the initializer list?

It doesn't compile. I forget the exact reason why, but I believe there are
ambiguous single-argument constructors or something. I'll try to get the error
output if I have a moment.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:51: return exit_code_;
On 2013/03/28 03:06:06, gab wrote:
> Such simple getters are usually written on a single-line.
> 
> In fact I feel like lines 49-56 could be shrunk to two lines:
> 
> DWORD exit_code() { return exit_code_; }
> bool success() { return success_; }

Done.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:74: NULL, 0) == 0) {
On 2013/03/28 03:06:06, gab wrote:
> How about using '!' instead of '== 0' check? Feels more natural to me.

I would defer to a senior Windows developer on this. The current form seems more
correct but I don't feel too strongly.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:75: PLOG(ERROR) << "CreatePipe failed.";
On 2013/03/28 03:06:06, gab wrote:
> DPLOG (avoids shipping a ton of log strings in the binary; unless you feel
> strongly this should make it in user's logs...?)
> 
> Same goes for LOG->DLOG all over this CL.

Done.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.cc:129: break;
On 2013/03/28 03:06:06, gab wrote:
> Remove break;, already preceded by a return statement.

Done.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
File apps/app_host/operation_launcher.h (right):

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.h:23: // write the operation exit code (as a
binary DWORD).
On 2013/03/28 03:06:06, gab wrote:
> s/(as a binary DWORD)/(as a DWORD)
> ?
> How can a DWORD be non-binary?!

I guess I meant in binary format as opposed to being encoded as a string. DWORD
alone did seem ambiguous to me.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.h:23: // write the operation exit code (as a
binary DWORD).
On 2013/03/28 03:06:06, gab wrote:
> nit: Indent 2nd line of comment 2 more spaces

Done.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.h:40: // Runs the provided command, passing
anonymous pipes to be used for operation
On 2013/03/28 03:06:06, gab wrote:
> s/command/|command_line/

Done.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/operation_l...
apps/app_host/operation_launcher.h:41: // output and exit code. To prevent
deadlocks, |output_write| must be drained
On 2013/03/28 03:06:06, gab wrote:
> Can you add an example of why this could cause a deadlock?

Done.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/test_operat...
File apps/app_host/test_operation.cc (right):

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/test_operat...
apps/app_host/test_operation.cc:15: const char* kDelegateWait = "delegate-wait";
On 2013/03/28 03:06:06, gab wrote:
> Use char[] instead of char* for literal constants.

Done.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/test_operat...
apps/app_host/test_operation.cc:23: ::CreateEvent(NULL, TRUE, FALSE,
L"AppHostTestOperationChildReady"));
On 2013/03/28 03:06:06, gab wrote:
> Add "Local\" prefix to the event name to make sure the event is created in the
> session namespace
>
(http://msdn.microsoft.com/en-ca/library/windows/desktop/ms682396%28v=vs.85%29...).
> 
> I'm also not sure why, but all event names I've seen in Chrome are postfixed
> with '!' (ref. process_singleton_win.cc).
> 
> (same for other event names below).

Done.

https://codereview.chromium.org/12674028/diff/80001/apps/app_host/test_operat...
apps/app_host/test_operation.cc:28: CommandLine::Init(0, NULL);
On 2013/03/28 03:06:06, gab wrote:
> Feels a bit weird that CommandLine::Init() takes argc+argv, but ignores them
on
> Windows; the behavior of that method is completely different on POSIX :(.

Agreed.

https://codereview.chromium.org/12674028/diff/80001/apps/apps.gypi
File apps/apps.gypi (right):

https://codereview.chromium.org/12674028/diff/80001/apps/apps.gypi#newcode115
apps/apps.gypi:115: 'SubSystem': '2',  # Set /SUBSYSTEM:WINDOWS
On 2013/03/28 03:06:06, gab wrote:
> Curious: what does this do?

Prevents a console window from being displayed by the app.

"Application does not require a console"
http://msdn.microsoft.com/en-us/library/fcc1zstk.aspx

https://codereview.chromium.org/12674028/diff/80001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/12674028/diff/80001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:11: #include <limits.h>
On 2013/03/28 03:06:06, gab wrote:
> C-headers come after C++ headers and are usually in their own "paragraph" I
> think.

You so crazy :)

C first.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...

I don't think there are any rules about dividing them. That link certainly shows
C and CPP grouped.

https://codereview.chromium.org/12674028/diff/80001/chrome/browser/operation_...
File chrome/browser/operation_output.h (right):

https://codereview.chromium.org/12674028/diff/80001/chrome/browser/operation_...
chrome/browser/operation_output.h:6: // definition of an operation.
On 2013/03/28 03:39:34, robertshield wrote:
> I don't have a strong preference (a chrome/browser OWNER probably will), but
did
> you consider sticking the browser part of the operation stuff under
> src/chrome/browser/operation?

I don't have a strong opinion either. I'll see what Nico thinks.

https://codereview.chromium.org/12674028/diff/80001/chrome/common/chrome_swit...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/12674028/diff/80001/chrome/common/chrome_swit...
chrome/common/chrome_switches.cc:1651: const char kTaskResultHandle[]           
  = "task-result-handle";
On 2013/03/28 14:31:48, robertshield wrote:
> All other flags here have comments on them, please do the same.  Also, please
> add them in alphabetic order as per the comment below (at least within the
> OS_WIN section).

Done.

[gab, 2013-04-19 14:37:47]

sgtm, please ping when ready for re-review.

https://codereview.chromium.org/12674028/diff/80001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/12674028/diff/80001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main.cc:11: #include <limits.h>
On 2013/04/18 17:43:04, erikwright wrote:
> On 2013/03/28 03:06:06, gab wrote:
> > C-headers come after C++ headers and are usually in their own "paragraph" I
> > think.
> 
> You so crazy :)

You know who I learnt to code-review from ;)!

> 
> C first.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...
> 
> I don't think there are any rules about dividing them. That link certainly
shows
> C and CPP grouped.

ok, sgtm :)

[gab, 2013-11-19 13:03:53]

ping? Shall we close this?