New issue
Advanced search Search tips

Issue 923503 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

Creating an AutocompleteMatch is too complicated

Project Member Reported by jdonnelly@chromium.org, Jan 18 (4 days ago)

Issue description

I recently reviewed some new code that creates an AutocompleteMatch to do a search for some text. While I believe the code was all correct, it's very complicated for what it's trying to do.

Namely, the intent of this code is to make a search suggestion, with a particular relevance, search terms string, contents and description. But to do this requires 20 lines of dense, arcane code:

https://cs.chromium.org/chromium/src/components/omnibox/browser/clipboard_url_provider.cc?l=148-167&rcl=c820f20fe0eeae423334641941e09138479573c3

A couple things stand out to me:

Creating a search URL is too complicated (lines 150-157). All this wants is a default search engine URL with a given string for the search terms. Seems like there could be a single method somewhere that could do this.

Applying the classifications (text styling) is too complicated (lines 160 and 165). Literally all this wants is no styling (NONE) applied to the entirety of both strings. Seems like this could be made to be a default and then not require these lines at all. And/or perhaps we can add a method that just applies a style to the entire string. For example:
AutocompleteMatch::ClassifyEntireString(match.contents, NONE, &match.contents_class);

There's 2 motivations here:
1. It's difficult and time consuming to write new code that produces a match.
2. Much more importantly, code this complex for a simple task invites bugs. I think that code is all correct but I'm honestly not sure. Methods to build matches for common cases and/or more defaults would help prevent errors.

manukh: can you survey the other parts of our code where we create instances of AutocompleteMatch and propose changes that might simplify some of this? My proposals above are just suggestions. Hopefully if you look at the existing cases, you can come up with some simplifications that will work in all cases.
 

Sign in to add a comment