New issue
Advanced search Search tips

Issue 900255 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Omnibox Feature & Flag Cleanup

Project Member Reported by orinj@chromium.org, Oct 30

Issue description

This is a code-health bug to help organize efforts at cleaning up old Omnibox features & flags.
 
Attached is a report that attempts to catalog the features and flags of various kinds that are found in code directories named starting with: omnibox, autocomplete, search, location_bar, toolbar .

Nothing in this document is final or official, it's just a starting point to help uncover ideas about what we can do to organize the cleanup.  For example: if you see this list and notice an omission, then point it out and maybe I will find more by searching its directory or code pattern.
feature_references_1029.ods
18.3 KB Download
Can you share this as a Google Doc to make viewing and commenting easier, and also easier to make sure that I'm viewing the most recent version?  thanks!
Sure, just promoted to Google Sheets and sent a link to mpearson@ and jdonnelly@.  I was holding back on the live Sheet so I could grok high-level feedback and changes before going official.
Can you please send share link to a-v-y@yandex-team.ru or post it here in ticket.
Sure, anyone can view here:

https://docs.google.com/spreadsheets/d/1Nj9uWjgrrAa2HGVRyCXVb7qZwMlWbGN0_SXfKkQ1NO0/edit?usp=sharing

If you need edit rights, shoot me an email - I am happy to have others making changes, just don't want to give unbounded write access.
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/992791999c711ba91223531f9b8ef22e9ba04969

commit 992791999c711ba91223531f9b8ef22e9ba04969
Author: Orin Jaworski <orinj@chromium.org>
Date: Wed Oct 31 17:08:58 2018

Eliminate dead omnibox switches code

The code eliminated was a switch constant that wasn't being used.
The .cc, .h, and related inclusions were eliminated with no effect.

Bug: 900255
Change-Id: I76521fb8acce5b7eb5aa9e38fa748953468105ea
Reviewed-on: https://chromium-review.googlesource.com/c/1308040
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604303}
[modify] https://crrev.com/992791999c711ba91223531f9b8ef22e9ba04969/chrome/browser/about_flags.cc
[modify] https://crrev.com/992791999c711ba91223531f9b8ef22e9ba04969/chrome/browser/autocomplete/search_provider_unittest.cc
[modify] https://crrev.com/992791999c711ba91223531f9b8ef22e9ba04969/components/omnibox/browser/BUILD.gn
[modify] https://crrev.com/992791999c711ba91223531f9b8ef22e9ba04969/components/omnibox/browser/autocomplete_result.cc
[modify] https://crrev.com/992791999c711ba91223531f9b8ef22e9ba04969/components/omnibox/browser/omnibox_field_trial.cc
[delete] https://crrev.com/e927c49ebe45ef2d3f7194b40fd75234b5bccd73/components/omnibox/browser/omnibox_switches.cc
[delete] https://crrev.com/e927c49ebe45ef2d3f7194b40fd75234b5bccd73/components/omnibox/browser/omnibox_switches.h

Cc: -orinj@chromium.org
Owner: orinj@chromium.org
Orin, looks like you're overall owning this bug, so I marked you as the owner for now.

I'll submit some patches against this bug.
Project Member

Comment 8 by bugdroid1@chromium.org, Nov 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/21da435215c8da0d12c639ff18cec1e8565998f6

commit 21da435215c8da0d12c639ff18cec1e8565998f6
Author: Tommy C. Li <tommycli@chromium.org>
Date: Tue Nov 20 16:35:28 2018

Omnibox: Delete kUIExperimentElideSuggestionUrlAfterHost flag

We stopped using this experiment some time ago.

Bug: 900255
Change-Id: I95bf3e3b065ed7113d9cc172680a5095520f288f
Reviewed-on: https://chromium-review.googlesource.com/c/1343279
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609724}
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/chrome/browser/about_flags.cc
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/chrome/browser/flag-metadata.json
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/omnibox/browser/autocomplete_match.cc
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/omnibox/browser/autocomplete_match.h
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/omnibox/browser/autocomplete_match_unittest.cc
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/omnibox/browser/clipboard_url_provider.cc
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/omnibox/browser/history_match.cc
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/omnibox/browser/history_match.h
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/omnibox/browser/history_quick_provider.cc
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/omnibox/browser/history_url_provider.cc
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/omnibox/browser/history_url_provider_unittest.cc
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/omnibox/browser/omnibox_field_trial.cc
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/omnibox/browser/omnibox_field_trial.h
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/omnibox/browser/scored_history_match.cc
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/omnibox/browser/scored_history_match_unittest.cc
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/omnibox/browser/search_suggestion_parser.cc
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/omnibox/browser/titled_url_match_utils.cc
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/omnibox/browser/zero_suggest_provider.cc
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/url_formatter/url_formatter.cc
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/url_formatter/url_formatter.h
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/components/url_formatter/url_formatter_unittest.cc
[modify] https://crrev.com/21da435215c8da0d12c639ff18cec1e8565998f6/ios/chrome/browser/about_flags.mm

Project Member

Comment 9 by bugdroid1@chromium.org, Nov 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/346037e24859c80fac8f235a8c4b707298ff3a13

commit 346037e24859c80fac8f235a8c4b707298ff3a13
Author: Tommy C. Li <tommycli@chromium.org>
Date: Tue Nov 20 19:36:28 2018

Omnibox: Delete kUIExperimentJogTextfieldOnPopup flag

We aren't experimenting on this anymore.

Bug: 900255
Change-Id: Idc932239bb767ffb602323d66b9cf2f89321891d
Reviewed-on: https://chromium-review.googlesource.com/c/1343362
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: manuk hovanesian <manukh@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609773}
[modify] https://crrev.com/346037e24859c80fac8f235a8c4b707298ff3a13/chrome/browser/about_flags.cc
[modify] https://crrev.com/346037e24859c80fac8f235a8c4b707298ff3a13/chrome/browser/flag-metadata.json
[modify] https://crrev.com/346037e24859c80fac8f235a8c4b707298ff3a13/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/346037e24859c80fac8f235a8c4b707298ff3a13/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/346037e24859c80fac8f235a8c4b707298ff3a13/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc
[modify] https://crrev.com/346037e24859c80fac8f235a8c4b707298ff3a13/chrome/browser/ui/views/location_bar/icon_label_bubble_view.h
[modify] https://crrev.com/346037e24859c80fac8f235a8c4b707298ff3a13/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/346037e24859c80fac8f235a8c4b707298ff3a13/chrome/browser/ui/views/location_bar/location_icon_view.cc
[modify] https://crrev.com/346037e24859c80fac8f235a8c4b707298ff3a13/chrome/browser/ui/views/location_bar/location_icon_view.h
[modify] https://crrev.com/346037e24859c80fac8f235a8c4b707298ff3a13/components/omnibox/browser/omnibox_field_trial.cc
[modify] https://crrev.com/346037e24859c80fac8f235a8c4b707298ff3a13/components/omnibox/browser/omnibox_field_trial.h

Project Member

Comment 10 by bugdroid1@chromium.org, Nov 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8e954f2fb42070304a6290bd3a98f4713a1063e6

commit 8e954f2fb42070304a6290bd3a98f4713a1063e6
Author: Tommy C. Li <tommycli@chromium.org>
Date: Tue Nov 20 22:19:23 2018

Omnibox: Delete kUIExperimentShowSuggestionFavicons flag

We have launched this to Stable some time ago.

Bug: 900255
Change-Id: I52ea22c451cee3b0bf7df67710978ef272f781ae
Reviewed-on: https://chromium-review.googlesource.com/c/1343366
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Orin Jaworski <orinj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609824}
[modify] https://crrev.com/8e954f2fb42070304a6290bd3a98f4713a1063e6/chrome/browser/about_flags.cc
[modify] https://crrev.com/8e954f2fb42070304a6290bd3a98f4713a1063e6/chrome/browser/flag-metadata.json
[modify] https://crrev.com/8e954f2fb42070304a6290bd3a98f4713a1063e6/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/8e954f2fb42070304a6290bd3a98f4713a1063e6/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/8e954f2fb42070304a6290bd3a98f4713a1063e6/components/omnibox/browser/omnibox_field_trial.cc
[modify] https://crrev.com/8e954f2fb42070304a6290bd3a98f4713a1063e6/components/omnibox/browser/omnibox_field_trial.h
[modify] https://crrev.com/8e954f2fb42070304a6290bd3a98f4713a1063e6/components/omnibox/browser/omnibox_popup_model.cc
[modify] https://crrev.com/8e954f2fb42070304a6290bd3a98f4713a1063e6/components/omnibox/browser/omnibox_view.cc
[modify] https://crrev.com/8e954f2fb42070304a6290bd3a98f4713a1063e6/components/omnibox/browser/omnibox_view_unittest.cc

Okay, I've updated the spreadsheet to reflect those three flags being deleted from the code.

It's actually a decent volume of code deleted per flag, so I encourage others also to delete ones they find no longer necessary.
Project Member

Comment 12 by bugdroid1@chromium.org, Jan 17 (6 days ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c5778366ffd0cc2044e6fc178eeb633435b77d62

commit c5778366ffd0cc2044e6fc178eeb633435b77d62
Author: Orin Jaworski <orinj@chromium.org>
Date: Thu Jan 17 02:45:57 2019

[omnibox] Clean up all usage of IsNewAnswerLayoutEnabled

This CL eliminates a bunch of code that depended on feature
flag kOmniboxNewAnswerLayout which is no longer needed on
desktop, and has since been repurposed for another feature
on Android.  See crrev.com/c/1405882 for additional context.

Bug: 900255
Change-Id: Id20aaeaa648807a1e257d0d80c6e8c33646acb37
Reviewed-on: https://chromium-review.googlesource.com/c/1412993
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623534}
[modify] https://crrev.com/c5778366ffd0cc2044e6fc178eeb633435b77d62/chrome/browser/ui/views/omnibox/omnibox_match_cell_view.cc
[modify] https://crrev.com/c5778366ffd0cc2044e6fc178eeb633435b77d62/chrome/browser/ui/views/omnibox/omnibox_text_view.cc
[modify] https://crrev.com/c5778366ffd0cc2044e6fc178eeb633435b77d62/components/omnibox/browser/autocomplete_controller.cc
[modify] https://crrev.com/c5778366ffd0cc2044e6fc178eeb633435b77d62/components/omnibox/browser/omnibox_field_trial.cc
[modify] https://crrev.com/c5778366ffd0cc2044e6fc178eeb633435b77d62/components/omnibox/browser/omnibox_field_trial.h
[modify] https://crrev.com/c5778366ffd0cc2044e6fc178eeb633435b77d62/components/omnibox/browser/search_suggestion_parser.cc
[modify] https://crrev.com/c5778366ffd0cc2044e6fc178eeb633435b77d62/components/omnibox/browser/suggestion_answer.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Jan 18 (5 days ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e999b4f5ce92d333708d316b78acfb359d9a987e

commit e999b4f5ce92d333708d316b78acfb359d9a987e
Author: Orin Jaworski <orinj@chromium.org>
Date: Fri Jan 18 04:16:04 2019

Remove omnibox-new-answer-layout flag usage from desktop

The kOmniboxNewAnswerLayout flag used to be desktop only, but now it is
Android-only so the preprocessor conditions and considerations for
desktop are cleaned up in this CL.

Bug: 900255
Change-Id: Id203bfd0b9dbe9ee473faf759a0a47b0817d8c1e
Reviewed-on: https://chromium-review.googlesource.com/c/1413353
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624007}
[modify] https://crrev.com/e999b4f5ce92d333708d316b78acfb359d9a987e/chrome/browser/about_flags.cc
[modify] https://crrev.com/e999b4f5ce92d333708d316b78acfb359d9a987e/components/omnibox/browser/omnibox_field_trial.cc
[modify] https://crrev.com/e999b4f5ce92d333708d316b78acfb359d9a987e/testing/variations/fieldtrial_testing_config.json

Sign in to add a comment