Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit-Queue | +1 |
Huanpo LinWhat is the diff?
The system sent out notification automatically and it wasn't updated yet. I've made updates and added descriptions. PTAL
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Diff from the original CLs:
Could you add 1. the reason for revert and then describe 2. how we fixed it.
https://chromium-review.googlesource.com/c/chromium/src/+/7015190
https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20MSan%20Tests/59207/overview
The reason is use-of-uninitialized-value, detected by MSan. (The code diff looks good.)
bool accept_language_overridden = false;
[nit]
https://chromium-review.googlesource.com/c/chromium/src/+/6719107/comment/fb8cd6c4_b75f0087/
Note that I personally prefer using designated initializer as using `false` is a logic of `ApplyEmulationOverrides()` and it's nice to sit in the function.
bool accept_language_overridden = false;
[nit]
https://chromium-review.googlesource.com/c/chromium/src/+/6719107/comment/fb8cd6c4_b75f0087/
Note that I personally prefer using designated initializer as using `false` is a logic of `ApplyEmulationOverrides()` and it's nice to sit in the function.
in this case. (I usually use default initialize params in headers to prevent use-before-initialization.)
Code-Review | +1 |
bool accept_language_overridden = false;
Ken Okada[nit]
https://chromium-review.googlesource.com/c/chromium/src/+/6719107/comment/fb8cd6c4_b75f0087/
Note that I personally prefer using designated initializer as using `false` is a logic of `ApplyEmulationOverrides()` and it's nice to sit in the function.
in this case. (I usually use default initialize params in headers to prevent use-before-initialization.)
IMHO it is natural enough to set default values here, given the risk of using uninitialized fields. But I don't have a strong opinion on this.
Commit-Queue | +1 |
Could you add 1. the reason for revert and then describe 2. how we fixed it.
https://chromium-review.googlesource.com/c/chromium/src/+/7015190
https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20MSan%20Tests/59207/overviewThe reason is use-of-uninitialized-value, detected by MSan. (The code diff looks good.)
Done
bool accept_language_overridden = false;
Ken Okada[nit]
https://chromium-review.googlesource.com/c/chromium/src/+/6719107/comment/fb8cd6c4_b75f0087/
Note that I personally prefer using designated initializer as using `false` is a logic of `ApplyEmulationOverrides()` and it's nice to sit in the function.
Takashi Nakayamain this case. (I usually use default initialize params in headers to prevent use-before-initialization.)
IMHO it is natural enough to set default values here, given the risk of using uninitialized fields. But I don't have a strong opinion on this.
`DevtoolsOverriddenOutputParams` is going to be used by `ApplyNetworkRequestOverrides` in another follow-up refactor CL. Maybe it will save more code to do it like the current PS instead of putting initialization into individual functions if all functions use the default value `false`?
cc the owner @rak...@chromium.org if have any preference
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
bool accept_language_overridden = false;
Ken Okada[nit]
https://chromium-review.googlesource.com/c/chromium/src/+/6719107/comment/fb8cd6c4_b75f0087/
Note that I personally prefer using designated initializer as using `false` is a logic of `ApplyEmulationOverrides()` and it's nice to sit in the function.
Takashi Nakayamain this case. (I usually use default initialize params in headers to prevent use-before-initialization.)
Huanpo LinIMHO it is natural enough to set default values here, given the risk of using uninitialized fields. But I don't have a strong opinion on this.
`DevtoolsOverriddenOutputParams` is going to be used by `ApplyNetworkRequestOverrides` in another follow-up refactor CL. Maybe it will save more code to do it like the current PS instead of putting initialization into individual functions if all functions use the default value `false`?
cc the owner @rak...@chromium.org if have any preference
Seems fine to use default values like in the current PS.
Code-Review | +1 |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
LGTM (defer to other reviewers for the existing discussions)