[NullAway] Add @NullMarked to customtabs/ files [chromium/src : main]
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh added 1 comment![]( "Open in Gerrit")
Related details
- Andrew Grieve
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Grieve (Gerrit)
Andrew Grieve added 9 comments![]( "Open in Gerrit")
tricky one!
assumeNonNull(mTabModelSelectorSupplier.get()),assertNonNull
if (mChromeActivityNativeDelegate != nullnit: probably fine to just assume here. Looks like probably mActivity != null implies mChromeActivityNativeDelegate != null.
assumeNonNull(mBottomBarContentView).removeOnLayoutChangeListener(this);nit: I think you can mark this `@MonotonicNonNull` instead.
assumeNonNull(mBottomBarView);nit: a bit better to do:
```
CustomTabBottomBarView bottomBarView = mBottomBarView;
if (bottomBarView == null) return;
bottomBarView
.animate()
.alpha(0)
.setInterpolator(
Interpolators.FAST_OUT_SLOW_IN_INTERPOLATOR)
.setDuration(SLIDE_ANIMATION_DURATION_MS)
.withEndAction(
() -> bottomBarView.setVisibility(View.GONE))
.start();
```
return new CustomTabDelegateFactory(I suspect this API is a mistake since most APIs in this class assume these are all non-null. Probably worth using `null` where this is currently used rather than marking all fields as nullable.
If you want to skip this scope expansion for now, maybe just mark this as `@SuppressWarnings("NullAway") // TODO: Remove this method.`
Hopefully will mean you can remove most changes to ActivityTabWebContentsDelegateAndroid and where ShareDelegate{Supplier} was marked @Nullable.
if (mProfileProviderSupplier.get().hasOffTheRecordProfile()) {nit: better to do:
```
Profile otrProfile = mProfileProviderSupplier.get().getOffTheRecordProfile(/* createIfNeeded= */ false);
if (otrProfile != null) {
ProfileManager.destroyWhenAppropriate(otrProfile);
}
```
public @Nullable Profile getProfile() {Should not be nullable since `createIfNeeded=true`.
Maybe worth creating a `getOrCreateOffTheRecordProfile()` in `ProfileProvider` so that each call site doesn't need to add an assert.
@Nullable BrowserControlsStateProvider browserControlsStateProvider) {Why nullable? Seems the class assumed it wouldn't be, so likely should fix at the call site.
Related details
- Aishwarya Rajesh
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh added 8 comments![]( "Open in Gerrit")
assumeNonNull(mTabModelSelectorSupplier.get()),Aishwarya RajeshassertNonNull
Obsolete with changes in latest PS per suggestion from other comment.
nit: probably fine to just assume here. Looks like probably mActivity != null implies mChromeActivityNativeDelegate != null.
Obsolete with changes in latest PS per suggestion from other comment.
assumeNonNull(mBottomBarContentView).removeOnLayoutChangeListener(this);nit: I think you can mark this `@MonotonicNonNull` instead.
That's true - updated.
nit: a bit better to do:
```
CustomTabBottomBarView bottomBarView = mBottomBarView;
if (bottomBarView == null) return;
bottomBarView
.animate()
.alpha(0)
.setInterpolator(
Interpolators.FAST_OUT_SLOW_IN_INTERPOLATOR)
.setDuration(SLIDE_ANIMATION_DURATION_MS)
.withEndAction(
() -> bottomBarView.setVisibility(View.GONE))
.start();
```
Done
I suspect this API is a mistake since most APIs in this class assume these are all non-null. Probably worth using `null` where this is currently used rather than marking all fields as nullable.
If you want to skip this scope expansion for now, maybe just mark this as `@SuppressWarnings("NullAway") // TODO: Remove this method.`
Hopefully will mean you can remove most changes to ActivityTabWebContentsDelegateAndroid and where ShareDelegate{Supplier} was marked @Nullable.
That's a great callout, thanks. :)
Updated the 3 callsites of this method to refrain from doing `setDelegateFactory(#createEmpty())` since this is currently defined to be `@Nullable` anyway.
Removed this method in the latest PS, so this simplifies / reverts a whole bunch of nullability changes in the existing PS like you mentioned.
if (mProfileProviderSupplier.get().hasOffTheRecordProfile()) {nit: better to do:
```
Profile otrProfile = mProfileProviderSupplier.get().getOffTheRecordProfile(/* createIfNeeded= */ false);
if (otrProfile != null) {
ProfileManager.destroyWhenAppropriate(otrProfile);
}
```
Done
Should not be nullable since `createIfNeeded=true`.
Maybe worth creating a `getOrCreateOffTheRecordProfile()` in `ProfileProvider` so that each call site doesn't need to add an assert.
I gathered that this method currently does not have any usage - so removed it instead.
@Nullable BrowserControlsStateProvider browserControlsStateProvider) {Why nullable? Seems the class assumed it wouldn't be, so likely should fix at the call site.
Reverted nullability update in calling class, so this is now not relevant.
Related details
- Andrew Grieve
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh voted Commit-Queue+0![]( "Open in Gerrit")
| Commit-Queue | +0 |
Related details
- Andrew Grieve
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh added 1 comment![]( "Open in Gerrit")
assert mDelegateFactory != null;It is apparently problematic if we pass a `null` `mDelegateFactory` to `#initialize()` although `mDelegateFactory` [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/tab/TabImpl.java;l=1264;drc=ee9e4b7dbb33a548fa1cc97e734e4bda272b5c3f) is technically `@Nullable` - this change [breaks](https://chromium-review.googlesource.com/c/chromium/src/+/6912607?checksPatchset=5&tab=checks) a few tests and it is unclear to me if it is safe to continue passing a `null` `CustomTabDelegateFactory` and update the failing test fixtures instead.
Related details
- Andrew Grieve
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh voted and added 1 comment![]( "Open in Gerrit")
Votes added by Aishwarya Rajesh
| Commit-Queue | +1 |
1 comment
assert mDelegateFactory != null;It is apparently problematic if we pass a `null` `mDelegateFactory` to `#initialize()` although `mDelegateFactory` [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/tab/TabImpl.java;l=1264;drc=ee9e4b7dbb33a548fa1cc97e734e4bda272b5c3f) is technically `@Nullable` - this change [breaks](https://chromium-review.googlesource.com/c/chromium/src/+/6912607?checksPatchset=5&tab=checks) a few tests and it is unclear to me if it is safe to continue passing a `null` `CustomTabDelegateFactory` and update the failing test fixtures instead.
Okay, I just realized there are ~300 test failures, so this method probably requires a non-null `delegateFactory` after all. The fallback on `null` value, `TabImpl#getDelegateFactory()` on line 193, also returns a `@Nullable` value so if we don't provide a non-null, "empty" `delegateFactory`, we're going to break some implementations.
I'll probably revert the recent changes and add the `createEmpty()` method back, unless you have a better idea, please LMK. :)
Related details
- Aishwarya Rajesh
- Andrew Grieve
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh voted Commit-Queue+0![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Grieve (Gerrit)
Andrew Grieve added 1 comment![]( "Open in Gerrit")
assert mDelegateFactory != null;Aishwarya RajeshIt is apparently problematic if we pass a `null` `mDelegateFactory` to `#initialize()` although `mDelegateFactory` [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/tab/TabImpl.java;l=1264;drc=ee9e4b7dbb33a548fa1cc97e734e4bda272b5c3f) is technically `@Nullable` - this change [breaks](https://chromium-review.googlesource.com/c/chromium/src/+/6912607?checksPatchset=5&tab=checks) a few tests and it is unclear to me if it is safe to continue passing a `null` `CustomTabDelegateFactory` and update the failing test fixtures instead.
Okay, I just realized there are ~300 test failures, so this method probably requires a non-null `delegateFactory` after all. The fallback on `null` value, `TabImpl#getDelegateFactory()` on line 193, also returns a `@Nullable` value so if we don't provide a non-null, "empty" `delegateFactory`, we're going to break some implementations.
I'll probably revert the recent changes and add the `createEmpty()` method back, unless you have a better idea, please LMK. :)
Reverting back sgtm since it seems like it's involved. Maybe instead of adding back all the @Nullables though, just mark `createEmpty()` with `@SuppressWarnings("NullAway")` and add a TODO to have callers use null instead.
Related details
- Aishwarya Rajesh
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh added 1 comment![]( "Open in Gerrit")
assert mDelegateFactory != null;Aishwarya RajeshIt is apparently problematic if we pass a `null` `mDelegateFactory` to `#initialize()` although `mDelegateFactory` [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/tab/TabImpl.java;l=1264;drc=ee9e4b7dbb33a548fa1cc97e734e4bda272b5c3f) is technically `@Nullable` - this change [breaks](https://chromium-review.googlesource.com/c/chromium/src/+/6912607?checksPatchset=5&tab=checks) a few tests and it is unclear to me if it is safe to continue passing a `null` `CustomTabDelegateFactory` and update the failing test fixtures instead.
Andrew GrieveOkay, I just realized there are ~300 test failures, so this method probably requires a non-null `delegateFactory` after all. The fallback on `null` value, `TabImpl#getDelegateFactory()` on line 193, also returns a `@Nullable` value so if we don't provide a non-null, "empty" `delegateFactory`, we're going to break some implementations.
I'll probably revert the recent changes and add the `createEmpty()` method back, unless you have a better idea, please LMK. :)
Reverting back sgtm since it seems like it's involved. Maybe instead of adding back all the @Nullables though, just mark `createEmpty()` with `@SuppressWarnings("NullAway")` and add a TODO to have callers use null instead.
- Andrew Grieve
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Grieve (Gerrit)
Andrew Grieve voted and added 5 comments![]( "Open in Gerrit")
Votes added by Andrew Grieve
| Code-Review | +1 |
5 comments
a few nits. Thanks for working through this! Still not a very null-friendly class!
assert mIntentDataProvider != null && mIntentDataProvider.isAuthTab();nit: use 2 asserts so that if it fails we know what happened.
if (mIntentDataProvider == null || mIntentDataProvider.isAuthTab()) return;nit: `assumeNonNull()`. Don't want to fail silently if it's a bug to call this without a provider.
if (mActivity == null) return;nit: probably an assert would be better than not doing what we promise to do.
if (intentDataProvider != null && intentDataProvider.hasTargetNetwork()) {nit: assumeNonNull()
Related details
- Aishwarya Rajesh
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh added 4 comments![]( "Open in Gerrit")
assert mIntentDataProvider != null && mIntentDataProvider.isAuthTab();nit: use 2 asserts so that if it fails we know what happened.
Done
if (mIntentDataProvider == null || mIntentDataProvider.isAuthTab()) return;nit: `assumeNonNull()`. Don't want to fail silently if it's a bug to call this without a provider.
Done
nit: probably an assert would be better than not doing what we promise to do.
Done
if (intentDataProvider != null && intentDataProvider.hasTargetNetwork()) {Aishwarya Rajeshnit: assumeNonNull()
I realized this has just one caller that is already doing `assumeNonNull()` so this input should be non-null already, removed `@Nullable` on the input. :)
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
7 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabDelegateFactory.java
Insertions: 6, Deletions: 6.
@@ -171,13 +171,14 @@
@Override
public void returnAsActivityResult(GURL url) {
- assert mIntentDataProvider != null && mIntentDataProvider.isAuthTab();
+ assert mIntentDataProvider != null;
+ assert mIntentDataProvider.isAuthTab();
mAuthTabVerifier.returnAsActivityResult(url);
}
@Override
public void maybeRecordExternalNavigationSchemeHistogram(GURL url) {
- if (mIntentDataProvider == null || mIntentDataProvider.isAuthTab()) return;
+ if (assumeNonNull(mIntentDataProvider).isAuthTab()) return;
// Only record for Custom Tabs that we think are launched for auth purposes.
Uri urlToLoad = Uri.parse(mIntentDataProvider.getUrlToLoad());
@@ -257,7 +258,7 @@
@Override
protected void bringActivityToForeground() {
- if (mActivity == null) return;
+ assert mActivity != null;
((ActivityManager) mActivity.getSystemService(Context.ACTIVITY_SERVICE))
.moveTaskToFront(mActivity.getTaskId(), 0);
}
@@ -609,9 +610,8 @@
}
private static @ChromeContextMenuPopulator.ContextMenuMode int getContextMenuMode(
- @Nullable BrowserServicesIntentDataProvider intentDataProvider,
- @ActivityType int activityType) {
- if (intentDataProvider != null && intentDataProvider.hasTargetNetwork()) {
+ BrowserServicesIntentDataProvider intentDataProvider, @ActivityType int activityType) {
+ if (intentDataProvider.hasTargetNetwork()) {
return ChromeContextMenuPopulator.ContextMenuMode.NETWORK_BOUND_TAB;
}
return isWebappOrWebApk(activityType)
```
Change information
[NullAway] Add @NullMarked to customtabs/ files
- M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomButtonParamsImpl.java
- M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuHelper.java
- M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java
- M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabBottomBarDelegate.java
- M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabColorProviderImpl.java
- M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabCompositorContentInitializer.java
- M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabDelegateFactory.java
- M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabDownloadObserver.java
- M chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIncognitoManager.java
- M chrome/android/java/src/org/chromium/chrome/browser/native_page/NativePageFactory.java
- M chrome/browser/android/browserservices/intents/java/src/org/chromium/chrome/browser/browserservices/intents/CustomButtonParams.java
Code-Review: +1 by Andrew Grieve
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |