Commit 760cc225 authored by Friedrich Horschig's avatar Friedrich Horschig Committed by Commit Bot

[Android][Performance] Prevent accessory from over-openening keyboard

This CL prevents the keyboard accessory from triggering a keyboard that
hasn't been requested.

Example: Open youtube or chrome://flags
Before this patch:
  The keyboard accessory controller detects that there are passwords for
  this site. Because the search field is focused by default, it will
  request openening the keyboard.
Why this is wrong:
  The controller wrongfully assumed that a focused input field always
  triggers the keyboard. This is wrong, esp. for pageloads.
After this patch:
  The controller requests to open the keyboard iff the accessory sheet
  is opened.

Why has this become a problem now?
Before https://crrev.com/c/1177361, the controller would just request
the accessory to be closed. This caused nasty artifacts and bugs.
Therefore, it requested to open keyboard to dismiss the sheet gently.
This resulted in opening the keyboard too often.

What is potentially impacted by this:
 - Performance tests with search bars: Confirmed
   --> smoothness.*/youtube_pinch_2018 not failing consistently
 - Health tests: Likely confirmed
   system_health.memory_mobile/browse.tools.maps debug run changed
   memory:chrome:all_processes:reported_by_chrome:effective_size
   by 7.7%
 - Startup with previously opened sites: Confirmed
   Following instructions in issue 876228 don't reproduce error.

