[Side Panel] Delay view detachment to prevent flickering [chromium/src : main]
Shu Yang (Gerrit)
Shu Yang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Shu Yang
| Commit-Queue | +1 |
Related details
- Linyu He
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Linyu He (Gerrit)
Linyu He added 4 comments![]( "Open in Gerrit")
"//components/thin_webview:java",Not needed? (`SidePanelContainerCoordinatorImplUnitTest` isn't changed.)
View oldView = mCurrentContent != null ? mCurrentContent.mView : null;Per offline discussion, we'll move the `runOnNextFrame()` logic to `startReplacingPanelContent()` (the method is added in http://crrev.com/c/8028149 and targets the "replace content" case).
Runnable removeOldViewRunnable =Q: Will this CL handle rapid tab switches, i.e., `removeOldViewRunnable` doesn't have time to run before the side panel content is replaced again?
One solution can be to keep `removeOldViewRunnable` as a mutable state and set it to null at the end of `runOnNextFrame()`.
If it's not null before `runOnNextFrame()`, immediately run it and replace its value with the new runnable.
private @Nullable ThinWebView findThinWebView(View view) {nit: let's move this private method to the end of the class so all public methods stay together.
Related details
- Shu Yang
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. |
Shu Yang (Gerrit)
Shu Yang voted and added 4 comments![]( "Open in Gerrit")
Votes added by Shu Yang
| Commit-Queue | +1 |
4 comments
Not needed? (`SidePanelContainerCoordinatorImplUnitTest` isn't changed.)
Done
View oldView = mCurrentContent != null ? mCurrentContent.mView : null;Per offline discussion, we'll move the `runOnNextFrame()` logic to `startReplacingPanelContent()` (the method is added in http://crrev.com/c/8028149 and targets the "replace content" case).
Done
Q: Will this CL handle rapid tab switches, i.e., `removeOldViewRunnable` doesn't have time to run before the side panel content is replaced again?
One solution can be to keep `removeOldViewRunnable` as a mutable state and set it to null at the end of `runOnNextFrame()`.
If it's not null before `runOnNextFrame()`, immediately run it and replace its value with the new runnable.
Thanks for the suggestion! Updated.
Also added a guard flag inside the runnable to ensure it doesn't execute its logic again when the next frame eventually fires after we've already run it early.
nit: let's move this private method to the end of the class so all public methods stay together.
- Linyu He
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shu Yang (Gerrit)
Shu Yang added 1 comment![]( "Open in Gerrit")
The test failure in MediaNotificationServiceLifecycleTest is not caused by this change.
Linyu He (Gerrit)
Linyu He added 8 comments![]( "Open in Gerrit")
Include-Ci-Only-Tests: trueTo run side panel tests in CQ, we can add:
```
Cq-Include-Trybots: luci.chromium.try:android-desktop-x64-rel
Include-Ci-Only-Tests: true
```
private @Nullable Runnable mPendingReplaceRunnable;nit: need javadoc for `mPendingReplaceRunnable`
```suggestion
/**
* {@link Runnable} for {@link #startReplacingPanelContent} to remove the old content View.
*/
private @Nullable Runnable mPendingReplaceRunnable;
```
if (mPendingReplaceRunnable != null) {
mPendingReplaceRunnable.run();
}Can we set `mPendingReplaceRunnable` to `null` after Line 177? It should always be `null` after it's run.
```suggestion
if (mPendingReplaceRunnable != null) {
mPendingReplaceRunnable.run();
mPendingRepalceRunnable = null;
}
```
View oldView = mCurrentContent != null ? mCurrentContent.mView : null;For replacing panel content, `oldView` must be non-null, so:
```suggestion
assert mCurrentContent != null : "no content to replace";
View oldView = mCurrentContent.mView;
```
// We use a custom Runnable class with a `mRan` flag because ThinWebView's runOnNextFrame()
// does not support cancellation. If a new content replacement happens before the next
// frame renders, we immediately run this runnable to clean up the old state. When the
// next frame eventually fires, the guard flag prevents running the cleanup logic again
// (which would cause duplicate JNI callbacks and native crashes).For this comment, I think it's also important to explain why `ThinWebView#runOnNextFrame()` needs a `Runnable` reference separate from `mPendingReplaceRunnable`, i.e., why `removeOldViewRunnable` and `mPendingReplaceRunnable` may point to different instances and cause bugs.
mRan = true;
if (mPendingReplaceRunnable == this) {
mPendingReplaceRunnable = null;
}nit - it's more intuitive to place Line 197 ~ Line 201 at the end of `run()`.
```
@Override
public void run() {
if (mRan) return;
// Do work...
// Now the method is run.
mRan = true;
if (mPendingReplaceRunnable == this) {
mPendingReplaceRunnable = null;
}
}
```
if (oldView != null && oldView != newContent.mView) {
mContainerView.removeView(oldView);
}I think we can remove Line 202.
(1) `oldView` should always be non-null.
(2) If new View is the same as `oldView`, `addView()` at Line 183 will crash.
if (thinWebView != null) {
thinWebView.runOnNextFrame(removeOldViewRunnable);
} else {
removeOldViewRunnable.run();
}(1) Let's keep `BuildConfig.IS_FOR_TEST`.
(2) We should set `mPendingReplaceRunnable` to `null` if the old View is synchronously removed.
```suggestion
if (thinWebView == null || BuildConfig.IS_FOR_TEST) {
mPendingReplaceRunnable.run();
mPendingReplaceRunnable = null;
} else {
thinWebView.runOnNextFrame(removeOldViewRunnable);
}
```
Related details
- Shu Yang
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. |
Shu Yang (Gerrit)
Shu Yang added 8 comments![]( "Open in Gerrit")
To run side panel tests in CQ, we can add:
```
Cq-Include-Trybots: luci.chromium.try:android-desktop-x64-rel
Include-Ci-Only-Tests: true
```
Done
nit: need javadoc for `mPendingReplaceRunnable`
```suggestion/**
* {@link Runnable} for {@link #startReplacingPanelContent} to remove the old content View.
*/
private @Nullable Runnable mPendingReplaceRunnable;
```
Done
if (mPendingReplaceRunnable != null) {
mPendingReplaceRunnable.run();
}Can we set `mPendingReplaceRunnable` to `null` after Line 177? It should always be `null` after it's run.
```suggestion
if (mPendingReplaceRunnable != null) {
mPendingReplaceRunnable.run();
mPendingRepalceRunnable = null;
}
```
The code currently handles setting mPendingReplaceRunnable to null inside the runnable's run() method itself.
This check is important because we only want to set mPendingReplaceRunnable to null if it still points to the runnable that is currently executing. If a newer replacement has occurred, mPendingReplaceRunnable will have been updated to the newer runnable, and we shouldn't clear it.
If we explicitly set it to null in startReplacingPanelContent like this, it is redundant because calling mPendingReplaceRunnable.run() will set it to null anyway.
Do you still prefer to add it explicitly for readability, or are you comfortable keeping the current implementation since it already handles nulling it out safely?
View oldView = mCurrentContent != null ? mCurrentContent.mView : null;For replacing panel content, `oldView` must be non-null, so:
```suggestion
assert mCurrentContent != null : "no content to replace";
View oldView = mCurrentContent.mView;
```
Done
// We use a custom Runnable class with a `mRan` flag because ThinWebView's runOnNextFrame()
// does not support cancellation. If a new content replacement happens before the next
// frame renders, we immediately run this runnable to clean up the old state. When the
// next frame eventually fires, the guard flag prevents running the cleanup logic again
// (which would cause duplicate JNI callbacks and native crashes).For this comment, I think it's also important to explain why `ThinWebView#runOnNextFrame()` needs a `Runnable` reference separate from `mPendingReplaceRunnable`, i.e., why `removeOldViewRunnable` and `mPendingReplaceRunnable` may point to different instances and cause bugs.
Done
mRan = true;
if (mPendingReplaceRunnable == this) {
mPendingReplaceRunnable = null;
}nit - it's more intuitive to place Line 197 ~ Line 201 at the end of `run()`.
```
@Override
public void run() {
if (mRan) return;
// Do work...
// Now the method is run.
mRan = true;
if (mPendingReplaceRunnable == this) {
mPendingReplaceRunnable = null;
}
}
```
I understand that placing it at the end feels more intuitive logically, but keeping mRan = true at the beginning of run() is a defensive programming practice to prevent re-entrancy bugs.
If onPanelContentReplaced.run() (which calls into native C++ via JNI) synchronously triggers another content replacement:
1. The new replacement request would call mPendingReplaceRunnable.run() (which is still the current runnable).
2. If mRan is not yet true, the current runnable would execute again recursively, leading to duplicate JNI callbacks and potential crashes.
By setting mRan = true at the very beginning, we guarantee that any synchronous re-entrant calls to run() will return immediately.
Given this, would you still prefer to move it to the end, or should we keep it at the beginning for re-entrancy protection?
if (oldView != null && oldView != newContent.mView) {
mContainerView.removeView(oldView);
}I think we can remove Line 202.
(1) `oldView` should always be non-null.
(2) If new View is the same as `oldView`, `addView()` at Line 183 will crash.
Good catch!
Done.
if (thinWebView != null) {
thinWebView.runOnNextFrame(removeOldViewRunnable);
} else {
removeOldViewRunnable.run();
}(1) Let's keep `BuildConfig.IS_FOR_TEST`.
(2) We should set `mPendingReplaceRunnable` to `null` if the old View is synchronously removed.```suggestion
if (thinWebView == null || BuildConfig.IS_FOR_TEST) {
mPendingReplaceRunnable.run();
mPendingReplaceRunnable = null;
} else {
thinWebView.runOnNextFrame(removeOldViewRunnable);
}
```
(1) Done
(2) Calling mPendingReplaceRunnable.run() executes the custom Runnable we just created, which checks if (mPendingReplaceRunnable == this) and sets it to null internally. Since it is still pointing to the same runnable instance, it will already be set to null by the time run() returns.
Do you still prefer to add it explicitly for readabilit?
Related details
- Linyu He
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. |
Linyu He (Gerrit)
Linyu He added 3 comments![]( "Open in Gerrit")
if (mPendingReplaceRunnable != null) {
mPendingReplaceRunnable.run();
}Shu YangCan we set `mPendingReplaceRunnable` to `null` after Line 177? It should always be `null` after it's run.
```suggestion
if (mPendingReplaceRunnable != null) {
mPendingReplaceRunnable.run();
mPendingRepalceRunnable = null;
}
```
The code currently handles setting mPendingReplaceRunnable to null inside the runnable's run() method itself.
This check is important because we only want to set mPendingReplaceRunnable to null if it still points to the runnable that is currently executing. If a newer replacement has occurred, mPendingReplaceRunnable will have been updated to the newer runnable, and we shouldn't clear it.
If we explicitly set it to null in startReplacingPanelContent like this, it is redundant because calling mPendingReplaceRunnable.run() will set it to null anyway.
Do you still prefer to add it explicitly for readability, or are you comfortable keeping the current implementation since it already handles nulling it out safely?
"`mPendingReplaceRunnable.run()` will set `mPendingReplaceRunnable` to null" is quite unusual since usually dereferencing a variable won't make it null.
I did try to suggest refactoring the code to avoid this unusual behavior, but it seemed hard.
So we need to make it easier to understand when `mPendingReplaceRunnable` is cleared. We can either explicitly set it to null or add a comment explaining that `mPendingReplaceRunnable.run()` will set `mPendingReplaceRunnable` to null.
mRan = true;
if (mPendingReplaceRunnable == this) {
mPendingReplaceRunnable = null;
}Shu Yangnit - it's more intuitive to place Line 197 ~ Line 201 at the end of `run()`.
```
@Override
public void run() {
if (mRan) return;
// Do work...
// Now the method is run.
mRan = true;
if (mPendingReplaceRunnable == this) {
mPendingReplaceRunnable = null;
}
}
```
I understand that placing it at the end feels more intuitive logically, but keeping mRan = true at the beginning of run() is a defensive programming practice to prevent re-entrancy bugs.
If onPanelContentReplaced.run() (which calls into native C++ via JNI) synchronously triggers another content replacement:
1. The new replacement request would call mPendingReplaceRunnable.run() (which is still the current runnable).
2. If mRan is not yet true, the current runnable would execute again recursively, leading to duplicate JNI callbacks and potential crashes.By setting mRan = true at the very beginning, we guarantee that any synchronous re-entrant calls to run() will return immediately.
Given this, would you still prefer to move it to the end, or should we keep it at the beginning for re-entrancy protection?
keeping mRan = true at the beginning of run() is a defensive programming practice to prevent re-entrancy bugs.
SG, but please add comments to explain this is for preventing re-entrancy bugs so that no future CLs will move `mRan = true`.
For the `mPendingReplaceRunnable` logic, can we still moved it to the end of the method?
```
if (mRan) return;
// Immediately set mRun to true to prevent reentrancy.
mRan = true;
// Do work.
// If the work is for the current runnable, clear the runnable.
if (mPendingReplaceRunnable == this) {
mPendingReplaceRunnable = null;
}
```
if (thinWebView != null) {
thinWebView.runOnNextFrame(removeOldViewRunnable);
} else {
removeOldViewRunnable.run();
}Shu Yang(1) Let's keep `BuildConfig.IS_FOR_TEST`.
(2) We should set `mPendingReplaceRunnable` to `null` if the old View is synchronously removed.```suggestion
if (thinWebView == null || BuildConfig.IS_FOR_TEST) {
mPendingReplaceRunnable.run();
mPendingReplaceRunnable = null;
} else {
thinWebView.runOnNextFrame(removeOldViewRunnable);
}
```
(1) Done
(2) Calling mPendingReplaceRunnable.run() executes the custom Runnable we just created, which checks if (mPendingReplaceRunnable == this) and sets it to null internally. Since it is still pointing to the same runnable instance, it will already be set to null by the time run() returns.
Do you still prefer to add it explicitly for readabilit?
For (2), please see this comment: https://chromium-review.git.corp.google.com/c/chromium/src/+/7962991/comment/5ede631a_ef8ce103/
Related details
- Shu Yang
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. |
Shu Yang (Gerrit)
Shu Yang added 3 comments![]( "Open in Gerrit")
if (mPendingReplaceRunnable != null) {
mPendingReplaceRunnable.run();
}Shu YangCan we set `mPendingReplaceRunnable` to `null` after Line 177? It should always be `null` after it's run.
```suggestion
if (mPendingReplaceRunnable != null) {
mPendingReplaceRunnable.run();
mPendingRepalceRunnable = null;
}
```
Linyu HeThe code currently handles setting mPendingReplaceRunnable to null inside the runnable's run() method itself.
This check is important because we only want to set mPendingReplaceRunnable to null if it still points to the runnable that is currently executing. If a newer replacement has occurred, mPendingReplaceRunnable will have been updated to the newer runnable, and we shouldn't clear it.
If we explicitly set it to null in startReplacingPanelContent like this, it is redundant because calling mPendingReplaceRunnable.run() will set it to null anyway.
Do you still prefer to add it explicitly for readability, or are you comfortable keeping the current implementation since it already handles nulling it out safely?
"`mPendingReplaceRunnable.run()` will set `mPendingReplaceRunnable` to null" is quite unusual since usually dereferencing a variable won't make it null.
I did try to suggest refactoring the code to avoid this unusual behavior, but it seemed hard.
So we need to make it easier to understand when `mPendingReplaceRunnable` is cleared. We can either explicitly set it to null or add a comment explaining that `mPendingReplaceRunnable.run()` will set `mPendingReplaceRunnable` to null.
sg! Now I explicitly set mPendingReplaceRunnable = null in the synchronous block, while adding a comment explaining that this is done for readability even though it's technically redundant.
mRan = true;
if (mPendingReplaceRunnable == this) {
mPendingReplaceRunnable = null;
}Yes we should be able to move the mPendingReplaceRunnable logic to the end of the method.
if (thinWebView != null) {
thinWebView.runOnNextFrame(removeOldViewRunnable);
} else {
removeOldViewRunnable.run();
}Shu Yang(1) Let's keep `BuildConfig.IS_FOR_TEST`.
(2) We should set `mPendingReplaceRunnable` to `null` if the old View is synchronously removed.```suggestion
if (thinWebView == null || BuildConfig.IS_FOR_TEST) {
mPendingReplaceRunnable.run();
mPendingReplaceRunnable = null;
} else {
thinWebView.runOnNextFrame(removeOldViewRunnable);
}
```
Linyu He(1) Done
(2) Calling mPendingReplaceRunnable.run() executes the custom Runnable we just created, which checks if (mPendingReplaceRunnable == this) and sets it to null internally. Since it is still pointing to the same runnable instance, it will already be set to null by the time run() returns.
Do you still prefer to add it explicitly for readabilit?
For (2), please see this comment: https://chromium-review.git.corp.google.com/c/chromium/src/+/7962991/comment/5ede631a_ef8ce103/
- Linyu He
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Linyu He (Gerrit)
Linyu He voted and added 1 comment![]( "Open in Gerrit")
- Shu Yang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shu Yang (Gerrit)
Shu Yang voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |