Attention is currently required from: Hidehiko Abe, Mark Mentovai.
To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Hidehiko Abe, Miriam Zimmerman.
Patch set 1:Code-Review +1
1 comment:
File handler/linux/cros_crash_report_exception_handler.cc:
Patch Set #1, Line 255: constexpr int32_t kFixedVersion = 15363;
This number doesn't mean anything to me, so I trust that you've gotten it right.
To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Hidehiko Abe.
2 comments:
Patchset:
Thanks!
File handler/linux/cros_crash_report_exception_handler.cc:
Patch Set #1, Line 255: constexpr int32_t kFixedVersion = 15363;
This number doesn't mean anything to me, so I trust that you've gotten it right.
Verifiable by looking at gerrit UI on https://crrev.com/c/4265753, which says "Landed in R113-15363.0.0"
To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Hidehiko Abe.
Patch set 1:Commit-Queue +1
Attention is currently required from: Hidehiko Abe.
1 comment:
Patchset:
this change breaks because mini_chromium doesn't include base/system/sys_info.h (or any of base/system)
To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Hidehiko Abe, Miriam Zimmerman.
3 comments:
Patchset:
Since this is temporary:
File handler/linux/cros_crash_report_exception_handler.cc:
Patch Set #1, Line 21: #include "base/system/sys_info.h"
You can wrap this in `#if defined(CRASHPAD_IS_IN_CHROMIUM)`.
Patch Set #1, Line 251: int32_t major_version = 0, minor_version = 0, bugfix_version = 0;
And do something similar here:
```
#if defined(CRASHPAD_IS_IN_CHROMIUM)
int32_t major_version = …
const bool use_chrome_signal_argument = major_version >= kFixedVersion;
#else
cosnt bool use_chrome_signal_argument = true;
#endif
if (use_chrome_signal_argument) {
const ExceptionShapshot* const exception_snapshot = …
}
```
To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Hidehiko Abe.
Patch set 2:Commit-Queue +1
2 comments:
File handler/linux/cros_crash_report_exception_handler.cc:
Patch Set #1, Line 21: #include "base/system/sys_info.h"
You can wrap this in `#if defined(CRASHPAD_IS_IN_CHROMIUM)`.
Done
Patch Set #1, Line 251: int32_t major_version = 0, minor_version = 0, bugfix_version = 0;
And do something similar here: […]
Falling back to a default of *false* just in case there's unexpected version skew with crashpad *not* in chromium.
To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Hidehiko Abe.
1 comment:
Patchset:
this change breaks because mini_chromium doesn't include base/system/sys_info. […]
Done
To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Hidehiko Abe, Miriam Zimmerman.
Patch set 2:Code-Review +1
1 comment:
File handler/linux/cros_crash_report_exception_handler.cc:
Patch Set #2, Line 21: #if defined(CRASHPAD_IS_IN_CHROMIUM)
One more thing I forgot to mention: in handler/BUILD.gn, you need to add config to get this set.
At:
```
static_library("handler") {
[…]
if (crashpad_is_linux) {
sources += [
"linux/cros_crash_report_exception_handler.cc",
"linux/cros_crash_report_exception_handler.h",
]
}
}
```
Introduce this config alongside the `sources`:
```
configs += [
"../build:crashpad_is_in_chromium",
]
```
along with a TODO to remove it in the future, just like the TODO in this file.
To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Hidehiko Abe.
Patch set 3:Commit-Queue +1
1 comment:
File handler/linux/cros_crash_report_exception_handler.cc:
Patch Set #2, Line 21: #if defined(CRASHPAD_IS_IN_CHROMIUM)
One more thing I forgot to mention: in handler/BUILD.gn, you need to add config to get this set. […]
Done
To view, visit change 4308129. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Hidehiko Abe, Miriam Zimmerman.
Patch set 3:Code-Review +1Commit-Queue +2
Crashpad LUCI CQ submitted this change.
Restrict new crash_reporter flag to valid versions
Lacros can be up to 2 milestones ahead of ash (and consequently the
platform code), so until the crash_reporter change has been in for 2
milestones, we need to manually check version compatibility.
BUG=chromium:1420445
TEST=Build, deploy, check that flag is set only on right version
Change-Id: Ic99d5ac58840814f7eeecd47c628ea0e8107f675
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/4308129
Commit-Queue: Mark Mentovai <ma...@chromium.org>
Reviewed-by: Mark Mentovai <ma...@chromium.org>
---
M handler/BUILD.gn
M handler/linux/cros_crash_report_exception_handler.cc
2 files changed, 28 insertions(+), 8 deletions(-)