Remove caret visibility logic from iOS |
||
Issue descriptionOmnibox has the following APIs: OmniboxView::ApplyCaretVisibility(); OmniboxModel::SetCaretVisibility(bool visible); On Desktop, those are used to actually toggle the caret visibility in the omnibox. On iOS, we provide an empty implementation for ApplyCaretVisibility(); We used to call SetCaretVisibility(b) for the side effect of collecting the following metric: - UserActionRate.MobileFocusedFakeboxOnNtp - UserActionRate.MobileFocusedOmniboxNotOnNtp - UserActionRate.MobileFocusedOmniboxOnNtp But these metrics were deprecated and are not collected since M64. However, now we have the following metrics: - INSTANT_NTP_WITH_FAKEBOX_AS_STARTING_FOCUS (new tab page with fakebox as starting focus) - INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS (new tab page with omnibox as starting focus) Those seem to be collected when an autocomplete suggestion is being opened. The way it works is as follows: - OmniboxEditModel::OpenMatch(...) generates an OmniboxLog by calling ClassifyPage() and passes it to OmniboxEventGlobalTracker::OnURLOpened - OmniboxMetricsProvider::OnURLOpenedFromOmnibox() is called and - OmniboxMetricsProvider::RecordOmniboxOpenedURL() collects the metric So by the time OpenMatch is called, ClassifyPage() needs to know whether it was focused from the Fakebox or Omnibox. For this, OmniboxEditModel's focus_source_ is used. How is it set on iOS? When tapped on the fakebox: 1. [ContentSuggestionsHeaderViewController fakeOmniboxTapped:] 2. [ToolbarCoordinator focusFakebox] 3. ONLY ON IPAD: 3a. [LocationBarCoordinator focusOmniboxFromFakebox] 3b. OmniboxEditModel::OnSetFocus() <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 3c. OmniboxEditModel::SetCaretVisibility(false) 4. [LocationBarCoordinator focusOmnibox] 5. [OTFI becomeFirstResponder] 6. OmniboxViewIOS::OnDidBeginEditing() 7. ONLY ON IPHONE: OmniboxEditModel::OnSetFocus() <<<<<<<<<<<<<<<<<<<<<<<<<<<<< When tapped on the omnibox (on NTP or any other page): 1. [OTFI becomeFirstResponder] 2. OmniboxViewIOS::OnDidBeginEditing() 3. OmniboxEditModel::OnSetFocus() When "New Search" is tapped in Quick Actions widget: 1. MainApplicationDelegate, App State, etc 2. [BVC tabModel:newTabWillOpen:inBackground:] 3. [LocationBarCoordinator focusOmnibox] 4. [OTFI becomeFirstResponder] 5. OmniboxViewIOS::OnDidBeginEditing() 6. OmniboxEditModel::OnSetFocus() Now, none of this actually sets the focus_source_ variable of OmniboxEditModel; but once any character is typed: 1. -[UITextFieldDelegate textFieldDidChange:] 2. OmniboxViewIOS::OnDidChange 3. OmniboxViewIOS::OnAfterPossibleChange 4. OmniboxEditModel::OnAfterPossibleChange At this state, a workaround for Linux with a TODO(samarth) (dating to 2013) is setting focus_source_. -------------------------------------------------------------------------------------------------------------------------------------------- The above calls for the following questions: 1. Does anybody care about these metrics? 2. How to better collect these metrics without relying on SetCaretVisibility() as a side effect? For (2), I propose: 1. Expose a setter for focus_source_ 2. Call the setter before OmniboxEditModel::OnAfterPossibleChange() 3. Since this way we don't go into the linux workaround of if (focus_source_ == INVALID), focus_source_ value is not reset and is correctly logged later.
,
Feb 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dd9167fea8c51708e806fec75db76a403f6d24df commit dd9167fea8c51708e806fec75db76a403f6d24df Author: stkhapugin@chromium.org <stkhapugin@chromium.org> Date: Fri Feb 16 18:07:31 2018 [iOS] Set focus source of omnibox edit model directly. Exposes a getter and setter for focus_source_ on OmniboxEditModel and uses them directly to track the focus source (omnibox or fakebox), instead of relying on a workaround for Linux and using caret visibility as a proxy. Bug: 807600 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I6e23c77ca7c8cac39e07cc45c351b076a9183d1c Reviewed-on: https://chromium-review.googlesource.com/894325 Commit-Queue: Stepan Khapugin <stkhapugin@chromium.org> Reviewed-by: Gauthier Ambard <gambard@chromium.org> Reviewed-by: Justin Cohen <justincohen@chromium.org> Reviewed-by: Justin Donnelly <jdonnelly@chromium.org> Cr-Commit-Position: refs/heads/master@{#537350} [modify] https://crrev.com/dd9167fea8c51708e806fec75db76a403f6d24df/components/omnibox/browser/omnibox_edit_model.h [modify] https://crrev.com/dd9167fea8c51708e806fec75db76a403f6d24df/ios/chrome/browser/ui/content_suggestions/BUILD.gn [modify] https://crrev.com/dd9167fea8c51708e806fec75db76a403f6d24df/ios/chrome/browser/ui/content_suggestions/ntp_home_egtest.mm [modify] https://crrev.com/dd9167fea8c51708e806fec75db76a403f6d24df/ios/chrome/browser/ui/location_bar/location_bar_coordinator.h [modify] https://crrev.com/dd9167fea8c51708e806fec75db76a403f6d24df/ios/chrome/browser/ui/location_bar/location_bar_coordinator.mm [modify] https://crrev.com/dd9167fea8c51708e806fec75db76a403f6d24df/ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm [modify] https://crrev.com/dd9167fea8c51708e806fec75db76a403f6d24df/ios/chrome/browser/ui/toolbar/adaptive/primary_toolbar_coordinator.mm [modify] https://crrev.com/dd9167fea8c51708e806fec75db76a403f6d24df/ios/chrome/browser/ui/toolbar/clean/toolbar_coordinator.mm
,
Feb 19 2018
I've added an EarlGrey test to verify that when being focused from fakebox on NTP we mark the source correctly. Thank you mpearson@ for your advice. |
||
►
Sign in to add a comment |
||
Comment 1 by mpear...@chromium.org
, Jan 31 2018