Adding crash utilities to chromecast/crash.

Adding crash utilities to chromecast/crash.

Includes initial commits for writing, managing, and uploading information generated on Linux crashes.

Change-Id: I6ff36ffb54bf9153e5663844b2a428ce7b03773f

BUG= b/20121400

Committed: https://crrev.com/f48fe876ae18ebe0a639eb271c3b7192f410bc0b
Cr-Commit-Position: refs/heads/master@{#335727}

[slan, 2015-06-09 17:03:05]

slan@chromium.org changed reviewers:
+ alokp@chromium.org, gunsch@chromium.org, wzhong@chromium.org

[slan, 2015-06-09 17:04:54]

PTAL at the changes and the description. This is the initial review.

I plan on addressing all TODOs in the description before submitting, but I would
like to make sure the organization and style are heading in the right directions
while I write tests, etc.

[gunsch, 2015-06-09 18:46:19]

Exciting CL! Couple extra comments:

1) Thanks for the internal link for diffs, that helped the review a lot.
Probably put that in the first mail on this CL though and remove it from the
commit message.

2) One or more of these could be named "*Linux", e.g. "MinidumpWriterLinux". We
use pretty much none of this on ATV and it'd be helpful to make that distinction
in filenames.

3) Are you planning to fill out chromecast/crash/BUILD.gn? Can you also update
GN for other touched files, e.g. chromecast/base/BUILD.gn?

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/break...
File chromecast/crash/breakpad_util.cc (right):

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/break...
chromecast/crash/breakpad_util.cc:5: #include "chromecast/crash/breakpad_util.h"
naming: "breakpad_util" --> "crash_util"

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/break...
chromecast/crash/breakpad_util.cc:23: std::string GetReleaseVersion() {
Oof, don't do this parsing. We can just add another line to version.h for
CAST_BUILD_RELEASE.

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/cast_...
File chromecast/crash/cast_crash_reporter_client_uploader.cc (right):

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/cast_...
chromecast/crash/cast_crash_reporter_client_uploader.cc:12: bool
CastCrashReporterClient::HandleCrashDump(const char* crashdump_filename) {
This can just get inlined into cast_crash_reporter_client.cc, and then
cast_crash_reporter_client_simple.cc can be deleted!

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/dump_...
File chromecast/crash/dump_info.h (right):

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/dump_...
chromecast/crash/dump_info.h:14: struct MinidumpParams {
This struct could/should be in its own file. This file is dump_info.h and
previously (internal diff_ only contained DumpInfo.

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/dump_...
chromecast/crash/dump_info.h:64: // Accessors.
nit: remove comment

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/minid...
File chromecast/crash/minidump_manager_unittest.cc (right):

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/minid...
chromecast/crash/minidump_manager_unittest.cc:17: 
nit: remove blank line

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/minid...
chromecast/crash/minidump_manager_unittest.cc:132: TEST_F(MinidumpManagerTest,
DoNotBlockOnAcquiringLock) {
You can rename the test as DISABLED_DoNotBlockOnAcquiringLock and then it won't
run (without having to comment everything out).

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/minid...
File chromecast/crash/minidump_writer.h (right):

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/minid...
chromecast/crash/minidump_writer.h:36: friend class MinidumpWriterTest;
nit: prefer using public *ForTest methods (enforced by presubmit) rather than
having test classes invoke private members. Makes future refactoring/code change
easier.

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/minid...
File chromecast/crash/minidump_writer_unittest.cc (right):

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/minid...
chromecast/crash/minidump_writer_unittest.cc:25: class MinidumpWriterTest :
public testing::Test {
note: prefer not using test fixtures when possible. You only have one test case
here, so it could simplify the test to simply inline all of this.

[slan, 2015-06-10 01:45:16]

> 1) Thanks for the internal link for diffs, that helped the review a lot.
> Probably put that in the first mail on this CL though and remove it from the
> commit message.

Will do.
 
> 2) One or more of these could be named "*Linux", e.g. "MinidumpWriterLinux".
We
> use pretty much none of this on ATV and it'd be helpful to make that
distinction
> in filenames.

The only files depended on by android crash stuff is cast_crash_keys.cc/h. If
you would like to make the distinction, I propose all Linux-specific stuff be
moved into a linux/ subdir, rather than appending every class and file with
"Linux" / "_linux". WDYT?


> 3) Are you planning to fill out chromecast/crash/BUILD.gn? Can you also update
> GN for other touched files, e.g. chromecast/base/BUILD.gn?

Sorry, that should have been populated for initial review. PTAL in most recent
patch.


All other comments addressed inline.

[slan, 2015-06-10 01:49:13]

Comments addressed. alokp@ please look at the BUILD.gn files. I would like your
assistance in establishing good conventions for the gn build.

As requested by gunsch@, link to internal diff, comments, and TODOs included
below:

[Internal]
This initial commit adds crash utilities to Chrome from
chromecast/internal/breakpad/. In addition to moving the code to TOT Chrome,
this is an opportunity to bring this code up to Chrome standards, gain unittest
and end-to-end test coverage, and introduce abstraction in the code for greater
flexibility.

Diffs from internal code may be seen here:
https://eureka-internal-review.git.corp.google.com/#/c/35336

TODO(slan) before push:
 - Full code coverage with unittests.
 - End-to-end test with Chrome IO thread permissions disabled.
 - Upstream crashdump and uploader logic in this CL or elsewhere.
 - Internal companion CLs must link here.

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/break...
File chromecast/crash/breakpad_util.cc (right):

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/break...
chromecast/crash/breakpad_util.cc:5: #include "chromecast/crash/breakpad_util.h"
On 2015/06/09 18:46:18, gunsch wrote:
> naming: "breakpad_util" --> "crash_util"

Done.

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/break...
chromecast/crash/breakpad_util.cc:23: std::string GetReleaseVersion() {
On 2015/06/09 18:46:19, gunsch wrote:
> Oof, don't do this parsing. We can just add another line to version.h for
> CAST_BUILD_RELEASE.

Done.

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/cast_...
File chromecast/crash/cast_crash_reporter_client_uploader.cc (right):

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/cast_...
chromecast/crash/cast_crash_reporter_client_uploader.cc:12: bool
CastCrashReporterClient::HandleCrashDump(const char* crashdump_filename) {
On 2015/06/09 18:46:19, gunsch wrote:
> This can just get inlined into cast_crash_reporter_client.cc, and then
> cast_crash_reporter_client_simple.cc can be deleted!

Done.

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/dump_...
File chromecast/crash/dump_info.h (right):

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/dump_...
chromecast/crash/dump_info.h:14: struct MinidumpParams {
On 2015/06/09 18:46:19, gunsch wrote:
> This struct could/should be in its own file. This file is dump_info.h and
> previously (internal diff_ only contained DumpInfo.

Done.

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/dump_...
chromecast/crash/dump_info.h:64: // Accessors.
On 2015/06/09 18:46:19, gunsch wrote:
> nit: remove comment

Done.

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/minid...
File chromecast/crash/minidump_manager_unittest.cc (right):

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/minid...
chromecast/crash/minidump_manager_unittest.cc:17: 
On 2015/06/09 18:46:19, gunsch wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/minid...
chromecast/crash/minidump_manager_unittest.cc:132: TEST_F(MinidumpManagerTest,
DoNotBlockOnAcquiringLock) {
On 2015/06/09 18:46:19, gunsch wrote:
> You can rename the test as DISABLED_DoNotBlockOnAcquiringLock and then it
won't
> run (without having to comment everything out).

Done.

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/minid...
File chromecast/crash/minidump_writer.h (right):

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/minid...
chromecast/crash/minidump_writer.h:36: friend class MinidumpWriterTest;
On 2015/06/09 18:46:19, gunsch wrote:
> nit: prefer using public *ForTest methods (enforced by presubmit) rather than
> having test classes invoke private members. Makes future refactoring/code
change
> easier.

Done.

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/minid...
File chromecast/crash/minidump_writer_unittest.cc (right):

https://codereview.chromium.org/1154383006/diff/120001/chromecast/crash/minid...
chromecast/crash/minidump_writer_unittest.cc:25: class MinidumpWriterTest :
public testing::Test {
On 2015/06/09 18:46:19, gunsch wrote:
> note: prefer not using test fixtures when possible. You only have one test
case
> here, so it could simplify the test to simply inline all of this.

Done.

[gunsch, 2015-06-10 16:33:31]

+1 to the idea of moving many of these files to chromecast/crash/linux/.

I believe both GYP and GN will automatically guard files in a folder with
"OS=='linux'" so you don't have to add a condition.

https://codereview.chromium.org/1154383006/diff/140001/chromecast/BUILD.gn
File chromecast/BUILD.gn (right):

https://codereview.chromium.org/1154383006/diff/140001/chromecast/BUILD.gn#ne...
chromecast/BUILD.gn:30: ]
I believe we have //chromecast/media:unittests as well

(that target should be renamed to cast_media_unittests)

https://codereview.chromium.org/1154383006/diff/140001/chromecast/base/BUILD.gn
File chromecast/base/BUILD.gn (right):

https://codereview.chromium.org/1154383006/diff/140001/chromecast/base/BUILD....
chromecast/base/BUILD.gn:5: import("//chrome/version.gni")
Hmm, we can't import anything from //chrome. But it looks like other parts of
the code (including //media!) do.

Maybe a TODO can be added like this, and hopefully noticed whenever someone does
a cleanup:

https://code.google.com/p/chromium/codesearch#chromium/src/media/cdm/ppapi/BU...

https://codereview.chromium.org/1154383006/diff/140001/chromecast/base/BUILD....
chromecast/base/BUILD.gn:8: process_version("cast_version") {
can we put this below the other targets, so that "base" remains first in this
folder?

https://codereview.chromium.org/1154383006/diff/140001/chromecast/chromecast_...
File chromecast/chromecast_tests.gypi (right):

https://codereview.chromium.org/1154383006/diff/140001/chromecast/chromecast_...
chromecast/chromecast_tests.gypi:25: 'target_name': 'cast_crash_unittests',
Needs to be added to:

1) testing/buildbot/chromium.linux.json
2) cast_test_generator target (below)

https://codereview.chromium.org/1154383006/diff/140001/chromecast/crash/app_s...
File chromecast/crash/app_state_tracker.cc (right):

https://codereview.chromium.org/1154383006/diff/140001/chromecast/crash/app_s...
chromecast/crash/app_state_tracker.cc:23: }
// namespace

https://codereview.chromium.org/1154383006/diff/140001/chromecast/crash/cast_...
File chromecast/crash/cast_crash_reporter_client.cc (right):

https://codereview.chromium.org/1154383006/diff/140001/chromecast/crash/cast_...
chromecast/crash/cast_crash_reporter_client.cc:9: #include
"chromecast/crash/crash_util.h"
ordering (how did this get through git cl upload checks?)

[slan, 2015-06-11 02:34:48]

https://codereview.chromium.org/1154383006/diff/140001/chromecast/BUILD.gn
File chromecast/BUILD.gn (right):

https://codereview.chromium.org/1154383006/diff/140001/chromecast/BUILD.gn#ne...
chromecast/BUILD.gn:30: ]
On 2015/06/10 16:33:30, gunsch wrote:
> I believe we have //chromecast/media:unittests as well
> 
> (that target should be renamed to cast_media_unittests)

Done.

https://codereview.chromium.org/1154383006/diff/140001/chromecast/base/BUILD.gn
File chromecast/base/BUILD.gn (right):

https://codereview.chromium.org/1154383006/diff/140001/chromecast/base/BUILD....
chromecast/base/BUILD.gn:5: import("//chrome/version.gni")
On 2015/06/10 16:33:30, gunsch wrote:
> Hmm, we can't import anything from //chrome. But it looks like other parts of
> the code (including //media!) do.
> 
> Maybe a TODO can be added like this, and hopefully noticed whenever someone
does
> a cleanup:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/cdm/ppapi/BU...

Done.

https://codereview.chromium.org/1154383006/diff/140001/chromecast/base/BUILD....
chromecast/base/BUILD.gn:8: process_version("cast_version") {
On 2015/06/10 16:33:30, gunsch wrote:
> can we put this below the other targets, so that "base" remains first in this
> folder?

Done.

https://codereview.chromium.org/1154383006/diff/140001/chromecast/crash/cast_...
File chromecast/crash/cast_crash_reporter_client.cc (right):

https://codereview.chromium.org/1154383006/diff/140001/chromecast/crash/cast_...
chromecast/crash/cast_crash_reporter_client.cc:9: #include
"chromecast/crash/crash_util.h"
On 2015/06/10 16:33:30, gunsch wrote:
> ordering (how did this get through git cl upload checks?)

chromecast < components :)

[gunsch, 2015-06-12 00:37:09]

mostly nits

https://codereview.chromium.org/1154383006/diff/140001/chromecast/crash/cast_...
File chromecast/crash/cast_crash_reporter_client.cc (right):

https://codereview.chromium.org/1154383006/diff/140001/chromecast/crash/cast_...
chromecast/crash/cast_crash_reporter_client.cc:9: #include
"chromecast/crash/crash_util.h"
On 2015/06/11 02:34:48, slan wrote:
> On 2015/06/10 16:33:30, gunsch wrote:
> > ordering (how did this get through git cl upload checks?)
> 
> chromecast < components :)

Ha, I can't read. Carry on.

https://codereview.chromium.org/1154383006/diff/160001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1154383006/diff/160001/BUILD.gn#newcode76
BUILD.gn:76: "//chromecast:chromecast_unittests",
Remove this for now. Brett and others have expressed not wanting top-level
BUILD.gn to depend on chromecast, and that we should be using the
root_extra_deps workaround instead.

https://codereview.chromium.org/1154383006/diff/160001/chromecast/base/error_...
File chromecast/base/error_codes.cc (right):

https://codereview.chromium.org/1154383006/diff/160001/chromecast/base/error_...
chromecast/base/error_codes.cc:50: const std::string error_file_path =
GetInitialErrorFilePath().value();
const std::string&

https://codereview.chromium.org/1154383006/diff/160001/chromecast/crash/BUILD.gn
File chromecast/crash/BUILD.gn (right):

https://codereview.chromium.org/1154383006/diff/160001/chromecast/crash/BUILD...
chromecast/crash/BUILD.gn:9: "app_state_tracker.cc",
this is no longer accurate

https://codereview.chromium.org/1154383006/diff/160001/chromecast/crash/app_s...
File chromecast/crash/app_state_tracker.cc (right):

https://codereview.chromium.org/1154383006/diff/160001/chromecast/crash/app_s...
chromecast/crash/app_state_tracker.cc:47: // TODO(slan): Currently
SetCrashKeyValue is a no-op on chromecast until
This would be a pretty exciting follow-up, btw. We do this in a really
roundabout way for Chromecast, IIRC.

https://codereview.chromium.org/1154383006/diff/160001/testing/buildbot/ninja...
File testing/buildbot/ninja_to_gn.pyl (right):

https://codereview.chromium.org/1154383006/diff/160001/testing/buildbot/ninja...
testing/buildbot/ninja_to_gn.pyl:26: "cast_base_unittests":              
"//cast:cast_base_unittests",
if you're touching this file, these should be //chromecast

https://codereview.chromium.org/1154383006/diff/160001/testing/buildbot/ninja...
testing/buildbot/ninja_to_gn.pyl:28: "cast_unittests":                   
"//cast:cast_unittests",
what is this line? (note: I've never seen this file before)

[lcwu1, 2015-06-12 00:59:59]

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

[lcwu1, 2015-06-12 01:00:00]

https://codereview.chromium.org/1154383006/diff/160001/testing/buildbot/ninja...
File testing/buildbot/ninja_to_gn.pyl (right):

https://codereview.chromium.org/1154383006/diff/160001/testing/buildbot/ninja...
testing/buildbot/ninja_to_gn.pyl:28: "cast_unittests":                   
"//cast:cast_unittests",
On 2015/06/12 00:37:09, gunsch wrote:
> what is this line? (note: I've never seen this file before)

This is not our test. It belongs to cloudview team's cast code.

[gunsch, 2015-06-12 16:38:27]

https://codereview.chromium.org/1154383006/diff/160001/testing/buildbot/ninja...
File testing/buildbot/ninja_to_gn.pyl (right):

https://codereview.chromium.org/1154383006/diff/160001/testing/buildbot/ninja...
testing/buildbot/ninja_to_gn.pyl:28: "cast_unittests":                   
"//cast:cast_unittests",
On 2015/06/12 00:59:59, lcwu1 wrote:
> On 2015/06/12 00:37:09, gunsch wrote:
> > what is this line? (note: I've never seen this file before)
> 
> This is not our test. It belongs to cloudview team's cast code.

Are we sure about that? There isn't a //cast directory checked in upstream, only
//media/cast, unless they keep it internal/private.

If these two lines are for cloudview, then we shouldn't be adding
"cast_crash_unittests" here

[slan, 2015-06-15 17:01:45]

PTAL. Testing is complete, and internal companion CL is ready for review as
well: https://eureka-internal-review.git.corp.google.com/#/c/35458

https://codereview.chromium.org/1154383006/diff/160001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1154383006/diff/160001/BUILD.gn#newcode76
BUILD.gn:76: "//chromecast:chromecast_unittests",
On 2015/06/12 00:37:09, gunsch wrote:
> Remove this for now. Brett and others have expressed not wanting top-level
> BUILD.gn to depend on chromecast, and that we should be using the
> root_extra_deps workaround instead.

Done.

https://codereview.chromium.org/1154383006/diff/160001/chromecast/base/error_...
File chromecast/base/error_codes.cc (right):

https://codereview.chromium.org/1154383006/diff/160001/chromecast/base/error_...
chromecast/base/error_codes.cc:50: const std::string error_file_path =
GetInitialErrorFilePath().value();
On 2015/06/12 00:37:09, gunsch wrote:
> const std::string&

Done.

https://codereview.chromium.org/1154383006/diff/160001/chromecast/crash/BUILD.gn
File chromecast/crash/BUILD.gn (right):

https://codereview.chromium.org/1154383006/diff/160001/chromecast/crash/BUILD...
chromecast/crash/BUILD.gn:9: "app_state_tracker.cc",
On 2015/06/12 00:37:09, gunsch wrote:
> this is no longer accurate

This patch left the build code in a weird state. GN and GYP are both current
now.

https://codereview.chromium.org/1154383006/diff/160001/chromecast/crash/app_s...
File chromecast/crash/app_state_tracker.cc (right):

https://codereview.chromium.org/1154383006/diff/160001/chromecast/crash/app_s...
chromecast/crash/app_state_tracker.cc:47: // TODO(slan): Currently
SetCrashKeyValue is a no-op on chromecast until
On 2015/06/12 00:37:09, gunsch wrote:
> This would be a pretty exciting follow-up, btw. We do this in a really
> roundabout way for Chromecast, IIRC.

b/21815373

https://codereview.chromium.org/1154383006/diff/160001/testing/buildbot/ninja...
File testing/buildbot/ninja_to_gn.pyl (right):

https://codereview.chromium.org/1154383006/diff/160001/testing/buildbot/ninja...
testing/buildbot/ninja_to_gn.pyl:28: "cast_unittests":                   
"//cast:cast_unittests",
On 2015/06/12 16:38:27, gunsch wrote:
> On 2015/06/12 00:59:59, lcwu1 wrote:
> > On 2015/06/12 00:37:09, gunsch wrote:
> > > what is this line? (note: I've never seen this file before)
> > 
> > This is not our test. It belongs to cloudview team's cast code.
> 
> Are we sure about that? There isn't a //cast directory checked in upstream,
only
> //media/cast, unless they keep it internal/private.
> 
> If these two lines are for cloudview, then we shouldn't be adding
> "cast_crash_unittests" here

Inclusion of this line is enforced by a presubmit check, but these entries are
all incorrect. Will add the correct lines in for all chromecast unittests in a
separate CL.

[Internal]: b/21816910

[alokp, 2015-06-15 17:51:49]

https://codereview.chromium.org/1154383006/diff/200001/chromecast/app/cast_ma...
File chromecast/app/cast_main_delegate.cc (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/app/cast_ma...
chromecast/app/cast_main_delegate.cc:19: #include
"chromecast/crash/linux/cast_crash_reporter_client.h"
Is this cc file linux only? If not it seems weird to include a file from linux
dir.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/base/error_...
File chromecast/base/error_codes.h (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/base/error_...
chromecast/base/error_codes.h:10: enum ErrorCode {
nit: I think "Code" suffix is not necessary.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/base/error_...
chromecast/base/error_codes.h:11: NO_ERROR = 0,
nit: ERROR_NONE

https://codereview.chromium.org/1154383006/diff/200001/chromecast/base/error_...
chromecast/base/error_codes.h:33: // The volume is reset on unknown error.
The comment about volume is not clear to me.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/BUILD.gn
File chromecast/crash/BUILD.gn (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/BUILD...
chromecast/crash/BUILD.gn:58: "//testing/gtest",
not necessary. //base/test:run_all_unittests pulls in gtest.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/app_s...
File chromecast/crash/app_state_tracker.cc (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/app_s...
chromecast/crash/app_state_tracker.cc:15: std::string last_launched_app;
what is the difference between "previous_app" and "last_launched_app". Please
add a comment.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/app_s...
chromecast/crash/app_state_tracker.cc:59:
base::debug::SetCrashKeyValue(crash_keys::kPreviousApp,
nit: chack formatting

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/cast_...
File chromecast/crash/cast_crashdump_uploader.cc (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/cast_...
chromecast/crash/cast_crashdump_uploader.cc:28:
CastCrashdumpData::CastCrashdumpData() {
Do you need constructor/destructor for this struct?

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/cast_...
chromecast/crash/cast_crashdump_uploader.cc:107: LOG(INFO) << "Sending request
to " << data_.crash_server;
VLOG?

I am not sure what convention we follow in cast code to decide which logging
macro to use.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/cast_...
File chromecast/crash/cast_crashdump_uploader.h (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/cast_...
chromecast/crash/cast_crashdump_uploader.h:53: std::map<std::string /* label */,
std::string /* file */> attachments_;
nit: It might be easier to read if the documentation for key-value pairs is
included separately as <label, file>.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
File chromecast/crash/linux/crash_util.cc (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
chromecast/crash/linux/crash_util.cc:28: }  // namespace
nit: space

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
File chromecast/crash/linux/crash_util.h (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
chromecast/crash/linux/crash_util.h:19: static void RequestUploadCrashDump(const
std::string& existing_minidump_path,
should it return success/failure?

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
File chromecast/crash/linux/dump_info.h (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
chromecast/crash/linux/dump_info.h:40: std::string crashed_process_dump() const
{ return crashed_process_dump_; }
return const reference. here and elsewhere in this file.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
File chromecast/crash/linux/minidump_manager.h (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
chromecast/crash/linux/minidump_manager.h:23: class MinidumpManager {
It is not clear from the class name what this class does.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
chromecast/crash/linux/minidump_manager.h:29: int DoWorkLocked();
I would rename this to what this function actually does - "DoWork" is very
generic and does not convey anything. Also, since locking is an internal concept
for derived classes, "Locked" should not be part of the public function name.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
chromecast/crash/linux/minidump_manager.h:32: bool nonblocking() { return
nonblocking_; }
nit: non_blocking.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
chromecast/crash/linux/minidump_manager.h:35: void SetNonblocking(bool
nonblocking) { nonblocking_ = nonblocking; }
set_non_blocking

https://codereview.chromium.org/1154383006/diff/200001/chromecast/media/BUILD.gn
File chromecast/media/BUILD.gn (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/media/BUILD...
chromecast/media/BUILD.gn:15: test("cast_media_unittests") {
I would keep the target name as unittests and add an output name so that it does
not conflict with other targets in the out directory. Referencing this target
from other GN files looks prettier that way.

[gunsch, 2015-06-15 18:05:59]

https://codereview.chromium.org/1154383006/diff/200001/chromecast/app/cast_ma...
File chromecast/app/cast_main_delegate.cc (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/app/cast_ma...
chromecast/app/cast_main_delegate.cc:19: #include
"chromecast/crash/linux/cast_crash_reporter_client.h"
On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> Is this cc file linux only? If not it seems weird to include a file from linux
> dir.

+1, maybe guard with OS_LINUX below now that it has a new location

https://codereview.chromium.org/1154383006/diff/200001/chromecast/base/error_...
File chromecast/base/error_codes.h (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/base/error_...
chromecast/base/error_codes.h:10: enum ErrorCode {
On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> nit: I think "Code" suffix is not necessary.

To all of Alok's comments here: I'd actually lean toward leaving this file
as-is. This is a direct upstream from internal.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/BUILD.gn
File chromecast/crash/BUILD.gn (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/BUILD...
chromecast/crash/BUILD.gn:58: "//testing/gtest",
On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> not necessary. //base/test:run_all_unittests pulls in gtest.

Does Chromium not advise following same IWYU rules for deps?

https://codereview.chromium.org/1154383006/diff/200001/chromecast/media/BUILD.gn
File chromecast/media/BUILD.gn (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/media/BUILD...
chromecast/media/BUILD.gn:15: test("cast_media_unittests") {
On 2015/06/15 17:51:49, Alok Priyadarshi wrote:
> I would keep the target name as unittests and add an output name so that it
does
> not conflict with other targets in the out directory. Referencing this target
> from other GN files looks prettier that way.

Disagreed. See:
https://code.google.com/p/chromium/codesearch#search/&q=file:BUILD.gn$%20_uni...

Convention seems to be to use full test name.

[alokp, 2015-06-15 18:14:40]

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/BUILD.gn
File chromecast/crash/BUILD.gn (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/BUILD...
chromecast/crash/BUILD.gn:58: "//testing/gtest",
On 2015/06/15 18:05:59, gunsch wrote:
> On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> > not necessary. //base/test:run_all_unittests pulls in gtest.
> 
> Does Chromium not advise following same IWYU rules for deps?

I dont think it spells it out as it does for header files. Including all deps
may also be unwieldy and hard to maintain IMO.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/media/BUILD.gn
File chromecast/media/BUILD.gn (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/media/BUILD...
chromecast/media/BUILD.gn:15: test("cast_media_unittests") {
On 2015/06/15 18:05:59, gunsch wrote:
> On 2015/06/15 17:51:49, Alok Priyadarshi wrote:
> > I would keep the target name as unittests and add an output name so that it
> does
> > not conflict with other targets in the out directory. Referencing this
target
> > from other GN files looks prettier that way.
> 
> Disagreed. See:
>
https://code.google.com/p/chromium/codesearch#search/&q=file:BUILD.gn$%20_uni...
> 
> Convention seems to be to use full test name.

You are right. Thanks for including the search results.

[gunsch, 2015-06-15 18:16:12]

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/cast_...
File chromecast/crash/cast_crashdump_uploader.cc (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/cast_...
chromecast/crash/cast_crashdump_uploader.cc:107: LOG(INFO) << "Sending request
to " << data_.crash_server;
On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> VLOG?
> 
> I am not sure what convention we follow in cast code to decide which logging
> macro to use.

In general we stick with LOG(INFO) for things we want to see in our logs. We
don't have the same automatic pushback to LOG(INFO) that Chrome team does.

This is especially the case since logs from user reports are one of our primary
debugging mechanisms.

[slan, 2015-06-16 14:57:49]

Renamed MinidumpManager -> SynchronizedMinidumpManager, and addressed some other
small issues.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/app/cast_ma...
File chromecast/app/cast_main_delegate.cc (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/app/cast_ma...
chromecast/app/cast_main_delegate.cc:19: #include
"chromecast/crash/linux/cast_crash_reporter_client.h"
On 2015/06/15 18:05:59, gunsch wrote:
> On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> > Is this cc file linux only? If not it seems weird to include a file from
linux
> > dir.
> 
> +1, maybe guard with OS_LINUX below now that it has a new location

Done. Guarded via #else, to keep consistent with the rest of the file.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/base/error_...
File chromecast/base/error_codes.h (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/base/error_...
chromecast/base/error_codes.h:10: enum ErrorCode {
On 2015/06/15 18:05:59, gunsch wrote:
> On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> > nit: I think "Code" suffix is not necessary.
> 
> To all of Alok's comments here: I'd actually lean toward leaving this file
> as-is. This is a direct upstream from internal.

To keep this CL as clean as possible, this will be kept as a direct upstream.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/base/error_...
chromecast/base/error_codes.h:11: NO_ERROR = 0,
On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> nit: ERROR_NONE

Keeping it for now.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/base/error_...
chromecast/base/error_codes.h:33: // The volume is reset on unknown error.
On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> The comment about volume is not clear to me.

I agree, this seems to be the wrong place for this comment.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/BUILD.gn
File chromecast/crash/BUILD.gn (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/BUILD...
chromecast/crash/BUILD.gn:58: "//testing/gtest",
On 2015/06/15 18:14:39, Alok Priyadarshi wrote:
> On 2015/06/15 18:05:59, gunsch wrote:
> > On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> > > not necessary. //base/test:run_all_unittests pulls in gtest.
> > 
> > Does Chromium not advise following same IWYU rules for deps?
> 
> I dont think it spells it out as it does for header files. Including all deps
> may also be unwieldy and hard to maintain IMO.

Since there seems to be no prevailing pattern in Chromium, and this pattern
matches the rest of the Chromecast internal and external code, let's keep this
for now, leaning towards explicitly including every building each dependency in
build files (IWYU)

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/app_s...
File chromecast/crash/app_state_tracker.cc (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/app_s...
chromecast/crash/app_state_tracker.cc:15: std::string last_launched_app;
On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> what is the difference between "previous_app" and "last_launched_app". Please
> add a comment.

Added in header file. This state tracker will likely change soon with the
introduction of background apps.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/app_s...
chromecast/crash/app_state_tracker.cc:59:
base::debug::SetCrashKeyValue(crash_keys::kPreviousApp,
On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> nit: chack formatting

This indentation comes from the clang format tool with Chromium style:

clang-format-diff-3.5 -i -p1 -style=Chromium

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/cast_...
File chromecast/crash/cast_crashdump_uploader.cc (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/cast_...
chromecast/crash/cast_crashdump_uploader.cc:28:
CastCrashdumpData::CastCrashdumpData() {
On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> Do you need constructor/destructor for this struct?

This is enforced by chromium-style for anything containing non-POD members. See
compiler output below:

../../chromecast/crash/cast_crashdump_uploader.h:19:1: error: [chromium-style]
Complex class/struct needs an explicit out-of-line constructor.
struct CastCrashdumpData {
^
../../chromecast/crash/cast_crashdump_uploader.h:19:1: error: [chromium-style]
Complex class/struct needs an explicit out-of-line destructor.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/cast_...
chromecast/crash/cast_crashdump_uploader.cc:107: LOG(INFO) << "Sending request
to " << data_.crash_server;
On 2015/06/15 18:16:12, gunsch wrote:
> On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> > VLOG?
> > 
> > I am not sure what convention we follow in cast code to decide which logging
> > macro to use.
> 
> In general we stick with LOG(INFO) for things we want to see in our logs. We
> don't have the same automatic pushback to LOG(INFO) that Chrome team does.
> 
> This is especially the case since logs from user reports are one of our
primary
> debugging mechanisms.

Acknowledged.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/cast_...
File chromecast/crash/cast_crashdump_uploader.h (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/cast_...
chromecast/crash/cast_crashdump_uploader.h:53: std::map<std::string /* label */,
std::string /* file */> attachments_;
On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> nit: It might be easier to read if the documentation for key-value pairs is
> included separately as <label, file>.

Done.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
File chromecast/crash/linux/crash_util.cc (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
chromecast/crash/linux/crash_util.cc:28: }  // namespace
On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> nit: space

Done.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
File chromecast/crash/linux/crash_util.h (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
chromecast/crash/linux/crash_util.h:19: static void RequestUploadCrashDump(const
std::string& existing_minidump_path,
On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> should it return success/failure?

Done.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
File chromecast/crash/linux/dump_info.h (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
chromecast/crash/linux/dump_info.h:40: std::string crashed_process_dump() const
{ return crashed_process_dump_; }
On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> return const reference. here and elsewhere in this file.

Done.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
File chromecast/crash/linux/minidump_manager.h (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
chromecast/crash/linux/minidump_manager.h:23: class MinidumpManager {
On 2015/06/15 17:51:49, Alok Priyadarshi wrote:
> It is not clear from the class name what this class does.

Renamed to SynchronizedMinidumpManager

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
chromecast/crash/linux/minidump_manager.h:29: int DoWorkLocked();
On 2015/06/15 17:51:48, Alok Priyadarshi wrote:
> I would rename this to what this function actually does - "DoWork" is very
> generic and does not convey anything. Also, since locking is an internal
concept
> for derived classes, "Locked" should not be part of the public function name.

DoWorkLocked() is now AcquireLockAndDoWork(), a protected method. It is up to
the derived class to expose a method which calls this, effectively hiding this
impl detail from the public interface. For an example, please see
MinidumpWriter.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
chromecast/crash/linux/minidump_manager.h:32: bool nonblocking() { return
nonblocking_; }
On 2015/06/15 17:51:49, Alok Priyadarshi wrote:
> nit: non_blocking.

Done.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/crash/linux...
chromecast/crash/linux/minidump_manager.h:35: void SetNonblocking(bool
nonblocking) { nonblocking_ = nonblocking; }
On 2015/06/15 17:51:49, Alok Priyadarshi wrote:
> set_non_blocking

Done.

https://codereview.chromium.org/1154383006/diff/200001/chromecast/media/BUILD.gn
File chromecast/media/BUILD.gn (right):

https://codereview.chromium.org/1154383006/diff/200001/chromecast/media/BUILD...
chromecast/media/BUILD.gn:15: test("cast_media_unittests") {
On 2015/06/15 18:14:40, Alok Priyadarshi wrote:
> On 2015/06/15 18:05:59, gunsch wrote:
> > On 2015/06/15 17:51:49, Alok Priyadarshi wrote:
> > > I would keep the target name as unittests and add an output name so that
it
> > does
> > > not conflict with other targets in the out directory. Referencing this
> target
> > > from other GN files looks prettier that way.
> > 
> > Disagreed. See:
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=file:BUILD.gn$%20_uni...
> > 
> > Convention seems to be to use full test name.
> 
> You are right. Thanks for including the search results.

Acknowledged.

[gunsch, 2015-06-18 18:34:22]

lgtm!

[gunsch, 2015-06-18 18:35:57]

https://codereview.chromium.org/1154383006/diff/220001/chromecast/base/error_...
File chromecast/base/error_codes.h (right):

https://codereview.chromium.org/1154383006/diff/220001/chromecast/base/error_...
chromecast/base/error_codes.h:25: // END_OF_REBOOT_SECTION is defined in
content/shell/ui/resources/script.js
note: this file path doesn't exist, it might be a very old holdover from merge29
days

[slan, 2015-06-22 04:45:04]

https://codereview.chromium.org/1154383006/diff/220001/chromecast/base/error_...
File chromecast/base/error_codes.h (right):

https://codereview.chromium.org/1154383006/diff/220001/chromecast/base/error_...
chromecast/base/error_codes.h:25: // END_OF_REBOOT_SECTION is defined in
content/shell/ui/resources/script.js
On 2015/06/18 18:35:57, gunsch wrote:
> note: this file path doesn't exist, it might be a very old holdover from
merge29
> days

Great catch. This value is defined in an internal webui js file, so I'll modify
the note, but keep this value defined.

[slan, 2015-06-22 15:53:51]

The CQ bit was checked by slan@chromium.org

[slan, 2015-06-22 15:53:52]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1154383006/#ps260001
(title: "Nit")

[commit-bot: I haz the power, 2015-06-22 15:54:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154383006/260001

[commit-bot: I haz the power, 2015-06-22 15:54:35]

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

[commit-bot: I haz the power, 2015-06-22 15:54:36]

CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true
and NOPRESUBMIT=true in order for the CQ to process them

[gunsch, 2015-06-22 15:55:31]

Looks like you've got the wrong Base URL :)

[slan, 2015-06-22 15:58:24]

The CQ bit was checked by slan@chromium.org

[slan, 2015-06-22 15:58:25]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1154383006/#ps280001
(title: "Change target ref")

[commit-bot: I haz the power, 2015-06-22 15:58:56]

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

[slan, 2015-06-22 15:59:22]

Should be fixed. Sorry for the spam, all!

On Mon, Jun 22, 2015 at 8:55 AM, <gunsch@chromium.org> wrote:

> Looks like you've got the wrong Base URL :)
>
> https://codereview.chromium.org/1154383006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2015-06-22 16:17:18]

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

[commit-bot: I haz the power, 2015-06-22 16:17:20]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)

[slan, 2015-06-22 16:35:02]

The CQ bit was checked by slan@chromium.org

[slan, 2015-06-22 16:35:03]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1154383006/#ps300001
(title: "Fix old Android build issue.")

[commit-bot: I haz the power, 2015-06-22 16:35:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154383006/300001

[commit-bot: I haz the power, 2015-06-22 17:04:44]

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

[commit-bot: I haz the power, 2015-06-22 17:04:46]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)

[gunsch, 2015-06-22 17:07:54]

https://codereview.chromium.org/1154383006/diff/300001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/1154383006/diff/300001/chromecast/chromecast....
chromecast/chromecast.gyp:99: '../content/public/common/content_switches.cc',
whoa, when did this get added? That's definitely going to cause problems
(obviously per the build). I think I missed this during review.

[slan, 2015-06-22 21:24:54]

Please stand by while Android crash client is moved to new location as well.

https://codereview.chromium.org/1154383006/diff/320001/chromecast/BUILD.gn
File chromecast/BUILD.gn (right):

https://codereview.chromium.org/1154383006/diff/320001/chromecast/BUILD.gn#ne...
chromecast/BUILD.gn:31: "//chromecast/app:cast_shell_unittests",
Lexigraphical ordering.

https://codereview.chromium.org/1154383006/diff/320001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/1154383006/diff/320001/chromecast/chromecast....
chromecast/chromecast.gyp:120: 'target_name': 'cast_crash_client',
Android crash client should be moved here too.

[slan, 2015-06-22 22:32:25]

PTAL. The dependency violations have been cleared up. crash/ is now a set of
utilities depended on by cast_crash_client, which is an application-level
target.

https://codereview.chromium.org/1154383006/diff/320001/chromecast/BUILD.gn
File chromecast/BUILD.gn (right):

https://codereview.chromium.org/1154383006/diff/320001/chromecast/BUILD.gn#ne...
chromecast/BUILD.gn:31: "//chromecast/app:cast_shell_unittests",
On 2015/06/22 21:24:54, slan wrote:
> Lexigraphical ordering.

Done.

https://codereview.chromium.org/1154383006/diff/320001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/1154383006/diff/320001/chromecast/chromecast....
chromecast/chromecast.gyp:120: 'target_name': 'cast_crash_client',
On 2015/06/22 21:24:54, slan wrote:
> Android crash client should be moved here too.

Done.

[slan, 2015-06-22 22:36:12]

The CQ bit was checked by slan@chromium.org

[slan, 2015-06-22 22:36:13]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1154383006/#ps340001
(title: "Moved android crash client to app/ to keep dependencies consistent")

[commit-bot: I haz the power, 2015-06-22 22:37:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154383006/340001

[gunsch, 2015-06-22 22:37:52]

new structure lgtm % nit

also, looks like you'll need a //breakpad OWNER to sign off as well

https://codereview.chromium.org/1154383006/diff/340001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/1154383006/diff/340001/chromecast/chromecast....
chromecast/chromecast.gyp:96: 
nit: rm blank line (before/after dependencies)

[commit-bot: I haz the power, 2015-06-22 22:44:34]

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

[commit-bot: I haz the power, 2015-06-22 22:44:35]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[slan, 2015-06-22 22:54:58]

The CQ bit was checked by slan@chromium.org

[slan, 2015-06-22 22:54:59]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1154383006/#ps290037
(title: "Resolving merge.")

[commit-bot: I haz the power, 2015-06-22 22:56:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154383006/290037

[wzhong, 2015-06-22 23:02:32]

https://codereview.chromium.org/1154383006/diff/340001/chromecast/crash/linux...
File chromecast/crash/linux/dump_info_unittest.cc (right):

https://codereview.chromium.org/1154383006/diff/340001/chromecast/crash/linux...
chromecast/crash/linux/dump_info_unittest.cc:29: struct tm tm;
You have flakiness for DST, right? 

tm_isdst not initialized. tm = {} or memset.

[commit-bot: I haz the power, 2015-06-22 23:12:43]

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

[commit-bot: I haz the power, 2015-06-22 23:12:44]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[slan, 2015-06-22 23:25:01]

The CQ bit was checked by slan@chromium.org

[slan, 2015-06-22 23:25:01]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1154383006/#ps370001
(title: "Added cast_shell_unittests to test_generator")

[commit-bot: I haz the power, 2015-06-22 23:25:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154383006/370001

[slan, 2015-06-22 23:28:13]

Added cast_shell_unittests to the test_generator target

https://codereview.chromium.org/1154383006/diff/340001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/1154383006/diff/340001/chromecast/chromecast....
chromecast/chromecast.gyp:96: 
On 2015/06/22 22:37:52, gunsch wrote:
> nit: rm blank line (before/after dependencies)

Done.

https://codereview.chromium.org/1154383006/diff/340001/chromecast/crash/linux...
File chromecast/crash/linux/dump_info_unittest.cc (right):

https://codereview.chromium.org/1154383006/diff/340001/chromecast/crash/linux...
chromecast/crash/linux/dump_info_unittest.cc:29: struct tm tm;
On 2015/06/22 23:02:32, wzhong wrote:
> You have flakiness for DST, right? 
> 
> tm_isdst not initialized. tm = {} or memset.

Good catch. The flakiness persists though. I think that the test running with
multiple threads is causing concurrency issues accessing a static member.

[slan, 2015-06-22 23:36:55]

slan@chromium.org changed reviewers:
+ thestig@chromium.org

[slan, 2015-06-22 23:36:56]

+thestig

4 files depending on breakpad have been moved from chromecast/crash/android to
chromecast/app/android. DEPS file being updated is
//chromecast/app/android/DEPS.

PTAL. Thank you!

[commit-bot: I haz the power, 2015-06-22 23:38:07]

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

[commit-bot: I haz the power, 2015-06-22 23:38:08]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Lei Zhang, 2015-06-22 23:54:56]

On 2015/06/22 23:36:56, slan wrote:
> +thestig
> 
> 4 files depending on breakpad have been moved from chromecast/crash/android to
> chromecast/app/android. DEPS file being updated is
> //chromecast/app/android/DEPS.
> 
> PTAL. Thank you!

lgtm

BTW, chromecast/crash/DEPS has an entry for "+components/crash" that's no longer
used.

[slan, 2015-06-23 15:24:45]

On 2015/06/22 23:54:56, Lei Zhang wrote:
> On 2015/06/22 23:36:56, slan wrote:
> > +thestig
> > 
> > 4 files depending on breakpad have been moved from chromecast/crash/android
to
> > chromecast/app/android. DEPS file being updated is
> > //chromecast/app/android/DEPS.
> > 
> > PTAL. Thank you!
> 
> lgtm
> 
> BTW, chromecast/crash/DEPS has an entry for "+components/crash" that's no
longer
> used.

Thanks for the catch!

[slan, 2015-06-23 15:25:08]

The CQ bit was checked by slan@chromium.org

[slan, 2015-06-23 15:25:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1154383006/#ps390001
(title: "Clean up DEPS")

[commit-bot: I haz the power, 2015-06-23 15:25:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154383006/390001

[commit-bot: I haz the power, 2015-06-23 15:51:01]

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

[commit-bot: I haz the power, 2015-06-23 15:51:03]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)

[slan, 2015-06-23 18:08:36]

The CQ bit was checked by slan@chromium.org

[slan, 2015-06-23 18:08:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1154383006/#ps410001
(title: " ")

[commit-bot: I haz the power, 2015-06-23 18:11:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154383006/410001

[slan, 2015-06-23 18:44:16]

The CQ bit was checked by slan@chromium.org

[slan, 2015-06-23 18:44:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1154383006/#ps430001
(title: "cast_shell_unittests not built for android")

[commit-bot: I haz the power, 2015-06-23 18:44:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154383006/430001

[commit-bot: I haz the power, 2015-06-23 19:42:09]

Committed patchset #23 (id:430001)

[commit-bot: I haz the power, 2015-06-23 19:42:59]

Patchset 23 (id:??) landed as
https://crrev.com/f48fe876ae18ebe0a639eb271c3b7192f410bc0b
Cr-Commit-Position: refs/heads/master@{#335727}