New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 807600 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug


Show other hotlists

Hotlists containing this issue:
Hotlist-11


Sign in to add a comment

Remove caret visibility logic from iOS

Project Member Reported by stkhapugin@chromium.org, Jan 31 2018

Issue description

Omnibox 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.


 
drive-by:
* No one cares about the UserActionRate histograms.
* If possible, please do not regress the handling of page classification (INSTANT_NTP_WITH_FAKEBOX_AS_STARTING_FOCUS, INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS, etc.).  It's not only used for logging.  It's actually used for ranking, i.e., for deciding what suggestions tos how in what order.  This has to be set correctly by the time a StartAutocomplete call happens.
Project Member

Comment 2 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
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