Preparation patch for moving the chrome crashpad integration into chrome_elf.

Preparation patch for moving the chrome crashpad integration into chrome_elf.

Changes in this patch as below :
1. Moved the ChromeCrashReporterClient class on Windows to chrome/app/chrome_crash_reporter_client_win.cc/.h
   These files still contain common code from chrome_crash_reporter_client.cc/.h. Will fix this in an upcoming patch.

2. Use the chrome_crash_reporter_client_win.cc/.h on Windows.

3. Moved the chrome_elf_util.cc/.h files to a new target chrome/install_static. These files have been renamed to
   install_util.cc/.h. The new target install_static_util is a static library and is currently lined with chrome_elf and related
   targets.

4. Used the install_static_util target in chrome_elf targets.

BUG=604923
Committed: https://crrev.com/0a700e23869fd67a39b24a556a0232b1bafaaa5e
Cr-Commit-Position: refs/heads/master@{#388666}

[ananta, 2016-04-19 23:17:03]

ananta@chromium.org changed reviewers:
+ scottmg@chromium.org

[scottmg, 2016-04-19 23:27:17]

This seems good to me, I don't own anything here though.

https://codereview.chromium.org/1904613002/diff/20001/chrome/install_static/B...
File chrome/install_static/BUILD.gn (right):

https://codereview.chromium.org/1904613002/diff/20001/chrome/install_static/B...
chrome/install_static/BUILD.gn:28: "advapi32.lib",
This is allowed?

https://codereview.chromium.org/1904613002/diff/20001/chrome_elf/chrome_elf.gyp
File chrome_elf/chrome_elf.gyp (right):

https://codereview.chromium.org/1904613002/diff/20001/chrome_elf/chrome_elf.g...
chrome_elf/chrome_elf.gyp:46: '../chrome/chrome.gyp:install_static_util',
indent of all the additions in this file are wrong

[ananta, 2016-04-19 23:42:43]

https://codereview.chromium.org/1904613002/diff/20001/chrome/install_static/B...
File chrome/install_static/BUILD.gn (right):

https://codereview.chromium.org/1904613002/diff/20001/chrome/install_static/B...
chrome/install_static/BUILD.gn:28: "advapi32.lib",
On 2016/04/19 23:27:17, scottmg wrote:
> This is allowed?

Registry API's are in advapi32.

https://codereview.chromium.org/1904613002/diff/20001/chrome_elf/chrome_elf.gyp
File chrome_elf/chrome_elf.gyp (right):

https://codereview.chromium.org/1904613002/diff/20001/chrome_elf/chrome_elf.g...
chrome_elf/chrome_elf.gyp:46: '../chrome/chrome.gyp:install_static_util',
On 2016/04/19 23:27:17, scottmg wrote:
> indent of all the additions in this file are wrong

Done.

[ananta, 2016-04-19 23:47:22]

ananta@chromium.org changed reviewers:
+ robertshield@chromium.org, sky@chromium.org

[ananta, 2016-04-19 23:47:23]

+sky for src\chrome
+robertshield for src\chrome_elf

[sky, 2016-04-20 02:15:23]

LGTM

https://codereview.chromium.org/1904613002/diff/100001/chrome/app/chrome_cras...
File chrome/app/chrome_crash_reporter_client_win.h (left):

https://codereview.chromium.org/1904613002/diff/100001/chrome/app/chrome_cras...
chrome/app/chrome_crash_reporter_client_win.h:21: class
ChromeCrashReporterClient : public crash_reporter::CrashReporterClient {
I think this code would be clearer if you named this class
ChromeCrashReporterClientWin. But we do have code that follows the pattern you
have here. WDYT?

https://codereview.chromium.org/1904613002/diff/100001/chrome/chrome_installe...
File chrome/chrome_installer_static.gypi (right):

https://codereview.chromium.org/1904613002/diff/100001/chrome/chrome_installe...
chrome/chrome_installer_static.gypi:1: # Copyright (c) 2016 The Chromium
Authors. All rights reserved.
nit: no (c)

[sky, 2016-04-20 02:15:50]

Also, can you get a local OWNER for nacl please?

[robertshield, 2016-04-20 17:32:50]

https://codereview.chromium.org/1904613002/diff/120001/chrome/chrome_installe...
File chrome/chrome_installer_static.gypi (right):

https://codereview.chromium.org/1904613002/diff/120001/chrome/chrome_installe...
chrome/chrome_installer_static.gypi:11: # This part is shared between the two
versions of the target.
Does there exist gyp magic such that only kernel32 and advapi32 may be depended
on by targets in this file? (There appears to be something like this in the .gn
file). If not, please add a comment asking people not to add things that depend
on other system libraries.  

There's a release-only (due to differences in how GN builds debug libraries)
unit test on elf that enforces this
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome_elf/elf_imp...),
but not much else.

https://codereview.chromium.org/1904613002/diff/120001/chrome/install_static/...
File chrome/install_static/BUILD.gn (right):

https://codereview.chromium.org/1904613002/diff/120001/chrome/install_static/...
chrome/install_static/BUILD.gn:8: # This file only contains utility functions
which can depend on kernel32 and
s/can depend/must only depend/

https://codereview.chromium.org/1904613002/diff/120001/chrome/install_static/...
File chrome/install_static/install_util.h (right):

https://codereview.chromium.org/1904613002/diff/120001/chrome/install_static/...
chrome/install_static/install_util.h:3: // found in the LICENSE file.
Can you add a comment here as well saying why these functions are here and a
note about their dependencies.

[ananta, 2016-04-20 20:15:36]

https://codereview.chromium.org/1904613002/diff/100001/chrome/app/chrome_cras...
File chrome/app/chrome_crash_reporter_client_win.h (left):

https://codereview.chromium.org/1904613002/diff/100001/chrome/app/chrome_cras...
chrome/app/chrome_crash_reporter_client_win.h:21: class
ChromeCrashReporterClient : public crash_reporter::CrashReporterClient {
On 2016/04/20 02:15:22, sky wrote:
> I think this code would be clearer if you named this class
> ChromeCrashReporterClientWin. But we do have code that follows the pattern you
> have here. WDYT?

I guess it would make it clearer. I think we probably need a factory function to
create an instance of the class. I will do that in the next patch.

https://codereview.chromium.org/1904613002/diff/100001/chrome/chrome_installe...
File chrome/chrome_installer_static.gypi (right):

https://codereview.chromium.org/1904613002/diff/100001/chrome/chrome_installe...
chrome/chrome_installer_static.gypi:1: # Copyright (c) 2016 The Chromium
Authors. All rights reserved.
On 2016/04/20 02:15:22, sky wrote:
> nit: no (c)

Done.

https://codereview.chromium.org/1904613002/diff/120001/chrome/chrome_installe...
File chrome/chrome_installer_static.gypi (right):

https://codereview.chromium.org/1904613002/diff/120001/chrome/chrome_installe...
chrome/chrome_installer_static.gypi:11: # This part is shared between the two
versions of the target.
On 2016/04/20 17:32:50, robertshield wrote:
> Does there exist gyp magic such that only kernel32 and advapi32 may be
depended
> on by targets in this file? (There appears to be something like this in the
.gn
> file). If not, please add a comment asking people not to add things that
depend
> on other system libraries.  
> 
> There's a release-only (due to differences in how GN builds debug libraries)
> unit test on elf that enforces this
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome_elf/elf_imp...),
> but not much else. 

Done. Added comments here and in the build.gn file

https://codereview.chromium.org/1904613002/diff/120001/chrome/install_static/...
File chrome/install_static/BUILD.gn (right):

https://codereview.chromium.org/1904613002/diff/120001/chrome/install_static/...
chrome/install_static/BUILD.gn:8: # This file only contains utility functions
which can depend on kernel32 and
On 2016/04/20 17:32:50, robertshield wrote:
> s/can depend/must only depend/

Done.

https://codereview.chromium.org/1904613002/diff/120001/chrome/install_static/...
File chrome/install_static/install_util.h (right):

https://codereview.chromium.org/1904613002/diff/120001/chrome/install_static/...
chrome/install_static/install_util.h:3: // found in the LICENSE file.
On 2016/04/20 17:32:50, robertshield wrote:
> Can you add a comment here as well saying why these functions are here and a
> note about their dependencies.

Done.

[ananta, 2016-04-20 22:02:04]

ananta@chromium.org changed reviewers:
+ sehr@chromium.org

[ananta, 2016-04-20 22:02:05]

+sehr for nacl owners

[robertshield, 2016-04-21 00:18:31]

LGTM w/indent nits

https://codereview.chromium.org/1904613002/diff/160001/chrome/chrome_installe...
File chrome/chrome_installer_static.gypi (right):

https://codereview.chromium.org/1904613002/diff/160001/chrome/chrome_installe...
chrome/chrome_installer_static.gypi:41: # Please don't add dependencies on other
system libraries.
nit: indent

https://codereview.chromium.org/1904613002/diff/160001/chrome/chrome_installe...
chrome/chrome_installer_static.gypi:47: },
indent