Bug: 875859, 876025, 876228
Change-Id: Ic5873c07cb8652bc0da5edcca8324f2680baac11
Reviewed-on: https://chromium-review.googlesource.com/1184847Reviewed-by: default avatarVasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Friedrich Horschig [CEST] <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585454}
parent e4855f51
...@@ -72,10 +72,10 @@ public class ManualFillingCoordinator { ...@@ -72,10 +72,10 @@ public class ManualFillingCoordinator {
} }
/** /**
* Tries to reopen the keyboard which will implicitly show the keyboard accessory bar again. * Opens the keyboard which implicitly dismisses the sheet. Without open sheet, this is a NoOp.
*/ */
public void openKeyboard() { public void swapSheetWithKeyboard() {
mMediator.onOpenKeyboard(); mMediator.swapSheetWithKeyboard();
} }
void registerActionProvider( void registerActionProvider(
......
...@@ -262,6 +262,13 @@ class ManualFillingMediator ...@@ -262,6 +262,13 @@ class ManualFillingMediator
mAccessorySheet.hide(); mAccessorySheet.hide();
} }
/**
* Opens the keyboard which implicitly dismisses the sheet. Without open sheet, this is a NoOp.
*/
void swapSheetWithKeyboard() {
if (mAccessorySheet.isShown()) onOpenKeyboard();
}
@Override @Override
public void onOpenKeyboard() { public void onOpenKeyboard() {
assert mActivity != null : "ManualFillingMediator needs initialization."; assert mActivity != null : "ManualFillingMediator needs initialization.";
......
...@@ -75,8 +75,8 @@ class PasswordAccessoryBridge { ...@@ -75,8 +75,8 @@ class PasswordAccessoryBridge {
} }
@CalledByNative @CalledByNative
private void openKeyboard() { private void swapSheetWithKeyboard() {
mManualFillingCoordinator.openKeyboard(); mManualFillingCoordinator.swapSheetWithKeyboard();
} }
@CalledByNative @CalledByNative
......
...@@ -69,8 +69,8 @@ void PasswordAccessoryViewAndroid::CloseAccessorySheet() { ...@@ -69,8 +69,8 @@ void PasswordAccessoryViewAndroid::CloseAccessorySheet() {
base::android::AttachCurrentThread(), java_object_); base::android::AttachCurrentThread(), java_object_);
} }
void PasswordAccessoryViewAndroid::OpenKeyboard() { void PasswordAccessoryViewAndroid::SwapSheetWithKeyboard() {
Java_PasswordAccessoryBridge_openKeyboard( Java_PasswordAccessoryBridge_swapSheetWithKeyboard(
base::android::AttachCurrentThread(), java_object_); base::android::AttachCurrentThread(), java_object_);
} }
......
...@@ -30,7 +30,7 @@ class PasswordAccessoryViewAndroid : public PasswordAccessoryViewInterface { ...@@ -30,7 +30,7 @@ class PasswordAccessoryViewAndroid : public PasswordAccessoryViewInterface {
void OnItemsAvailable(const std::vector<AccessoryItem>& items) override; void OnItemsAvailable(const std::vector<AccessoryItem>& items) override;
void OnAutomaticGenerationStatusChanged(bool available) override; void OnAutomaticGenerationStatusChanged(bool available) override;
void CloseAccessorySheet() override; void CloseAccessorySheet() override;
void OpenKeyboard() override; void SwapSheetWithKeyboard() override;
// Called from Java via JNI: // Called from Java via JNI:
void OnFaviconRequested( void OnFaviconRequested(
......
...@@ -173,7 +173,7 @@ void PasswordAccessoryController::OnFilledIntoFocusedField( ...@@ -173,7 +173,7 @@ void PasswordAccessoryController::OnFilledIntoFocusedField(
autofill::FillingStatus status) { autofill::FillingStatus status) {
if (status != autofill::FillingStatus::SUCCESS) if (status != autofill::FillingStatus::SUCCESS)
return; // TODO(crbug/853766): Record success rate. return; // TODO(crbug/853766): Record success rate.
view_->OpenKeyboard(); // Bring up the keyboard for the still focused field. view_->SwapSheetWithKeyboard();
} }
void PasswordAccessoryController::RefreshSuggestionsForField( void PasswordAccessoryController::RefreshSuggestionsForField(
...@@ -184,7 +184,7 @@ void PasswordAccessoryController::RefreshSuggestionsForField( ...@@ -184,7 +184,7 @@ void PasswordAccessoryController::RefreshSuggestionsForField(
current_origin_ = origin; current_origin_ = origin;
view_->OnItemsAvailable(CreateViewItems(origin, origin_suggestions_[origin], view_->OnItemsAvailable(CreateViewItems(origin, origin_suggestions_[origin],
is_password_field)); is_password_field));
view_->OpenKeyboard(); // Should happen automatically. view_->SwapSheetWithKeyboard();
} else { } else {
// For unfillable fields, reset the origin and send the empty state message. // For unfillable fields, reset the origin and send the empty state message.
current_origin_ = url::Origin(); current_origin_ = url::Origin();
......
...@@ -65,7 +65,7 @@ class MockPasswordAccessoryView : public PasswordAccessoryViewInterface { ...@@ -65,7 +65,7 @@ class MockPasswordAccessoryView : public PasswordAccessoryViewInterface {
MOCK_METHOD0(OnViewDestroyed, void()); MOCK_METHOD0(OnViewDestroyed, void());
MOCK_METHOD1(OnAutomaticGenerationStatusChanged, void(bool)); MOCK_METHOD1(OnAutomaticGenerationStatusChanged, void(bool));
MOCK_METHOD0(CloseAccessorySheet, void()); MOCK_METHOD0(CloseAccessorySheet, void());
MOCK_METHOD0(OpenKeyboard, void()); MOCK_METHOD0(SwapSheetWithKeyboard, void());
private: private:
DISALLOW_COPY_AND_ASSIGN(MockPasswordAccessoryView); DISALLOW_COPY_AND_ASSIGN(MockPasswordAccessoryView);
...@@ -259,7 +259,7 @@ class PasswordAccessoryControllerTest : public ChromeRenderViewHostTestHarness { ...@@ -259,7 +259,7 @@ class PasswordAccessoryControllerTest : public ChromeRenderViewHostTestHarness {
mock_dialog_factory_.Get(), favicon_service()); mock_dialog_factory_.Get(), favicon_service());
NavigateAndCommit(GURL(kExampleSite)); NavigateAndCommit(GURL(kExampleSite));
EXPECT_CALL(*view(), CloseAccessorySheet()).Times(AnyNumber()); EXPECT_CALL(*view(), CloseAccessorySheet()).Times(AnyNumber());
EXPECT_CALL(*view(), OpenKeyboard()).Times(AnyNumber()); EXPECT_CALL(*view(), SwapSheetWithKeyboard()).Times(AnyNumber());
} }
PasswordAccessoryController* controller() { PasswordAccessoryController* controller() {
...@@ -406,12 +406,12 @@ TEST_F(PasswordAccessoryControllerTest, ProvidesEmptySuggestionsMessage) { ...@@ -406,12 +406,12 @@ TEST_F(PasswordAccessoryControllerTest, ProvidesEmptySuggestionsMessage) {
TEST_F(PasswordAccessoryControllerTest, ClosesViewOnSuccessfullFillingOnly) { TEST_F(PasswordAccessoryControllerTest, ClosesViewOnSuccessfullFillingOnly) {
// If the filling wasn't successful, no call is expected. // If the filling wasn't successful, no call is expected.
EXPECT_CALL(*view(), CloseAccessorySheet()).Times(0); EXPECT_CALL(*view(), CloseAccessorySheet()).Times(0);
EXPECT_CALL(*view(), OpenKeyboard()).Times(0); EXPECT_CALL(*view(), SwapSheetWithKeyboard()).Times(0);
controller()->OnFilledIntoFocusedField(FillingStatus::ERROR_NOT_ALLOWED); controller()->OnFilledIntoFocusedField(FillingStatus::ERROR_NOT_ALLOWED);
controller()->OnFilledIntoFocusedField(FillingStatus::ERROR_NO_VALID_FIELD); controller()->OnFilledIntoFocusedField(FillingStatus::ERROR_NO_VALID_FIELD);
// If the filling completed successfully, let the view know. // If the filling completed successfully, let the view know.
EXPECT_CALL(*view(), OpenKeyboard()); EXPECT_CALL(*view(), SwapSheetWithKeyboard());
controller()->OnFilledIntoFocusedField(FillingStatus::SUCCESS); controller()->OnFilledIntoFocusedField(FillingStatus::SUCCESS);
} }
...@@ -421,7 +421,8 @@ TEST_F(PasswordAccessoryControllerTest, ClosesViewWhenRefreshingSuggestions) { ...@@ -421,7 +421,8 @@ TEST_F(PasswordAccessoryControllerTest, ClosesViewWhenRefreshingSuggestions) {
EXPECT_CALL(*view(), OnItemsAvailable(_)).Times(AnyNumber()); EXPECT_CALL(*view(), OnItemsAvailable(_)).Times(AnyNumber());
EXPECT_CALL(*view(), CloseAccessorySheet()); EXPECT_CALL(*view(), CloseAccessorySheet());
EXPECT_CALL(*view(), OpenKeyboard()).Times(0); // Don't touch the keyboard! EXPECT_CALL(*view(), SwapSheetWithKeyboard())
.Times(0); // Don't touch the keyboard!
controller()->RefreshSuggestionsForField( controller()->RefreshSuggestionsForField(
url::Origin::Create(GURL(kExampleSite)), url::Origin::Create(GURL(kExampleSite)),
/*is_fillable=*/false, /*is_fillable=*/false,
......
...@@ -74,8 +74,8 @@ class PasswordAccessoryViewInterface { ...@@ -74,8 +74,8 @@ class PasswordAccessoryViewInterface {
// Called to inform the view that the accessory sheet should be closed now. // Called to inform the view that the accessory sheet should be closed now.
virtual void CloseAccessorySheet() = 0; virtual void CloseAccessorySheet() = 0;
// Called to inform the view that the accessory sheet should be closed now. // Opens a keyboard which dismisses the sheet. NoOp without open sheet.
virtual void OpenKeyboard() = 0; virtual void SwapSheetWithKeyboard() = 0;
private: private:
friend class PasswordAccessoryController; friend class PasswordAccessoryController;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment