[iOS] Throttle keyboardWillShow refreshes [chromium/src : main]
Leo Zhao (Gerrit)
Leo Zhao added 11 comments![]( "Open in Gerrit")
[iOS] flags for the experiment on keyboardWillShow triggered suggestion refreshesLeo ZhaoI would focus on the feature for the title:
"Throttle keyboardWillShow refreshes"
👍
BASE_DECLARE_FEATURE(kAutofillKeyboardWillShowSuggestionRefreshOptional);Leo ZhaoI think we could shorten this:
kAutofillThrottleOptionalSuggestionRefresh
Updated.
// 2. avoid triggering a -retrieveSuggestions call with staled data just before
// -webState:didRegisterFormActivity:inFrame:.Leo Zhaonot sure we prevent that, because the cooldown starts after the activity
but yes, there is throttling in place to avoid over doing it, which could be the case with stale data
This is the result of the delay. When a field is focused, `keyboardWillShow` arrives first. -webState:didRegisterFormActivity:inFrame: immediately after that. The delay schedules the update, which then is cancelled by the subsequent -webState:didRegisterFormActivity:inFrame: call.
// both.Leo Zhaoput in comment the sum of the 2 delays
an alternative is to add the constants in the .h so they can be reused here I think you can sum 2 constexpr + a buffer, up to you
I have added the constants in the +testing.h file. This value is not used for measure anything timing. As long as it is bigger than the longest one, which is the cooldown period. I am using 2x cooldown.
constexpr base::TimeDelta kKeyboardWillShowFastForwardSeconds =Leo Zhao`kDelayForAcceptingOptionalUpdates`
Updated.
void PostKeyboardWillShowNotification(int count = 1) {Leo Zhaoadd `s`
I added a s to the end, PostKeyboardWillShowNotification(s).
[[NSNotificationCenter defaultCenter]Leo Zhaowe could fast forward the clock between each
This is used to post consecutive keyboard notifications. If a fast forward is needed, I use PostKeyboardWillShowNotifications(1) and then fast forward.
// testing. Otherwise, suggestion refresh requests may be blocked due to app
// state being inactive while running unit tests.Leo Zhaohow does it happen ?
updateSuggestionsIfNeeded calls IsSuggestionRefreshAllowed. IsSuggestionRefreshAllowed checks app state when this feature is enabled, and blocks refreshes if the app is inactive. This is not happening for unit testings, so I am removing it.
{}, {kSuppressKeyboardWillShowSuggestionRefresh,Leo Zhaoadd `/*enabled_features=*/` to the first arg, `/*disabled_features=*/` to the second arg
👍
Leo Zhaowe could add a test that tests the updates are rolled over when throttling enable, without the cooldown period, testing throttling in isolation
I have added a new test to verify the restarting of the delay: keyboardWillShowRefresh_RollingOver.
TEST_F(FormInputAccessoryMediatorTest, keyboardWillShowRefresh_NotSuppressed) {Leo Zhaohow is it different from keyboardWillShowRefresh_NotThrottled ?
They are the same actually. I have removed one.
Related details
- Vincent Boisselle
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vincent Boisselle (Gerrit)
Vincent Boisselle added 6 comments![]( "Open in Gerrit")
extern const NSTimeInterval kOptionalUpdateCooldownPeriod;this won't work with constexpr, I think we could just put those in the header file then use `inline` in the header file to avoid collisions
extern const NSTimeInterval kOptionalUpdateCooldownPeriod = 2.0;use `base::TimeDelta::Seconds(2)` , can be used as constexpr
extern const NSTimeInterval kOptionalUpdateDelay = 0.5;use `base::TimeDelta::Seconds(0.5)` , can be used as constexpr
base::Milliseconds(kOptionalUpdateDelay * 1000),you will be able to use this one as is once converted
[weakSelf performOptionalUpdateIfAllowed];I think we can call `updateSuggestionsIfNeeded` directly since we now stop the scheduled task on `-retrieveSuggestionsForForm`, this simplifies the code a bit
base::Seconds(kOptionalUpdateCooldownPeriod * 2);I think timedelta constants supports operators,
Related details
- Leo Zhao
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. |
Leo Zhao (Gerrit)
Leo Zhao added 6 comments![]( "Open in Gerrit")
extern const NSTimeInterval kOptionalUpdateCooldownPeriod;this won't work with constexpr, I think we could just put those in the header file then use `inline` in the header file to avoid collisions
`kOptionalUpdateCooldownPeriod` is now in the header file.
extern const NSTimeInterval kOptionalUpdateCooldownPeriod = 2.0;use `base::TimeDelta::Seconds(2)` , can be used as constexpr
This has been moved into .h file and becomes `inline constexpr`.
extern const NSTimeInterval kOptionalUpdateDelay = 0.5;use `base::TimeDelta::Seconds(0.5)` , can be used as constexpr
This has been moved into .h file and is now an `inline constexpr`.
base::Milliseconds(kOptionalUpdateDelay * 1000),you will be able to use this one as is once converted
Updated.
I think we can call `updateSuggestionsIfNeeded` directly since we now stop the scheduled task on `-retrieveSuggestionsForForm`, this simplifies the code a bit
👍 this is updated to `updateSuggestionsIfNeeded` and `performOptionalUpdateIfAllowed` is removed.
I think timedelta constants supports operators,
In patchset 16, this is updated to use `kOptionalUpdateCooldownPeriod` with 10ms extra.
Related details
- Vincent Boisselle
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vincent Boisselle (Gerrit)
Vincent Boisselle voted and added 8 comments![]( "Open in Gerrit")
Votes added by Vincent Boisselle
| Code-Review | +1 |
8 comments
// The relationship of the three feature flags used in this test and a few other2 ?
task_environment_.FastForwardBy(kDelayForAcceptingOptionalUpdates);do we need this delay when all throttling features are disabled and there is effectively no cooldown period?
EXPECT_GE(count, 3);can we expect equality here ? , i.e. EXPET_EQ
task_environment_.FastForwardBy(kDelayForAcceptingOptionalUpdates);we could use the delay for scheduling the refresh + a small buffer to be a bit more accurate
TEST_F(FormInputAccessoryMediatorTest, keyboardWillShowRefresh_RollingOver) {keyboardWillShowRefresh_Throttled_RollingOver
Related details
- Leo Zhao
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. |
Vincent Boisselle (Gerrit)
Vincent Boisselle voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Leo Zhao
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. |
Leo Zhao (Gerrit)
Leo Zhao added 7 comments![]( "Open in Gerrit")
#include "base/time/time.h"Leo Zhaonit: #import
Done
// The relationship of the three feature flags used in this test and a few otherLeo Zhao2 ?
Done
task_environment_.FastForwardBy(kDelayForAcceptingOptionalUpdates);do we need this delay when all throttling features are disabled and there is effectively no cooldown period?
Done
can we expect equality here ? , i.e. EXPET_EQ
Done
task_environment_.FastForwardBy(kDelayForAcceptingOptionalUpdates);we could use the delay for scheduling the refresh + a small buffer to be a bit more accurate
Done
TEST_F(FormInputAccessoryMediatorTest, keyboardWillShowRefresh_RollingOver) {Leo ZhaokeyboardWillShowRefresh_Throttled_RollingOver
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leo Zhao (Gerrit)
Leo Zhao added 1 comment![]( "Open in Gerrit")
Pathset 18 is a rebase (on main after the cut off CL is merged). Pathset 19 added a change suggested in the cut off CL, moving setup code with no assertions into FormInputAccessoryViewControllerTest's constructor.
Related details
- Vincent Boisselle
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vincent Boisselle (Gerrit)
Vincent Boisselle voted Code-Review+1![]( "Open in Gerrit")
- Leo Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vincent Boisselle (Gerrit)
Vincent Boisselle voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Leo Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Martino (Gerrit)
Tommy Martino added 4 comments![]( "Open in Gerrit")
// The period from an optional update is requested to it is being carried out.nit: "The period from when an optional update is requested to when it is carried out."
WDYT about moving the new logic (the new methods, the timers, etc) into a new C++ class, something like `KeyboardAccessoryRefreshScheduler`? The mediator could own an instance of the scheduler, and the scheduler could hold a `__weak id<SomeNewProtocol>` where `SomeNewProtocol` declares just `updateSuggestionsIfNeeded`.
It would let us remove some logic from this (huge) class, it would be a nice single-purpose object, and I think it would be more testable.
// 2. avoid triggering a -retrieveSuggestions call with staled data just beforesp: stale
// The graph below shows the timeline of optional updates.
// suggestions update
// request ^
// delay v cooldown period delay|
// |------|...|---------------------|---------|.....
// ^ | | ^ ^ ^
// | v v | | |
// | update update optional | |
// | update | optional
// optional (rejected) | update (reset delay)
// update optional
// (scheduled) update
// (scheduled)
// In the graph above:
This is a great comment, thanks for this!
Related details
- Leo Zhao
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. |
Leo Zhao (Gerrit)
Leo Zhao added 3 comments![]( "Open in Gerrit")
// The period from an optional update is requested to it is being carried out.nit: "The period from when an optional update is requested to when it is carried out."
Updated.
WDYT about moving the new logic (the new methods, the timers, etc) into a new C++ class, something like `KeyboardAccessoryRefreshScheduler`? The mediator could own an instance of the scheduler, and the scheduler could hold a `__weak id<SomeNewProtocol>` where `SomeNewProtocol` declares just `updateSuggestionsIfNeeded`.
It would let us remove some logic from this (huge) class, it would be a nice single-purpose object, and I think it would be more testable.
I have moved the timers and the checking logic into the new C++ class. Early in this CL, "refresh" is opted out in favor of "optional update". So, `KeyboardAccessoryRefreshScheduler` becomes `KeyboardAccessoryOptionalUpdateScheduler`. The function with the long comment remains in this file, with internal logic moved into the new c++ class. `scheduleOptionalUpdate` could have been removed too. However, the comment describes the result of the scheduler and how it is being utilized, so it is fitting to keep the comment still in this class. So, scheduleOptionalUpdate is largely just to accommodate the comment.
// 2. avoid triggering a -retrieveSuggestions call with staled data just beforeLeo Zhaosp: stale
Updated.
Related details
- Tommy Martino
- Vincent Boisselle
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Martino (Gerrit)
Tommy Martino voted and added 3 comments![]( "Open in Gerrit")
Votes added by Tommy Martino
| Code-Review | +1 |
3 comments
std::unique_ptr<KeyboardAccessoryOptionalUpdateScheduler>Generally, avoid `unique_ptr` for objects that won't change ownership/be moved. Instead, either hold the object directly when possible, or use an std::optional when delayed initialization is necessary.
__weak id<KeyboardAccessoryOptionalUpdateSchedulerDelegate> weak_delegate =
delegate_;`delegate_` is already weak 😊
Related details
- Leo Zhao
- Vincent Boisselle
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. |
Vincent Boisselle (Gerrit)
Vincent Boisselle voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Leo Zhao
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. |
Leo Zhao (Gerrit)
Leo Zhao added 2 comments![]( "Open in Gerrit")
std::unique_ptr<KeyboardAccessoryOptionalUpdateScheduler>Generally, avoid `unique_ptr` for objects that won't change ownership/be moved. Instead, either hold the object directly when possible, or use an std::optional when delayed initialization is necessary.
Updated.
__weak id<KeyboardAccessoryOptionalUpdateSchedulerDelegate> weak_delegate =
delegate_;Leo Zhao`delegate_` is already weak 😊
👍
Related details
- Tommy Martino
- Vincent Boisselle
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Martino (Gerrit)
Tommy Martino voted Code-Review+1![]( "Open in Gerrit")
- Leo Zhao
- Vincent Boisselle
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leo Zhao (Gerrit)
Leo Zhao added 1 comment![]( "Open in Gerrit")
Retrying the Dry CQ Run. The error seems not related to this CL. I have seen a similar issue early this week where a test failed, but I can not find that test in our codebase.
Related details
- Vincent Boisselle
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |