/C++ readability/ - Make Chromoting Host report crashes to Breakpad (Windows only). The user must e…

C++ readability review fixes for r141239: Make Chromoting Host report crashes to Breakpad (Windows only).

BUG=130678
TEST=remoting_unittests.BreakpadWinDeathTest

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

[Peter Kasting, 2012-06-14 20:16:27]

This looks fairly good, most of the comments below are minor.

In some cases I only noted something in one or a few places, but there are more
cases in the rest of the change.  Please make sure to address all changes
globally.

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad.h
File remoting/base/breakpad.h (right):

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad.h#newco...
remoting/base/breakpad.h:14: // thing in main()) to catch crashes occured during
the process startup.
Nit: s/occured/occurring/ ; s/the//

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad.h#newco...
remoting/base/breakpad.h:15: // The crashes occurred while invoking the static
objects' constructors will not
Nit: How about "Crashes which occur during the global static construction phase
will not..."

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad.h#newco...
remoting/base/breakpad.h:16: // be caught and reported. This should not be a
problem as the static non POD
Nit: s/the// ; s/non POD/non-POD/

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_linux.cc
File remoting/base/breakpad_linux.cc (right):

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_linux.c...
remoting/base/breakpad_linux.cc:10: // Crash dump collection is not implemented
on Linux yet.
For temporary comments like this, use TODOs as noted in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=TODO_C...
:

