[meta bug] Rename Omnibox Classes for Clarity |
|||
Issue descriptionThere are a lot of suggestions in pkasting@'s UI doc: https://docs.google.com/document/d/1_XhJkYvl6alT9kW_Xlx-nX9VJ0HKK75_lsNt7bFMW3A/edit Many of these can probably be done using automated tools such as ClangMR, then manual cleanup of comments.
,
Oct 8
Is there anything actionable here?
,
Oct 10
> Is there anything actionable here? Yes. Examples: >>> The LocationBarView is what most people think of as “the omnibox”. From a source code perspective, it contains more than just the omnibox proper; it also holds the “location icon” on the left, the content settings icons, star, and other icons on the right, and various other views not pictured above. In the code, “omnibox” generally refers specifically to the editable URL display, the suggestion dropdown, and the backing classes that power them; all other page-related state display/interactions are handled by other classes contained by the LocationBarView. One Chrome team member commented that inverting this concept, and calling the textfield itself the “location bar” while the containing thing was the “omnibox”, would make more sense to him. Neither seems obviously better to me, but one change that might make sense would be to consistently call what we current call the location bar the “omnibox” and call the contained textfield the “omnibox textfield”; see also the OmniboxViewViews section. >>> and >>> The LocationIconView is so named because, other than for EV certs, it used to primarily display an icon (a globe, a lock, etc.). The name is increasingly misleading due to security changes that have made the LocationIconView responsible for displaying an ever-growing number of labels along with the icon: “Secure”, as shown here, “Chrome” on an internal URL, an extension name on an extension URL, and others. (I am trying to standardize how these strings are referred to in the code, but there’s no good name; they’re not always explicitly “security”-related, so I’m generally calling them “location icon text”.) >>> and >>> It subclasses IconLabelBubbleView, a location bar-specific view used to show “an icon and some text”, which is also used for keyword search and content setting UI. The word “bubble” in this name comes from the pre-MD appearance of these pieces of UI, which was rounded and glossy, and which led to various parts of the code calling them “bubbles”: EV bubble, keyword search bubble, content settings bubble. This is confusing, as Views also refers to some transient, round-cornered dialogs as “bubbles” (in Harmony, the UX designers are trying to rename these “popovers”), but the two are unrelated. The use of the “bubble” name in location bar code was a bad choice to begin with (we should maybe have used “chip” or “chit”), and is doubly so now. This terminology should all be changed. >>> and >>> The OmniboxViewViews is the editable textfield itself. The awkward name is because it is the Views-specific implementation of the (partly concrete) OmniboxView base class; there is an OmniboxViewMac as well, for example. ... A better name for this class would be OmniboxEditView[Views,Mac,etc.], because “OmniboxEdit” is already used in the OmniboxEditModel and OmniboxEditController classes. Even better might be to standardize all these names but use Textfield instead of Edit, since Edit comes from the aforementioned CRichEditCtrl, whereas Textfield would align with the current implementation. This would be consistent with my earlier suggestion to call this the “omnibox textfield” consistently. >>> and >>> The code refers to the dropdown as a “popup” fairly consistently, which, like the use of “Edit” in the name of the textfield controls, is an artifact of the original Windows implementation as a standalone WS_POPUP HWND. These days this term is confusing, and I wouldn’t be sad to see “popup” replaced with “dropdown” everywhere. Really, even “dropdown” is arguably confusing given that the dropdown is styled to look like part of the page content; this styling came from the “Instant Extended” days, when for a while the dropdown contents actually were rendered by the page. >>> and >>> OmniboxPopupContentsView implements the abstract OmniboxPopupView base class. A more consistent (with the textfield) name would therefore be OmniboxPopupViewViews, and other platforms actually use this scheme (see e.g. OmniboxPopupViewMac). >>> and >>> Each OmniboxResultView “view” is backed by one AutocompleteMatch “model”; AutocompleteMatch is a class which I won’t discuss here as it’s part of the underlying suggestion-generation system. That system does lead to yet another case of confusing names, though, since it uses a class called AutocompleteResult to hold all the “matches” in the dropdown. This means in one place “result” refers to a single suggestion and in a nearby place it refers to all of them. These should standardize on terminology, and I suggest doing it by renaming OmniboxResultView to OmniboxMatchView. >>> (There may be others I have missed.) |
|||
►
Sign in to add a comment |
|||
Comment 1 by sheriffbot@chromium.org
, Oct 5Status: Untriaged (was: Available)