Andrew Grieve | 8d9e40f | 2023-03-15 21:04:42 | [diff] [blame] | 1 | # Chromium Java Style Guide |
nyquist | 9d61f98 | 2017-02-10 00:29:08 | [diff] [blame] | 2 | |
| 3 | _For other languages, please see the [Chromium style |
John Palmer | be05130 | 2021-05-19 11:48:35 | [diff] [blame] | 4 | guides](https://chromium.googlesource.com/chromium/src/+/main/styleguide/styleguide.md)._ |
nyquist | 9d61f98 | 2017-02-10 00:29:08 | [diff] [blame] | 5 | |
| 6 | Chromium follows the [Android Open Source style |
| 7 | guide](http://source.android.com/source/code-style.html) unless an exception |
| 8 | is listed below. |
| 9 | |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 10 | You can propose changes to this style guide by sending an email to |
| 11 | `[email protected]`. Ideally, the list will arrive at some consensus and you can |
| 12 | request review for a change to this file. If there's no consensus, |
John Palmer | be05130 | 2021-05-19 11:48:35 | [diff] [blame] | 13 | [`//styleguide/java/OWNERS`](https://chromium.googlesource.com/chromium/src/+/main/styleguide/java/OWNERS) |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 14 | get to decide. |
| 15 | |
agrieve | 0e6bdf2 | 2018-08-03 14:25:24 | [diff] [blame] | 16 | [TOC] |
| 17 | |
Andrew Grieve | 99e388f | 2023-11-02 20:46:51 | [diff] [blame] | 18 | ## Java Language Features |
Nate Fischer | 03308e9 | 2022-11-07 18:14:59 | [diff] [blame] | 19 | |
Andrew Grieve | 99e388f | 2023-11-02 20:46:51 | [diff] [blame] | 20 | ### Type Deduction using "var" {#var} |
Nate Fischer | 03308e9 | 2022-11-07 18:14:59 | [diff] [blame] | 21 | |
| 22 | A variable declaration can use the `var` keyword in place of the type (similar |
| 23 | to the `auto` keyword in C++). In line with the [guidance for |
| 24 | C++](https://google.github.io/styleguide/cppguide.html#Type_deduction), the |
| 25 | `var` keyword may be used when it aids readability and the type of the value is |
| 26 | already clear (ex. `var bundle = new Bundle()` is OK, but `var something = |
| 27 | returnValueIsNotObvious()` may be unclear to readers who are new to this part of |
| 28 | the code). |
| 29 | |
| 30 | The `var` keyword may also be used in try-with-resources when the resource is |
| 31 | not directly accessed (or when it falls under the previous guidance), such as: |
| 32 | |
| 33 | ```java |
| 34 | try (var ignored = StrictModeContext.allowDiskWrites()) { |
| 35 | // 'var' is permitted so long as the 'ignored' variable is not used directly |
| 36 | // in the code. |
| 37 | } |
| 38 | ``` |
| 39 | |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 40 | ### Exceptions |
Andrew Grieve | 99e388f | 2023-11-02 20:46:51 | [diff] [blame] | 41 | |
Andrew Grieve | 1210d14 | 2024-06-06 16:31:47 | [diff] [blame] | 42 | A quick primer: |
| 43 | |
| 44 | * `Throwable`: Base class for all exceptions |
| 45 | * `Error`: Base class for exceptions which are meant to crash the app. |
| 46 | * `Exception`: Base class for exceptions that make sense the `catch`. |
| 47 | * `RuntimeException`: Base class for exceptions that do not need to be |
| 48 | declared as `throws` ("unchecked exceptions"). |
| 49 | |
| 50 | #### Broad Catch Handlers {#broad-catches} |
| 51 | |
| 52 | Use catch statements that do not catch exceptions they are not meant to. |
| 53 | * There is rarely a valid reason to `catch (Throwable t)`, since that |
| 54 | includes the (generally unrecoverable) `Error` types. |
| 55 | |
| 56 | Use `catch (Exception e)` when working with OS APIs that might throw |
| 57 | (assuming the program can recover from them). |
Andrew Grieve | 318b3532 | 2023-01-13 16:03:23 | [diff] [blame] | 58 | * There have been many cases of crashes caused by `IllegalStateException` / |
| 59 | `IllegalArgumentException` / `SecurityException` being thrown where only |
Andrew Grieve | 1210d14 | 2024-06-06 16:31:47 | [diff] [blame] | 60 | `RemoteException` was being caught. Unless catch handlers will differ |
| 61 | based on exception type, just catch `Exception`. |
| 62 | |
| 63 | Do not use `catch (RuntimeException e)`. |
| 64 | * It is useful to extend `RuntimeException` to make unchecked exception |
| 65 | types, but the type does not make much sense in `catch` clauses, as |
| 66 | there are not times when you'd want to catch all unchecked exceptions, |
| 67 | but not also want to catch all checked exceptions. |
| 68 | |
| 69 | #### Exception Messages {#exception-messages} |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 70 | |
Andrew Grieve | 318b3532 | 2023-01-13 16:03:23 | [diff] [blame] | 71 | Avoid adding messages to exceptions that do not aid in debugging. For example: |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 72 | |
agrieve | 50430de | 2018-08-15 17:49:16 | [diff] [blame] | 73 | ```java |
| 74 | try { |
Nate Fischer | fc38dc81 | 2024-04-24 17:07:26 | [diff] [blame] | 75 | somethingThatThrowsIOException(); |
agrieve | 50430de | 2018-08-15 17:49:16 | [diff] [blame] | 76 | } catch (IOException e) { |
Nate Fischer | fc38dc81 | 2024-04-24 17:07:26 | [diff] [blame] | 77 | // Bad - message does not tell you more than the stack trace does: |
| 78 | throw new RuntimeException("Failed to parse a file.", e); |
| 79 | // Good - conveys that this block failed along with the "caused by" exception. |
| 80 | throw new RuntimeException(e); |
| 81 | // Good - adds useful information. |
| 82 | throw new RuntimeException(String.format("Failed to parse %s", fileName), e); |
agrieve | 50430de | 2018-08-15 17:49:16 | [diff] [blame] | 83 | } |
| 84 | ``` |
| 85 | |
Andrew Grieve | 1210d14 | 2024-06-06 16:31:47 | [diff] [blame] | 86 | #### Wrapping with RuntimeException {#throw-unchecked} |
| 87 | |
| 88 | It is common to wrap a checked exception with a RuntimeException for cases |
| 89 | where a checked exception is not recoverable, or not possible. In order to |
| 90 | reduce the number of stack trace "caused by" clauses, and to save on binary |
| 91 | size, use [`JavaUtils.throwUnchecked()`] instead. |
| 92 | |
| 93 | ```java |
| 94 | try { |
| 95 | somethingThatThrowsIOException(); |
| 96 | } catch (IOException e) { |
| 97 | // Bad - RuntimeException adds no context and creates longer stack traces. |
| 98 | throw new RuntimeException(e); |
| 99 | // Good - Original exception is preserved. |
| 100 | throw JavaUtils.throwUnchecked(e); |
| 101 | } |
| 102 | ``` |
| 103 | |
| 104 | *** note |
| 105 | Do not use `throwUnchecked()` when the exception may want to be caught. |
| 106 | *** |
| 107 | |
| 108 | |
| 109 | [`JavaUtils.throwUnchecked()`]: https://source.chromium.org/search?q=symbol:JavaUtils.throwUnchecked |
| 110 | |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 111 | ### Asserts |
Andrew Grieve | 8d9e40f | 2023-03-15 21:04:42 | [diff] [blame] | 112 | |
Andrew Grieve | 08734276 | 2023-07-25 01:14:25 | [diff] [blame] | 113 | The build system: |
| 114 | * strips asserts in release builds (via R8), |
| 115 | * enables them in debug builds, |
| 116 | * and enables them in report-only mode for Canary builds. |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 117 | |
| 118 | ```java |
Andrew Grieve | 08734276 | 2023-07-25 01:14:25 | [diff] [blame] | 119 | // Code for assert expressions & messages is removed when asserts are disabled. |
| 120 | assert someCallWithoutSideEffects(param) : "Call failed with: " + param; |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 121 | ``` |
| 122 | |
Andrew Grieve | 08734276 | 2023-07-25 01:14:25 | [diff] [blame] | 123 | Use your judgement for when to use asserts vs exceptions. Generally speaking, |
| 124 | use asserts to check program invariants (e.g. parameter constraints) and |
| 125 | exceptions for unrecoverable error conditions (e.g. OS errors). You should tend |
| 126 | to use exceptions more in privacy / security-sensitive code. |
| 127 | |
| 128 | Do not add checks when the code will crash anyways. E.g.: |
| 129 | |
| 130 | ```java |
| 131 | // Don't do this. |
| 132 | assert(foo != null); |
| 133 | foo.method(); // This will throw anyways. |
| 134 | ``` |
| 135 | |
| 136 | For multi-statement asserts, use [`BuildConfig.ENABLE_ASSERTS`] to guard your |
| 137 | code (similar to `#if DCHECK_IS_ON()` in C++). E.g.: |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 138 | |
| 139 | ```java |
Nate Fischer | 4570ebc3 | 2021-06-04 00:44:45 | [diff] [blame] | 140 | import org.chromium.build.BuildConfig; |
| 141 | |
| 142 | ... |
| 143 | |
| 144 | if (BuildConfig.ENABLE_ASSERTS) { |
Nate Fischer | fc38dc81 | 2024-04-24 17:07:26 | [diff] [blame] | 145 | // Any code here will be stripped in release builds by R8. |
| 146 | ... |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 147 | } |
| 148 | ``` |
| 149 | |
Andrew Grieve | 08734276 | 2023-07-25 01:14:25 | [diff] [blame] | 150 | [`BuildConfig.ENABLE_ASSERTS`]: https://source.chromium.org/search?q=symbol:BuildConfig%5C.ENABLE_ASSERTS |
| 151 | |
Andrew Grieve | 99e388f | 2023-11-02 20:46:51 | [diff] [blame] | 152 | #### DCHECKS vs Java Asserts {#asserts} |
Andrew Grieve | 08734276 | 2023-07-25 01:14:25 | [diff] [blame] | 153 | |
| 154 | `DCHECK` and `assert` are similar, but our guidance for them differs: |
| 155 | * CHECKs are preferred in C++, whereas asserts are preferred in Java. |
| 156 | |
| 157 | This is because as a memory-safe language, logic bugs in Java are much less |
| 158 | likely to be exploitable. |
| 159 | |
Andrew Grieve | 99e388f | 2023-11-02 20:46:51 | [diff] [blame] | 160 | ### toString() {#toString} |
Andrew Grieve | 8d9e40f | 2023-03-15 21:04:42 | [diff] [blame] | 161 | |
Andrew Grieve | 99e388f | 2023-11-02 20:46:51 | [diff] [blame] | 162 | Use explicit serialization methods (e.g. `toDebugString()` or `getDescription()`) |
| 163 | instead of `toString()` when dynamic dispatch is not required. |
Andrew Grieve | 8d9e40f | 2023-03-15 21:04:42 | [diff] [blame] | 164 | |
Andrew Grieve | 99e388f | 2023-11-02 20:46:51 | [diff] [blame] | 165 | 1. R8 cannot detect when `toString()` is unused, so overrides will not be stripped |
| 166 | when unused. |
| 167 | 2. R8 cannot optimize / inline these calls as well as non-overriding methods. |
Andrew Grieve | 8d9e40f | 2023-03-15 21:04:42 | [diff] [blame] | 168 | |
Andrew Grieve | 99e388f | 2023-11-02 20:46:51 | [diff] [blame] | 169 | ### Records & AutoValue {#records} |
| 170 | |
| 171 | ```java |
| 172 | // Banned. |
| 173 | record Rectangle(float length, float width) {} |
| 174 | ``` |
| 175 | |
| 176 | **Rationale:** |
| 177 | * To avoid dead code: |
| 178 | * Records and `@AutoValue` generate `equals()`, `hashCode()`, and `toString()`, |
| 179 | which `R8` is unable to remove when unused. |
| 180 | * When these methods are required, implement them explicitly so that the |
| 181 | intention is clear. |
| 182 | * Also - supporting `record` requires build system work ([crbug/1493366]). |
| 183 | |
| 184 | Example with `equals()` and `hashCode()`: |
| 185 | |
| 186 | ```java |
| 187 | public class ValueClass { |
| 188 | private final SomeClass mObjMember; |
| 189 | private final int mIntMember; |
| 190 | |
| 191 | @Override |
| 192 | public boolean equals(Object o) { |
| 193 | return o instanceof ValueClass vc |
| 194 | && Objects.equals(mObjMember, vc.mObjMember) |
| 195 | && mIntMember == vc.mIntMember; |
| 196 | } |
| 197 | |
| 198 | @Override |
| 199 | public int hashCode() { |
| 200 | return Objects.hash(mObjMember, mIntMember); |
| 201 | } |
| 202 | } |
| 203 | ``` |
| 204 | |
| 205 | [crbug/1493366]: https://crbug.com/1493366 |
| 206 | |
| 207 | ### Enums |
| 208 | |
| 209 | Banned. Use [`@IntDef`](#intdefs) instead. |
| 210 | |
| 211 | **Rationale:** |
| 212 | |
| 213 | Java enums generate a lot of bytecode. Use constants where possible. When a |
| 214 | custom type hierarchy is required, use explicit classes with inheritance. |
Andrew Grieve | 8d9e40f | 2023-03-15 21:04:42 | [diff] [blame] | 215 | |
agrieve | 16c6fe8 | 2018-11-27 17:47:49 | [diff] [blame] | 216 | ### Finalizers |
Andrew Grieve | 8d9e40f | 2023-03-15 21:04:42 | [diff] [blame] | 217 | |
Andrew Grieve | ec828460 | 2023-10-16 15:53:25 | [diff] [blame] | 218 | In line with [Google's Java style guide] and [Android's Java style guide], |
agrieve | 16c6fe8 | 2018-11-27 17:47:49 | [diff] [blame] | 219 | never override `Object.finalize()`. |
| 220 | |
| 221 | Custom finalizers: |
| 222 | * are called on a background thread, and at an unpredicatble point in time, |
| 223 | * swallow all exceptions (asserts won't work), |
| 224 | * causes additional garbage collector jank. |
| 225 | |
| 226 | Classes that need destructor logic should provide an explicit `destroy()` |
John Palmer | be05130 | 2021-05-19 11:48:35 | [diff] [blame] | 227 | method. Use [LifetimeAssert](https://chromium.googlesource.com/chromium/src/+/main/base/android/java/src/org/chromium/base/LifetimeAssert.java) |
Bo Liu | 9bb53ca | 2020-09-22 00:48:10 | [diff] [blame] | 228 | to ensure in debug builds and tests that `destroy()` is called. |
agrieve | 16c6fe8 | 2018-11-27 17:47:49 | [diff] [blame] | 229 | |
Andrew Grieve | ec828460 | 2023-10-16 15:53:25 | [diff] [blame] | 230 | [Google's Java style guide]: https://google.github.io/styleguide/javaguide.html#s6.4-finalizers |
| 231 | [Android's Java style guide]: https://source.android.com/docs/setup/contribute/code-style#dont-use-finalizers |
| 232 | |
Andrew Grieve | 19c214d | 2025-01-14 20:50:06 | [diff] [blame] | 233 | ## Nullability Annotations |
| 234 | |
| 235 | A migration to add `@NullMarked` to all Java files is currently underway |
Andrew Grieve | 506d562 | 2025-01-29 21:15:55 | [diff] [blame] | 236 | ([crbug.com/389129271]). See [nullaway.md] for how to use `@Nullable` and |
| 237 | related annotations. |
Andrew Grieve | 19c214d | 2025-01-14 20:50:06 | [diff] [blame] | 238 | |
| 239 | [crbug.com/389129271]: https://crbug.com/389129271 |
| 240 | [nullaway.md]: nullaway.md |
| 241 | |
Andrew Grieve | 99e388f | 2023-11-02 20:46:51 | [diff] [blame] | 242 | ## Java Library APIs |
Andrew Grieve | 8d9e40f | 2023-03-15 21:04:42 | [diff] [blame] | 243 | |
Andrew Grieve | 99e388f | 2023-11-02 20:46:51 | [diff] [blame] | 244 | Android provides the ability to bundle copies of `java.*` APIs alongside |
| 245 | application code, known as [Java Library Desugaring]. However, since this |
| 246 | bundling comes with a performance cost, Chrome does not use it. Treat `java.*` |
| 247 | APIs the same as you would `android.*` ones and guard them with |
| 248 | `Build.VERSION.SDK_INT` checks [when necessary]. The one exception is if the |
| 249 | method is [directly backported by D8] (these are okay to use, since they are |
| 250 | lightweight). Android Lint will fail if you try to use an API without a |
| 251 | corresponding `Build.VERSION.SDK_INT` guard or `@RequiresApi` annotation. |
| 252 | |
| 253 | [Java Library Desugaring]: https://developer.android.com/studio/write/java8-support-table |
| 254 | [when necessary]: https://developer.android.com/reference/packages |
| 255 | [directly backported by D8]: https://source.chromium.org/chromium/chromium/src/+/main:third_party/r8/backported_methods.txt |
| 256 | |
| 257 | ### Logging |
| 258 | |
| 259 | * Use `org.chromium.base.Log` instead of `android.util.Log`. |
| 260 | * It provides `%s` support, and ensures log stripping works correctly. |
| 261 | * Minimize the use of `Log.w()` and `Log.e()`. |
| 262 | * Debug and Info log levels are stripped by ProGuard in release builds, and |
| 263 | so have no performance impact for shipping builds. However, Warning and |
| 264 | Error log levels are not stripped. |
| 265 | * Function calls in log parameters are *not* stripped by ProGuard. |
| 266 | |
| 267 | ```java |
| 268 | Log.d(TAG, "There are %d cats", countCats()); // countCats() not stripped. |
| 269 | ``` |
| 270 | |
| 271 | ### Streams |
| 272 | |
Andrew Grieve | 31928c9 | 2024-11-27 21:00:13 | [diff] [blame] | 273 | Using [Java streams] outside of tests is strongly discouraged. If you can write |
| 274 | your code as an explicit loop, then do so. The primary reason for this guidance |
| 275 | is because the lambdas and method references needed for streams almost always |
| 276 | result in larger binary size than their loop equivalents (see |
| 277 | [crbug.com/344943957] for examples). |
Andrew Grieve | 99e388f | 2023-11-02 20:46:51 | [diff] [blame] | 278 | |
| 279 | The `parallel()` and `parallelStream()` APIs are simpler than their loop |
Andrew Grieve | 31928c9 | 2024-11-27 21:00:13 | [diff] [blame] | 280 | equivalents, but are banned due to a lack of a compelling use case in Chrome. |
| 281 | If you find one, please discuss on `[email protected]`. |
| 282 | |
| 283 | Use of `stream()` without a lambda / method reference is allowed. E.g.: |
| 284 | |
| 285 | ```java |
| 286 | @SuppressWarnings("NoStreams") |
| 287 | private static List<Integer> boxInts(int[] arr) { |
| 288 | return Arrays.stream(arr).boxed().collect(Collectors.toList()); |
| 289 | } |
| 290 | |
| 291 | @SuppressWarnings("NoStreams") |
| 292 | private static List<String> readLines(BufferedReader bufferedReader) { |
| 293 | return bufferedReader.lines().collect(Collectors.toList()); |
| 294 | } |
| 295 | ``` |
Andrew Grieve | 99e388f | 2023-11-02 20:46:51 | [diff] [blame] | 296 | |
| 297 | [Java streams]: https://docs.oracle.com/javase/8/docs/api/java/util/stream/package-summary.html |
Andrew Grieve | 31928c9 | 2024-11-27 21:00:13 | [diff] [blame] | 298 | [crbug.com/344943957]: https://crbug.com/344943957 |
Andrew Grieve | 99e388f | 2023-11-02 20:46:51 | [diff] [blame] | 299 | |
| 300 | ### AndroidX Annotations {#annotations} |
| 301 | |
| 302 | * Use them liberally. They are [documented here](https://developer.android.com/studio/write/annotations). |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 303 | * They generally improve readability. |
Andrew Grieve | 99e388f | 2023-11-02 20:46:51 | [diff] [blame] | 304 | * Many make lint more useful. |
Andrew Grieve | 506d562 | 2025-01-29 21:15:55 | [diff] [blame] | 305 | * What about `androidx.annotation.Nullable`? |
| 306 | * We are migrating away from it (see [nullaway.md]). |
| 307 | * Keep using it in files that have not yet been migrated. |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 308 | |
Andrew Grieve | 99e388f | 2023-11-02 20:46:51 | [diff] [blame] | 309 | #### IntDefs {#intdefs} |
Carlos Knippschild | f2e58c1 | 2021-06-03 01:43:37 | [diff] [blame] | 310 | |
Andrew Grieve | 99e388f | 2023-11-02 20:46:51 | [diff] [blame] | 311 | Values can be declared outside or inside the `@interface`. Chromium style is |
| 312 | to declare inside. |
Carlos Knippschild | f2e58c1 | 2021-06-03 01:43:37 | [diff] [blame] | 313 | |
| 314 | ```java |
| 315 | @IntDef({ContactsPickerAction.CANCEL, ContactsPickerAction.CONTACTS_SELECTED, |
| 316 | ContactsPickerAction.SELECT_ALL, ContactsPickerAction.UNDO_SELECT_ALL}) |
| 317 | @Retention(RetentionPolicy.SOURCE) |
| 318 | public @interface ContactsPickerAction { |
| 319 | int CANCEL = 0; |
| 320 | int CONTACTS_SELECTED = 1; |
| 321 | int SELECT_ALL = 2; |
| 322 | int UNDO_SELECT_ALL = 3; |
| 323 | int NUM_ENTRIES = 4; |
| 324 | } |
| 325 | // ... |
| 326 | void onContactsPickerUserAction(@ContactsPickerAction int action, ...); |
| 327 | ``` |
| 328 | |
| 329 | Values of `Integer` type are also supported, which allows using a sentinel |
| 330 | `null` if needed. |
| 331 | |
| 332 | [@IntDef annotation]: https://developer.android.com/studio/write/annotations#enum-annotations |
| 333 | [Android lint]: https://chromium.googlesource.com/chromium/src/+/HEAD/build/android/docs/lint.md |
| 334 | |
Andrew Grieve | 99e388f | 2023-11-02 20:46:51 | [diff] [blame] | 335 | |
| 336 | ## Style / Formatting {#style} |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 337 | |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 338 | ### File Headers |
John Palmer | be05130 | 2021-05-19 11:48:35 | [diff] [blame] | 339 | * Use the same format as in the [C++ style guide](https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#File-headers). |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 340 | |
agrieve | 398286b | 2018-08-15 01:44:45 | [diff] [blame] | 341 | ### TODOs |
Andrew Grieve | 8d9e40f | 2023-03-15 21:04:42 | [diff] [blame] | 342 | |
Alison Gale | 3701577 | 2024-06-28 19:47:48 | [diff] [blame] | 343 | * TODO should follow chromium convention. Examples: |
| 344 | * `TODO(username): Some sentence here.` |
| 345 | * `TODO(crbug.com/40192027): Even better to use a bug for context.` |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 346 | |
Andrew Grieve | d99e3c0 | 2023-10-25 14:56:48 | [diff] [blame] | 347 | ### Parameter Comments |
| 348 | |
| 349 | Use [parameter comments] when they aid in the readability of a function call. |
| 350 | |
| 351 | E.g.: |
| 352 | |
| 353 | ```java |
| 354 | someMethod(/* enabled= */ true, /* target= */ null, defaultValue); |
| 355 | ``` |
| 356 | |
| 357 | [parameter comments]: https://errorprone.info/bugpattern/ParameterName |
| 358 | |
| 359 | ### Default Field Initializers |
Andrew Grieve | 8d9e40f | 2023-03-15 21:04:42 | [diff] [blame] | 360 | |
nyquist | 9d61f98 | 2017-02-10 00:29:08 | [diff] [blame] | 361 | * Fields should not be explicitly initialized to default values (see |
| 362 | [here](https://groups.google.com/a/chromium.org/d/topic/chromium-dev/ylbLOvLs0bs/discussion)). |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 363 | |
Andrew Grieve | 8d9e40f | 2023-03-15 21:04:42 | [diff] [blame] | 364 | ### Curly Braces |
| 365 | |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 366 | Conditional braces should be used, but are optional if the conditional and the |
| 367 | statement can be on a single line. |
| 368 | |
| 369 | Do: |
| 370 | |
| 371 | ```java |
| 372 | if (someConditional) return false; |
| 373 | for (int i = 0; i < 10; ++i) callThing(i); |
| 374 | ``` |
| 375 | |
| 376 | or |
| 377 | |
| 378 | ```java |
| 379 | if (someConditional) { |
Nate Fischer | fc38dc81 | 2024-04-24 17:07:26 | [diff] [blame] | 380 | return false; |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 381 | } |
| 382 | ``` |
| 383 | |
| 384 | Do NOT do: |
| 385 | |
| 386 | ```java |
| 387 | if (someConditional) |
Nate Fischer | fc38dc81 | 2024-04-24 17:07:26 | [diff] [blame] | 388 | return false; |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 389 | ``` |
| 390 | |
nyquist | 2d192c4c | 2017-03-06 21:36:51 | [diff] [blame] | 391 | ### Import Order |
Andrew Grieve | 8d9e40f | 2023-03-15 21:04:42 | [diff] [blame] | 392 | |
nyquist | 2d192c4c | 2017-03-06 21:36:51 | [diff] [blame] | 393 | * Static imports go before other imports. |
| 394 | * Each import group must be separated by an empty line. |
| 395 | |
| 396 | This is the order of the import groups: |
| 397 | |
| 398 | 1. android |
Yun Liu | f40227d9 | 2019-04-04 17:37:46 | [diff] [blame] | 399 | 1. androidx |
nyquist | 2d192c4c | 2017-03-06 21:36:51 | [diff] [blame] | 400 | 1. com (except com.google.android.apps.chrome) |
| 401 | 1. dalvik |
| 402 | 1. junit |
| 403 | 1. org |
| 404 | 1. com.google.android.apps.chrome |
| 405 | 1. org.chromium |
| 406 | 1. java |
| 407 | 1. javax |
| 408 | |
Andrew Grieve | 11c37007 | 2023-07-18 13:46:46 | [diff] [blame] | 409 | ## Testing |
| 410 | |
| 411 | Googlers, see [go/clank-test-strategy](http://go/clank-test-strategy). |
| 412 | |
| 413 | In summary: |
| 414 | |
| 415 | * Use real dependencies when feasible and fast. Use Mockito’s `@Mock` most |
| 416 | of the time, but write fakes for frequently used dependencies. |
| 417 | |
Andrew Grieve | f8a603ce | 2024-05-21 20:09:24 | [diff] [blame] | 418 | * Do not use Robolectric Shadows for Chromium code. |
| 419 | * Shadows make code harder to refactor. |
| 420 | * Prefer to refactor code to make it more testable. |
| 421 | * When you really need to use a test double for a static method, add a |
| 422 | `setFooForTesting() [...]` method to make the test contract explicit. |
| 423 | * Use [`ResettersForTesting.register()`] from within `ForTesting()` |
| 424 | methods to ensure that state is reset between tests. |
Andrew Grieve | 11c37007 | 2023-07-18 13:46:46 | [diff] [blame] | 425 | |
| 426 | * Use Robolectric when possible (when tests do not require native). Other |
| 427 | times, use on-device tests with one of the following annotations: |
| 428 | * [`@Batch(UNIT_TESTS)`] for unit tests |
| 429 | * [`@Batch(PER_CLASS)`] for integration tests |
| 430 | * [`@DoNotBatch`] for when each test method requires an app restart |
| 431 | |
| 432 | [`ResettersForTesting.register()`]: https://source.chromium.org/search?q=symbol:ResettersForTesting.register |
| 433 | [`@Batch(UNIT_TESTS)`]: https://source.chromium.org/search?q=symbol:Batch.UNIT_TESTS |
| 434 | [`@Batch(PER_CLASS)`]: https://source.chromium.org/search?q=symbol:Batch.PER_CLASS |
| 435 | [`@DoNotBatch`]: https://source.chromium.org/search?q=symbol:DoNotBatch |
| 436 | |
| 437 | ### Test-only Code |
Andrew Grieve | 8d9e40f | 2023-03-15 21:04:42 | [diff] [blame] | 438 | |
Andrew Grieve | 0872aad | 2023-06-26 14:16:31 | [diff] [blame] | 439 | Functions and fields used only for testing should have `ForTesting` as a |
| 440 | suffix so that: |
Caitlin Fischer | 210cfab | 2020-05-07 20:04:30 | [diff] [blame] | 441 | |
Andrew Grieve | 0872aad | 2023-06-26 14:16:31 | [diff] [blame] | 442 | 1. The `android-binary-size` trybot can [ensure they are removed] in |
| 443 | non-test optimized builds (by R8). |
| 444 | 2. [`PRESUMBIT.py`] can ensure no calls are made to such methods outside of |
| 445 | tests, and |
| 446 | |
| 447 | `ForTesting` methods that are `@CalledByNative` should use |
| 448 | `@CalledByNativeForTesting` instead. |
| 449 | |
| 450 | Symbols that are made public (or package-private) for the sake of tests |
| 451 | should be annotated with [`@VisibleForTesting`]. Android Lint will check |
| 452 | that calls from non-test code respect the "otherwise" visibility. |
| 453 | |
Andrew Grieve | e27a4f71 | 2023-07-04 15:16:35 | [diff] [blame] | 454 | Symbols with a `ForTesting` suffix **should not** be annotated with |
Andrew Grieve | 0872aad | 2023-06-26 14:16:31 | [diff] [blame] | 455 | `@VisibleForTesting`. While `otherwise=VisibleForTesting.NONE` exists, it |
| 456 | is redundant given the "ForTesting" suffix and the associated lint check |
Andrew Grieve | e27a4f71 | 2023-07-04 15:16:35 | [diff] [blame] | 457 | is redundant given our trybot check. You should, however, use it for |
| 458 | test-only constructors. |
Andrew Grieve | 0872aad | 2023-06-26 14:16:31 | [diff] [blame] | 459 | |
| 460 | [ensure they are removed]: /docs/speed/binary_size/android_binary_size_trybot.md#Added-Symbols-named-ForTest |
| 461 | [`PRESUMBIT.py`]: https://chromium.googlesource.com/chromium/src/+/main/PRESUBMIT.py |
| 462 | [`@VisibleForTesting`]: https://developer.android.com/reference/androidx/annotation/VisibleForTesting |
Sam Maier | 7452a0d | 2022-07-20 18:24:35 | [diff] [blame] | 463 | |
nyquist | 9d61f98 | 2017-02-10 00:29:08 | [diff] [blame] | 464 | ## Location |
Andrew Grieve | 8d9e40f | 2023-03-15 21:04:42 | [diff] [blame] | 465 | |
nyquist | 9d61f98 | 2017-02-10 00:29:08 | [diff] [blame] | 466 | "Top level directories" are defined as directories with a GN file, such as |
John Palmer | be05130 | 2021-05-19 11:48:35 | [diff] [blame] | 467 | [//base](https://chromium.googlesource.com/chromium/src/+/main/base/) |
nyquist | 9d61f98 | 2017-02-10 00:29:08 | [diff] [blame] | 468 | and |
John Palmer | be05130 | 2021-05-19 11:48:35 | [diff] [blame] | 469 | [//content](https://chromium.googlesource.com/chromium/src/+/main/content/), |
nyquist | 9d61f98 | 2017-02-10 00:29:08 | [diff] [blame] | 470 | Chromium Java should live in a directory named |
| 471 | `<top level directory>/android/java`, with a package name |
| 472 | `org.chromium.<top level directory>`. Each top level directory's Java should |
| 473 | build into a distinct JAR that honors the abstraction specified in a native |
John Palmer | be05130 | 2021-05-19 11:48:35 | [diff] [blame] | 474 | [checkdeps](https://chromium.googlesource.com/chromium/buildtools/+/main/checkdeps/checkdeps.py) |
nyquist | 9d61f98 | 2017-02-10 00:29:08 | [diff] [blame] | 475 | (e.g. `org.chromium.base` does not import `org.chromium.content`). The full |
| 476 | path of any java file should contain the complete package name. |
| 477 | |
| 478 | For example, top level directory `//base` might contain a file named |
| 479 | `base/android/java/org/chromium/base/Class.java`. This would get compiled into a |
| 480 | `chromium_base.jar` (final JAR name TBD). |
| 481 | |
| 482 | `org.chromium.chrome.browser.foo.Class` would live in |
| 483 | `chrome/android/java/org/chromium/chrome/browser/foo/Class.java`. |
| 484 | |
| 485 | New `<top level directory>/android` directories should have an `OWNERS` file |
| 486 | much like |
John Palmer | be05130 | 2021-05-19 11:48:35 | [diff] [blame] | 487 | [//base/android/OWNERS](https://chromium.googlesource.com/chromium/src/+/main/base/android/OWNERS). |
nyquist | 9d61f98 | 2017-02-10 00:29:08 | [diff] [blame] | 488 | |
Andrew Grieve | 0872aad | 2023-06-26 14:16:31 | [diff] [blame] | 489 | ## Tools |
| 490 | |
Andrew Grieve | ec828460 | 2023-10-16 15:53:25 | [diff] [blame] | 491 | `google-java-format` is used to auto-format Java files. Formatting of its code |
| 492 | should be accepted in code reviews. |
Andrew Grieve | 0872aad | 2023-06-26 14:16:31 | [diff] [blame] | 493 | |
| 494 | You can run `git cl format` to apply the automatic formatting. |
| 495 | |
Andrew Grieve | ec828460 | 2023-10-16 15:53:25 | [diff] [blame] | 496 | Chromium also makes use of several [static analysis] tools. |
Andrew Grieve | 0872aad | 2023-06-26 14:16:31 | [diff] [blame] | 497 | |
Andrew Grieve | ec828460 | 2023-10-16 15:53:25 | [diff] [blame] | 498 | [static analysis]: /build/android/docs/static_analysis.md |
Andrew Grieve | 0872aad | 2023-06-26 14:16:31 | [diff] [blame] | 499 | |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 500 | ## Miscellany |
Andrew Grieve | 8d9e40f | 2023-03-15 21:04:42 | [diff] [blame] | 501 | |
nyquist | aae4c7c | 2017-02-15 20:41:42 | [diff] [blame] | 502 | * Use UTF-8 file encodings and LF line endings. |