// TODO(alexeypa): Implement crash dump collection on Linux; see
http://crbug.com/130678 .

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_mac.mm
File remoting/base/breakpad_mac.mm (right):

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_mac.mm#...
remoting/base/breakpad_mac.mm:11: // awakeFromNib method.
Nit: Is it possible to point to where that method is defined?  ("...initialized
in awakeFromNib(); see src/blah/foo/x.mm.")

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_win.cc
File remoting/base/breakpad_win.cc (right):

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_win.cc#...
remoting/base/breakpad_win.cc:6: // handler. This implementation is based on
Chrome/Crome Frame crash reporitng
Nit: s/reporitng/reporting/

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_win.cc#...
remoting/base/breakpad_win.cc:61: static BreakpadWin& GetInstance();
Nit: While I'm not sure non-const refs are banned as return types (as they are
as function types), they're less common in Google code; consider returning a
pointer instead.

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_win.cc#...
remoting/base/breakpad_win.cc:74: static bool OnExceptionCallback(void*,
EXCEPTION_POINTERS*,
Nit: One arg per line; name the arguments.

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_win.cc#...
remoting/base/breakpad_win.cc:110: DWORD length = ::GetTempPath(MAX_PATH,
temp_directory);
Nit: Is :: necessary?

Also consider using GetTempDir from base/file_util_win.cc, though this might not
make your code any simpler or safer.

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_win.cc#...
remoting/base/breakpad_win.cc:155: string16 version;
This should be a wstring (and you should call UTF16ToWide() on
product_version()).

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_win.cc#...
remoting/base/breakpad_win.cc:209: // Touch the object to make sure it is
initialized.
Nit: For maintenance safety, I suggest calling InitializeCrashReporting() here
instead (and ommitting the comment), in case later someon wants to add a step to
that function.

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_win_uni...
File remoting/base/breakpad_win_unittest.cc (right):

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_win_uni...
remoting/base/breakpad_win_unittest.cc:19: namespace remoting {
Nit: Does the anonymous namespace need to be inside the remoting namespace?

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_win_uni...
remoting/base/breakpad_win_unittest.cc:35: virtual ~MockCrashServerCallbacks()
{}
Nit: Even in tests, try to avoid inlining functions into the class declaration
unless you would inline them under normal circumstances.  This helps keep the
class declaration shorter and simpler to read.  In this case, you should at
least not inline the destructor (since it's virtual) and
OnClientDumpRequestCallback() (since it's not a simple accessor).

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_win_uni...
remoting/base/breakpad_win_unittest.cc:67: pipe_name_ = UTF8ToUTF16(pipe_name);
Use UTF8ToWide() here (see below).

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_win_uni...
remoting/base/breakpad_win_unittest.cc:73: RPC_STATUS status =
::UuidCreate(&guid);
Nit: Is the :: really necessary?

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_win_uni...
remoting/base/breakpad_win_unittest.cc:76: pipe_name_ = kPipeNamePrefix +
Nit: If using StringPrintf() anyway, add a "%s" to the front of the format
string and supply |kPipeNamePrefix| as the first subsequent arg.

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_win_uni...
remoting/base/breakpad_win_unittest.cc:84: UTF16ToUTF8(pipe_name_));
Use WideToUTF8 here (see below).

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_win_uni...
remoting/base/breakpad_win_unittest.cc:85: EXPECT_TRUE(result);
Nit: Unlike with DCHECK(), it's OK to directly wrap function calls in ASSERT or
EXPECT checks without using a temp to store the call result.

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_win_uni...
remoting/base/breakpad_win_unittest.cc:91: pipe_name_, NULL,
Nit: Strange wrapping here.  More common choices are "as many args on each line
as will fit" and "one arg on each line".

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_win_uni...
remoting/base/breakpad_win_unittest.cc:106: string16 pipe_name_;
This should be a wstring.

http://codereview.chromium.org/10535082/diff/1/remoting/base/breakpad_win_uni...
remoting/base/breakpad_win_unittest.cc:115:
ASSERT_DEATH(*reinterpret_cast<int*>(0) = 1, "");
Nit: 0 -> NULL?

http://codereview.chromium.org/10535082/diff/1/remoting/host/breakpad.h
File remoting/host/breakpad.h (right):

http://codereview.chromium.org/10535082/diff/1/remoting/host/breakpad.h#newcode6
remoting/host/breakpad.h:6: #define REMOTING_HOST_BREAKPAD_H_
I'm not a big fan of having two headers named identically in parallel
subdirectories.  Not only is it confusing which one to use, but this makes it
harder to e.g. "open breakpad.h" in the Visual Studio command prompt and get the
right one.

Can we combine these into one header, or rename them and make it clear why they
should be separate?

(The same goes for breakpad_win.cc.)

http://codereview.chromium.org/10535082/diff/1/remoting/host/breakpad_win.cc
File remoting/host/breakpad_win.cc (right):

http://codereview.chromium.org/10535082/diff/1/remoting/host/breakpad_win.cc#...
remoting/host/breakpad_win.cc:17: // the user's consent to collect crash dumps
is recorded.
Nit: "...where we record whether the user has consented to crash dump
collection."?

http://codereview.chromium.org/10535082/diff/1/remoting/host/breakpad_win.cc#...
remoting/host/breakpad_win.cc:31: string16 client_state =
StringPrintf(kOmahaClientStateKeyFormat,
|client_state| should be a std::wstring given the types used by StringPrintf()
and RegKey::Open() (and you can replace the string16.h include with <string>).

Nit: Consider factoring this block out into a helper that takes a const
wchar_t[] (the key name), so you can simply call it twice from this function.

http://codereview.chromium.org/10535082/diff/1/remoting/host/breakpad_win.cc#...
remoting/host/breakpad_win.cc:39: if (result == ERROR_SUCCESS) {
Nit: If you like, you may omit {} on one-line conditional bodies (though either
way is fine).

http://codereview.chromium.org/10535082/diff/1/remoting/host/constants.h
File remoting/host/constants.h (right):

http://codereview.chromium.org/10535082/diff/1/remoting/host/constants.h#newc...
remoting/host/constants.h:8: #include "base/string16.h"
Nit: #ifdef this as well.  I suggest moving the block below up to here so you
can use a single #if for both.

http://codereview.chromium.org/10535082/diff/1/remoting/host/constants.h#newc...
remoting/host/constants.h:30: 
Nit: Extra blank line here and below.  There aren't definitive rules on this,
but the style guide examples generally place blank lines inside most namespace
definitions, and not inside most preprocessor directive blocks.

http://codereview.chromium.org/10535082/diff/1/remoting/host/constants.h#newc...
remoting/host/constants.h:32: extern const char16 kHostOmahaAppid[];
This should be a wchar_t rather than a string16, given how it will be used in
breakpad_win.cc.

http://codereview.chromium.org/10535082/diff/1/remoting/host/constants_win.cc
File remoting/host/constants_win.cc (right):

http://codereview.chromium.org/10535082/diff/1/remoting/host/constants_win.cc...
remoting/host/constants_win.cc:11: #if defined(OS_WIN)
Is this directive needed?  This is already a _win.cc file.

http://codereview.chromium.org/10535082/diff/1/remoting/host/constants_win.cc...
remoting/host/constants_win.cc:17:
TO_L_STRING("{b210701e-ffc4-49e3-932b-370728c72662}");
Just use L directly here, rather than TO_L_STRING, whose main utility is
stringizing preprocessor directives.

http://codereview.chromium.org/10535082/diff/1/remoting/host/elevated_control...
File remoting/host/elevated_controller_module_win.cc (right):

http://codereview.chromium.org/10535082/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_module_win.cc:34: // Initializes the crash
dump reports.
Nit: This comment doesn't add anything to the code.  Just remove it.

http://codereview.chromium.org/10535082/diff/1/remoting/host/plugin/daemon_in...
File remoting/host/plugin/daemon_installer_win.cc (right):

http://codereview.chromium.org/10535082/diff/1/remoting/host/plugin/daemon_in...
remoting/host/plugin/daemon_installer_win.cc:25: }  // namespace omaha
No other files in the project which #include this file put it in a namespace. 
Is this truly necessary?

I ask this because if we can eliminate this, the correct ordering of the
#includes (including the one you're adding) becomes clearer.

http://codereview.chromium.org/10535082/diff/1/remoting/host/plugin/daemon_in...
remoting/host/plugin/daemon_installer_win.cc:35: const char16
kOmahaElevationMoniker[] =
This file contains many instances of problem's I've suggested correcting in your
new code elsewhere (use of char16 and string16 where wchar_t/wstring are more
correct, use of stringize macros instead of L).  Fixing all these may be beyond
the scope of this change.  Consider filing a bug to clean these up.

[alexeypa (please no reviews), 2012-06-15 18:45:49]

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad.h
File remoting/base/breakpad.h (right):

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad.h:14: // thing in main()) to catch crashes occured during
the process startup.
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: s/occured/occurring/ ; s/the//

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad.h:15: // The crashes occurred while invoking the static
objects' constructors will not
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: How about "Crashes which occur during the global static construction
phase
> will not..."

Yes. This sounds better.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad.h:16: // be caught and reported. This should not be a
problem as the static non POD
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: s/the// ; s/non POD/non-POD/

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
File remoting/base/breakpad_linux.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_linux.cc:10: // Crash dump collection is not implemented
on Linux yet.
On 2012/06/14 20:16:27, Peter Kasting wrote:
> For temporary comments like this, use TODOs as noted in
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=TODO_C...
> :
> 
> // TODO(alexeypa): Implement crash dump collection on Linux; see
> http://crbug.com/130678 .

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
File remoting/base/breakpad_mac.mm (right):

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_mac.mm:11: // awakeFromNib method.
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: Is it possible to point to where that method is defined? 
("...initialized
> in awakeFromNib(); see src/blah/foo/x.mm.")

awakeFromNib() belongs to an object that controls life time of the Breakpad
object. I'd put this to the comment but it seems that awakeFromNib() is not the
only way how it can be done. So I'll put a generic "Implement crash dump
collection on Mac" instead.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
File remoting/base/breakpad_win.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_win.cc:6: // handler. This implementation is based on
Chrome/Crome Frame crash reporitng
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: s/reporitng/reporting/

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_win.cc:61: static BreakpadWin& GetInstance();
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: While I'm not sure non-const refs are banned as return types (as they are
> as function types), they're less common in Google code; consider returning a
> pointer instead.

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_win.cc:74: static bool OnExceptionCallback(void*,
EXCEPTION_POINTERS*,
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: One arg per line; name the arguments.

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_win.cc:110: DWORD length = ::GetTempPath(MAX_PATH,
temp_directory);
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: Is :: necessary?

It is a common pattern in Chromium code. This is however a Windows-only file, so
I don't think it makes much sense.

> Also consider using GetTempDir from base/file_util_win.cc, though this might
not
> make your code any simpler or safer.

The reason not to use GetTempDir from base is to run as little code as we can
before all the hooks are installed. This way we minimize the chance of crashing
before we can report it.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_win.cc:155: string16 version;
On 2012/06/14 20:16:27, Peter Kasting wrote:
> This should be a wstring (and you should call UTF16ToWide() on
> product_version()).

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_win.cc:209: // Touch the object to make sure it is
initialized.
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: For maintenance safety, I suggest calling InitializeCrashReporting() here
> instead (and ommitting the comment), in case later someon wants to add a step
to
> that function.

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
File remoting/base/breakpad_win_unittest.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_win_unittest.cc:19: namespace remoting {
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: Does the anonymous namespace need to be inside the remoting namespace?

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_win_unittest.cc:35: virtual ~MockCrashServerCallbacks()
{}
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: Even in tests, try to avoid inlining functions into the class declaration
> unless you would inline them under normal circumstances.  This helps keep the
> class declaration shorter and simpler to read.  In this case, you should at
> least not inline the destructor (since it's virtual) and
> OnClientDumpRequestCallback() (since it's not a simple accessor).

Done. I removed inlining of the constructor as well to match the style used
elsewhere in the code.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_win_unittest.cc:67: pipe_name_ = UTF8ToUTF16(pipe_name);
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Use UTF8ToWide() here (see below).

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_win_unittest.cc:73: RPC_STATUS status =
::UuidCreate(&guid);
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: Is the :: really necessary?

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_win_unittest.cc:76: pipe_name_ = kPipeNamePrefix +
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: If using StringPrintf() anyway, add a "%s" to the front of the format
> string and supply |kPipeNamePrefix| as the first subsequent arg.

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_win_unittest.cc:84: UTF16ToUTF8(pipe_name_));
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Use WideToUTF8 here (see below).

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_win_unittest.cc:85: EXPECT_TRUE(result);
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: Unlike with DCHECK(), it's OK to directly wrap function calls in ASSERT
or
> EXPECT checks without using a temp to store the call result.

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_win_unittest.cc:91: pipe_name_, NULL,
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: Strange wrapping here.  More common choices are "as many args on each
line
> as will fit" and "one arg on each line".

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_win_unittest.cc:106: string16 pipe_name_;
On 2012/06/14 20:16:27, Peter Kasting wrote:
> This should be a wstring.

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_win_unittest.cc:115:
ASSERT_DEATH(*reinterpret_cast<int*>(0) = 1, "");
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: 0 -> NULL?

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/breakpad.h
File remoting/host/breakpad.h (right):

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/breakpad...
remoting/host/breakpad.h:6: #define REMOTING_HOST_BREAKPAD_H_
On 2012/06/14 20:16:27, Peter Kasting wrote:
> I'm not a big fan of having two headers named identically in parallel
> subdirectories.  Not only is it confusing which one to use, but this makes it
> harder to e.g. "open breakpad.h" in the Visual Studio command prompt and get
the
> right one.
> 
> Can we combine these into one header, or rename them and make it clear why
they
> should be separate?
> 
> (The same goes for breakpad_win.cc.)

Agree. It makes sense to rename host/breakpad*.* into
host/usage_stats_consent*.*. Note that there is a later CL that renames
CrashReporting into UsageStats for consistency reasons.

There is also a similar issue with constants.* which is going to be consolidated
by a separate CL.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/breakpad...
File remoting/host/breakpad_win.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/breakpad...
remoting/host/breakpad_win.cc:17: // the user's consent to collect crash dumps
is recorded.
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: "...where we record whether the user has consented to crash dump
> collection."?

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/breakpad...
remoting/host/breakpad_win.cc:31: string16 client_state =
StringPrintf(kOmahaClientStateKeyFormat,
On 2012/06/14 20:16:27, Peter Kasting wrote:
> |client_state| should be a std::wstring given the types used by StringPrintf()
> and RegKey::Open() (and you can replace the string16.h include with <string>).
> 
> Nit: Consider factoring this block out into a helper that takes a const
> wchar_t[] (the key name), so you can simply call it twice from this function.

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/breakpad...
remoting/host/breakpad_win.cc:39: if (result == ERROR_SUCCESS) {
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: If you like, you may omit {} on one-line conditional bodies (though
either
> way is fine).

I tend to keep {} even for one-liners because they outline the boundaries of the
condition statement better.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/constants.h
File remoting/host/constants.h (right):

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/constant...
remoting/host/constants.h:8: #include "base/string16.h"
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: #ifdef this as well.  I suggest moving the block below up to here so you
> can use a single #if for both.

I got rid of string16 entirely. 

BTW I don't think I could move this include below. The guide says that platform
specific includes should go right after regular includes:

"For platform-specific #includes, put them below the "regular" includes,
separated with a blank line."

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/constant...
remoting/host/constants.h:30: 
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: Extra blank line here and below.  There aren't definitive rules on this,
> but the style guide examples generally place blank lines inside most namespace
> definitions, and not inside most preprocessor directive blocks.

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/constant...
remoting/host/constants.h:32: extern const char16 kHostOmahaAppid[];
On 2012/06/14 20:16:27, Peter Kasting wrote:
> This should be a wchar_t rather than a string16, given how it will be used in
> breakpad_win.cc.

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/constant...
File remoting/host/constants_win.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/constant...
remoting/host/constants_win.cc:11: #if defined(OS_WIN)
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Is this directive needed?  This is already a _win.cc file.

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/constant...
remoting/host/constants_win.cc:17:
TO_L_STRING("{b210701e-ffc4-49e3-932b-370728c72662}");
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Just use L directly here, rather than TO_L_STRING, whose main utility is
> stringizing preprocessor directives.

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/elevated...
File remoting/host/elevated_controller_module_win.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/elevated...
remoting/host/elevated_controller_module_win.cc:34: // Initializes the crash
dump reports.
On 2012/06/14 20:16:27, Peter Kasting wrote:
> Nit: This comment doesn't add anything to the code.  Just remove it.

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/plugin/d...
File remoting/host/plugin/daemon_installer_win.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/plugin/d...
remoting/host/plugin/daemon_installer_win.cc:25: }  // namespace omaha
On 2012/06/14 20:16:27, Peter Kasting wrote:
> No other files in the project which #include this file put it in a namespace. 
> Is this truly necessary?
> 
> I ask this because if we can eliminate this, the correct ordering of the
> #includes (including the one you're adding) becomes clearer.

Unfortunately, the state constants declared in
"google_update/google_update_idl.h" conflict with constants defined in some
other header so I had to put this one into a namespace.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/plugin/d...
remoting/host/plugin/daemon_installer_win.cc:35: const char16
kOmahaElevationMoniker[] =
On 2012/06/14 20:16:27, Peter Kasting wrote:
> This file contains many instances of problem's I've suggested correcting in
your
> new code elsewhere (use of char16 and string16 where wchar_t/wstring are more
> correct, use of stringize macros instead of L).  Fixing all these may be
beyond
> the scope of this change.  Consider filing a bug to clean these up.

I filed http://crbug.com/133003 for this.

[Peter Kasting, 2012-06-15 19:02:42]

I'll try and take another pass through later today if possible.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
File remoting/base/breakpad_win.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_win.cc:110: DWORD length = ::GetTempPath(MAX_PATH,
temp_directory);
On 2012/06/15 18:45:49, alexeypa wrote:
> On 2012/06/14 20:16:27, Peter Kasting wrote:
> > Nit: Is :: necessary?
> 
> It is a common pattern in Chromium code. This is however a Windows-only file,
so
> I don't think it makes much sense.

(FWIW, I object when I see this in other code too, if it's not actually
necessary -- I don't see it all that often.)

> > Also consider using GetTempDir from base/file_util_win.cc, though this might
> not
> > make your code any simpler or safer.
> 
> The reason not to use GetTempDir from base is to run as little code as we can
> before all the hooks are installed. This way we minimize the chance of
crashing
> before we can report it.

I see.  Might want a quick comment to that effect.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/breakpad...
File remoting/host/breakpad_win.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/breakpad...
remoting/host/breakpad_win.cc:39: if (result == ERROR_SUCCESS) {
On 2012/06/15 18:45:49, alexeypa wrote:
> On 2012/06/14 20:16:27, Peter Kasting wrote:
> > Nit: If you like, you may omit {} on one-line conditional bodies (though
> either
> > way is fine).
> 
> I tend to keep {} even for one-liners because they outline the boundaries of
the
> condition statement better.

That's fine.  Just make sure you're consistent with code around you.  The
majority of Chrome tends to omit these, so others might be surprised.  Trying to
be consistent at least across major divisions (e.g. all of remoting/) is
generally a good rule.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/constants.h
File remoting/host/constants.h (right):

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/constant...
remoting/host/constants.h:8: #include "base/string16.h"
On 2012/06/15 18:45:49, alexeypa wrote:
> On 2012/06/14 20:16:27, Peter Kasting wrote:
> > Nit: #ifdef this as well.  I suggest moving the block below up to here so
you
> > can use a single #if for both.
> 
> I got rid of string16 entirely. 
> 
> BTW I don't think I could move this include below. The guide says that
platform
> specific includes should go right after regular includes:
> 
> "For platform-specific #includes, put them below the "regular" includes,
> separated with a blank line."

Yes, I was suggesting the opposite (to move some code below up to here rather
than to move this #include downwards) as you're correct that we don't want
#includes in the middle of the file.  Sorry if I was unclear.  (And it's moot
now anyway.)

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/plugin/d...
File remoting/host/plugin/daemon_installer_win.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/plugin/d...
remoting/host/plugin/daemon_installer_win.cc:25: }  // namespace omaha
On 2012/06/15 18:45:49, alexeypa wrote:
> On 2012/06/14 20:16:27, Peter Kasting wrote:
> > No other files in the project which #include this file put it in a
namespace. 
> > Is this truly necessary?
> > 
> > I ask this because if we can eliminate this, the correct ordering of the
> > #includes (including the one you're adding) becomes clearer.
> 
> Unfortunately, the state constants declared in
> "google_update/google_update_idl.h" conflict with constants defined in some
> other header so I had to put this one into a namespace.

Which other header has the conflict?  Can we change the names declared there? 
(The google_update_idl.h things are autogenerated so changing them might be
trickier.)  Namespacing around a #include is something I really have never seen
other Chrome code do so I'd be willing to go to some length to avoid it.

[alexeypa (please no reviews), 2012-06-15 19:51:24]

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
File remoting/base/breakpad_win.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/base/breakpad...
remoting/base/breakpad_win.cc:110: DWORD length = ::GetTempPath(MAX_PATH,
temp_directory);
On 2012/06/15 19:02:42, Peter Kasting wrote:
> > > Also consider using GetTempDir from base/file_util_win.cc, though this
might
> > not
> > > make your code any simpler or safer.
> > 
> > The reason not to use GetTempDir from base is to run as little code as we
can
> > before all the hooks are installed. This way we minimize the chance of
> crashing
> > before we can report it.
> 
> I see.  Might want a quick comment to that effect.

Done.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/constants.h
File remoting/host/constants.h (right):

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/constant...
remoting/host/constants.h:8: #include "base/string16.h"
On 2012/06/15 19:02:42, Peter Kasting wrote:
> Sorry if I was unclear.

Well, actually your comment was quite clear. I just misread it.

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/plugin/d...
File remoting/host/plugin/daemon_installer_win.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/1/remoting/host/plugin/d...
remoting/host/plugin/daemon_installer_win.cc:25: }  // namespace omaha
On 2012/06/15 19:02:42, Peter Kasting wrote:
> Which other header has the conflict?  Can we change the names declared there? 
> (The google_update_idl.h things are autogenerated so changing them might be
> trickier.)  Namespacing around a #include is something I really have never
seen
> other Chrome code do so I'd be willing to go to some length to avoid it.

It turns out there is no conflict anymore and omaha namespace is not needed at
all. I verified the compiled code has all proper jumps (just in case).

[Peter Kasting, 2012-06-15 21:19:30]

LGTM.  Once this is submitted, I'll mark the buganizer bug fixed, which should
grant you readability.

https://chromiumcodereview.appspot.com/10535082/diff/23001/remoting/base/brea...
File remoting/base/breakpad_win.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/23001/remoting/base/brea...
remoting/base/breakpad_win.cc:181: MDRawAssertionInfo* /* assertion */) {
Nit: Don't comment out the parameter names.  MSVC doesn't warn for unused named
parameters.

https://chromiumcodereview.appspot.com/10535082/diff/23001/remoting/base/brea...
remoting/base/breakpad_win.cc:182: BreakpadWin* self =
BreakpadWin::GetInstance();
Nit: You may also inline this in the following line.

https://chromiumcodereview.appspot.com/10535082/diff/23001/remoting/base/brea...
remoting/base/breakpad_win.cc:196: ::TerminateProcess(::GetCurrentProcess(),
Nit: ::

https://chromiumcodereview.appspot.com/10535082/diff/23001/remoting/host/usag...
File remoting/host/usage_stats_consent_win.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/23001/remoting/host/usag...
remoting/host/usage_stats_consent_win.cc:30: if (result != ERROR_SUCCESS) {
Nit: You may also shorten as follows:

  return (result == ERROR_SUCCESS) ?
      key.ReadValueDW(kOmahaUsagestatsValue, usagestats_out) : result;

[alexeypa (please no reviews), 2012-06-15 21:25:55]

https://chromiumcodereview.appspot.com/10535082/diff/23001/remoting/base/brea...
File remoting/base/breakpad_win.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/23001/remoting/base/brea...
remoting/base/breakpad_win.cc:181: MDRawAssertionInfo* /* assertion */) {
On 2012/06/15 21:19:30, Peter Kasting wrote:
> Nit: Don't comment out the parameter names.  MSVC doesn't warn for unused
named
> parameters.

This is required by Google's C++ style guide:
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Function_Declarations...

[Peter Kasting, 2012-06-15 21:33:00]

https://chromiumcodereview.appspot.com/10535082/diff/23001/remoting/base/brea...
File remoting/base/breakpad_win.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/23001/remoting/base/brea...
remoting/base/breakpad_win.cc:181: MDRawAssertionInfo* /* assertion */) {
On 2012/06/15 21:25:55, alexeypa wrote:
> On 2012/06/15 21:19:30, Peter Kasting wrote:
> > Nit: Don't comment out the parameter names.  MSVC doesn't warn for unused
> named
> > parameters.
> 
> This is required by Google's C++ style guide:
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Function_Declarations...

True; however, the underlying thrust of that item is to always supply parameter
names and never leave things as bare types.  In google3, where almost everything
is compiled on Linux, compilers generally warn for unusued named parameters,
hence the style recommendation.  We don't normally follow this rule in Chrome
code (at least on Windows) because MSVC allows us to compile it without
warnings, and therefore we can supply names without having to resort to
comments.

Good catch though!

[alexeypa (please no reviews), 2012-06-15 22:13:57]

The last few comments. I'll be checking it in as soon as it passes try bots.
Thanks for an excellent review!

https://chromiumcodereview.appspot.com/10535082/diff/23001/remoting/base/brea...
File remoting/base/breakpad_win.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/23001/remoting/base/brea...
remoting/base/breakpad_win.cc:182: BreakpadWin* self =
BreakpadWin::GetInstance();
On 2012/06/15 21:19:30, Peter Kasting wrote:
> Nit: You may also inline this in the following line.

This makes the line longer 80 characters.

https://chromiumcodereview.appspot.com/10535082/diff/23001/remoting/base/brea...
remoting/base/breakpad_win.cc:196: ::TerminateProcess(::GetCurrentProcess(),
On 2012/06/15 21:19:30, Peter Kasting wrote:
> Nit: ::

Done.

https://chromiumcodereview.appspot.com/10535082/diff/23001/remoting/host/usag...
File remoting/host/usage_stats_consent_win.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/23001/remoting/host/usag...
remoting/host/usage_stats_consent_win.cc:30: if (result != ERROR_SUCCESS) {
On 2012/06/15 21:19:30, Peter Kasting wrote:
> Nit: You may also shorten as follows:
> 
>   return (result == ERROR_SUCCESS) ?
>       key.ReadValueDW(kOmahaUsagestatsValue, usagestats_out) : result;

Ternary operators are less common in code. It seems to be that people tend to
see it as a necessary hack rather than a trivial if statement. So it takes a
little bit more effort to read the code. I don't think it worth a couple of
saved lines.

[Peter Kasting, 2012-06-16 01:56:42]

https://chromiumcodereview.appspot.com/10535082/diff/23001/remoting/base/brea...
File remoting/base/breakpad_win.cc (right):

https://chromiumcodereview.appspot.com/10535082/diff/23001/remoting/base/brea...
remoting/base/breakpad_win.cc:182: BreakpadWin* self =
BreakpadWin::GetInstance();
On 2012/06/15 22:13:57, alexeypa wrote:
> On 2012/06/15 21:19:30, Peter Kasting wrote:
> > Nit: You may also inline this in the following line.
> 
> This makes the line longer 80 characters.

Well, appropriate wrapping was assumed :)

In any case, it is perfectly fine not to inline as well.

[commit-bot: I haz the power, 2012-06-16 03:22:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10535082/13006

[commit-bot: I haz the power, 2012-06-16 03:22:11]

Presubmit check for 10535082-13006 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  remoting/base/breakpad_win.cc:28
    void InitializeCrashReportingForTest(const wchar_t* pipe_name);

** Presubmit Warnings **
New code should not use wstrings.  If you are calling an API that accepts a
wstring, fix the API.
    remoting/base/breakpad_win.cc:159
    remoting/host/usage_stats_consent_win.cc:25

Presubmit checks took 1.4s to calculate.