[NullAway] Add @NullMarked to compositor/overlays/strip files [chromium/src : main]
Aishwarya Rajesh (Gerrit)
Aishwarya Rajesh voted Commit-Queue+0![]( "Open in Gerrit")
| Commit-Queue | +0 |
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 and added 1 comment![]( "Open in Gerrit")
Votes added by Aishwarya Rajesh
| Commit-Queue | +1 |
1 comment
Related details
- Neil Coronado
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Neil Coronado (Gerrit)
Neil Coronado added 8 comments![]( "Open in Gerrit")
PTAL, thanks!
Thanks for the work here!
@Nullable Token groupId, @Nullable String titleString, boolean incognito) {I think we should instead `assumeNonNull` or assert [here](https://crsrc.org/c/chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/StripDragShadowView.java;l=281;drc=eacc76afbe1d2a055571fc8f66ab67d17530956c;bpv=0;bpt=1).
Not sure what's ideal; tab strip code has this pattern of forking which method we call based on if the tab is in a group or not, so it's often safe to assume non-null, but not sure if we should just adopt a different pattern to be more explicit.
TabGroupModelFilter filter, Context context, @Nullable Token groupId, String title) {I think NonNull if we change in LayerTitleCache
TabGroupModelFilter modelFilter, @Nullable Tab tab1, Tab tab2) {Similar; this should be non-null (I think at this [callsite](https://crsrc.org/c/chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/TabReorderStrategy.java;l=258;drc=eacc76afbe1d2a055571fc8f66ab67d17530956c;bpv=0;bpt=1))
@Nullable Token tabGroupId,Similar; this should be non-null (I think [here](https://crsrc.org/c/chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/ReorderStrategyBase.java;l=151;drc=eacc76afbe1d2a055571fc8f66ab67d17530956c))
Tab interactingTab = mModel.getTabByIdChecked(interactingStripTab.getTabId());:o Neat! I wasn't aware of this pattern, but I think a lot of other places in tab strip can be cleaned up with this and similar things (though not in this CL).
return assumeNonNull(null);Should this be `assertNonNull` https://chromium.googlesource.com/chromium/src/+/main/styleguide/java/nullaway.md#nullaway-configuration?
boolean isInGroup = curTab != null && mTabGroupModelFilter.isTabInTabGroup(curTab);I think we can `assumeNonNull(curTab)`
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 2 comments![]( "Open in Gerrit")
@Nullable Token groupId, @Nullable String titleString, boolean incognito) {I think we should instead `assumeNonNull` or assert [here](https://crsrc.org/c/chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/StripDragShadowView.java;l=281;drc=eacc76afbe1d2a055571fc8f66ab67d17530956c;bpv=0;bpt=1).
Not sure what's ideal; tab strip code has this pattern of forking which method we call based on if the tab is in a group or not, so it's often safe to assume non-null, but not sure if we should just adopt a different pattern to be more explicit.
Thanks for the context Neil!
For this particular case, I noticed that the implementation has a null check on `tabGroupId` right after this line: https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/StripDragShadowView.java;l=281;drc=eacc76afbe1d2a055571fc8f66ab67d17530956c
So it may be odd to `assumeNonNull` here?
not sure if we should just adopt a different pattern to be more explicit
In my experience, it is typical to add assumes / asserts at the callsite if we have prior knowledge about an input being non-null _before_ passing it on vs making the input to the method `@Nullable`, especially if among multiple callsites, there is one or fewer exceptions. In this case, given that we have a null-check right after we extract this value makes me think that assuming this value is `null` may be incorrect.
But as a feature OWNER if you're sure that this method will ensure a non-null `groupId`, we could remove the `@Nullable` annotation in LayerTitleCache and instead add an assert at this callsite + remove the null check. I'd choose `assumeNonNull` only if the variable is dereferenced soon after it is extracted (and therefore establishing the validity of this assumption), which is not the case here.
return assumeNonNull(null);Should this be `assertNonNull` https://chromium.googlesource.com/chromium/src/+/main/styleguide/java/nullaway.md#nullaway-configuration?
It's typical to use `assumeNonNull` in cases like these, other examples: https://source.chromium.org/search?q=assumeNonNull%5C(null%5C)
This is because we're already doing `assert false` above, so it is technically safe to "assume" we're not null at this point if we've got past that. :)
Related details
- Neil Coronado
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 6 comments![]( "Open in Gerrit")
Thanks for your keen insights, Neil :)
@Nullable Token groupId, @Nullable String titleString, boolean incognito) {Aishwarya RajeshI think we should instead `assumeNonNull` or assert [here](https://crsrc.org/c/chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/StripDragShadowView.java;l=281;drc=eacc76afbe1d2a055571fc8f66ab67d17530956c;bpv=0;bpt=1).
Not sure what's ideal; tab strip code has this pattern of forking which method we call based on if the tab is in a group or not, so it's often safe to assume non-null, but not sure if we should just adopt a different pattern to be more explicit.
Thanks for the context Neil!
For this particular case, I noticed that the implementation has a null check on `tabGroupId` right after this line: https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/StripDragShadowView.java;l=281;drc=eacc76afbe1d2a055571fc8f66ab67d17530956c
So it may be odd to `assumeNonNull` here?
not sure if we should just adopt a different pattern to be more explicit
In my experience, it is typical to add assumes / asserts at the callsite if we have prior knowledge about an input being non-null _before_ passing it on vs making the input to the method `@Nullable`, especially if among multiple callsites, there is one or fewer exceptions. In this case, given that we have a null-check right after we extract this value makes me think that assuming this value is `null` may be incorrect.
But as a feature OWNER if you're sure that this method will ensure a non-null `groupId`, we could remove the `@Nullable` annotation in LayerTitleCache and instead add an assert at this callsite + remove the null check. I'd choose `assumeNonNull` only if the variable is dereferenced soon after it is extracted (and therefore establishing the validity of this assumption), which is not the case here.
I looked closely at `StripDragShadowView#prepareForGroupDrag()` and realized from the docs that the tab _should_ be in a tab group, so makes sense to assert non-null at the callsite like you recommended - I made this update in the latest PS. :)
TabGroupModelFilter filter, Context context, @Nullable Token groupId, String title) {I think NonNull if we change in LayerTitleCache
Done
TabGroupModelFilter modelFilter, @Nullable Tab tab1, Tab tab2) {Similar; this should be non-null (I think at this [callsite](https://crsrc.org/c/chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/TabReorderStrategy.java;l=258;drc=eacc76afbe1d2a055571fc8f66ab67d17530956c;bpv=0;bpt=1))
SG - done!
Similar; this should be non-null (I think [here](https://crsrc.org/c/chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/ReorderStrategyBase.java;l=151;drc=eacc76afbe1d2a055571fc8f66ab67d17530956c))
SG - done!
boolean isInGroup = curTab != null && mTabGroupModelFilter.isTabInTabGroup(curTab);I think we can `assumeNonNull(curTab)`
Realized I can do `getTabAtChecked()` above while extracting `curTab` instead :)
Related details
- Neil Coronado
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Neil Coronado (Gerrit)
Neil Coronado voted and added 2 comments![]( "Open in Gerrit")
Votes added by Neil Coronado
| Code-Review | +1 |
2 comments
@Nullable Token groupId, @Nullable String titleString, boolean incognito) {Aishwarya RajeshI think we should instead `assumeNonNull` or assert [here](https://crsrc.org/c/chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/StripDragShadowView.java;l=281;drc=eacc76afbe1d2a055571fc8f66ab67d17530956c;bpv=0;bpt=1).
Not sure what's ideal; tab strip code has this pattern of forking which method we call based on if the tab is in a group or not, so it's often safe to assume non-null, but not sure if we should just adopt a different pattern to be more explicit.
Aishwarya RajeshThanks for the context Neil!
For this particular case, I noticed that the implementation has a null check on `tabGroupId` right after this line: https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/StripDragShadowView.java;l=281;drc=eacc76afbe1d2a055571fc8f66ab67d17530956c
So it may be odd to `assumeNonNull` here?
not sure if we should just adopt a different pattern to be more explicit
In my experience, it is typical to add assumes / asserts at the callsite if we have prior knowledge about an input being non-null _before_ passing it on vs making the input to the method `@Nullable`, especially if among multiple callsites, there is one or fewer exceptions. In this case, given that we have a null-check right after we extract this value makes me think that assuming this value is `null` may be incorrect.
But as a feature OWNER if you're sure that this method will ensure a non-null `groupId`, we could remove the `@Nullable` annotation in LayerTitleCache and instead add an assert at this callsite + remove the null check. I'd choose `assumeNonNull` only if the variable is dereferenced soon after it is extracted (and therefore establishing the validity of this assumption), which is not the case here.
I looked closely at `StripDragShadowView#prepareForGroupDrag()` and realized from the docs that the tab _should_ be in a tab group, so makes sense to assert non-null at the callsite like you recommended - I made this update in the latest PS. :)
Thanks for the explanation wrt `assumeNonNull`! Also, for context, the null-check was added by the tabs team when they were doing a large scale migration from rootId -> stableId, but we previously didn't validate.
return assumeNonNull(null);Aishwarya RajeshShould this be `assertNonNull` https://chromium.googlesource.com/chromium/src/+/main/styleguide/java/nullaway.md#nullaway-configuration?
It's typical to use `assumeNonNull` in cases like these, other examples: https://source.chromium.org/search?q=assumeNonNull%5C(null%5C)
This is because we're already doing `assert false` above, so it is technically safe to "assume" we're not null at this point if we've got past that. :)
Oops, I linked to the wrong part of the page, but I was referring to this https://screenshot.googleplex.com/LyN9PwwDszW6AzX.png. But I also only see this being done in one other place so ¯\\_(ツ)_/¯
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")
return assumeNonNull(null);Aishwarya RajeshShould this be `assertNonNull` https://chromium.googlesource.com/chromium/src/+/main/styleguide/java/nullaway.md#nullaway-configuration?
Neil CoronadoIt's typical to use `assumeNonNull` in cases like these, other examples: https://source.chromium.org/search?q=assumeNonNull%5C(null%5C)
This is because we're already doing `assert false` above, so it is technically safe to "assume" we're not null at this point if we've got past that. :)
Oops, I linked to the wrong part of the page, but I was referring to this https://screenshot.googleplex.com/LyN9PwwDszW6AzX.png. But I also only see this being done in one other place so ¯\\_(ツ)_/¯
Got it, thanks for the screenshot. :)
I guess `assume` is preferable here and in the other examples that I linked because of the preceding `assert false` :D
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. |
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
6 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/compositor/overlays/strip/reorder/StripDragShadowView.java
Insertions: 1, Deletions: 2.
@@ -281,10 +281,9 @@
assumeNonNull(modelFilter);
// Background color
- @TabGroupColorId int colorId = TabGroupColorId.GREY;
Token tabGroupId = tab.getTabGroupId();
assert tabGroupId != null : "The tab group ID should be non-null";
- colorId = modelFilter.getTabGroupColorWithFallback(tabGroupId);
+ @TabGroupColorId int colorId = modelFilter.getTabGroupColorWithFallback(tabGroupId);
@ColorInt
int groupColor =
```
Change information
[NullAway] Add @NullMarked to compositor/overlays/strip files
- M chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java
- M chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/ExternalViewDragDropReorderStrategy.java
- M chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/GroupReorderStrategy.java
- M chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/MultiTabReorderStrategy.java
- M chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/ReorderDelegate.java
- M chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/ReorderStrategyBase.java
- M chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/SourceViewDragDropReorderStrategy.java
- M chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/StripDragShadowView.java
- M chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/reorder/TabReorderStrategy.java
Code-Review: +1 by Neil Coronado
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |