Issue metadata
Sign in to add a comment
|
Use enum to determine the type of Zero Suggest suggestions |
||||||||||||||||||
Issue description
Currently zero_suggest_provider is using a number of gates to determine what external calls are to be made, what has to be displayed / cleanedup etc. Since all these gates are interrelated, readability is quite poor. In particular, the following aspects should be reconsidered:
- Clarify situations where Most Visited are requested/displayed
- Clarify situations where Contextual suggestions are requested/displayed
- Consider removing one of `can_attach_current_url` and `can_send_current_url`. It seems odd that they are both still around.
- Consider wrapping MaybeUseCachedSuggestions with an if statement as follows
if (OmniboxFieldTrial::InZeroSuggestPersonalizedFieldTrial(
client()->GetPrefs()))
and return after it uses Cached Suggestions.
,
Jan 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9a364f573a9f47c756bd3cc6c14a81dbf08cb3b4 commit 9a364f573a9f47c756bd3cc6c14a81dbf08cb3b4 Author: Gheorghe Comanici <gcomanici@chromium.org> Date: Wed Jan 24 17:22:00 2018 Use enum to determine the type of Zero Suggest suggestions This CL introduces a ZeroSuggestService enum, which determines the service that the ZeroSuggestProvider is using at any time. This provider is providing any of the following 4 services: - most visited URLs - personalized suggestions from the default search provider - contextual suggestions from the default search provider This CL is intended to improve readability by using a state variable as a way to keep track of the service used. Prior to this CL, this was quite unclear and the flow of the code was very hard to follow, as one would constantly have to keep track of the service provider. With this change, some state variables are no longer needed and some confusing methods are largely simplified. Bug: 789137 Change-Id: I79ba9fc66423b05d5a8c577376a401f25d31aa86 Reviewed-on: https://chromium-review.googlesource.com/848087 Commit-Queue: Gheorghe Comanici <gcomanici@chromium.org> Reviewed-by: Mark Pearson <mpearson@chromium.org> Cr-Commit-Position: refs/heads/master@{#531580} [modify] https://crrev.com/9a364f573a9f47c756bd3cc6c14a81dbf08cb3b4/components/omnibox/browser/zero_suggest_provider.cc [modify] https://crrev.com/9a364f573a9f47c756bd3cc6c14a81dbf08cb3b4/components/omnibox/browser/zero_suggest_provider.h
,
Jan 29 2018
Mark as fixed? (Are all the bullets in the original report done?)
,
Jan 29 2018
,
Jan 29 2018
The only remaining bullet is the following:
- Consider wrapping MaybeUseCachedSuggestions with an if statement as follows
if (OmniboxFieldTrial::InZeroSuggestPersonalizedFieldTrial(
client()->GetPrefs())).
Based on the current implementation, MaybeUseCachedSuggestions should only be run when `result_type_running_ == DEFAULT_SERP`. See link below for the place where result_type_running_ is determined.
https://cs.chromium.org/chromium/src/components/omnibox/browser/zero_suggest_provider.cc?q=zero_suggest&sq=package:chromium&dr=CSs&l=150
,
Feb 27 2018
I can pick this up. Is the issue that 'default search engine' is only compatible with this field trial? That sounds strange, so something else is going on?
,
Feb 27 2018
The default search engine is used to get both contextual suggestions (i.e. DEFAULT_SERP_FOR_URL) and non-contextual suggestions (i.e. DEFAULT_SERP). As it it right now, non-contextual suggestions are provided only when users are in this field trial. Maybe when the feature was initially built it was different, but there is no code path that will trigger non-contextual suggestions outside of this field trial.
,
Mar 3 2018
The NextAction date has arrived: 2018-03-03 |
|||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||
Comment 1 by mpear...@chromium.org
, Nov 28 2017Status: Assigned (was: Untriaged)