Due to not having type check in python, both the implementation and the
test were assuming `OldContents()` and `NewContents()` to be string.
They are list of strings instead.
The function `count(str)` being defined on both, it was passing locally,
when I was using DanglingUntriaged on its own line.
Bug: chromium:1395297
Change-Id: I2b20e92e2dfd0054bb4b2c3630f35af20661bbdc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5012813
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1222807}
Non-inlined constexpr variable definitions in a header lead every file
that includes this header to create a separate copy of the constant.
There appears to be no upside, but potential downsides (larger binary
size, differing memory addresses when one expect them to be equal)
from that behavior.
This CL adds a PRESUBMIT warning for changed or modified non-inline
constexpr variable definitions in headers. The check excludes
a) definitions in .cc files (which should hopefully not be included)
b) static constexpr member definitions inside structs and classes,
as these are implicitly inline.
Relevant links:
- https://groups.google.com/a/chromium.org/g/cxx/c/hmyGFD80ocE
(includes a discussion of adding this check)
- https://abseil.io/tips/168
Trial CL to just replace all non-inline constexpr seems to make bots
happy (apart from presubmit checks):
https://chromium-review.googlesource.com/c/chromium/src/+/4708325
Bug: 1467190
Change-Id: I187cecc0cb0676ee0e5950a6791f15cba96cb19c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4710609
Reviewed-by: Dominic Battre <battre@chromium.org>
Commit-Queue: Jan Keitel <jkeitel@google.com>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1209049}
This is a reland of commit 8228ad6aef
What happens in the reland:
---------------------------
I added an early return when `input_api.nodiff` is true. This disable
the check completely.
Performance evaluation:
-----------------------
Because the original presubmit check was reverted due to the performance
cost when running `git cl presubmit --all`, it comes the question of the
performance cost during normal `git cl presubmit`.
I am evaluating the cost to be in the range: [-0.178%, -1.04%].
This is the 95 confidence interval using a t-test.
This was tested on a very large patch modifying 535 files. The data was:
Without With
------- ----
91.1s 91.5s
91.6s 91.8s
91.6s 92.1s
90.9s 92.0s
91.3s 91.9s
Original change's description:
> PRESUBMIT: Warn against adding new dangling pointers.
>
> The DanglingPointerDetector is enabled on the CQ. It means we are now
> blocking new dangling pointers and can detect regressions.
>
> This blocks ~10 patches per day. The error message contains links toward
> the documentation and a guide to fix dangling pointers:
> - docs/dangling_ptr.md
> - docs/dangling_ptr_guide.md
>
> This is enough for most developers to fix them. However, it happens some
> are simply adding new `DanglingUntriaged` occurrences. Most are
> copy-pasting nearby code thinking this is the correct way to do.
>
> This patch adds a PRESUBMIT warning developers.
>
> Change-Id: I05d09e3f988f65b461a040c76a6ba2904073864b
> Bug: chromium:1395297
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4853628
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1200144}
Bug: chromium:1395297
Change-Id: I0a8305383eda3c3b50c0f433bd6751cf19069584
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4907727
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1204558}
This reverts commit 8228ad6aef.
Reason for revert: https://crbug.com/1486612
The check takes too long when running `git cl presubmit --all`.
I will reland it later with a different strategy.
Original change's description:
> PRESUBMIT: Warn against adding new dangling pointers.
>
> The DanglingPointerDetector is enabled on the CQ. It means we are now
> blocking new dangling pointers and can detect regressions.
>
> This blocks ~10 patches per day. The error message contains links toward
> the documentation and a guide to fix dangling pointers:
> - docs/dangling_ptr.md
> - docs/dangling_ptr_guide.md
>
> This is enough for most developers to fix them. However, it happens some
> are simply adding new `DanglingUntriaged` occurrences. Most are
> copy-pasting nearby code thinking this is the correct way to do.
>
> This patch adds a PRESUBMIT warning developers.
>
> Change-Id: I05d09e3f988f65b461a040c76a6ba2904073864b
> Bug: chromium:1395297
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4853628
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1200144}
Bug: chromium:1395297
Change-Id: I5c6ade4930e0383dcb8f139ca80451ab05930d11
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4889652
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Auto-Submit: Arthur Sonzogni <arthursonzogni@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1201647}
The DanglingPointerDetector is enabled on the CQ. It means we are now
blocking new dangling pointers and can detect regressions.
This blocks ~10 patches per day. The error message contains links toward
the documentation and a guide to fix dangling pointers:
- docs/dangling_ptr.md
- docs/dangling_ptr_guide.md
This is enough for most developers to fix them. However, it happens some
are simply adding new `DanglingUntriaged` occurrences. Most are
copy-pasting nearby code thinking this is the correct way to do.
This patch adds a PRESUBMIT warning developers.
Change-Id: I05d09e3f988f65b461a040c76a6ba2904073864b
Bug: chromium:1395297
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4853628
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1200144}
- Add `enum class base::internal::MemorySafetyCheck` - each enum item
represents memory safety check, implemented in PA and UaF detector.
- Add macro `ADVANCED_MEMORY_SAFETY_CHECKS()` - when used inside class
declaration, it specifies a set of checks
`base::internal::kAdvancedMemorySafetyChecks` enforced over that
object type.
- This macro will be used as a part of MiracleObject project,
see the bug for the details.
- Add following constexpr functions to determine enabled safety checks for type
`T`.
- `base::internal::get_memory_safety_checks<T>`
- `base::internal::is_memory_safety_checked<T, Checks>`
Bug: 1462223
Change-Id: I611a825d260705ed110932f08cbe756ab44ea7e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4664244
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Mikihito Matsuura <mikt@google.com>
Cr-Commit-Position: refs/heads/main@{#1194241}
Tweak presubmit warnings that are intended to encourage
developers to add Android dump tree/event test expectations:
* No warnings when other platform expectations are not added -- it's
common to only care about the Blink output, or to not even care
about the output but just test for crashes.
* No warnings on modifications or deletes -- there isn't a high
value for these, and it makes the other new tweaks more
difficult.
This should significantly reduce noise on presubmits and thus
developer friction. It will also reduce times when developers
not as familiar with our requirements will go overboard in
producing platform expectations files.
R=mschillaci@chromium.org
Bug: none
Change-Id: I9557cc38e920c5dab25490d5d81942bd3c187a3e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4794290
Auto-Submit: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: Mark Schillaci <mschillaci@google.com>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1185423}
The PRESUBMIT checks look for @Batch or @DoNotBatch on test files and
give warnings if they are not present. These should not include
simple @interface's like DisabledTest.java or EnormousTest.java etc.
AX-Relnotes: N/A
Bug: N/A
Change-Id: I3a3278925a6d78f548e4154aca52f12e600f37b6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4697253
Commit-Queue: Mark Schillaci <mschillaci@google.com>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: James Shen <zhiyuans@google.com>
Cr-Commit-Position: refs/heads/main@{#1172010}
This was happening for me locally. Ctrl-C showed:
Traceback (most recent call last):
File "build/print_python_deps.py", line 186, in <module>
sys.exit(main())
File "build/print_python_deps.py", line 182, in main
out.write(prefix + path.replace('\\', '/') + '\n')
BrokenPipeError: [Errno 32] Broken pipe
Which means the process was blocked waiting for something to read its
output.
Bug: None
Change-Id: Ibcb45a6d8bf3af882fab3f0c3b0b607277817eb1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4673673
Reviewed-by: Peter Wen <wnwen@chromium.org>
Commit-Queue: Peter Wen <wnwen@chromium.org>
Auto-Submit: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1169982}
Updates the UNIT_TEST presubmit check to do more robust checking
for the UNIT_TEST #define used in source files. Specifically,
the check will now look for cases such as `#if defined(UNIT_TEST)`.
Also, remove UNIT_TEST ifdef blocks from one .cc file that was
using them.
Bug: None
Change-Id: I541d3d6de139f59a32285962afd9035ae99d2706
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4672065
Commit-Queue: Andrew Williams <awillia@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1168134}
Add <if> conditionals to chromium_strings.grd to replace the most
prominent strings in the UI:
- Global menu bar
- 3-dot menu
- View site information
- Settings > Default browser
- Settings > About
As a trade-off, we choose this approach over creating
a new GRD file for CfT as it would be costly to maintain otherwise,
in spite of the considerations in b/1110882.
CfT strings should be identical to Chromium strings, with the exception
of the product name, which is replaced from "Chromium" with "Chromium
for Testing" as per
https://goo.gle/chrome-for-testing#bookmark=id.n1rat320av91
Bug: 1428061
Change-Id: I7cb5afe0d619551daea3204b9611854d36f5af98
Tested: `autoninja -C out/Default chrome/app:chromium_strings components/strings:components_chromium_strings chrome`
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4582551
Reviewed-by: Jens Mueller <muellerj@google.com>
Commit-Queue: Thiago Perrotta <tperrotta@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1153347}
Benmason@ explained that a recent submission of a png file as png.sha1
file resulted in all images on that day not to be uploaded to TC.
2nd attempt after https://crrev.com/c/4374003 was reverted due to
"Evaluation of CheckStrings failed: Access outside the repository root is denied."
This is now using input_api.AffectedFiles().NewContents instead of
opening the file directly.
Bug: 1428712
Change-Id: Ia1bcb5b132f5d92ce7378a986be74b9958175680
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4432832
Reviewed-by: Ben Mason <benmason@chromium.org>
Reviewed-by: Dominic Battre <battre@chromium.org>
Commit-Queue: Ben Mason <benmason@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1142068}
The java changes should be a noop since we automatically do this rewrite
in compile_java.py. Also replace
//third_party/android_support_test_runner targets with
//third_party/androidx ones.
Additionally fix non-java files eg: docs, presubmits, scripts
This affects files outside //android_webview, //chrome, //components and
//weblayer which are covered by other cls.
Bug: 1428304
Change-Id: I2906b7530127983e6110e9ab5773843df0dd3ba8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4375857
Reviewed-by: Peter Wen <wnwen@chromium.org>
Owners-Override: Peter Wen <wnwen@chromium.org>
Commit-Queue: Mohamed Heikal <mheikal@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1123104}
The presubmit currently prevents moving a JavaScript file to a new
location. This CL allows such CLs by using a warning instead of an error
if a script of the same name was deleted and added in the examined diff.
Change-Id: I0c5761b5f774bea8a6733e78c2ef0a31d47eab9a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4335695
Reviewed-by: Dominic Battré <battre@chromium.org>
Commit-Queue: Mike Dougherty <michaeldo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1117004}
VectorDrawableCompat Could take a long time to complete, so adding
a ban to let users use the alternative class with trace events.
BUG=b/242038130
Change-Id: I174c918e7dd5fef15a551eace33d1bdfb3d203e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4266131
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1108408}
When a new crosapi mojom file being added, we expect there should
also be a browser test being added.
This is a non-blocker check and only happens during git cl upload.
See my local test:
:~/chromium/src$ git diff HEAD~1
diff --git a/chromeos/crosapi/mojom/newfile.mojom b/chromeos/crosapi/mojom/newfile.mojom
new file mode 100644
index 0000000000000..fa1237e46e92b
--- /dev/null
+++ b/chromeos/crosapi/mojom/newfile.mojom
@@ -0,0 +1,4 @@
+// Copyright 2022 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
:~/chromium/src$ git cl upload
Running Python 3 presubmit upload checks ...
** Presubmit Warnings: 1 **
You are adding a new crosapi, but there is no file ends with browsertest.cc file being added. It is important to add crosapi browser test coverage to avoid version skew issues.
Check //docs/lacros/test_instructions.md for more information.
Presubmit checks took 2.5s to calculate.
There were Python 3 presubmit warnings. Are you sure you wish to continue? (y/N):
Bug: 1402823
Change-Id: Ibcac5f90fb7332cf7199e04a8eab48fda3ac5eb4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4118248
Reviewed-by: Erik Chen <erikchen@chromium.org>
Reviewed-by: Ben Pastene <bpastene@chromium.org>
Commit-Queue: Sven Zheng <svenzheng@chromium.org>
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1086084}
It can be helpful to distinguish between modified and added .grd
entries. This came up recently when trying to fix the indentation in a
.grd file and these more specific messages _might_ have helped.
Typical messages will look like this:
** Presubmit ERRORS: 1 **
You are modifying UI strings or their meanings.
To ensure the best translations, take screenshots of the relevant UI (https://g.co/chrome/translation) and add these files to your changelist:
chromeos\chromeos_strings_grd\IDS_SCANNING_APP_SCAN_BUTTON_TEXT.png.sha1
Actually fixing the indentation will be tried in a follow-up.
Bug: 1394970
Change-Id: I60fee3086a9826ef2f233c681164e5b309a6eec6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4081196
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1081514}
Provide a warning when rebase_path specifies target_gen_dir and an
absolute path. This can cause build failures when building Chromium
with Portage because that can keep the out/ directory outside of the
source root. Advise that root_build_dir is the right way to specify
the path.
Bug: 1376581
Change-Id: I8ef0a9c32358e086abfffa53b61c4c4189cee185
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3964725
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Sean Kau <skau@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@google.com>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1063502}
This is a reland of commit 6168e0b65d
Original change's description:
> Add presubmit check to prevent new JavaScript in iOS directories
>
> `//ios/*` now fully supports TypeScript which is preferred over
> JavaScript for new feature scripts.
>
> In addition to a PRESUBMIT error, the boilerplate script was updated to
> help developers avoid creating new JS files. Without this check,
> developers may not be aware of the restriction until their feature is
> implemented is complete locally.
>
> Bug: 1325547
> Change-Id: I40a9cc9f701f5396d14c7b87648ebff747bb6f47
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3939161
> Commit-Queue: Mike Dougherty <michaeldo@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Robert Sesek <rsesek@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1060096}
Bug: 1325547
Change-Id: I0aa048972377afa434350803079049169a0dcd82
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3964730
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Commit-Queue: Mike Dougherty <michaeldo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1061334}
This reverts commit 6168e0b65d.
Reason for revert: The presubmit is buggy, and triggers presubmit errors on existing files. See crbug.com/1376103
Original change's description:
> Add presubmit check to prevent new JavaScript in iOS directories
>
> `//ios/*` now fully supports TypeScript which is preferred over
> JavaScript for new feature scripts.
>
> In addition to a PRESUBMIT error, the boilerplate script was updated to
> help developers avoid creating new JS files. Without this check,
> developers may not be aware of the restriction until their feature is
> implemented is complete locally.
>
> Bug: 1325547
> Change-Id: I40a9cc9f701f5396d14c7b87648ebff747bb6f47
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3939161
> Commit-Queue: Mike Dougherty <michaeldo@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Robert Sesek <rsesek@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1060096}
Bug: 1325547, 1376103
Change-Id: I343d5e13c3ab4fb58fc30acc257257541c15ec2c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3964250
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Bruce Dawson <brucedawson@chromium.org>
Commit-Queue: Mike Dougherty <michaeldo@chromium.org>
Reviewed-by: Mike Dougherty <michaeldo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1060521}
`//ios/*` now fully supports TypeScript which is preferred over
JavaScript for new feature scripts.
In addition to a PRESUBMIT error, the boilerplate script was updated to
help developers avoid creating new JS files. Without this check,
developers may not be aware of the restriction until their feature is
implemented is complete locally.
Bug: 1325547
Change-Id: I40a9cc9f701f5396d14c7b87648ebff747bb6f47
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3939161
Commit-Queue: Mike Dougherty <michaeldo@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1060096}
View::Layout() is getting an overhaul. Direct calls to Layout() from within tests and eventually production code will be forbidden. There are new test-only functions available to manually invoke a layout pass properly.
This presubmit will attempt catch direct calls to Layout() from within files ending in unittest, browsertest, or ui_test. It should still allow Layout() overrides. While this presubmit should not trigger for existing calls to Layout(), those calls will have been eliminated in other CLs.
To manually test this presubmit:
> git cl presubmit --force --files path_to/some/test/foo_unittest.cc
This was tested with and without the Layout() call and was confirmed to trigger when the line of code with the Layout() call was added or changed.
Change-Id: Ib784203e7f795cc4a437508d4c37aecf8f90d6fd
Bug: 1350521
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3899633
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1050232}
This function was previously optimized (70x faster?) by not calling
f.OldContents() so often, but the correct call count is actually zero.
The correct way to tell whether a file is new in the presubmit system
is f.Action() == 'A'. The three possible actions are 'A'dded,
'M'odified, and 'D'eleted.
Note that there is no guarantee that the mocked files will be
internally consistent which is why I had to adjust the tests. Also
note that a test that the comment says was supposed to fail has been
passing for years. This change does not address that.
Bug: 1309977
Change-Id: I846b0403c1e196daf620dda8a02b4de82d8bf463
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3895053
Reviewed-by: Mike Pinkerton <pinkerton@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1046909}
When I read the Chromium source, I first noticed that some python files for presubmit located in the root directory had some problems (like incorrect function names, indentation, typos, etc.), so I tried to fix them.
BUG=nobug
Change-Id: I3646b9fc1761d3cd2af884aba8d815d09665e7be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3812981
Reviewed-by: Erik Staab <estaab@chromium.org>
Commit-Queue: Erik Staab <estaab@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1032601}
So far it only applies to png, svg and icon files, and only complains
about ones with "google" in the name (although this could fail to
catch some cases like `gmail.icon`).
Bug: 1245662
Change-Id: I5e4fa3520f4532aeab6965100ae7097788e13e85
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3808278
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1031665}
This CL attempts to limit the `DCHECK_IS_ON()` parentheses presubmit to
cases where exactly that macro is being used. Outside that scope,
`FOO_DCHECK_IS_ON` is permissible, as is `BUILDFLAG(FOO_DCHECK_IS_ON)`.
Change-Id: I15923ea342dc0e76714400ceb2947d5ac0235312
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3669918
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Commit-Queue: Kalvin Lee <kdlee@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1007856}
Since the original bug is primarily about the case of missed reviews
when an OWNERS file is newly modified, only trigger it when new per-file
security review restrictions are added to OWNERS files.
This still adds a bit of annoyance for initial changes in directories
where //chromeos/LACROS_OWNERS or //chromeos/SECURITY_OWNERS should also
be able to security review, but it becomes a one-time signoff instead.
This seems like an acceptable tradeoff between ease of use and not
missing security reviews.
Bug: 1327341
Change-Id: Ia0263c9b5b185322a83f92cd0fc552bfa1196841
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3657265
Reviewed-by: Alex Gough <ajgo@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1006055}
Originally, this was selected to match the warning level of other
security owners checks. However:
- The other security owners check has been upgraded into an error at all
times, to prevent less last-minute surprises from missing OWNERS.
- Having a warning that prompts at upload time is annoying when
repeatedly uploading CLs for something that's still heavily in
revision.
Bug: 801315
Change-Id: I7fb2ebc3e1f491fb4274ddb2a966fa041c419e9c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3655008
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1005079}
The security owners check only issues a blocking error now if there is
a Gerrit issue attached to the change: it is not possible (through the
normal tooling anyway) to directly commit a change without first
uploading it to Gerrit, so no safety is lost there. In addition, the
security owners check now respects Owners-Override as well.
Testing this fix also revealed an issue where the security review
exceptions for Blink metrics enums did not work quite correctly, since
the same directory contained both mojoms that required security review
and mojoms that did not.
To fix this, move all the mojoms that do not require a security review
into their own subdirectory and merge the PRESUBMIT scripts into one,
updating the PRESUBMIT to the latest style.
Moving the enums also required updating numerous paths throughout
Chrome. It also slightly changes the output of the enum validation
script, so regenerate enums.xml to reflect the new paths. This also
updates the shebang line in the enum generation scripts to use
python3 explicitly instead of just python (which is often not present
on gLinux machines, or is mapped to python2).
Bug: 801315
Change-Id: If1809126a0103e295b7c350a8996401c766a2931
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3648594
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Owners-Override: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1003666}
CQ+1 runs the presubmit commit hooks, but it is not useful for the
missing security reviewers check to require approval, since there may be
reviewers that have yet to respond. Only CQ+2 should generate an error
for a missing approval.
Bug: 801315
Change-Id: I76ed35d515987a1efb9a8a2344210bc601a22849
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3646825
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Giovanni Ortuno Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1002990}
The current security owners check ensures that the per-file rules are
present in the various OWNERS files; however, for security reasons [1],
the actual owners checks evaluates owners requirements against the
*currently* checked-in version of OWNERS files and not the updated
version of OWNERS files being reviewed. The net result is that it was
a fairly common occurrence to add a new .mojom file, add new per-file
restrictions to OWNERS, but not actually send it to a security reviewer.
It turns out that there is already logic to check for security reviewers
for changes that include calls to security-sensitive functions. This
logic has been factored out so it can be reused by the per-file security
owners checker. In addition:
- The required per-file rules checker has been refactored so that IPC
and Fuchsia can re-use the same core logic.
- ipc-security-reviews@chromium.org is no longer CCed on changes that
are security-sensitive for Fuchsia but not IPC.
- Test logic consolidation and cleanup.
[1] If the owners requirements were evaluated against the updated
version of OWNERS files being reviewed, it would be possible for someone
to add a new OWNER and for that newly-added OWNER to approve the CL
adding themselves.
Bug: 801315
Change-Id: If824e4698368a089474ba992601edbcf4c7d009c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3641150
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1002697}
The messages for missing include guards are quite verbose, listing the
file name and recommended include guard twice each. This is because they
set message, items, and long_text. When there is just a single warning
this is okay, but when there is a long list of warnings it makes it
harder to read them. The old output looks like this:
Missing include guard CONTENT_SHELL_APP_RESOURCE_H_
content\shell\app\resource.h
***************
Missing include guard in content\shell\app\resource.h
Recommended name: CONTENT_SHELL_APP_RESOURCE_H_
This check can be disabled by having the string
no-include-guard-because-multiply-included in the header.
***************
The new warning - just setting message - looks like this:
Missing include guard in content\shell\app\resource.h
Recommended name: CONTENT_SHELL_APP_RESOURCE_H_
This check can be disabled by having the string
no-include-guard-because-multiply-included in the header.
All of the information is included, in less than half the space. The
presubmit tests were also updated as needed.
Bug: 1309977
Change-Id: Ia8c2de3ff3f1e71df6afc260c5edc1b430f80a17
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3579758
Reviewed-by: Mike Pinkerton <pinkerton@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/main@{#991060}
At this point, this is just a helper class for conveying info when
binding and unbinding an associated remote endpoint, so remove all other
functionality.
Bug: 875030, 955171
Change-Id: I7711b97deb306536fe4951f5a575ef1231c2ebec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3527063
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#982544}
Chrome Mojo interfaces should prefer one of the higher-level primitives
built on top of the native Mojo handle<shared_buffer>:
- mojo_base.mojom.ReadOnlySharedMemoryRegion
- mojo_base.mojom.WritableSharedMemoryRegion
- mojo_base.mojom.UnsafeSharedMemoryRegion
Fixed: 1132241
Change-Id: I082d512594db6cedb9146928e40f7387a492bbe2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3526187
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#981728}
This CL fixes a small typo in the PRESUBMIT script that was creating a
false positive warning during certain presubmit checks. We also make
a small change to the accessibility rebase script to make it more
python3 friendly.
AX-Relnotes: N/A
Bug: N/A
Change-Id: I51af1814a5d781dcbcf42609ca1e2f9965545ba4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3469668
Reviewed-by: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Mark Schillaci <mschillaci@google.com>
Cr-Commit-Position: refs/heads/main@{#972551}
