[Omnibox] Skip no-op TextView.setText when rebinding suggestion text [chromium/src : main]
Patrick Noland (Gerrit)
Patrick Noland voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Tomasz Wiszkowski
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tomasz Wiszkowski (Gerrit)
Tomasz Wiszkowski added 1 comment![]( "Open in Gerrit")
if (isSameText(mLine1Text, text)) return;
mLine1Text = text;isn't that the viewbinder's job?
also this won't really work well.
- we get some 4 AutocompleteResult pushes per keystroke
- we have no mechanism in place to state "pick views using this key: `<key>`"
- but even if we did - we would need a proper, thorough span range checking, because even if text is same across two `Spannable`s - the highlight may not be
- spannable.equals doesn't really check spans iiuc. that's why we had to add span checks to urlbar..
Related details
- Patrick Noland
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Noland (Gerrit)
Patrick Noland added 1 comment on change 8618724![]( "Open in Gerrit")
if (isSameText(mLine1Text, text)) return;
mLine1Text = text;isn't that the viewbinder's job?
also this won't really work well.
- we get some 4 AutocompleteResult pushes per keystroke
- we have no mechanism in place to state "pick views using this key: `<key>`"
- but even if we did - we would need a proper, thorough span range checking, because even if text is same across two `Spannable`s - the highlight may not be
- spannable.equals doesn't really check spans iiuc. that's why we had to add span checks to urlbar..
isn't that the viewbinder's job?
Yes, I will fix this; this got messed up in rebasing on top of the line caching fix.
- It's true that we have no way of picking view based on text matching, but in practice we get a pretty good hit rate (~50%), maybe because the 4 updates don't shift the state of the list that much so in practice the new model is getting bound to a view whose previous model was very similar
- SuggestionSpannable overrides equals() in a way that looks fairly robust/thorough to me, but let me look into that question a bit more.
Related details
- Tomasz Wiszkowski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Noland (Gerrit)
Patrick Noland added 1 comment on change 8618724![]( "Open in Gerrit")
if (isSameText(mLine1Text, text)) return;
mLine1Text = text;Patrick Nolandisn't that the viewbinder's job?
also this won't really work well.
- we get some 4 AutocompleteResult pushes per keystroke
- we have no mechanism in place to state "pick views using this key: `<key>`"
- but even if we did - we would need a proper, thorough span range checking, because even if text is same across two `Spannable`s - the highlight may not be
- spannable.equals doesn't really check spans iiuc. that's why we had to add span checks to urlbar..
isn't that the viewbinder's job?
Yes, I will fix this; this got messed up in rebasing on top of the line caching fix.
- It's true that we have no way of picking view based on text matching, but in practice we get a pretty good hit rate (~50%), maybe because the 4 updates don't shift the state of the list that much so in practice the new model is getting bound to a view whose previous model was very similar
- SuggestionSpannable overrides equals() in a way that looks fairly robust/thorough to me, but let me look into that question a bit more.
we would need a proper, thorough span range checking, because even if text is same across two Spannables - the highlight may not be
This case and similar ones are indeed covered by SuggestionSpannable#equals. The logic isn't bulletproof; it can produce false negatives in case of differing insertion order and trips an assert for ImageSpan (which TIL we use). But for the purposes of this optimization I think it's robust.
Related details
- Tomasz Wiszkowski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Noland (Gerrit)
Patrick Noland added 1 comment on change 8618724![]( "Open in Gerrit")
if (isSameText(mLine1Text, text)) return;
mLine1Text = text;Patrick Nolandisn't that the viewbinder's job?
also this won't really work well.
- we get some 4 AutocompleteResult pushes per keystroke
- we have no mechanism in place to state "pick views using this key: `<key>`"
- but even if we did - we would need a proper, thorough span range checking, because even if text is same across two `Spannable`s - the highlight may not be
- spannable.equals doesn't really check spans iiuc. that's why we had to add span checks to urlbar..
Patrick Nolandisn't that the viewbinder's job?
Yes, I will fix this; this got messed up in rebasing on top of the line caching fix.
- It's true that we have no way of picking view based on text matching, but in practice we get a pretty good hit rate (~50%), maybe because the 4 updates don't shift the state of the list that much so in practice the new model is getting bound to a view whose previous model was very similar
- SuggestionSpannable overrides equals() in a way that looks fairly robust/thorough to me, but let me look into that question a bit more.
we would need a proper, thorough span range checking, because even if text is same across two Spannables - the highlight may not beThis case and similar ones are indeed covered by SuggestionSpannable#equals. The logic isn't bulletproof; it can produce false negatives in case of differing insertion order and trips an assert for ImageSpan (which TIL we use). But for the purposes of this optimization I think it's robust.
Moved equality check back to ViewBinder and added some tests around SuggestionSpannable equality/robustness
Related details
- Tomasz Wiszkowski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tomasz Wiszkowski (Gerrit)
Tomasz Wiszkowski added 1 comment on change 8618724![]( "Open in Gerrit")
private static SuggestionSpannable twoBoldRegionsPlusColor(### Observation
Private static helper methods (twoBoldRegionsPlusColor on line 428 and withOvalImageSpan on line 494) are interspersed between test methods. Collocating private helper methods together (either right after setUp or at the bottom of the class after all test methods) keeps the test suite easier to scan.
### Recommended approach
Move twoBoldRegionsPlusColor and withOvalImageSpan to the bottom of SuggestionSpannableUnitTest (or immediately below setUp) so all test methods are grouped contiguously:
```java
private static SuggestionSpannable twoBoldRegionsPlusColor(
int bold1Start, int bold1End, int bold2Start, int bold2End) {
...
}
private static SuggestionSpannable withOvalImageSpan(int color, int edge, int alignment) {
...
}
```Related details
- Patrick Noland
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Noland (Gerrit)
Patrick Noland added 1 comment on change 8618724![]( "Open in Gerrit")
private static SuggestionSpannable twoBoldRegionsPlusColor(### Observation
Private static helper methods (twoBoldRegionsPlusColor on line 428 and withOvalImageSpan on line 494) are interspersed between test methods. Collocating private helper methods together (either right after setUp or at the bottom of the class after all test methods) keeps the test suite easier to scan.### Recommended approach
Move twoBoldRegionsPlusColor and withOvalImageSpan to the bottom of SuggestionSpannableUnitTest (or immediately below setUp) so all test methods are grouped contiguously:
```java
private static SuggestionSpannable twoBoldRegionsPlusColor(
int bold1Start, int bold1End, int bold2Start, int bold2End) {
...
}private static SuggestionSpannable withOvalImageSpan(int color, int edge, int alignment) {
...
}
```
- Tomasz Wiszkowski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Noland (Gerrit)
Message from Patrick Noland on change 8618724![]( "Open in Gerrit")
- Patrick Noland
- Tomasz Wiszkowski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tomasz Wiszkowski (Gerrit)
Tomasz Wiszkowski added 8 comments on change 8618724![]( "Open in Gerrit")
### Careful
This doesn't have to be resolved in this CL, but it may cause trouble later. The commit message says the real cost comes from the suggestion list being rebuilt from scratch several times per keystroke. This CL soaks up that cost at the leaf, inside the TextView, and the hit rate depends on which recycled view each model happens to land in. Coalescing AutocompleteResult pushes that don't change anything visible, or reusing PropertyModels for unchanged suggestions, would remove the wasted binds where they start. Please file or link a follow-up bug and call this CL a stopgap in the description, so the leaf-level check can be revisited later.
/**
* Custom equals method that addresses some of the issues found in SpannableStringInternal:
*
* <ul>
* <li>getSpan may reorder returned spans that was not reflected in original code until
* http://b2/73359036 (http://shortn/_vKcawFBIoL)
* <li>Individual Span objects still do not necessarily override .equals themselves (eg.
* ForegroundColorSpan), making them always fail the comparison.
* </ul>
*
* <p>This code simply addresses some issues in SpannableString.equals() method, therefore does
* not come paired with hashCode() call. hashCode() is correctly supplied by SpannableString.
*/### Finding
This CL changes what equals() does: unknown span types now compare as unequal instead of triggering an assert, and image spans have their own comparison. The Javadoc doesn't mention either change, even though the binder now relies on this behavior to skip work safely. //chrome/browser/ui/android/omnibox/GEMINI.md asks for Javadoc to stay in sync with the method contract.
### Recommended approach
```suggestion
* <p>Only the visual span types handled below (ForegroundColorSpan, BackgroundColorSpan,
* StyleSpan, TextAppearanceSpan, ImageSpan) are compared structurally. Any other span type
* causes the strings to be reported as different, so this never yields false positives.
*/
```
default -> {
// Unknown span type: we cannot prove the two render identically, so report them
// as different. This only costs an unnecessary TextView.setText.
return false;
}
}
return true;
}### Issue
This patchset deletes the closing brace of the for loop in equals(). Line 111 now closes the switch, return true sits inside the loop, and line 113 closes the loop instead of the method. As uploaded this doesn't compile: imageSpansEqual ends up declared inside equals(), and the class is missing a closing brace.
If the brace count were fixed by adding one at the end of the file, the result would be worse: equals() would return true after comparing only the first span. Two suggestions with the same text and the same full-length color span but different bold match ranges would compare equal, the rebind would be skipped, and the old highlight would stay on screen. That's exactly the failure this CL has to rule out. The new multi-span tests would catch it once the file compiles.
### Recommended approach
Put the loop's closing brace back so return true runs only after every span has been compared:
```suggestion
default -> {
// Unknown span type: we cannot prove the two render identically, so report them
// as different. This only costs an unnecessary TextView.setText.
return false;
}
}
}
return true;
}
```
private static boolean imageSpansEqual(ImageSpan a, ImageSpan b) {
if (a.getVerticalAlignment() != b.getVerticalAlignment()) return false;
Drawable da = a.getDrawable();
Drawable db = b.getDrawable();
if (da == db) return true;
if (da == null || db == null) return false;
if (!da.getBounds().equals(db.getBounds())) return false;
if (da instanceof ShapeDrawable && db instanceof ShapeDrawable) {
ShapeDrawable sa = (ShapeDrawable) da;
ShapeDrawable sb = (ShapeDrawable) db;
Shape shapeA = sa.getShape();
Shape shapeB = sb.getShape();
return sa.getPaint().getColor() == sb.getPaint().getColor()
&& (shapeA == null ? shapeB == null : shapeA.getClass() == shapeB.getClass());
}
if (da instanceof ColorDrawable && db instanceof ColorDrawable) {
return ((ColorDrawable) da).getColor() == ((ColorDrawable) db).getColor();
}
return false;### Finding
Two problems in imageSpansEqual:
1. The shape check on line 133 throws a NullPointerException when the first shape is non-null and the second is null. A ShapeDrawable built with the no-arg constructor has a null shape. A crash on the suggestion bind path is much worse than the extra setText this method exists to avoid.
2. The structural comparison can report two drawables as equal when they look different. Shape geometry (for example RoundRectShape radii), drawable alpha, tint or color filter, and paint style or stroke width are all ignored. If a processor ever sends a drawable that differs only in one of those, the rebind is skipped and the old icon stays on screen with no error.
The most a match here can save is one setText, so being conservative costs almost nothing.
### Recommended approach
Compare drawables by identity only. If tab-group rows need cache hits, have the processor reuse one Drawable instance per color rather than looking into Drawable internals here. This also removes the instanceof and cast chain that //chrome/browser/ui/android/omnibox/GEMINI.md asks us to avoid.
```java
private static boolean imageSpansEqual(ImageSpan a, ImageSpan b) {
// Drawables have no meaningful equals(); only identical instances are known to render the
// same. Anything else costs at most one extra setText.
return a.getVerticalAlignment() == b.getVerticalAlignment()
&& a.getDrawable() == b.getDrawable();
}
```
If you keep the structural comparison, at least guard the null case and use pattern variables:
```java
&& (shapeA == null || shapeB == null
? shapeA == shapeB
: shapeA.getClass() == shapeB.getClass());
```
a.setSpan(
new ImageSpan(new ColorDrawable(Color.RED), ImageSpan.ALIGN_CENTER),
0,
1,
Spanned.SPAN_INCLUSIVE_EXCLUSIVE);
SuggestionSpannable b = new SuggestionSpannable(" x");
b.setSpan(
new ImageSpan(new ColorDrawable(Color.RED), ImageSpan.ALIGN_CENTER),
0,
1,
Spanned.SPAN_INCLUSIVE_EXCLUSIVE);
SuggestionSpannable c = new SuggestionSpannable(" x");
c.setSpan(
new ImageSpan(new ColorDrawable(Color.GREEN), ImageSpan.ALIGN_CENTER),
0,
1,
Spanned.SPAN_INCLUSIVE_EXCLUSIVE);
assertTrue(a.equals(b));
assertFalse(a.equals(c));
}
@Test### Observation
This test builds the same image span setup three times, and the only difference is the color. That makes the one thing that varies hard to spot. A small helper next to withOvalImageSpan would make it as short as the other tests.
### Recommended approach
```java
@Test
public void spannableEquals_imageSpan_colorDrawables() {
SuggestionSpannable base = withColorImageSpan(Color.RED);
assertTrue(base.equals(withColorImageSpan(Color.RED)));
assertFalse(base.equals(withColorImageSpan(Color.GREEN)));
}
private static SuggestionSpannable withColorImageSpan(int color) {
SuggestionSpannable s = new SuggestionSpannable(" x");
s.setSpan(
new ImageSpan(new ColorDrawable(color), ImageSpan.ALIGN_CENTER),
0,
1,
Spanned.SPAN_INCLUSIVE_EXCLUSIVE);
return s;
}
``` @Test
public void line2_skipsSetText_whenReboundWithEqualSpannable() {
mModel.set(SuggestionViewProperties.TEXT_LINE_2_TEXT, colored("example.com", Color.RED));
CharSequence applied = mView.getLine2().getText();
assertEquals("example.com", applied.toString());
assertEquals(View.VISIBLE, mView.getLine2().getVisibility());
mModel.set(SuggestionViewProperties.TEXT_LINE_2_TEXT, colored("example.com", Color.RED));
assertSame(applied, mView.getLine2().getText());
assertEquals(View.VISIBLE, mView.getLine2().getVisibility());
}### Observation
Line 2 has its own call to shouldSetText, but the tests only check that it skips an equal rebind and keeps its text across hide and show. Nothing checks that line 2 picks up new text or spans. If the line 2 path were wired wrong (for example, passed line 1's cached text), line 2 would show stale text and every test here would still pass.
### Recommended approach
```java
@Test
public void line2_setsText_whenTextOrSpansDiffer() {
mModel.set(SuggestionViewProperties.TEXT_LINE_2_TEXT, colored("example.com", Color.RED));
CharSequence first = mView.getLine2().getText();
mModel.set(SuggestionViewProperties.TEXT_LINE_2_TEXT, colored("example.com", Color.BLUE));
assertNotSame(first, mView.getLine2().getText());
assertEquals(View.VISIBLE, mView.getLine2().getVisibility());
}
``` @Test(expected = AssertionError.class)
public void shouldSetText_assertsWhenBoundSpannableWasMutated() {
assumeTrue(BuildConfig.ENABLE_ASSERTS);
// Skipping a set is only safe if the cached spannable still matches the TextView's copy.
SuggestionSpannable bound = colored("query", Color.RED);
mModel.set(SuggestionViewProperties.TEXT_LINE_1_TEXT, bound);
bound.setSpan(new StyleSpan(Typeface.BOLD), 0, 2, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE);
SuggestionSpannable equalToMutated = colored("query", Color.RED);
equalToMutated.setSpan(
new StyleSpan(Typeface.BOLD), 0, 2, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE);
SuggestionViewViewBinder.shouldSetText(mView.getLine1(), bound, equalToMutated);
}### Observation
The expected attribute on the Test annotation passes if any AssertionError is thrown anywhere in the method, including in the setup calls before the call under test. Narrowing it with assertThrows makes the test prove the assert fires on the skip check specifically. (If you adopt the cache-free approach suggested on TwoLineSuggestionView, this test can be deleted.)
### Recommended approach
```java
assertThrows(
AssertionError.class,
() -> SuggestionViewViewBinder.shouldSetText(
mView.getLine1(), bound, equalToMutated));
```
// The SuggestionSpannable most recently applied to each line via setLine{1,2}Text(). These
// hold the caller's instance (not a copy) so that SuggestionViewViewBinder can cheaply detect
// a no-op rebind. Contract: a SuggestionSpannable must not be mutated after it has been
// handed to the view; the TextView keeps its own copy, so a later mutation would make the
// cached instance diverge from what is displayed and a needed setText could be skipped.
// SuggestionViewViewBinder asserts this invariant whenever it skips a set.
private @Nullable SuggestionSpannable mLine1Text;
private @Nullable SuggestionSpannable mLine2Text;### Observation
The equality check moved to the binder, but the View now keeps the caller's mutable spannable as a second copy of what is shown. That copy is only correct if two things hold. First, nobody mutates the spannable after binding, and only a debug assert checks that. Second, every text write goes through setLine1Text or setLine2Text. But getLine1() and getLine2() still return the raw TextViews, so any direct setText leaves the cache stale, and release builds would skip a setText the row actually needs. Under go/clank-ui-architecture the View should stay a plain presentation layer and not hold model objects for the binder to query later. Also, the commit message says the value is kept in the ViewHolder, but it's stored on the View.
### Recommended approach
The TextView already holds what is displayed. With a Spanned input and the default buffer type, TextView#getText returns a copy that keeps the original span order (your assert already relies on this). Turn the span comparison into a static helper that takes any two Spanned values, and compare against the TextView directly. Then TwoLineSuggestionView needs no changes, and shouldSetText, its assert, and the mutation test can all go away:
```java
// SuggestionSpannable
public static boolean contentEquals(Spanned a, Spanned b) { /* existing span loop */ }
@Override
public final boolean equals(@Nullable Object obj) {
return obj instanceof SuggestionSpannable other && contentEquals(this, other);
}
// SuggestionViewViewBinder
private static void setTextIfChanged(TextView tv, @Nullable SuggestionSpannable text) {
if (text != null
&& tv.getText() instanceof Spanned shown
&& SuggestionSpannable.contentEquals(shown, text)) {
return;
}
tv.setText(text);
}
```
If you keep the cache, make the new getters and setters package-private and stop handing out the TextViews for text writes, so nothing can bypass it.
Related details
- Patrick Noland
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tomasz Wiszkowski (Gerrit)
Tomasz Wiszkowski added 2 comments on change 8618724![]( "Open in Gerrit")
default -> {
// Unknown span type: we cannot prove the two render identically, so report them
// as different. This only costs an unnecessary TextView.setText.
return false;
}
}
return true;
}### Issue
This patchset deletes the closing brace of the for loop in equals(). Line 111 now closes the switch, return true sits inside the loop, and line 113 closes the loop instead of the method. As uploaded this doesn't compile: imageSpansEqual ends up declared inside equals(), and the class is missing a closing brace.If the brace count were fixed by adding one at the end of the file, the result would be worse: equals() would return true after comparing only the first span. Two suggestions with the same text and the same full-length color span but different bold match ranges would compare equal, the rebind would be skipped, and the old highlight would stay on screen. That's exactly the failure this CL has to rule out. The new multi-span tests would catch it once the file compiles.
### Recommended approach
Put the loop's closing brace back so return true runs only after every span has been compared:
```suggestion
default -> {
// Unknown span type: we cannot prove the two render identically, so report them
// as different. This only costs an unnecessary TextView.setText.
return false;
}
}
}
return true;
}
```
from older ps#.
private static boolean imageSpansEqual(ImageSpan a, ImageSpan b) {
if (a.getVerticalAlignment() != b.getVerticalAlignment()) return false;
Drawable da = a.getDrawable();
Drawable db = b.getDrawable();
if (da == db) return true;
if (da == null || db == null) return false;
if (!da.getBounds().equals(db.getBounds())) return false;
if (da instanceof ShapeDrawable && db instanceof ShapeDrawable) {
ShapeDrawable sa = (ShapeDrawable) da;
ShapeDrawable sb = (ShapeDrawable) db;
Shape shapeA = sa.getShape();
Shape shapeB = sb.getShape();
return sa.getPaint().getColor() == sb.getPaint().getColor()
&& (shapeA == null ? shapeB == null : shapeA.getClass() == shapeB.getClass());
}
if (da instanceof ColorDrawable && db instanceof ColorDrawable) {
return ((ColorDrawable) da).getColor() == ((ColorDrawable) db).getColor();
}
return false;### Finding
Two problems in imageSpansEqual:1. The shape check on line 133 throws a NullPointerException when the first shape is non-null and the second is null. A ShapeDrawable built with the no-arg constructor has a null shape. A crash on the suggestion bind path is much worse than the extra setText this method exists to avoid.
2. The structural comparison can report two drawables as equal when they look different. Shape geometry (for example RoundRectShape radii), drawable alpha, tint or color filter, and paint style or stroke width are all ignored. If a processor ever sends a drawable that differs only in one of those, the rebind is skipped and the old icon stays on screen with no error.The most a match here can save is one setText, so being conservative costs almost nothing.
### Recommended approach
Compare drawables by identity only. If tab-group rows need cache hits, have the processor reuse one Drawable instance per color rather than looking into Drawable internals here. This also removes the instanceof and cast chain that //chrome/browser/ui/android/omnibox/GEMINI.md asks us to avoid.
```java
private static boolean imageSpansEqual(ImageSpan a, ImageSpan b) {
// Drawables have no meaningful equals(); only identical instances are known to render the
// same. Anything else costs at most one extra setText.
return a.getVerticalAlignment() == b.getVerticalAlignment()
&& a.getDrawable() == b.getDrawable();
}
```
If you keep the structural comparison, at least guard the null case and use pattern variables:
```java
&& (shapeA == null || shapeB == null
? shapeA == shapeB
: shapeA.getClass() == shapeB.getClass());
```
ignore this. while correct - we don't do image spans in the omnibox.
Related details
- Patrick Noland
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Noland (Gerrit)
Patrick Noland added 2 comments![]( "Open in Gerrit")
This doesn't have to be resolved in this CL, but it may cause trouble later. The commit message says the real cost comes from the suggestion list being rebuilt from scratch several times per keystroke. This CL soaks up that cost at the leaf, inside the TextView, and the hit rate depends on which recycled view each model happens to land in. Coalescing AutocompleteResult pushes that don't change anything visible, or reusing PropertyModels for unchanged suggestions, would remove the wasted binds where they start. Please file or link a follow-up bug and call this CL a stopgap in the description, so the leaf-level check can be revisited later.
Patrick Noland
See b/575959026 and b/575959074
- Tomasz Wiszkowski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Noland (Gerrit)
Patrick Noland voted and added 4 comments![]( "Open in Gerrit")
Votes added by Patrick Noland <pno...@chromium.org>
| Commit-Queue | +1 |
4 comments
/**
* Custom equals method that addresses some of the issues found in SpannableStringInternal:
*
* <ul>
* <li>getSpan may reorder returned spans that was not reflected in original code until
* http://b2/73359036 (http://shortn/_vKcawFBIoL)
* <li>Individual Span objects still do not necessarily override .equals themselves (eg.
* ForegroundColorSpan), making them always fail the comparison.
* </ul>
*
* <p>This code simply addresses some issues in SpannableString.equals() method, therefore does
* not come paired with hashCode() call. hashCode() is correctly supplied by SpannableString.
*/### Finding
This CL changes what equals() does: unknown span types now compare as unequal instead of triggering an assert, and image spans have their own comparison. The Javadoc doesn't mention either change, even though the binder now relies on this behavior to skip work safely. //chrome/browser/ui/android/omnibox/GEMINI.md asks for Javadoc to stay in sync with the method contract.
### Recommended approach
```suggestion
* <p>Only the visual span types handled below (ForegroundColorSpan, BackgroundColorSpan,
* StyleSpan, TextAppearanceSpan, ImageSpan) are compared structurally. Any other span type
* causes the strings to be reported as different, so this never yields false positives.
*/
```
Done
a.setSpan(
new ImageSpan(new ColorDrawable(Color.RED), ImageSpan.ALIGN_CENTER),
0,
1,
Spanned.SPAN_INCLUSIVE_EXCLUSIVE);
SuggestionSpannable b = new SuggestionSpannable(" x");
b.setSpan(
new ImageSpan(new ColorDrawable(Color.RED), ImageSpan.ALIGN_CENTER),
0,
1,
Spanned.SPAN_INCLUSIVE_EXCLUSIVE);
SuggestionSpannable c = new SuggestionSpannable(" x");
c.setSpan(
new ImageSpan(new ColorDrawable(Color.GREEN), ImageSpan.ALIGN_CENTER),
0,
1,
Spanned.SPAN_INCLUSIVE_EXCLUSIVE);
assertTrue(a.equals(b));
assertFalse(a.equals(c));
}
@Test### Observation
This test builds the same image span setup three times, and the only difference is the color. That makes the one thing that varies hard to spot. A small helper next to withOvalImageSpan would make it as short as the other tests.
### Recommended approach
```java
@Test
public void spannableEquals_imageSpan_colorDrawables() {
SuggestionSpannable base = withColorImageSpan(Color.RED);
assertTrue(base.equals(withColorImageSpan(Color.RED)));
assertFalse(base.equals(withColorImageSpan(Color.GREEN)));
}private static SuggestionSpannable withColorImageSpan(int color) {
SuggestionSpannable s = new SuggestionSpannable(" x");
s.setSpan(
new ImageSpan(new ColorDrawable(color), ImageSpan.ALIGN_CENTER),
0,
1,
Spanned.SPAN_INCLUSIVE_EXCLUSIVE);
return s;
}
```
Obsolete
@Test(expected = AssertionError.class)
public void shouldSetText_assertsWhenBoundSpannableWasMutated() {
assumeTrue(BuildConfig.ENABLE_ASSERTS);
// Skipping a set is only safe if the cached spannable still matches the TextView's copy.
SuggestionSpannable bound = colored("query", Color.RED);
mModel.set(SuggestionViewProperties.TEXT_LINE_1_TEXT, bound);
bound.setSpan(new StyleSpan(Typeface.BOLD), 0, 2, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE);
SuggestionSpannable equalToMutated = colored("query", Color.RED);
equalToMutated.setSpan(
new StyleSpan(Typeface.BOLD), 0, 2, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE);
SuggestionViewViewBinder.shouldSetText(mView.getLine1(), bound, equalToMutated);
}### Observation
The expected attribute on the Test annotation passes if any AssertionError is thrown anywhere in the method, including in the setup calls before the call under test. Narrowing it with assertThrows makes the test prove the assert fires on the skip check specifically. (If you adopt the cache-free approach suggested on TwoLineSuggestionView, this test can be deleted.)
### Recommended approach
```java
assertThrows(
AssertionError.class,
() -> SuggestionViewViewBinder.shouldSetText(
mView.getLine1(), bound, equalToMutated));
```
Obsolete
// The SuggestionSpannable most recently applied to each line via setLine{1,2}Text(). These
// hold the caller's instance (not a copy) so that SuggestionViewViewBinder can cheaply detect
// a no-op rebind. Contract: a SuggestionSpannable must not be mutated after it has been
// handed to the view; the TextView keeps its own copy, so a later mutation would make the
// cached instance diverge from what is displayed and a needed setText could be skipped.
// SuggestionViewViewBinder asserts this invariant whenever it skips a set.
private @Nullable SuggestionSpannable mLine1Text;
private @Nullable SuggestionSpannable mLine2Text;### Observation
The equality check moved to the binder, but the View now keeps the caller's mutable spannable as a second copy of what is shown. That copy is only correct if two things hold. First, nobody mutates the spannable after binding, and only a debug assert checks that. Second, every text write goes through setLine1Text or setLine2Text. But getLine1() and getLine2() still return the raw TextViews, so any direct setText leaves the cache stale, and release builds would skip a setText the row actually needs. Under go/clank-ui-architecture the View should stay a plain presentation layer and not hold model objects for the binder to query later. Also, the commit message says the value is kept in the ViewHolder, but it's stored on the View.
### Recommended approach
The TextView already holds what is displayed. With a Spanned input and the default buffer type, TextView#getText returns a copy that keeps the original span order (your assert already relies on this). Turn the span comparison into a static helper that takes any two Spanned values, and compare against the TextView directly. Then TwoLineSuggestionView needs no changes, and shouldSetText, its assert, and the mutation test can all go away:
```java
// SuggestionSpannable
public static boolean contentEquals(Spanned a, Spanned b) { /* existing span loop */ }@Override
public final boolean equals(@Nullable Object obj) {
return obj instanceof SuggestionSpannable other && contentEquals(this, other);
}// SuggestionViewViewBinder
private static void setTextIfChanged(TextView tv, @Nullable SuggestionSpannable text) {
if (text != null
&& tv.getText() instanceof Spanned shown
&& SuggestionSpannable.contentEquals(shown, text)) {
return;
}
tv.setText(text);
}
```
If you keep the cache, make the new getters and setters package-private and stop handing out the TextViews for text writes, so nothing can bypass it.
Done
Related details
- Tomasz Wiszkowski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tomasz Wiszkowski (Gerrit)
Tomasz Wiszkowski added 3 comments![]( "Open in Gerrit")
SuggestionViewViewBinder now remembers the last SuggestionSpannable
applied to each line in its ViewHolder and skips setText when an equal
value (text and visual spans, via SuggestionSpannable.equals) is bound
again. Visibility and min-height handling are unchanged.### Finding
This paragraph still describes the earlier approach, where the binder remembered the last SuggestionSpannable per line in its ViewHolder. This patchset caches nothing. It compares the incoming text against what the TextView already holds, using the new static SuggestionSpannable.contentEquals. The commit message should describe the code that actually lands.
The measurements in the next paragraph were presumably taken with the cached version. If they weren't re-measured with this approach, it's worth saying so, or re-running the scenario. The hit rate should be about the same, but each check now does one more getText call and compares against a SpannedString.
### Recommended approach
```suggestion
SuggestionViewViewBinder now compares the incoming SuggestionSpannable
against the text the TextView already holds (its own SpannedString
copy) via the new SuggestionSpannable.contentEquals, and skips setText
when both have the same text and visual spans. Nothing is cached, so
the check cannot go stale. Visibility and min-height handling are
unchanged.
```
* <p>Only the visual span types handled by {@link #contentEquals} (ForegroundColorSpan,
* BackgroundColorSpan, StyleSpan, TextAppearanceSpan, ImageSpan) are compared structurally. Any
* other span type causes the strings to be reported as different, so this never yields false
* positives.### Careful
This doesn't have to be resolved in this CL, but the guarantee in this Javadoc is broader than what the code checks. contentEquals only collects spans that implement UpdateAppearance (lines 74–75). The default branch, which reports unknown types as different, therefore only sees unknown UpdateAppearance spans. Paragraph spans such as AlignmentSpan, LeadingMarginSpan, LineHeightSpan or BulletSpan don't implement UpdateAppearance. They are silently ignored, so two strings that differ only in one of those compare as equal. Now that this result decides whether setText runs, that would be a missed update, not just a wasted one.
This isn't a bug today. The omnibox processors only add StyleSpan, ForegroundColorSpan (BaseSuggestionViewProcessor) and ImageSpan (TabGroupSuggestionProcessor), and all of them are UpdateAppearance. Switching the lookup to Object.class would close the gap, but it isn't free. It also changes equals() for its other callers. And if line 1 or line 2 ever gets a movement method or a SPANNABLE buffer type, getText() would hold the TextView's own watcher spans, so every comparison would fail and the optimization would silently stop working. Stating the real scope in the Javadoc is the cheaper fix.
### Recommended approach
```suggestion
* <p>Only spans implementing {@link UpdateAppearance} are compared. Of those, the types handled
* by {@link #contentEquals} (ForegroundColorSpan, BackgroundColorSpan, StyleSpan,
* TextAppearanceSpan, ImageSpan) are compared structurally, and any other UpdateAppearance
* span causes the strings to be reported as different. Spans that do not implement
* UpdateAppearance (e.g. paragraph spans such as AlignmentSpan or LeadingMarginSpan) are
* ignored, so suggestion text must not rely on them without extending this method.
```
/** Tests for {@link SuggestionViewViewBinder} and {@link TwoLineSuggestionView}. */### Finding
The class Javadoc still says these tests cover TwoLineSuggestionView. That was true when the view held the cache. This patchset doesn't touch the view, and these tests only use it as a fixture. Separately, the note about getText() identity on lines 68–69 floats between the helper and the first test, so it isn't attached to anything. It explains how every test here works, so it belongs in the class Javadoc. Once it moves there, delete lines 68–69.
### Recommended approach
```suggestion
/**
* Tests for {@link SuggestionViewViewBinder}.
*
* <p>TextView.setText always stores a fresh copy of the supplied CharSequence, so the identity of
* getText() tells whether the last bind reached the TextView.
*/
```
Related details
- Patrick Noland
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Noland (Gerrit)
Patrick Noland voted and added 4 comments![]( "Open in Gerrit")
Votes added by Patrick Noland <pno...@chromium.org>
| Commit-Queue | +1 |
4 comments
SuggestionViewViewBinder now remembers the last SuggestionSpannable
applied to each line in its ViewHolder and skips setText when an equal
value (text and visual spans, via SuggestionSpannable.equals) is bound
again. Visibility and min-height handling are unchanged.### Finding
This paragraph still describes the earlier approach, where the binder remembered the last SuggestionSpannable per line in its ViewHolder. This patchset caches nothing. It compares the incoming text against what the TextView already holds, using the new static SuggestionSpannable.contentEquals. The commit message should describe the code that actually lands.The measurements in the next paragraph were presumably taken with the cached version. If they weren't re-measured with this approach, it's worth saying so, or re-running the scenario. The hit rate should be about the same, but each check now does one more getText call and compares against a SpannedString.
### Recommended approach
```suggestion
SuggestionViewViewBinder now compares the incoming SuggestionSpannable
against the text the TextView already holds (its own SpannedString
copy) via the new SuggestionSpannable.contentEquals, and skips setText
when both have the same text and visual spans. Nothing is cached, so
the check cannot go stale. Visibility and min-height handling are
unchanged.
```
Done
* <p>Only the visual span types handled by {@link #contentEquals} (ForegroundColorSpan,
* BackgroundColorSpan, StyleSpan, TextAppearanceSpan, ImageSpan) are compared structurally. Any
* other span type causes the strings to be reported as different, so this never yields false
* positives.### Careful
This doesn't have to be resolved in this CL, but the guarantee in this Javadoc is broader than what the code checks. contentEquals only collects spans that implement UpdateAppearance (lines 74–75). The default branch, which reports unknown types as different, therefore only sees unknown UpdateAppearance spans. Paragraph spans such as AlignmentSpan, LeadingMarginSpan, LineHeightSpan or BulletSpan don't implement UpdateAppearance. They are silently ignored, so two strings that differ only in one of those compare as equal. Now that this result decides whether setText runs, that would be a missed update, not just a wasted one.This isn't a bug today. The omnibox processors only add StyleSpan, ForegroundColorSpan (BaseSuggestionViewProcessor) and ImageSpan (TabGroupSuggestionProcessor), and all of them are UpdateAppearance. Switching the lookup to Object.class would close the gap, but it isn't free. It also changes equals() for its other callers. And if line 1 or line 2 ever gets a movement method or a SPANNABLE buffer type, getText() would hold the TextView's own watcher spans, so every comparison would fail and the optimization would silently stop working. Stating the real scope in the Javadoc is the cheaper fix.
### Recommended approach
```suggestion
* <p>Only spans implementing {@link UpdateAppearance} are compared. Of those, the types handled
* by {@link #contentEquals} (ForegroundColorSpan, BackgroundColorSpan, StyleSpan,
* TextAppearanceSpan, ImageSpan) are compared structurally, and any other UpdateAppearance
* span causes the strings to be reported as different. Spans that do not implement
* UpdateAppearance (e.g. paragraph spans such as AlignmentSpan or LeadingMarginSpan) are
* ignored, so suggestion text must not rely on them without extending this method.
```
Done
/** Tests for {@link SuggestionViewViewBinder} and {@link TwoLineSuggestionView}. */### Finding
The class Javadoc still says these tests cover TwoLineSuggestionView. That was true when the view held the cache. This patchset doesn't touch the view, and these tests only use it as a fixture. Separately, the note about getText() identity on lines 68–69 floats between the helper and the first test, so it isn't attached to anything. It explains how every test here works, so it belongs in the class Javadoc. Once it moves there, delete lines 68–69.### Recommended approach
```suggestion
/**
* Tests for {@link SuggestionViewViewBinder}.
*
* <p>TextView.setText always stores a fresh copy of the supplied CharSequence, so the identity of
* getText() tells whether the last bind reached the TextView.
*/
```
Done
@Test
public void line2_skipsSetText_whenReboundWithEqualSpannable() {
mModel.set(SuggestionViewProperties.TEXT_LINE_2_TEXT, colored("example.com", Color.RED));
CharSequence applied = mView.getLine2().getText();
assertEquals("example.com", applied.toString());
assertEquals(View.VISIBLE, mView.getLine2().getVisibility());
mModel.set(SuggestionViewProperties.TEXT_LINE_2_TEXT, colored("example.com", Color.RED));
assertSame(applied, mView.getLine2().getText());
assertEquals(View.VISIBLE, mView.getLine2().getVisibility());
}### Observation
Line 2 has its own call to shouldSetText, but the tests only check that it skips an equal rebind and keeps its text across hide and show. Nothing checks that line 2 picks up new text or spans. If the line 2 path were wired wrong (for example, passed line 1's cached text), line 2 would show stale text and every test here would still pass.
### Recommended approach
```java
@Test
public void line2_setsText_whenTextOrSpansDiffer() {
mModel.set(SuggestionViewProperties.TEXT_LINE_2_TEXT, colored("example.com", Color.RED));
CharSequence first = mView.getLine2().getText();
mModel.set(SuggestionViewProperties.TEXT_LINE_2_TEXT, colored("example.com", Color.BLUE));
assertNotSame(first, mView.getLine2().getText());
assertEquals(View.VISIBLE, mView.getLine2().getVisibility());
}
```- Tomasz Wiszkowski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tomasz Wiszkowski (Gerrit)
Tomasz Wiszkowski added 1 comment on change 8618724![]( "Open in Gerrit")
static void setTextIfChanged(TextView tv, @Nullable SuggestionSpannable text) {sorry about a lengthy review but i think we're almost at the point where this converges nicely.
`SuggestionSpannable` is our class. We could probably trivialize the change here if we overrode the `equals()` method of SuggestionSpannable to compare its contents directly.
Related details
- Patrick Noland
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tomasz Wiszkowski (Gerrit)
Tomasz Wiszkowski added 1 comment on change 8618724![]( "Open in Gerrit")
public final boolean equals(@Nullable Object obj) {since this is already `equals` and the PropertyModel already uses `equals` for comparison - the static method below is hopefully redundant..?
i honestly don't understand this change.
- we require SuggestionSpannable via PropertyKey. so we know both elements are SuggestionSpannable.
- PropertyModel already invokes `.equals()` to make sure to suppress emission if two keys are same.
- iiuc we don't issue Image spans, so the logic below should work right from the start even as it was..
can you share what it is you're seeing?
Related details
- Patrick Noland
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tomasz Wiszkowski (Gerrit)
Tomasz Wiszkowski added 1 comment on change 8618724![]( "Open in Gerrit")
public final boolean equals(@Nullable Object obj) {since this is already `equals` and the PropertyModel already uses `equals` for comparison - the static method below is hopefully redundant..?
i honestly don't understand this change.
- we require SuggestionSpannable via PropertyKey. so we know both elements are SuggestionSpannable.
- PropertyModel already invokes `.equals()` to make sure to suppress emission if two keys are same.
- iiuc we don't issue Image spans, so the logic below should work right from the start even as it was..
can you share what it is you're seeing?
... perhaps all we need to do is allow `.equals()` to work on any `Spannable`, not juest on `SuggestionSpannable`?
Related details
- Patrick Noland
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |