Return future isActive and isVisible info once related func is triggered [chromium/src : main]
Lijin Shen (Gerrit)
Lijin Shen voted and added 17 comments![]( "Open in Gerrit")
Votes added by Lijin Shen
| Commit-Queue | +1 |
17 comments
Return future isActive info as soon as activate/show is triggeredLijin Shen`isActive` --> `isActive and isVisible`?
Done
This CL makes the isActive temporarily return true when activate/show isLijin Shenditto: update description to be consistent with the code changes.
Done
Lijin ShenCan we add unit tests for the new changes?
I was thinking the changes in IntergrationsTest have already covered the changes.
Do you mean it would better to add more unit tests in ChromeAndroidTaskImplUnitTest.java?
} else if (mState.get() == State.PENDING_UPDATE
&& mPendingActionManager.isActionRequested(PendingAction.MAXIMIZE)) {
return true;Lijin ShenCan we move this part to a subsequent CL to focus on `isActivate` and `isVisible` in this CL?
Done
} else if (mState.get() == State.PENDING_UPDATE
&& mPendingActionManager.isActionRequested(PendingAction.MINIMIZE)) {
return true;Lijin Shenditto: move this part to a separate CL focusing on `isMaximized()`/`isMinimized()`?
Done
if (mState.get() == State.PENDING_CREATE || mState.get() == State.PENDING_UPDATE) {Lijin Shenditto: move this to a separate CL, too.
Done
Lijin ShenBesides integration tests, let's also do some manual testing using the test extension:
(1) `update()` returns the updated state (the future state);
(2) wait for the update operation to complete, then `get()` returns the live state;
(3) change the window state without using the test extension, then `get()` still returns the correct live state.
It works normally. https://screenshot.googleplex.com/NaTHkKYaLKgDhiZ
CriteriaHelper.pollUiThread(
assumeNonNull(webPageStation.getActivity().getWindowAndroid())
::isTopResumedActivity);Lijin ShenI think `ExtensionWindowControllerBridgeIntegrationTest` will need similar fixes.
it seems good now. No activate or show operation is involved in that test.
CriteriaHelper.pollUiThread(Lijin ShenBefore `pollUiThread`, we can check `chromeAndroidTask.isActive()` returns true (the future state).
Then after `pollUiThread`, let's check that again (the live state).
Done
CriteriaHelper.pollUiThread(
assumeNonNull(webPageStation.getActivity().getWindowAndroid())
::isTopResumedActivity);Lijin ShenSame here: let's check both the future state and the live state before and after `pollUiThread`.
Done
CriteriaHelper.pollUiThread(() -> !secondChromeAndroidTask.isActive());Lijin ShenDo we need `pollUiThread` at line 351?
Also, ditto: check both the future state and the live state.
Done
Lijin ShenWe need unit tests for the new APIs in `PendingActionManager`:
- `isActive()`
- `isVisible()`
- `getAndClearTargetPendingActions()`
Done
private @Nullable Boolean mIsActive;Lijin Shen(1) To clearly indicate it's a predicted state based on pending actions, let's rename `mIsActive` as `mIsActiveFuture`. (Same for other states.)
(2) Each of the future states should be `@GuardedBy("mPendingActionsLock")`.
(3) Document what `null` means. You can remove line 98 and document this in the Java doc of `isActive()` and `isVisible()`, which would be clearer to callers.
Done
@Nullable Boolean isActive() {Lijin Shen* ditto - rename: `isActive()` -> `isActiveFuture()`
* Add Java doc to explain:
* The returned value is a predicted future state based on pending actions;
* What it means when `null` is returned.(Same for `isVisible()` below.)
Done
if (mIsClosed != null) return false;Lijin ShenCan we set `mIsActive` to `false` when there is `PendingAction.CLOSE`?
This way we can remove the `mIsClosed` state and simplify `isActive()` and `updatePendingStates()` (one fewer state to deal with).
(Same for `isVisible()` below.)
Linyu HeMy understanding was that the close has a very high priority. Calling close then activate still results in isActive being true. Does this make sense to you?
Lijin ShenI see. @aishw...@google.com and I discussed this in a separate CL, and the conclusion is that because "close" has a very high (actually the highest) priority, any subsequent actions should be ignored as we can't revive a destroyed window.
For `close` -> `activate`: `activate` should have no effect and `isActive()` should be `false` (window is destroyed).
If everything were synchronous, calling `activate()` after `close()` would be banned (app crashes).
Done
private void updatePendingStates() {Lijin Shennit - rename: `updatePendingStates()` -> `updateFutureStatesLocked()`
* We need to differentiate between "pending" and "future" in this class.
* "Pending": a request this class has received but not yet fulfilled.
* "Future": a predicted result based on "pending" requests.
- `*Locked()`: the method is `@GuardedBy` a lock.
Done
if (action == PendingAction.CLOSE) mIsClosed = true;Lijin ShenFollowing the comment for `isActive()` above: let's try removing `mIsClosed`.
```
switch (action) {
//...
case PendingAction.CLOSE: // <-- new
mIsActive = false;
break;
default:
break;
}
```(Same for `mIsVisible` below.)
Done
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 8 comments![]( "Open in Gerrit")
This CL makes the isActive and is Visible temporarily return true whennit:
```suggestion
This CL makes isActive() and isVisible() temporarily return true when
```
Lijin ShenCan we add unit tests for the new changes?
I was thinking the changes in IntergrationsTest have already covered the changes.
Do you mean it would better to add more unit tests in ChromeAndroidTaskImplUnitTest.java?
I was thinking the changes in IntergrationsTest have already covered the changes.
That's correct, but please see the reply below.
> Do you mean it would better to add more unit tests in ChromeAndroidTaskImplUnitTest.java?
Yes, `ChromeAndroidTaskIntegrationTest` is in CI so it won't be run by default (we know it needs to be run; folks not working on this project don't). Based on my gardening experience, when CI fails, it would take longer to identify and fix an issue.
So we'll use unit tests as quick sanity checks, similar to [these existing unit tests](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/browser_window/internal/android/java/src/org/chromium/chrome/browser/ui/browser_window/ChromeAndroidTaskImplUnitTest.java;l=1193,1204,1217;drc=b5a0fa6584b0931f30f672d3f877bb4833d107b9).
Usually when practically feasible, we add both unit tests and integration tests.
[ Note: we need to be clear what `_whenPending_` means in the test names now, i.e., existing tests should have `_whenPendingCreate_` and new ones are `_whenPendingUpdate_`. ]
"Activate should make isActive true immediately", chromeAndroidTask.isActive());```suggestion
"Show() should make isActive true immediately", chromeAndroidTask.isActive());
```
"Activate should make isActive true eventually", chromeAndroidTask.isActive());```suggestion
"Show() should make isActive true eventually", chromeAndroidTask.isActive());
```
/*WebPageStation webPageStation = mFreshCtaTransitTestRule.startOnBlankPage();Uncomment this? :)
Assert.assertTrue(
"Activate should make isActive true immediately", chromeAndroidTask.isActive());
CriteriaHelper.pollUiThread(
assumeNonNull(webPageStation.getActivity().getWindowAndroid())
::isTopResumedActivity);
Assert.assertTrue(
"Activate should make isActive true eventually", chromeAndroidTask.isActive());
CriteriaHelper.pollUiThread(() -> !secondChromeAndroidTask.isActive());As `showInactive()` is the method under test here, I think this part should be:
```
Assert.assertFalse(
"ShowInactive should make isActive() false immediately",
secondChromeAndroidTask.isActive());
// Wait for the first Task to become active.
CriteriaHelper.pollUiThread(
assumeNonNull(webPageStation.getActivity().getWindowAndroid())
::isTopResumedActivity);
Assert.assertFalse(
"ShowInactive should make isActive() false eventually",
secondChromeAndroidTask.isActive());
```
Assert.assertTrue(
"Activate should make isActive true immediately", chromeAndroidTask.isActive());
CriteriaHelper.pollUiThread(
assumeNonNull(webPageStation.getActivity().getWindowAndroid())
::isTopResumedActivity);
Assert.assertTrue(
"Activate should make isActive true eventually", chromeAndroidTask.isActive());
assertFalse(secondChromeAndroidTask.isActive());Same here, we are testing `deactivate()` for `secondChromeAndroidTask`:
```
Assert.assertFalse(
"Deactivate() should make isActive() false immediately",
secondChromeAndroidTask.isActive());
// Wait for the first Task to become active, which completes the deactivation
// of the second Task.
CriteriaHelper.pollUiThread(
assumeNonNull(webpageStation.getActivity().getWindowAndroid())
::isTopResumedActivity);
Assert.assertFalse(
"Deactivate() should make isActive() false eventually",
secondChromeAndroidTask.isActive());
```
Related details
- Lijin Shen
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. |