https://codereview.chromium.org/1904613002/diff/160001/chrome/chrome_installe...
chrome/chrome_installer_static.gypi:72: # Please don't add dependencies on other
system libraries.
indent

https://codereview.chromium.org/1904613002/diff/160001/chrome/chrome_installe...
chrome/chrome_installer_static.gypi:78: },
indent

[Derek Schuff, 2016-04-21 00:26:00]

dschuff@chromium.org changed reviewers:
+ dschuff@chromium.org

[Derek Schuff, 2016-04-21 00:26:00]

chrome/nacl LGTM

[ananta, 2016-04-21 00:33:47]

https://codereview.chromium.org/1904613002/diff/160001/chrome/chrome_installe...
File chrome/chrome_installer_static.gypi (right):

https://codereview.chromium.org/1904613002/diff/160001/chrome/chrome_installe...
chrome/chrome_installer_static.gypi:41: # Please don't add dependencies on other
system libraries.
On 2016/04/21 00:18:30, robertshield wrote:
> nit: indent

Done.

https://codereview.chromium.org/1904613002/diff/160001/chrome/chrome_installe...
chrome/chrome_installer_static.gypi:47: },
On 2016/04/21 00:18:31, robertshield wrote:
> indent

Done.

https://codereview.chromium.org/1904613002/diff/160001/chrome/chrome_installe...
chrome/chrome_installer_static.gypi:72: # Please don't add dependencies on other
system libraries.
On 2016/04/21 00:18:31, robertshield wrote:
> indent

Done.

https://codereview.chromium.org/1904613002/diff/160001/chrome/chrome_installe...
chrome/chrome_installer_static.gypi:78: },
On 2016/04/21 00:18:30, robertshield wrote:
> indent

Done.

[ananta, 2016-04-21 00:33:53]

The CQ bit was checked by ananta@chromium.org

[ananta, 2016-04-21 00:33:53]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
robertshield@chromium.org, dschuff@chromium.org
Link to the patchset: https://codereview.chromium.org/1904613002/#ps180001
(title: "Fix indentation")

[ananta, 2016-04-21 00:34:08]

The CQ bit was unchecked by ananta@chromium.org

[ananta, 2016-04-21 00:34:38]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2016-04-21 00:35:30]

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

[commit-bot: I haz the power, 2016-04-21 01:16:19]

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

[commit-bot: I haz the power, 2016-04-21 01:16:20]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[ananta, 2016-04-21 01:35:28]

The CQ bit was checked by ananta@chromium.org

[ananta, 2016-04-21 01:35:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertshield@chromium.org, dschuff@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1904613002/#ps200001
(title: "Fix build error on Windows builder")

[commit-bot: I haz the power, 2016-04-21 01:35:44]

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

[commit-bot: I haz the power, 2016-04-21 03:00:59]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-04-22 19:30:32]

Description was changed from

==========
Preparation patch for moving the chrome crashpad integration into chrome_elf.

Changes in this patch as below :
1. Moved the ChromeCrashReporterClient class on Windows to
chrome/app/chrome_crash_reporter_client_win.cc/.h
   These files still contain common code from
chrome_crash_reporter_client.cc/.h. Will fix this in an upcoming patch.

2. Use the chrome_crash_reporter_client_win.cc/.h on Windows.

3. Moved the chrome_elf_util.cc/.h files to a new target chrome/install_static.
These files have been renamed to
   install_util.cc/.h. The new target install_static_util is a static library
and is currently lined with chrome_elf and related
   targets.

4. Used the install_static_util target in chrome_elf targets.

BUG=604923
==========

to

==========
Preparation patch for moving the chrome crashpad integration into chrome_elf.

Changes in this patch as below :
1. Moved the ChromeCrashReporterClient class on Windows to
chrome/app/chrome_crash_reporter_client_win.cc/.h
   These files still contain common code from
chrome_crash_reporter_client.cc/.h. Will fix this in an upcoming patch.

2. Use the chrome_crash_reporter_client_win.cc/.h on Windows.

3. Moved the chrome_elf_util.cc/.h files to a new target chrome/install_static.
These files have been renamed to
   install_util.cc/.h. The new target install_static_util is a static library
and is currently lined with chrome_elf and related
   targets.

4. Used the install_static_util target in chrome_elf targets.

BUG=604923

Committed: https://crrev.com/0a700e23869fd67a39b24a556a0232b1bafaaa5e
Cr-Commit-Position: refs/heads/master@{#388666}
==========

[commit-bot: I haz the power, 2016-04-22 19:30:33]

Patchset 11 (id:??) landed as
https://crrev.com/0a700e23869fd67a39b24a556a0232b1bafaaa5e
Cr-Commit-Position: refs/heads/master@{#388666}