Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6731036:
Enabled pressing TAB to cycle through the Omnibox results. (Closed)
|
Created:
9 years, 9 months ago by aaron.randolph Modified:
8 years, 10 months ago Reviewers:
Peter Kasting, sky, Elliot Glaysher, Glen Murphy, Robert Sesek, sail, James Su, Scott Hess - ex-Googler CC:
chromium-reviews Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionEnabled pressing TAB to traverse through the Omnibox results, removed moving the caret to the end of the line with TAB, and filtered out redundant URLs.
This adds the ability to move through Omnibox result matches using TAB in addition to the arrow keys. To enable this, pressing TAB to move the caret to the end of the line was removed and the keyword hint/shortcut logic has been modified.
The Omnibox popup now shows keyword markers on the right side of matches that have associated keywords (represented by a right arrow). When this kind of match is selected, and the keyword is accepted, the match changes appearance by animating in the associated keyword match from the right to display the "Search X for <>" message.
If multiple matches have the same keyword then only the most relevant match will display the keyword marker and hint. Pressing TAB while a keyword hint is shown will enter keyword mode in place; the results will no longer change when keyword mode is entered. Additionally, substituting keyword provider matches will only be shown if a keyword substitution
is available.
Finally, results with redundant destination URLs (e.g., "foo.com", "www.foo.com") will have the less relevant URLs filtered out.
This also addresses some GTK omnibox browsertest flakiness; see bug 112041.
Contributed by aaron.randolph@gmail.com
BUG=57748, 76278, 77662, 80934, 84420
TEST=Press TAB to move the selection down the list of results, SHIFT+TAB to move up.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120005
Patch Set 1 : '' #Patch Set 2 : Enabled pressing TAB to cycle through the Omnibox results, removed moving the caret to the #
Total comments: 14
Patch Set 3 : '' #
Total comments: 16
Patch Set 4 : '' #
Total comments: 22
Patch Set 5 : '' #
Total comments: 17
Patch Set 6 : '' #
Total comments: 26
Patch Set 7 : '' #
Total comments: 38
Patch Set 8 : '' #
Total comments: 18
Patch Set 9 : '' #Patch Set 10 : '' #
Total comments: 5
Patch Set 11 : '' #
Total comments: 12
Patch Set 12 : '' #Patch Set 13 : '' #
Total comments: 22
Patch Set 14 : '' #
Total comments: 13
Patch Set 15 : '' #Patch Set 16 : '' #
Total comments: 12
Patch Set 17 : '' #
Total comments: 3
Patch Set 18 : '' #
Total comments: 2
Patch Set 19 : '' #
Total comments: 2
Patch Set 20 : '' #
Total comments: 5
Patch Set 21 : '' #
Total comments: 3
Patch Set 22 : '' #Patch Set 23 : '' #
Total comments: 1
Patch Set 24 : '' #Patch Set 25 : '' #Patch Set 26 : '' #
Total comments: 1
Patch Set 27 : '' #Patch Set 28 : '' #Messages
Total messages: 169 (0 generated)
I think you should not change the order of keyword results. IMO it is better to make tab act as an accelerator to enter keyword mode when that option is present (which will change the dropdown results), and otherwise cycle the dropdown, as suggested in bug 57748 comment 0. I don't think this causes problems when one of the dropdown entries is a keyword match. I realize this might frustrate people who mean to tab downward and unintentionally trigger keyword mode instead. I want to see how bad this problem is before going the route you've taken here.
Actually, my initial implementation tried to do as that bug suggested. As you say, the results did change and, personally, it was highly aggravating. Say I type in "goog" and see a history url I want to go to. I try to TAB down to that result but it enters keyword mode instead, which wouldn't be so bad except that the results changed and that history url no longer displays. So it turned into a case of having to determine whether to use TAB or the arrow keys to move to a result. Besides the results changing, the other main issue was that it would get stuck in a loop between the first two results. For example: 1) Type "ama". It shows a keyword hint for amazon. 2) Press TAB. It enters keyword mode. 3) Press TAB again. It moves to the second result, which also has a keyword hint for amazon. 4) Go to step 2. My initial thought was to add a flag specifying whether keyword mode had already been seen and not show subsequent hints, but that didn't work the way I planned for various reasons. However, I might have some other ideas on how to implement it now that I know the code a bit better, such as by not showing the hint if the first result is a keyword search. I'll change the scores back and upload a new patch with the bug implementation. Do you want to see a version where the results change when entering keyword mode, as it currently does, or should I try for a solution where they don't change? On 2011/03/25 19:49:30, Peter Kasting wrote: > I think you should not change the order of keyword results. IMO it is better to > make tab act as an accelerator to enter keyword mode when that option is present > (which will change the dropdown results), and otherwise cycle the dropdown, as > suggested in bug 57748 comment 0. I don't think this causes problems when one > of the dropdown entries is a keyword match. > > I realize this might frustrate people who mean to tab downward and > unintentionally trigger keyword mode instead. I want to see how bad this > problem is before going the route you've taken here.
OK, sky and I came up with what we think is a good solution to all these issues, but it's complex and will be a lot of work to implement :(. I'm going to go write it up on the bug.
Looks like this change disables Tab focus traversal in omnibox completely. I don't think it's acceptable.
On 2011/03/25 22:28:03, James Su wrote: > Looks like this change disables Tab focus traversal in omnibox completely. Do you mean, if the dropdown is closed, tab doesn't cycle focus? If so, that's a bug in the change -- tab should only navigate the dropdown if it's already open (unlike the down arrow key). This is also how Firefox behaves.
On 2011/03/25 22:29:23, Peter Kasting wrote: > On 2011/03/25 22:28:03, James Su wrote: > > Looks like this change disables Tab focus traversal in omnibox completely. > > Do you mean, if the dropdown is closed, tab doesn't cycle focus? If so, that's > a bug in the change -- tab should only navigate the dropdown if it's already > open (unlike the down arrow key). Yes. It would be the behavior if I understand this CL correctly. > > This is also how Firefox behaves.
In this version, pressing TAB with a closed popup does the same as the down arrow - it opens the popup. I'll fix that in the next update, which will be based off the new solution suggested in bug 57748. On 2011/03/25 22:42:58, James Su wrote: > On 2011/03/25 22:29:23, Peter Kasting wrote: > > On 2011/03/25 22:28:03, James Su wrote: > > > Looks like this change disables Tab focus traversal in omnibox completely. > > > > Do you mean, if the dropdown is closed, tab doesn't cycle focus? If so, > that's > > a bug in the change -- tab should only navigate the dropdown if it's already > > open (unlike the down arrow key). > Yes. It would be the behavior if I understand this CL correctly. > > > > > This is also how Firefox behaves.
I've updated the patch set with modifications that are more in line with the suggestions in bug 57748. I've implemented the logic but not the UI suggested in comment 38 yet. I believe it works fine without the UI changes but think they should still be done as they give the option for mouse-heavy users to enter keyword mode with a mouse click, which I don't think is currently available. I also included changes to suppress redundant URLs as noted in bug 77662. I'm not sure if that should instead be submitted as a separate issue?
Embarrassingly, I broke my last patch while fixing lint errors in a rush. I've uploaded a new patch that fixes the compile error and makes a couple other changes to better follow the code style guidelines.
Some more nitpicky comments below. Here are some higher-level comments: * You also need to touch autocomplete_edit_view_mac.mm. * I think adding UI will completely change the architecture or this patch. It'd be nice if (for Views only) you could take a shot at just adding the right-hand-side "magnifying glass" area, with no ability to enlarge/animate it and no handling of mouse events, but with tab moving the selection over to it. I would build the selection code to do something similar to (maybe as part of) arrowing up and down. This should allow you to eliminate the "freeze results" bit since in this case we already can change the omnibox contents without changing the result set. I think this will change how we store and calculate keyword data on an AutocompleteMatch too, because you'll need to know this all the time (a mutable cache member probably won't work well). Perhaps what we'll need to do is have the AutocompleteMatch for the URL hold on to a "linked" AutocompleteMatch (that's not directly in the result set) for the keyword? http://codereview.chromium.org/6731036/diff/21001/AUTHORS File AUTHORS (right): http://codereview.chromium.org/6731036/diff/21001/AUTHORS#newcode101 AUTHORS:101: Aaron Randolph <aaron.randolph@gmail.com> Have you signed the CLA? ( http://code.google.com/legal/individual-cla-v1.0.html ) http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.cc:644: // Normalize destination URLs Nit: This comment adds nothing, just eliminate it. http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.cc:645: for (ACMatches::iterator i = matches_.begin(); i != matches_.end(); ++i) { Nit: No {} http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit.cc (right): http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit.cc:680: Nit: Don't add a blank line http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit.cc:694: const AutocompleteResult::const_iterator found = Nit: It's probably simpler overall to eliminate the functor here and just directly write something like: for (AutocompleteResult::const_iterator i(result.begin()); ...) { if (i->keyword == keyword_) { is_keyword_hint_ = false; keyword_.clear(); break; } } http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right): http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1686: // Trigger Tab to search behavior only when Tab key is pressed. You need to rework this function in a similar way to the Windows changes. http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_views.cc (right): http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_views.cc:194: } else if (!handled && event.key_code() == ui::VKEY_UP) { You need to rework this function in a similar way to the Windows changes. http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right): http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit_view_win.cc:2025: const bool no_shift = GetKeyState(VK_SHIFT) >= 0; Nit: Reverse this and call the variable |shift_pressed|. http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_match.cc (right): http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.cc:92: bool AutocompleteMatch::DestinationSortFunc(const AutocompleteMatch& elem1, Nit: Do we still use this function and the next? http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.cc:182: if (destination_url.is_valid() && normalized_url.is_empty()) { Nit: I would combine this check with the compare() call, so that for invalid destinations we just copy the destination to the normalized URL. http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.cc:186: host.erase(0, prefix_len); Nit: You can eliminate this and use host.substr(...) in the SetHostStr() call below. http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_match.h (right): http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.h:135: void NormalizeDestination(); Nit: Perhaps "void ComputeStrippedDestinationURL();"? http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.h:170: GURL normalized_url; Nit: This would be more explicit as |destination_url_no_www|. http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... File chrome/browser/autocomplete/keyword_provider.cc (right): http://codereview.chromium.org/6731036/diff/21001/chrome/browser/autocomplete... chrome/browser/autocomplete/keyword_provider.cc:182: // Prune any extension keywords that are disallowed in incognito mode (if Nit: Move this comment inside the loop.
Oh, and one more. Doing what I suggest will probably allow us to make changes as well to how the edit and popup track the notion of a "keyword" and "is_keyword_hint". We'll know whether something should show a hint, or is a keyword itself, by examining whether the selected match has a linked "keyword match". Hopefully this simplifies some things. Sorry, this is a complex bunch of changes I'm asking for; you're running into the fact that our keyword code, never the clearest thing to begin with, has also been modified a lot over time until it's pretty torturous.
Thanks, Peter. That all makes sense and I'll get to work on fixing the nits and looking into adding keyword link on matches. Changing the accept keyword logic to behave more like the up/down key press makes much more sense; the 'freeze results' was definitely a hack. And yes, I've signed the CLA.
I just uploaded a new CL that fixes most of the nits and updates the UI to look like the mock-ups in bug 57748, comment 38. This includes adding a magnifying glass and ellipsis to the right side of matches that contain keyword hints, and animating that section to fill up most of the match when the keyword is accepted. I also updated the unit tests and made sure they all pass. To enable the UI changes I modified AutocompleteMatch to hold a pointer to an AutocompleteMatch::Keyword structure if the match contains a keyword. The structure keeps track of the keyword, along with whether to show a hint or show keyword mode. I also moved GetKeywordMatch from the popup model to the autocomplete controller since I needed to populate keywords ahead of time instead of on the fly, and it seemed to make more sense there to me. Some notes on the nits: * It was suggested to return the result of substr directly to SetHostStr. This won't work as expected because GURL::Replacements doesn't make copies of the strings it is passed; instead, it records a pointer and start and end offsets. Since the temporary string returned by substr is destroyed between the call to SetHostStr and ReplaceComponents, this results in some interesting URLs. * I thought destination_url_no_www was a bit too explicit, especially if stripped URLs are calculated differently at a later time. So I compromised and called it destination_url_normalized. * I modified the gtk and mac versions so that they should still compile, but I didn't implement all the behavior that the Windows version has. This was mainly because I have no way to test the changes and I don't want to check in something I only hope I got right.
I did a very fast review, mostly for style. Two general style comments in addition to the specific ones below: 1. All comments should be complete sentences with a trailing period. 2. When linebreaking a statement, subsequent lines are indented 4, not 2. We only use 2-space indents when the block-level goes up and down. Once you fix these, Scott and I should probably both take a close look at the functionality. http://codereview.chromium.org/6731036/diff/35003/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/6731036/diff/35003/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.cc:1019: Nit: Extra newline http://codereview.chromium.org/6731036/diff/35003/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.cc:1021: void AutocompleteController::GetKeywordForMatch(AutocompleteMatch* match) const { Nit: Line too long; wrap after '(' http://codereview.chromium.org/6731036/diff/35003/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.cc:1067: if (!extension || Nit: Indenting is one space short http://codereview.chromium.org/6731036/diff/35003/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.cc:1075: } Nit: Make sure there's a newline character at the end of this line, the linter can't tell. http://codereview.chromium.org/6731036/diff/35003/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete.h (right): http://codereview.chromium.org/6731036/diff/35003/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.h:660: // Gets the selected keyword or keyword hint for the given match. This function claims to be a getter but actually is something that updates the keyword field on the supplied match. Since I think this now only has one caller, when the controller wants to set the keywords on the return result, I suggest simply inlining this functionality into that loop. http://codereview.chromium.org/6731036/diff/35003/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.h:665: // represents a selected keyword. (|keyword| will always be set [though This function now seems to only have two cases: clear |keyword| and return false, or set it and return true. I suggest condensing it to just "string16 GetKeywordForText(const string16& text)". http://codereview.chromium.org/6731036/diff/35003/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_edit.cc (right): http://codereview.chromium.org/6731036/diff/35003/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit.cc:385: ResetCurrentMatchKeywordMode(); Your CL must be missing some locally modified files. This must be declared in the header but the header isn't up for review. http://codereview.chromium.org/6731036/diff/35003/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_edit.cc:588: if (visible_text.size() != 0) { Nit: Shorter: is_keyword_hint_ = visible_text.empty(); if (!is_keyword_hint_) keyword.clear(); http://codereview.chromium.org/6731036/diff/35003/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_match.h (right): http://codereview.chromium.org/6731036/diff/35003/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.h:15: #include "chrome/browser/profiles/profile.h" Nit: #includes in alphabetical order http://codereview.chromium.org/6731036/diff/35003/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.h:83: Nit: Extra newline http://codereview.chromium.org/6731036/diff/35003/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.h:183: GURL destination_url_normalized; Nit: How about |stripped_destination_url| then? http://codereview.chromium.org/6731036/diff/35003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/6731036/diff/35003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:115: static const int kLineOffset = 1; Nit: Declare in private section, or in the function which needs them, if only one. Also, my mock was a quick job in Paint; see if you can eliminate many or all of these constants via shared code or constants with the overall result view. In other words, make the spacing between divider and icon, icon and text, and text and edge match those for a normal match. http://codereview.chromium.org/6731036/diff/35003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:122: IconLabelView::IconLabelView(gfx::Font font) { Nit: Write definitions out-of-line, even for classes declared in .cc files http://codereview.chromium.org/6731036/diff/35003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:222: animation_.reset(new ui::SlideAnimation(this)); Nit: You can do this in the initializer list (use ALLOW_THIS_IN_INITIALIZER_LIST() if necessary) http://codereview.chromium.org/6731036/diff/35003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:257: const double expanded_x = text_x + Nit: float is plenty of precision, no need for double http://codereview.chromium.org/6731036/diff/35003/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.h (right): http://codereview.chromium.org/6731036/diff/35003/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.h:86: virtual void AnimationProgressed(const ui::Animation* animation); Nit: Add "// ui::AnimationDelegate:" above this, remove blank line between, use "OVERRIDE" declaration
Uploaded a new patch that corrects style issues and addresses the nits.
Updated that patch to fix lint not finding newlines at the end of a couple files.
I have been jammed up with stuff today but I will try and take a look at this when I can, hopefully on Monday.
Some nits below, but the larger issue with this patch is that how keyword state
is managed seems somewhat confused.
If I understand things right, there are three cases. One is that the main
match, itself, is a keyword. This is what happens when the user types in "foo
bar" directly where <foo> is a keyword.
The other two cases both happen when the main match is a non-keyword but there
is an associated keyword; the distinction between them is the in one case the
main match half is expanded, and in the other case the keyword half is expanded.
To track the differences between these three states, we have a motley collection
of variables. First, match->template_url is set iff we're in the first case.
match->keyword is non-NULL in all three cases, and, I think,
match->keyword->is_keyword_mode is set for cases 1 and 3 and
match->keyword->is_keyword_hint for cases 2 and 3. Then the
AutocompleteResultView also has a bool to track whether the keyword side is
expanded, as well as a refptr to match->keyword (!) that it only uses to check
in layout if is_keyword_hint is true. Finally, in some places in the code, we
check is_keyword_hint where we really mean !is_keyword_mode.
Correct me if anything I've said above is wrong.
It seems like we could make things much simpler and more robust. Here's a
possibility:
Eliminate the Keyword struct, replace it with the following member variables on
AutocompleteMatch:
enum KeywordState {
NO_KEYWORD,
KEYWORD,
DUAL_SHOWING_NON_KEYWORD,
DUAL_SHOWING_KEYWORD,
};
KeywordState keyword_state_;
string16 keyword_;
TemplateURL keyword_url_;
All providers create matches in state NO_KEYWORD with the other two variables
empty/NULL, except the keyword provider, which creates KEYWORD matches and
populates the other variables. The AutocompleteController is responsible for
appropriately transforming NO_KEYWORD matches into DUAL_xxx matches and
populating the other two variables.
Does this make sense and support all the cases we need? If so, I think it ought
to reduce complexity and bugginess compared to jugging multiple bools and having
two different TemplateURL fields.
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete.cc (right):
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete.cc:948: string16 keyword_text =
GetKeywordForText(match->fill_into_edit);
Nit: This should be moved inside the first "else" below, along with the
conditional that checks it.
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete.cc:954:
keyword_text.assign(match->template_url->keyword());
Nit: Indent 2, not 4
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete.cc:961:
std::pair<std::set<string16>::iterator, bool> result =
Nit: Instead of getting the result pair and checking .second, it's less code to
write:
if (!keywords.count(keyword_text)) {
// Insert, etc.
}
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete.cc:967: match->keyword = new
AutocompleteMatch::Keyword(
Nit: Seems like Keyword's constructor should init all 4 args
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete.cc:1037: const string16& text) const {
Nit: Indent 4, not 9
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete.cc:1038: // Creates keyword_hint first
in case |keyword| is a pointer to |text|.
Nit: This comment no longer makes sense.
Also, |keyword_hint| is no longer a good name for the temp, just |keyword| would
probably be better.
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete.cc:1058: (profile_->IsOffTheRecord() &&
Nit: Indent 4, not 3
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete.cc:1061: }
Nit: Remove unmatched }
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete.h (right):
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete.h:673: // Gets the selected keyword for
the given text.
Nit: This comment is wrong. Should be something like:
// If |text| corresponds (in the sense of
TemplateURLModel::CleanUserInputKeyword()) to an enabled, substituting keyword,
returns that keyword; returns the empty string otherwise.
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:556: if
(popup_->selected_line() != AutocompletePopupModel::kNoMatch) {
Nit: Ask if the popup is open instead (if it's open, there must be a selected
line).
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:557: AutocompleteMatch match =
CurrentMatch();
This function does not exist.
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:597: if
(popup_->selected_line() != AutocompletePopupModel::kNoMatch) {
Nit: Ask if the popup is open instead.
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:705: if
(popup_->selected_line() != AutocompletePopupModel::kNoMatch) {
Nit: Ask if the popup is open instead.
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:706: AutocompleteMatch match =
CurrentMatch();
This function does not exist.
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_match.cc (right):
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_match.cc:156: static const size_t
prefix_len = strlen(prefix);
Nit: Use arraysize(prefix) - 1 so this is a compile-time constant instead of a
runtime one.
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_match.cc:158: if
(!stripped_destination_url.is_empty())
Nit: This check seems unnecessary
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_match.h (right):
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_match.h:121: // Comparison functions
for removing matches with duplicate destinations.
Nit: You should document and/or name the functions to make it clear these act on
the stripped destinations.
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_match.h:147: //
stripped_destination_url
Nit: Add trailing period. I suggest using || around member names too.
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
File chrome/browser/autocomplete/keyword_provider.cc (right):
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/autocomplete...
chrome/browser/autocomplete/keyword_provider.cc:209: ++i;
Nit: Add blank line before this
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc
(right):
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:631:
string16 keyword;
Nit: Rewrite from here down as:
const GURL url(match.destination_url);
string16 keyword;
if (match.keyword.get() && !match.keyword->is_keyword_hint)
keyword = match.keyword->text;
edit_view_->OpenURL(url, disposition, match.transition, GURL(), index,
keyword);
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right):
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:651: else if
(!same_match || (same_match && !animation_->is_animating())) {
Nit: All arms of a conditional need {} if one has {}.
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autocomplete/autocomplete_result_view.h (right):
http://codereview.chromium.org/6731036/diff/34054/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autocomplete/autocomplete_result_view.h:88: virtual void
AnimationCanceled(const ui::Animation* animation) OVERRIDE;
You want to override AnimationEnded(), not AnimationCanceled(), but I don't
think you even need to override that?
Everything you said was correct and I agree it is overly complex. I was initially going to do something similar to what you're suggesting, but I also was trying to be as minimally invasive as I could. I probably took that a bit too far. :) Question on setting state in the keyword provider - doesn't the search provider also return keyword matches? I thought it did which is why I put state setting in the controller. I still see the CurrentMatch function in the repository so I'm not sure why it is saying it doesn't exist? For the keyword structure, I made it separate and using a ref counting pointer because matches get passed around by value a lot and I wanted to ensure keyword state changes were propagated. After looking at the code again, this is probably unnecessary, and changing it back would also get rid of the annoying keyword.get() checks.
On 2011/04/12 02:58:50, aaron.randolph wrote: > I also > was trying to be as minimally invasive as I could. I probably took that a bit > too far. :) I encourage people to never consider invasiveness when writing a patch. The long-term maintainability of a solution dwarfs any short-term costs, so just write the best patch possible. > Question on setting state in the keyword provider - doesn't the search provider > also return keyword matches? Oh, yeah, I forgot that. That's fine though. Any match that's explicitly a keyword from the beginning can be set that way by the provider, then the controller can change other matches to DUAL_xxx where necessary. > I still see the CurrentMatch function in the repository so I'm not sure why it > is saying it doesn't exist? My mistake. Ignore that comment. > For the keyword structure, I made it separate and using a ref counting pointer > because matches get passed around by value a lot and I wanted to ensure keyword > state changes were propagated. That's a fine goal, but just using member variables directly on AutocompleteMatch will do the same thing with a bit less complexity.
On that last paragraph I actually meant to say retrieved by value, not passed. I believe there are three different functions in AutocompleteEditModel that will get the current match, but all return a copy, not a reference. The other option I saw besides those functions was calling result().match_at and stripping off the const, but that seemed less appealing at the time due to other code that has since been removed. I figured the pointer would be cleaner but it ended up only being cleaner in a couple spots and added complexity everywhere else.
On 2011/04/12 19:18:21, aaron.randolph wrote: > On that last paragraph I actually meant to say retrieved by value, not passed. I wouldn't worry about it. I doubt the cost of copying matches is material anywhere important to perf.
I uploaded a new patch that changes the keyword handling to use a state enum instead of bools, removes the struct, addresses the nits, does some clean-up, and fixes a bug. One note: it seemed unecessary to have two TemplateURLs related to keywords on a match since they would never be used at the same time. So instead of adding a member for keyword_url, I re-used template_url for keyword hint URLs, and made some changes to consumers of template_url to keep their functionality intact.
On 2011/04/13 03:33:24, aaron.randolph wrote: > One note: it seemed unecessary to have two TemplateURLs related to keywords on a > match since they would never be used at the same time. So instead of adding a > member for keyword_url, I re-used template_url for keyword hint URLs, and made > some changes to consumers of template_url to keep their functionality intact. Yes, I had intended |keyword_url| to replace the existing |template_url| field, not exist alongside it. (I figured that would be a better name, unless there are non-keyword uses of this field?)
> Yes, I had intended |keyword_url| to replace the existing |template_url| field, > not exist alongside it. (I figured that would be a better name, unless there > are non-keyword uses of this field?) Okay, I renamed it to keyword_url and re-uploaded it.
I started to do a review (some comments below), but while considering the AutocompleteResultView and the new IconLabelView began wondering if yet another (!) rethink of the data model makes more sense. (I apologize for how many times I have rethought this; clearly, the patch is complex enough that the ideal design wasn't crystal-clear from the beginning.) The first warning sign is that while we look at a split row and see two different "matches", horizontally separated, the current data model combines them into one AutocompleteMatch, which then has to track which side is "selected", a concept that really seems more in the purview of the AutocompletePopupModel. The next is that, in order to determine the strings for the IconLabelView's "content/description" in AutocompleteResultView::SetMatch(), we have to duplicate logic that already exists in e.g. the keyword provider. Another issue is that IconLabelView has to basically duplicate the work that a standard AutocompleteResultView already does (and some of my comments below highlight ways it seems to diverge). Another problem is that because IconLabelView uses a single Label to hold all of the content and description, it elides differently and may suffer from RTL problems compared with the multiple-piece DrawString()-based code in the main result view. All of these together make me wonder if the following data model might not make even more sense: * AutocompleteMatch drops the keyword_state field. Instead it gains a "scoped_ptr<AutocompleteMatch> associated_keyword_". This is NULL by default and is only set by the AutocompleteController in cases where today we'd have a DUAL_xxx match. * The AutocompleteController uses functionality on the KeywordProvider (dunno what API changes might be needed) to construct the actual match to go here, so that constructing the content/description is always done by the same piece of code. * The AutocompletePopupModel is responsible for tracking which side of a linked pair is expanded. * I think, but am not sure, that the "keyword" string on AutocompleteMatch should go away, and the keyword string and hint state should once again be determined by asking the popup about the state of a particular row. * The AutocompletePopupContentsView holds a pair of AutocompleteResultViews and an Animation object for each line. It is responsible for setting bounds and visibility and doing animation. It also draws the divider between the result views. In this world, AutocompleteResultView needs very few changes (if any -- maybe a static function to report the desired width of a compacted match), because the split pieces are real matches/ResultViews that do painting/layout/event handling just like they do today. What do you think? Will this be better than what we have now? http://codereview.chromium.org/6731036/diff/49036/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_match.h (right): http://codereview.chromium.org/6731036/diff/49036/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.h:132: return keyword_state == DUAL_SHOWING_KEYWORD || This isn't right. DUAL_SHOWING_NON_KEYWORD is the only state that is a "hint" state. Callers should just check for that state directly. http://codereview.chromium.org/6731036/diff/49036/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.h:191: enum KeywordState { Nit: Probably want to document these. http://codereview.chromium.org/6731036/diff/49036/chrome/browser/autocomplete... File chrome/browser/autocomplete/keyword_provider.cc (right): http://codereview.chromium.org/6731036/diff/49036/chrome/browser/autocomplete... chrome/browser/autocomplete/keyword_provider.cc:437: result.keyword_url = element; Seems like this should be set regardless of |supports_replacement|. http://codereview.chromium.org/6731036/diff/49036/chrome/browser/autocomplete... chrome/browser/autocomplete/keyword_provider.cc:440: result.keyword.assign(keyword); Nit: Just use = http://codereview.chromium.org/6731036/diff/49036/chrome/browser/autocomplete... File chrome/browser/autocomplete/search_provider.cc (right): http://codereview.chromium.org/6731036/diff/49036/chrome/browser/autocomplete... chrome/browser/autocomplete/search_provider.cc:815: match.keyword.assign(providers_.keyword_provider().keyword()); Nit: Just use = http://codereview.chromium.org/6731036/diff/49036/chrome/browser/instant/inst... File chrome/browser/instant/instant_controller.cc (right): http://codereview.chromium.org/6731036/diff/49036/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.cc:639: // Extension keywords don't have a real destionation URL. Nit: While here, destionation -> destination http://codereview.chromium.org/6731036/diff/49036/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.cc:640: if (match.keyword_state == AutocompleteMatch::KEYWORD && I don't think we need to check the keyword state here. http://codereview.chromium.org/6731036/diff/49036/chrome/browser/instant/inst... chrome/browser/instant/instant_controller.cc:679: match.keyword_state == AutocompleteMatch::KEYWORD ? match.keyword_url : I think you also want to allow DUAL_SHOWING_KEYWORD here. I also think you want to write this like if (state == KEYWORD or DUAL_SHOWING_KEYWORD) { template_url = keyword_url; } else if (match.type == ...) { ... Because if we're actively showing a keyword, the right template URL is the one on the match, regardless of the default search provider. http://codereview.chromium.org/6731036/diff/49036/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/49036/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:635: keyword.assign(match.keyword); Nit: Eliminate the temp and do this with a ?: inside the OpenURL() call http://codereview.chromium.org/6731036/diff/49036/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/6731036/diff/49036/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:124: void SetSelected(bool flag); Nit: Name the arg |selected|. http://codereview.chromium.org/6731036/diff/49036/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:137: AddChildView(image_); Nit: I think our typical practice is to AddChildView() after the view's other attributes have been set rather than before, in case updating one would otherwise trigger any kind of re-layout/paint. http://codereview.chromium.org/6731036/diff/49036/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:148: image_->SetBounds(LocationBarView::kItemPadding * 2, The use of padding constants in these next two functions seems strange. I don't know why the IconLabelView is given the padding on both sides of the divider instead of just on its side, I don't understand why you're using kExtensionItemPadding below, and I don't see any use of kEdgeItemPadding which I'd expect to see for the padding on either side of the divider and after the end of the text. http://codereview.chromium.org/6731036/diff/49036/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:199: if (selected_) Nit: Collapse this to one call using ?: http://codereview.chromium.org/6731036/diff/49036/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:651: if (wants_keyword_mode && !is_keyword_mode) Do you need to check |same_match| here? Nit: All arms need {} if one does. http://codereview.chromium.org/6731036/diff/49036/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:655: else if ((!same_match) || (same_match && !animation_->is_animating())) { Nit: You can eliminate "same_match &&" and all the internal parens. http://codereview.chromium.org/6731036/diff/49036/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:661: search_view_->SetVisible(match_.has_keyword_hint()); Here you really want to check if the match is a DUAL_xxx type. http://codereview.chromium.org/6731036/diff/49036/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:667: match_.keyword, l10n_util::GetStringUTF16(IDS_EMPTY_KEYWORD_VALUE)); You should be using match_.keyword_url->AdjustedShortNameForLocaleDirection() instead of match_.keyword here.
No need to apologize. It's been an interesting experience and I always enjoy bouncing ideas back and forth until a "perfect" design comes out. > * AutocompleteMatch drops the keyword_state field. Instead it gains a > "scoped_ptr<AutocompleteMatch> associated_keyword_". I believe an issue with this is that scoped_ptr doesn't have a copy constructor, and, as I mentioned before, AutocompleteMatch instances get copied in several places. So is a ref counting pointer okay, or should the rest of the code be modified to use references? > * The AutocompletePopupContentsView holds a pair of AutocompleteResultViews > and an Animation object for each line. It is responsible for setting bounds and > visibility and doing animation. It also draws the divider between the result > views. I think this may cause a lot of fragile code in AutocompletePopupContentsView since it relies on result views being at the same index as a match is in the results list, and all the indexes would need to be offset to take into account any pairs. The second match in a pair could be added after all the others as a workaround, but I think that still would get convoluted. Instead, how about a subclass of AutocompleteResultView called AutocompleteCompositeResultView. This object would then hold the pair of AutocompleteResultViews as children and handle bounds, visibility, and animation. This would still require changes to how result views are created but I think it would provide a cleaner separation of concerns. However, an issue with this approach is the caching of result views that AutocompletePopupContentsView does and keeping straight which are normal result views and which are composite. So yet another alternative is to add a layer like the composite view between all result views and the contents view. If a match has a keyword match then it will show and use the second result view, if not then it won't. What do you think? > What do you think? Will this be better than what we have now? So far I think this would be better and cleaner. I think the toughest part will be clean handling of the split view results. > http://codereview.chromium.org/6731036/diff/49036/chrome/browser/autocomplete... > chrome/browser/autocomplete/keyword_provider.cc:440: > result.keyword.assign(keyword); > Nit: Just use = It seems that existing code uses a mixture of assign and =, and I think it trends more to assign. Is = the better accepted practice? > > http://codereview.chromium.org/6731036/diff/49036/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:148: > image_->SetBounds(LocationBarView::kItemPadding * 2, > The use of padding constants in these next two functions seems strange. I don't > know why the IconLabelView is given the padding on both sides of the divider > instead of just on its side, I don't understand why you're using > kExtensionItemPadding below, and I don't see any use of kEdgeItemPadding which > I'd expect to see for the padding on either side of the divider and after the > end of the text. > Good points. The padding on both sides of the divider was to give the divider a bit more separation between the icon and between the selection background (on either view), which is also why I do a ClipRect in OnPaint. I thought this looked better. I was using kExtensionItemPadding to get a bit more separation from the right side than kEdgeItemPadding gave. I'm not sure why it didn't also translate to Layout, which isn't a good answer.
On 2011/04/14 00:54:44, aaron.randolph wrote: > > * AutocompleteMatch drops the keyword_state field. Instead it gains a > > "scoped_ptr<AutocompleteMatch> associated_keyword_". > > I believe an issue with this is that scoped_ptr doesn't have a copy constructor, > and, as I mentioned before, AutocompleteMatch instances get copied in several > places. So is a ref counting pointer okay, or should the rest of the code be > modified to use references? I'd do neither. AutocompleteMatch is meant to be a copyable type; I'd just write a copy constructor and operator=(). Then inside those you can easily do "new_match.associated_keyword_.reset(*old_match.associated_keyword_);". This also prevents the problems in a refcounting world where you copy a match and then changes get unexpectedly reflected across the copies. > > * The AutocompletePopupContentsView holds a pair of AutocompleteResultViews > > and an Animation object for each line. It is responsible for setting bounds > and > > visibility and doing animation. It also draws the divider between the result > > views. > > I think this may cause a lot of fragile code in AutocompletePopupContentsView > since it relies on result views being at the same index as a match is in the > results list, and all the indexes would need to be offset to take into account > any pairs. The second match in a pair could be added after all the others as a > workaround, but I think that still would get convoluted. This isn't a problem, because associated matches would never be kept in the main result list. They're only reachable via the pointer on the original match. So the matches all still stay on the same rows, and the result set doesn't change size. As a result, I don't think we need to implement any of the alternate proposals you list. e.g. match 0 match 1 -> associated match match 2 match 3 match 4 -> associated match Only the primary matches (on the left) are items in the result set. > > http://codereview.chromium.org/6731036/diff/49036/chrome/browser/autocomplete... > > chrome/browser/autocomplete/keyword_provider.cc:440: > > result.keyword.assign(keyword); > > Nit: Just use = > > It seems that existing code uses a mixture of assign and =, and I think it > trends more to assign. Is = the better accepted practice? We should be using = everywhere. Feel free to fix any instances of assign() you see. > > http://codereview.chromium.org/6731036/diff/49036/chrome/browser/ui/views/aut... > > chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:148: > > image_->SetBounds(LocationBarView::kItemPadding * 2, > > The use of padding constants in these next two functions seems strange. I > don't > > know why the IconLabelView is given the padding on both sides of the divider > > instead of just on its side, I don't understand why you're using > > kExtensionItemPadding below, and I don't see any use of kEdgeItemPadding which > > I'd expect to see for the padding on either side of the divider and after the > > end of the text. > > > > Good points. The padding on both sides of the divider was to give the divider a > bit more separation between the icon and between the selection background (on > either view), which is also why I do a ClipRect in OnPaint. I thought this > looked better. What I meant was, any padding on the left of the divider ought to be part of the left view instead of the right, or mouse clicks go to the wrong view. > I was using kExtensionItemPadding to get a bit more separation from the right > side than kEdgeItemPadding gave. I'm not sure why it didn't also translate to > Layout, which isn't a good answer. We definitely shouldn't repurpose existing constants for a use with a different semantic intent. I'm willing to believe that we might not want kEdgeItemPadding on the sides of the divider, but for the moment I think my proposal above.
> This isn't a problem, because associated matches would never be kept in the main > result list. They're only reachable via the pointer on the original match. So > the matches all still stay on the same rows, and the result set doesn't change > size. As a result, I don't think we need to implement any of the alternate > proposals you list. > > e.g. > > match 0 > match 1 -> associated match > match 2 > match 3 > match 4 -> associated match > > Only the primary matches (on the left) are items in the result set. > Sorry, I don't think I was very clear. I was actually referring more to how AutocompletePopupContentsModel handles displaying matches. To my understanding, AutocompletePopupContentsModel adds a result view as a child for each match in a result. In other places it gets the view associated with a match by using GetChildViewAt and passing the index of the match in the results list. So if I want the view for match 2 I would call GetChildViewAt(2). If we do as you suggest and have a pair of result views for matches with associated keywords, additional result views will need to be added as children to AutocompletePopupContentsModel. So using your example above, if I want the view for match 2 in the results, I would need to call GetChildViewAt(3), not GetChildViewAt(2), to take into account the additional view for the associated match. That's why I made the suggestions, to avoid having to do offset magic or add views in various orders. (I also think having a composite view makes it easier to keep related views together and makes it easier to operate on them.) However, looking at your suggestion again, you say "The AutocompletePopupContentsView holds a pair of AutocompleteResultViews and an Animation object for each line." Did you mean a pair for each line irrespective if that line has an associated match, or pairs only for lines that have associated matches?
On 2011/04/14 01:40:27, aaron.randolph wrote: > However, looking at your suggestion again, you say "The > AutocompletePopupContentsView holds a pair of AutocompleteResultViews > and an Animation object for each line." Did you mean a pair for each line > irrespective if that line has an associated match, or pairs only for lines that > have associated matches? I meant a pair for every line, the way in the current patch the AutocompleteResultView has an IconLabelView for every line. Most of the time they're just not visible. There are various ways to do things, but that seemed easy. It lets you preserve the offset stuff too (just add *2s judiciously).
Alright, I think we're on the same page now. I'll look at implementing this over the next couple days. Thanks.
Better late than never, I've uploaded a new CL that implements tab cycling based on the suggestions by pkasting: - AutocompleteMatch now holds a pointer to an associated match that contains information for a keyword hint. - AutocompletePopupContentsView now handles all split result view / keyword hint logic and animations. - AutocompletePopupModel handles logic of whether a specific line is showing a normal or keyword match. - Added a method to the keyword provider to provide an AutocompleteMatch based on a keyword string.
FYI, I made a slight update to the CL that changed AutocompletePopupContentsView to use a ScopedVector for the animations instead of a vector<ui::SlideAnimation*>. Incidentally, I'd prefer if the associated_keyword on AutocompleteMatch was a ref counted pointer instead of a straight smart pointer since matches are copied so much. However, I'd need to implement AddRef/Release logic on AutocompleteMatch because using RefCounted<> won't work. If you agree then I can update AutocompleteMatch to support this.
This is looking pretty good. Why doesn't making AutocompleteMatch inherit from RefCounted<> work? http://codereview.chromium.org/6731036/diff/61089/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_match.cc (right): http://codereview.chromium.org/6731036/diff/61089/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.cc:90: if (match.associated_keyword.get()) Nit: Shorter: associated_keyword.reset(match.associated_keyword.get() ? new AutocompleteMatch(*match.associated_keyword) : NULL); http://codereview.chromium.org/6731036/diff/61089/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_match.h (right): http://codereview.chromium.org/6731036/diff/61089/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.h:192: // Set if this match has a keyword hint. Nit: I'm a little concerned about potential confusion with there being |keyword|, |associated_keyword|, and |template_url| fields all here. It seems like maybe we need more detailed comments with examples? http://codereview.chromium.org/6731036/diff/61089/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_popup_model.h (right): http://codereview.chromium.org/6731036/diff/61089/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_popup_model.h:83: // Activates the keyword result for the selected line based on |selected|. Nit: How about: // If the selected line has both a normal match and a keyword match, this can be used to choose which to select. It is an error to call this when the selected line does not have both matches (or there is no selection). ...and then make the implementation DCHECK that the above calling conditions hold. http://codereview.chromium.org/6731036/diff/61089/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_popup_model.h:123: bool keyword_selected_; Nit: Add comment: // If the selected line has both a normal match and a keyword match, this determines whether the normal match (if false) or the keyword match (if true) is selected. http://codereview.chromium.org/6731036/diff/61089/chrome/browser/autocomplete... File chrome/browser/autocomplete/keyword_provider.cc (left): http://codereview.chromium.org/6731036/diff/61089/chrome/browser/autocomplete... chrome/browser/autocomplete/keyword_provider.cc:435: if (supports_replacement) Do you know why the old code had this conditional? Or alternately, why did you need to remove it? http://codereview.chromium.org/6731036/diff/61089/chrome/browser/autocomplete... File chrome/browser/autocomplete/keyword_provider.cc (right): http://codereview.chromium.org/6731036/diff/61089/chrome/browser/autocomplete... chrome/browser/autocomplete/keyword_provider.cc:164: // Make sure the model is loaded. This is cheap and quickly bails out if Nit: I think we should factor this into some public getter function that this function, CreateAutocompleteMatch(), and Observe() all call. Then CreateAutocompleteMatch() can avoid taking this as an arg, and AutocompleteController::GetKeywordForText() can call this as well, eliminating the need for AutocompleteController to track the profile. http://codereview.chromium.org/6731036/diff/61089/chrome/browser/autocomplete... chrome/browser/autocomplete/keyword_provider.cc:434: AutocompleteMatch result(this, relevance, false, Nit: While you're here, can you rename |result| to |match|? It's really confusing to see "result" since I expect it to be type AutocompleteResult... http://codereview.chromium.org/6731036/diff/61089/chrome/browser/autocomplete... File chrome/browser/autocomplete/search_provider.cc (right): http://codereview.chromium.org/6731036/diff/61089/chrome/browser/autocomplete... chrome/browser/autocomplete/search_provider.cc:876: match.template_url = &providers_.keyword_provider(); This line isn't necessary (line 823 already does this). http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/cocoa/omn... File chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm (right): http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/cocoa/omn... chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm:546: match.keyword); Nit: Go ahead and indent even http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/gtk/omnib... File chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc (right): http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/gtk/omnib... chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc:480: match.keyword); Nit: Go ahead and indent even http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/omnibox/o... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/omnibox/o... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1036: omnibox_view->model()->ClearKeyword(string16()); Nit: Can this be tested with a keystroke instead? http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:306: if (opt_in_view_ && opt_in_view_->IsVisible()) How come we didn't use to have this? Was it an error? http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:322: if (line == model_->selected_line() && model_->keyword_selected()) { Nit: No need for {} http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:357: CreateCachedViews(child_rv_count, result_size * 2); Nit: It's always bugged me that we create child views on demand but don't destroy them when they're no longer needed. It seems like it would be simpler to just create all the children up front in the constructor, and make them all hidden. We could create all the keyword animations at that point too. http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:368: if (match.associated_keyword.get()) { Nit: Shorter: view->SetVisible(match.associated_keyword.get()); if (view->IsVisible()) view->SetMatch(*match.associated_keyword); http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:376: ui::SlideAnimation* anim = new ui::SlideAnimation(this); Nit: Instead of using a ScopedVector and allocating these on the heap, could we just use a ui::SlideAnimation[]? This way they'd all be stack-allocated and it'd hopefully be a fraction simpler. http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:429: LayoutChildren(); Is this call necessary? It seems like it ought not to be, because if it were, then we'd presumably also need to UpdateBlurRegion() like Layout() does. http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:582: // Draw dividing lines between results and keyword views Nit: Trailing period http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:594: canvas->drawLine( If I'm reading this right, it draws the line in the first column of pixels on the keyword side. I think it'd be better to actually have 1 px between the normal and keyword portions of the row and draw the line inside this pixel. That way the line would be outside the selected area no matter which side is selected. http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.h (right): http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.h:53: virtual gfx::Size GetPreferredSize() OVERRIDE; Nit: If you move the declaration, you should move the definition to stay in the same order. http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (left): http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1051: // Tab while showing instant commits instant immediately. I think this change depends on us ditching this behavior, which may require other changes. sky would know.
On 2011/07/27 20:18:25, Peter Kasting wrote: > This is looking pretty good. > > Why doesn't making AutocompleteMatch inherit from RefCounted<> work? The issue with deriving AutocompleteMatch from RefCounted is due to AutocompleteMatch getting stack allocated a lot. When a static allocated RefCounted object gets destructed, ~RefCountedBase asserts because a flag indicating the object was Released hasn't been set (so it thinks someone else called delete). In response to the comment on using SlideAnimation[], I would have preferred to do it this way but the main issue was SlideAnimation doesn’t have a default constructor, so I couldn’t do a plain array, and it doesn’t allow copy or assignment, so I couldn’t use a vector of stack allocated objects. I'm not sure if adding a default constructor and SetAnimationDelegate method was within the scope of this patch. (Or maybe there is another alternative I am missing?)
http://codereview.chromium.org/6731036/diff/61089/chrome/browser/autocomplete... File chrome/browser/autocomplete/keyword_provider.cc (left): http://codereview.chromium.org/6731036/diff/61089/chrome/browser/autocomplete... chrome/browser/autocomplete/keyword_provider.cc:435: if (supports_replacement) On 2011/07/27 20:18:25, Peter Kasting wrote: > Do you know why the old code had this conditional? Or alternately, why did you > need to remove it? I think this may have been an artifact of some earlier changes. I've restored it in the next update with no ill effects so far. http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:306: if (opt_in_view_ && opt_in_view_->IsVisible()) On 2011/07/27 20:18:25, Peter Kasting wrote: > How come we didn't use to have this? Was it an error? The old loop adjusted all children, implicitly including the opt_in_view. With the new loop, I increment by two, resulting in it skipping the final (opt_in_view) view. So I made it explicit. Looking at it again, it appears child_rv_count is unnecessary so I'll remove that. http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:357: CreateCachedViews(child_rv_count, result_size * 2); On 2011/07/27 20:18:25, Peter Kasting wrote: > Nit: It's always bugged me that we create child views on demand but don't > destroy them when they're no longer needed. It seems like it would be simpler > to just create all the children up front in the constructor, and make them all > hidden. We could create all the keyword animations at that point too. I agree. I'll make that change for the next update. http://codereview.chromium.org/6731036/diff/61089/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:429: LayoutChildren(); On 2011/07/27 20:18:25, Peter Kasting wrote: > Is this call necessary? It seems like it ought not to be, because if it were, > then we'd presumably also need to UpdateBlurRegion() like Layout() does. It's necessary so the result views will resize to include keyword hints. Without it, the keyword hints don't display. I'll change both these lines to just a call to Layout().
On 2011/07/28 00:34:42, aaron.randolph wrote: > On 2011/07/27 20:18:25, Peter Kasting wrote: > > Why doesn't making AutocompleteMatch inherit from RefCounted<> work? > > The issue with deriving AutocompleteMatch from RefCounted is due to > AutocompleteMatch getting stack allocated a lot. When a static allocated > RefCounted object gets destructed, ~RefCountedBase asserts because a flag > indicating the object was Released hasn't been set (so it thinks someone else > called delete). I see. You were concerned about matches getting copied a lot. Can you point out where this happens the most? Maybe we can find a way to simply reduce the amount of copying. (We've done this before; some code first adds matches directly to a vector and then modifies them in place to avoid a copy.) > In response to the comment on using SlideAnimation[], I would have preferred to > do it this way but the main issue was SlideAnimation doesn’t have a default > constructor, so I couldn’t do a plain array, and it doesn’t allow copy or > assignment, so I couldn’t use a vector of stack allocated objects. Ah, OK. I forgot about some of those requirements. (I bet we could do better in C++0x.) I think the ScopedVector method you have now is probably best, then.
On 2011/07/28 18:01:33, Peter Kasting wrote: > You were concerned about matches getting copied a lot. Can you point out where > this happens the most? Maybe we can find a way to simply reduce the amount of > copying. (We've done this before; some code first adds matches directly to a > vector and then modifies them in place to avoid a copy.) > I just ran a test that measured how often AutocompleteMatches were copied or assigned. The test was simply starting Chromium and typing 'a' in the Omnibox. The result was 343 copies and 407 assignments (750 total). 586 of those came in AutocompleteController::UpdateResult, of which 474 were from AutocompleteResult::SortAndCull and 83 from AppendMatches. On the other hand, it only resulted in 13 extra heap allocations of associated_keyword. So the concern may be reduced a bit. (Though that is still a lot of copying.)
On 2011/07/28 23:17:12, aaron.randolph wrote: > On 2011/07/28 18:01:33, Peter Kasting wrote: > > You were concerned about matches getting copied a lot. Can you point out > where > > this happens the most? Maybe we can find a way to simply reduce the amount of > > copying. (We've done this before; some code first adds matches directly to a > > vector and then modifies them in place to avoid a copy.) > > > > I just ran a test that measured how often AutocompleteMatches were copied or > assigned. The test was simply starting Chromium and typing 'a' in the Omnibox. > The result was 343 copies and 407 assignments (750 total). 586 of those came in > AutocompleteController::UpdateResult, of which 474 were from > AutocompleteResult::SortAndCull and 83 from AppendMatches. This suggests that if we wanted to eliminate most of the copies, we'd need to change AutocompleteResult from having a vector of matches to a vector of pointers, or else make AutocompleteMatch implemented using the pointer-to-impl pattern or equivalent. > On the other hand, it only resulted in 13 extra heap allocations of > associated_keyword. So the concern may be reduced a bit. (Though that is still > a lot of copying.) Honestly, a few hundred copies does not seem like a big deal to me. I can't imagine this taking measurable time. I would be more worried if we didn't already clamp the max matches from each provider to a low number, and the sorting algorithm was n^2. But neither of those is the case. So it seems like this is probably not worth trying to optimize as-is.
I've uploaded a new patch set that I believe addresses all the comments. It also moves GetKeywordForText to KeywordProvider. I felt it made the most sense there and it removed the need to track Profile in AutocompleteController.
http://codereview.chromium.org/6731036/diff/61089/chrome/browser/autocomplete... File chrome/browser/autocomplete/search_provider.cc (right): http://codereview.chromium.org/6731036/diff/61089/chrome/browser/autocomplete... chrome/browser/autocomplete/search_provider.cc:928: match.template_url = is_keyword ? &providers_.keyword_provider() : I don't think it's safe to remove this since AddMatchToMap() is not called for this function's matches. http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.cc:970: string16 keyword = keyword_provider_->GetKeywordForText( Nit: Slightly more readable if you linebreak after '=' instead http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_match.h (right): http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.h:194: // |associated_keyword| could be a KeywordProvider match for "amazon.com". This comment isn't right, is it? If the match were for "ama", we wouldn't show a hint to tab-to-search (unless there were a keyword "ama"), so we'd have no |associated_keyword|. http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.h:198: // |keyword| is the keyword text. I'm not sure this is right either. Isn't |keyword| also set if the |fill_into_edit| is a keyword? http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_popup_model.cc (right): http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_popup_model.cc:153: const AutocompleteResult& result = this->result(); Nit: Just call result() directly in the expressions below, no need for a temp. http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_popup_model.cc:154: DCHECK(!result.empty() && selected_line_ != kNoMatch); Nit: If you split this into a DCHECK() and a DCHECK_NE(), it will be easier to see later what's wrong if one fails. http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... File chrome/browser/autocomplete/keyword_provider.cc (right): http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... chrome/browser/autocomplete/keyword_provider.cc:166: TemplateURLService* model = GetTemplateURLService(); Nit: I also meant for the DCHECK() and Load() call to go into the getter. http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... chrome/browser/autocomplete/keyword_provider.cc:397: const string16& text) const { Nit: Indent 4 http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... chrome/browser/autocomplete/keyword_provider.cc:398: const string16 keyword(TemplateURLService::CleanUserInputKeyword(text)); Nit: Whole function should be indented 2, not 4 http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... chrome/browser/autocomplete/keyword_provider.cc:410: url_service->GetTemplateURLForKeyword(keyword); Nit: Indent 4 (3 places) http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... File chrome/browser/autocomplete/keyword_provider.h (right): http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... chrome/browser/autocomplete/keyword_provider.h:80: TemplateURLService* GetTemplateURLService() const; Nit: This can be private, I think http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:307: kw_x - LocationBarView::kItemPadding, Don't we only want to subtract 1? http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:586: LocationBarView::kNormalHorizontalEdgeThickness), Nit: Simpler: Use the even children and draw at (bounds.right()) instead of using the odd children and subtracting a constant. http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1047: // Tab while showing instant commits instant immediately. sky should still look at this to see if this order of checks is what we want, etc.
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:586: LocationBarView::kNormalHorizontalEdgeThickness), On 2011/07/29 21:08:44, Peter Kasting wrote: > Nit: Simpler: Use the even children and draw at (bounds.right()) instead of > using the odd children and subtracting a constant. I would still need to add the constant to the right edge in order to get the line centered between the two views. Otherwise, the line draws on the edge of the selection, and I was under the belief you wanted a gap from the selection border?
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:586: LocationBarView::kNormalHorizontalEdgeThickness), On 2011/07/29 21:57:10, aaron.randolph wrote: > On 2011/07/29 21:08:44, Peter Kasting wrote: > > Nit: Simpler: Use the even children and draw at (bounds.right()) instead of > > using the odd children and subtracting a constant. > > I would still need to add the constant to the right edge in order to get the > line centered between the two views. Otherwise, the line draws on the edge of > the selection, and I was under the belief you wanted a gap from the selection > border? I was imagining the left and right sides having exactly one pixel -- the border pixel -- between them. So e.g. left side is from [0, 10), border pixel at 10, right side at [11, 21) or similar. That would mean that the border pixel could be drawn at the left side's ".right()" since the left side would draw "up to but not including" that pixel. If you implement this and it looks dumb we can add padding, but I figured this would be more symmetrical with how we draw the selection right up to (but not including) the outside edge of the popup.
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:586: LocationBarView::kNormalHorizontalEdgeThickness), On 2011/07/30 00:00:29, Peter Kasting wrote: > On 2011/07/29 21:57:10, aaron.randolph wrote: > > On 2011/07/29 21:08:44, Peter Kasting wrote: > > > Nit: Simpler: Use the even children and draw at (bounds.right()) instead of > > > using the odd children and subtracting a constant. > > > > I would still need to add the constant to the right edge in order to get the > > line centered between the two views. Otherwise, the line draws on the edge of > > the selection, and I was under the belief you wanted a gap from the selection > > border? > > I was imagining the left and right sides having exactly one pixel -- the border > pixel -- between them. So e.g. left side is from [0, 10), border pixel at 10, > right side at [11, 21) or similar. That would mean that the border pixel could > be drawn at the left side's ".right()" since the left side would draw "up to but > not including" that pixel. > > If you implement this and it looks dumb we can add padding, but I figured this > would be more symmetrical with how we draw the selection right up to (but not > including) the outside edge of the popup. I'm with you now. I was thinking right() was inclusive, not exclusive. I've updated it for the next CL.
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_match.h (right): http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.h:198: // |keyword| is the keyword text. On 2011/07/29 21:08:44, Peter Kasting wrote: > I'm not sure this is right either. Isn't |keyword| also set if the > |fill_into_edit| is a keyword? How about "If this is a keyword search match, |keyword| is the keyword text."
I would expect shift-tab to do the opposite of tab, but it doesn't always. In
particular if the previous match has a keyword and you hit shift-tab I would
expect the keyword to be active.
The visuals of the magnifying glass on the right side and '...' leave a bit to
be desired. What does it look like if you only show the magnifying glass and no
bar or '...'?
With instant enabled I hit the following DCHECK when playing with this patch. I
believe it happened when I typed 'google.com a' then deleted the 'a'.
base.dll!base::debug::BreakDebugger() Line 107 C++
base.dll!logging::LogMessage::~LogMessage() Line 640 C++
chrome.dll!SearchProvider::Providers::default_provider() Line 113 + 0xbc
bytes C++
chrome.dll!SearchProvider::AddMatchToMap(const
std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > &
query_string="amazon", const
std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > &
input_text="a", int relevance=251, AutocompleteMatch::Type type=SEARCH_SUGGEST,
int accepted_suggestion=-2, bool is_keyword=false, bool
prevent_inline_autocomplete=false,
std::map<std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t>
>,AutocompleteMatch,std::less<std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t>
>
>,std::allocator<std::pair<std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t>
> const ,AutocompleteMatch> > > * map=[0]()) Line 822 + 0x26 bytes C++
> chrome.dll!SearchProvider::FinalizeInstantQuery(const
std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > &
input_text="a", const
std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > &
suggest_text="mazon") Line 134 C++
chrome.dll!AutocompleteEditModel::FinalizeInstantQuery(const
std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > &
input_text="a", const
std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > &
suggest_text="mazon", bool skip_inline_autocomplete=false) Line 179 C++
chrome.dll!AutocompleteEditModel::SetSuggestedText(const
std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > &
text="mazon", InstantCompleteBehavior behavior=INSTANT_COMPLETE_NOW) Line 187 +
0x50 bytes C++
chrome.dll!LocationBarView::SetSuggestedText(const
std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > &
text="mazon", InstantCompleteBehavior behavior=INSTANT_COMPLETE_NOW) Line
1128 C++
chrome.dll!Browser::SetSuggestedText(const
std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > &
text="mazon", InstantCompleteBehavior behavior=INSTANT_COMPLETE_NOW) Line 3804
+ 0x3e bytes C++
chrome.dll!InstantController::SetSuggestedTextFor(InstantLoader *
loader=0x095be0d8, const
std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > &
text="mazon", InstantCompleteBehavior behavior=INSTANT_COMPLETE_NOW) Line 499 +
0x1d bytes C++
chrome.dll!InstantLoader::SetCompleteSuggestedText(const
std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > &
complete_suggested_text="amazon", InstantCompleteBehavior
behavior=INSTANT_COMPLETE_NOW) Line 813 + 0x26 bytes C++
chrome.dll!InstantLoader::TabContentsDelegateImpl::OnSetSuggestions(int
page_id=7, const
std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char>
> > > & suggestions=[1]("amazon"), InstantCompleteBehavior
behavior=INSTANT_COMPLETE_NOW) Line 531 + 0x5a bytes C++
chrome.dll!DispatchToMethod<InstantLoader::TabContentsDelegateImpl,void
(__thiscall
InstantLoader::TabContentsDelegateImpl::*)(int,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char>
> > > const &,enum
InstantCompleteBehavior),int,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char>
> > >,enum InstantCompleteBehavior>(InstantLoader::TabContentsDelegateImpl *
obj=0x0952e9c0, void (int, const
std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char>
> > > &, InstantCompleteBehavior)* method=0x53248a50, const
Tuple3<int,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char>
> > >,enum InstantCompleteBehavior> & arg={...}) Line 564 + 0x1f bytes C++
chrome.dll!IPC::MessageWithTuple<Tuple3<int,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char>
> > >,enum InstantCompleteBehavior>
>::Dispatch<InstantLoader::TabContentsDelegateImpl,InstantLoader::TabContentsDelegateImpl,void
(__thiscall
InstantLoader::TabContentsDelegateImpl::*)(int,std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char>
> > > const &,enum InstantCompleteBehavior)>(const IPC::Message *
msg=0x0b3456a8, InstantLoader::TabContentsDelegateImpl * obj=0x0952e9c0,
InstantLoader::TabContentsDelegateImpl * sender=0x0952e9c0, void (int, const
std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char>
> > > &, InstantCompleteBehavior)* func=0x53248a50) Line 965 + 0x15 bytes C++
chrome.dll!InstantLoader::TabContentsDelegateImpl::OnMessageReceived(const
IPC::Message & message={...}) Line 511 + 0x2d bytes C++
chrome.dll!TabContents::OnMessageReceived(const IPC::Message & message={...})
Line 270 + 0x13 bytes C++
chrome.dll!RenderViewHost::OnMessageReceived(const IPC::Message & msg={...})
Line 648 + 0x21 bytes C++
chrome.dll!BrowserRenderProcessHost::OnMessageReceived(const IPC::Message &
msg={...}) Line 778 + 0x13 bytes C++
chrome.dll!IPC::ChannelProxy::Context::OnDispatchMessage(const IPC::Message &
message={...}) Line 256 + 0x19 bytes C++
chrome.dll!DispatchToMethod<IPC::ChannelProxy::Context,void (__thiscall
IPC::ChannelProxy::Context::*)(IPC::Message const
&),IPC::Message>(IPC::ChannelProxy::Context * obj=0x09514210, void (const
IPC::Message &)* method=0x5223a0a0, const Tuple1<IPC::Message> & arg={...})
Line 551 + 0xf bytes C++
chrome.dll!RunnableMethod<IPC::ChannelProxy::Context,void (__thiscall
IPC::ChannelProxy::Context::*)(IPC::Message const &),Tuple1<IPC::Message>
>::Run() Line 338 + 0x1e bytes C++
base.dll!base::subtle::TaskClosureAdapter::Run() Line 56 + 0x17 bytes C++
base.dll!base::internal::Invoker1<0,base::internal::InvokerStorage1<void
(__thiscall
base::subtle::TaskClosureAdapter::*)(void),base::subtle::TaskClosureAdapter
*>,void (__thiscall
base::subtle::TaskClosureAdapter::*)(void)>::DoInvoke(base::internal::InvokerStorageBase
* base=0x099389d8) Line 595 + 0x1b bytes C++
base.dll!base::Callback<void __cdecl(void)>::Run() Line 265 + 0xe bytes C++
base.dll!MessageLoop::RunTask(const MessageLoop::PendingTask &
pending_task={...}) Line 456 C++
base.dll!MessageLoop::DeferOrRunPendingTask(const MessageLoop::PendingTask &
pending_task={...}) Line 473 C++
base.dll!MessageLoop::DoWork() Line 661 + 0xc bytes C++
base.dll!base::MessagePumpForUI::DoRunLoop() Line 203 + 0x1d bytes C++
base.dll!base::MessagePumpWin::RunWithDispatcher(base::MessagePump::Delegate *
delegate=0x01edcf88, base::MessagePumpWin::Dispatcher * dispatcher=0x0030e184)
Line 51 + 0xf bytes C++
base.dll!MessageLoop::RunInternal() Line 417 C++
base.dll!MessageLoop::RunHandler() Line 395 C++
base.dll!MessageLoopForUI::Run(base::MessagePumpWin::Dispatcher *
dispatcher=0x0030e184) Line 796 C++
chrome.dll!`anonymous namespace'::RunUIMessageLoop(BrowserProcess *
browser_process=0x01ee9908) Line 709 + 0x1d bytes C++
chrome.dll!BrowserMain(const MainFunctionParams & parameters={...}) Line 2046
+ 0x11 bytes C++
chrome.dll!`anonymous namespace'::RunNamedProcessTypeMain(const
std::basic_string<char,std::char_traits<char>,std::allocator<char> > &
process_type="", const MainFunctionParams & main_function_params={...}) Line
550 + 0x12 bytes C++
chrome.dll!ChromeMain(HINSTANCE__ * instance=0x00340000,
sandbox::SandboxInterfaceInfo * sandbox_info=0x0030fbd8, wchar_t *
command_line_unused=0x00623062) Line 891 + 0x13 bytes C++
chrome.exe!MainDllLoader::Launch(HINSTANCE__ * instance=0x00340000,
sandbox::SandboxInterfaceInfo * sbox_info=0x0030fbd8) Line 259 + 0x1d bytes C++
chrome.exe!wWinMain(HINSTANCE__ * instance=0x00340000, HINSTANCE__ *
__formal=0x00000000, HINSTANCE__ * __formal=0x00000000, HINSTANCE__ *
__formal=0x00000000) Line 46 + 0x10 bytes C++
chrome.exe!__tmainCRTStartup() Line 578 + 0x35 bytes C
chrome.exe!wWinMainCRTStartup() Line 403 C
kernel32.dll!74e33677()
[Frames below may be incorrect and/or missing, no symbols loaded for
kernel32.dll]
ntdll.dll!77079f02()
ntdll.dll!77079ed5()
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete.cc (right):
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete.cc:964: std::set<string16> keywords;
What this loop is doing doesn't depend upon state in the function, so it seems
clearer to move the functionality into its own function.
Also, be sure and NULL check keyword_provider_ (can happen during tests).
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete.cc:973: // Only add the keyword if the
match does not have a duplicate keyword
This comment doesn't seem to match the if (it doesn't check relevance for one).
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:565:
autocomplete_controller_->Stop(false);
I'm pretty sure this results in not running the query for the new text. It's
different than what we have now, but maybe that's ok.
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:605: void
AutocompleteEditModel::ResetCurrentMatchKeywordMode() const {
Doesn't match position in header.
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:605: void
AutocompleteEditModel::ResetCurrentMatchKeywordMode() const {
Maybe this should be called ExitKeywordMode(), or for symmetry with
Accept/Clear, maybe ClearPopupKeyword
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_match.cc (right):
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_match.cc:51:
from_previous(match.from_previous),
Order doesn't match declaration order.
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_match.cc:90: new
AutocompleteMatch(*match.associated_keyword) : NULL);
nit: indent 2 more
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_match.cc:217: host =
host.substr(prefix_len);
Spacing is off.
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_popup_model.cc (right):
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:157:
DCHECK(match.associated_keyword.get());
nit: make the DCHECK a single line.
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_popup_model.h (right):
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.h:86: void
SelectKeyword(bool selected);
Using selected as the name to a function call Select is rather confusing. Use an
enum to make it clearer what the value does.
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete...
File chrome/browser/autocomplete/keyword_provider.cc (right):
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete...
chrome/browser/autocomplete/keyword_provider.cc:396: string16
KeywordProvider::GetKeywordForText(
DOesn't match position in header.
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete...
chrome/browser/autocomplete/keyword_provider.cc:419: (profile_->IsOffTheRecord()
&&
nit: indenting off (should be one past ( on previous line.
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc
(right):
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:251:
for (size_t i = 0; i < AutocompleteResult::kMaxMatches * 2; ++i) {
I don't like always creating the max result views and slideanimations.
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:259:
anim->SetSlideDuration(500);
500 is too long. Can you use the default?
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:296:
AutocompleteResultView* keyword =
Casts like this are easy to get wrong. Use an id or classname to find the views
you care about.
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:304:
collapsed_width) * keyword_animations_[i / 2]->GetCurrentValue()));
To make lifetime easier can be push the animation into the resultsview? And use
CurrentValueBetween.
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:332:
child_at(static_cast<int>(line * 2))->SchedulePaint();
spacing if off.
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut...
File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h
(right):
http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut...
chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:174:
typedef ScopedVector<ui::SlideAnimation> SlideAnimations;
Description?
On 2011/08/01 16:02:15, sky wrote: > I would expect shift-tab to do the opposite of tab, but it doesn't always. In > particular if the previous match has a keyword and you hit shift-tab I would > expect the keyword to be active. That was my explicit design after some thought about why people would be using shift-tab. The idea was that we're supporting a hybrid of tab-to-traverse and tab-to-search, and that shift-tab is a shortcut that only makes sense in the tab-to-traverse use case. It seemed bizarre to have shift-tab sometimes enter tab-to-search mode. Although I could be wrong. > The visuals of the magnifying glass on the right side and '...' leave a bit to > be desired. What does it look like if you only show the magnifying glass and no > bar or '...'? :/ I'm willing to believe that we could do better than what I've suggested so far, but each of these changes seems like a step backwards. Maybe we should ask Glen if he has any bright ideas.
>> The visuals of the magnifying glass on the right side and '...' leave a >> bit to be desired. What does it look like if you only show the magnifying >> glass and no bar or '...'? > > :/ Â I'm willing to believe that we could do better than what I've suggested > so far, but each of these changes seems like a step backwards. > > Maybe we should ask Glen if he has any bright ideas. This is a little out of context for me, anyone care to gesticulate?
On Mon, Aug 1, 2011 at 10:41 AM, Glen Murphy <glen@chromium.org> wrote: >>> The visuals of the magnifying glass on the right side and '...' leave a >>> bit to be desired. What does it look like if you only show the magnifying >>> glass and no bar or '...'? >> >> :/ Â I'm willing to believe that we could do better than what I've suggested >> so far, but each of these changes seems like a step backwards. >> >> Maybe we should ask Glen if he has any bright ideas. > > This is a little out of context for me, anyone care to gesticulate? See http://code.google.com/p/chromium/issues/detail?id=57748 . Many users expect tab to traverse the items in the omnibox popup. This patch adds such functionality. Of course adding such functionality is tricky for two reasons. One, how do you deal with entries that have tab to search? And how do you deal with instant delayed autocomplete? Currently the patch ignores the second problem. To solve the first each entry in the omnibox can effectively have two entries. One for the normal entry, the other corresponding to tab to search. The tab-to-search entry is shown as a magnifying glass on the right side. If you're on an entry that has a tab to search entry pressing tab slides the magnifying glass almost completely covering up the non-tab to search entry. This way you can tab through all the entries and deal with tab to search. If this all sounds confusing and you want to see it in action, give me a heads up and apply the patch and you can come see it in action. -Scott
On 2011/08/01 16:02:15, sky wrote: > With instant enabled I hit the following DCHECK when playing with this patch. I > believe it happened when I typed 'google.com a' then deleted the 'a'. > > base.dll!base::debug::BreakDebugger() Line 107 C++ > base.dll!logging::LogMessage::~LogMessage() Line 640 C++ > chrome.dll!SearchProvider::Providers::default_provider() Line 113 + 0xbc > bytes C++ > chrome.dll!SearchProvider::AddMatchToMap(const I was able to reproduce this and couldn't see where it went through any of the changes I made. So I got a fresh copy of the latest source to test and managed to hit the assertion in that as well. It appears to be due to revision 92360, search_provider.cc, line 147. When I revert that change I don't hit that assertion anymore. The way I hit the assert is to type "google.com a" and wait a couple seconds for instant to finish. No need to backspace. >> The visuals of the magnifying glass on the right side and '...' leave a >> bit to be desired. What does it look like if you only show the magnifying >> glass and no bar or '...'? > > :/ I'm willing to believe that we could do better than what I've suggested > so far, but each of these changes seems like a step backwards. Ignoring how to implement it for a moment, what if you replaced the ellipsis with the actual keyword? I believe this would add a bit of context when a user is scanning the results. And if the ability to click the keyword view to enable keyword search mode was implemented, I think this would (a) provide a larger area for the user to hit, and (b) give a better clue as to what will happen when the user clicks that area. http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.cc:973: // Only add the keyword if the match does not have a duplicate keyword On 2011/08/01 16:02:15, sky wrote: > This comment doesn't seem to match the if (it doesn't check relevance for one). It doesn't check relevance because it expects the matches to already have been sorted by relevance, so it knows the further in it gets the less relevant the matches are. http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:296: AutocompleteResultView* keyword = On 2011/08/01 16:02:15, sky wrote: > Casts like this are easy to get wrong. Use an id or classname to find the views > you care about. Just to be clear, are you suggesting I keep a list of AutocompleteResultView* as a member and iterate through that instead of using child_at? Or did you have an alternative idea in mind?
On Tue, Aug 2, 2011 at 2:54 PM, <aaron.randolph@gmail.com> wrote: > On 2011/08/01 16:02:15, sky wrote: > >> With instant enabled I hit the following DCHECK when playing with this >> patch. > > I >> >> believe it happened when I typed 'google.com a' then deleted the 'a'. > >>     base.dll!base::debug::BreakDebugger()  Line 107 C++ >>     base.dll!logging::LogMessage::~LogMessage()  Line 640  C++ >>     chrome.dll!SearchProvider::Providers::default_provider()  Line 113 >> + 0xbc >> bytes  C++ >>     chrome.dll!SearchProvider::AddMatchToMap(const > > > I was able to reproduce this and couldn't see where it went through any of > the > changes I made.  So I got a fresh copy of the latest source to test and > managed > to hit the assertion in that as well. It appears to be due to revision > 92360, > search_provider.cc, line 147.  When I revert that change I don't hit that > assertion anymore. > > The way I hit the assert is to type "google.com a" and wait a couple seconds > for > instant to finish.  No need to backspace. GAH! That would be me. I'll take a look. > >>> The visuals of the magnifying glass on the right side and '...' leave a >>> bit to be desired. What does it look like if you only show the magnifying >>> glass and no bar or '...'? > >> :/  I'm willing to believe that we could do better than what I've >> suggested >> so far, but each of these changes seems like a step backwards. > > Ignoring how to implement it for a moment, what if you replaced the ellipsis > with the actual keyword?  I believe this would add a bit of context when a > user > is scanning the results.  And if the ability to click the keyword view to > enable > keyword search mode was implemented, I think this would (a) provide a larger > area for the user to hit, and (b) give a better clue as to what will happen > when > the user clicks that area. > > > http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... > File chrome/browser/autocomplete/autocomplete.cc (right): > > http://codereview.chromium.org/6731036/diff/63067/chrome/browser/autocomplete... > chrome/browser/autocomplete/autocomplete.cc:973: // Only add the keyword > if the match does not have a duplicate keyword > On 2011/08/01 16:02:15, sky wrote: >> >> This comment doesn't seem to match the if (it doesn't check relevance > > for one). > > It doesn't check relevance because it expects the matches to already > have been sorted by relevance, so it knows the further in it gets the > less relevant the matches are. > > http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut... > File > chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc > (right): > > http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut... > chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:296: > AutocompleteResultView* keyword = > On 2011/08/01 16:02:15, sky wrote: >> >> Casts like this are easy to get wrong. Use an id or classname to find > > the views >> >> you care about. > > Just to be clear, are you suggesting I keep a list of > AutocompleteResultView* as a member and iterate through that instead of > using child_at?  Or did you have an alternative idea in mind? I'm fine with any of, keep a list of the views, set_id or checking the class name. -Scott
I've just uploaded a new CL that I believe addresses the comments by pkasting and sky. I've sped up the animation, reworked the layout and drawing logic, and updated the formatting and comments. I also changed AutocompletePopupModel::SelectKeyword(bool) to SetSelectedMatch(MatchState).
LGTM http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/63067/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:296: AutocompleteResultView* keyword = On 2011/08/01 16:02:15, sky wrote: > Casts like this are easy to get wrong. Use an id or classname to find the views > you care about. Nit: TBH, I don't think this suggestion of Scott's is a net win, especially if we're just DCHECKing it (GetClassName() will never get called in release mode, etc., meaning we add a lot of boilerplate for little gain). Maybe if instead of using "child_count()" above you use "AutocompleteResult::kMaxMatches * 2", this will be safer against the addition of more children later, and then you can go back to just casting directly. That will also make this code a bit less confusing w.r.t. apparently sizing all children in this loop but not actually doing so for the |opt_in_view_|. http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_match.h (right): http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_match.h:197: // If this is a keyword search match, |keyword| is the keyword text. Nit: Maybe the explicit "If this match is a keyword provider match, or shows a keyword hint, ..."? http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_popup_model.cc (right): http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_popup_model.cc:153: DCHECK(!this->result().empty()); Nit: Why add "this->"? (2 places) http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_popup_model.h (right): http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_popup_model.h:20: RESULT = 0, Nit: Maybe NORMAL would be better? http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_popup_model.h:131: // determines whether the normal match (if SELECTED) or the keyword match Nit: Wrong enum value name http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... File chrome/browser/autocomplete/keyword_provider.cc (right): http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... chrome/browser/autocomplete/keyword_provider.cc:123: TemplateURLService* model = TemplateURLServiceFactory::GetForProfile(profile); Nit: I wonder if there is a good way to make use of your new helper here. Maybe this function should become non-static? http://codereview.chromium.org/6731036/diff/101001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/101001/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:589: canvas->drawLine( Nit: This won't make a visible difference, but since you reserve kNormalHorizontalEdgeThickness worth of width, I suggest using a rect-drawing function here that draws a rect with that width, which will automatically do the right thing if we change it. http://codereview.chromium.org/6731036/diff/101001/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:592: LocationBarView::kNormalHorizontalEdgeThickness), Nit: It's wrong to use this constant to do vertical offsets. How do things look if you just draw through the entire row height? If they're not great, I suggest defining a local constant and using that. http://codereview.chromium.org/6731036/diff/101001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h (right): http://codereview.chromium.org/6731036/diff/101001/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.h:174: typedef ScopedVector<ui::SlideAnimation> SlideAnimations; Nit: Typedef goes atop the "private" section (just below class decls, I think)
http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_match.h (right): http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_match.h:197: // If this is a keyword search match, |keyword| is the keyword text. On 2011/08/04 20:07:53, Peter Kasting wrote: > Nit: Maybe the explicit "If this match is a keyword provider match, or shows a > keyword hint, ..."? I don't think that would be accurate because if this match shows a keyword hint then |associated_keyword| should be set and |keyword| should be blank. http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... File chrome/browser/autocomplete/keyword_provider.cc (right): http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... chrome/browser/autocomplete/keyword_provider.cc:123: TemplateURLService* model = TemplateURLServiceFactory::GetForProfile(profile); On 2011/08/04 20:07:53, Peter Kasting wrote: > Nit: I wonder if there is a good way to make use of your new helper here. Maybe > this function should become non-static? I can make the helper take a Profile as an argument and then make it public and static. That should allow it to be reused here. Though if it is just a static function, is KeywordProvider the best place for it?
http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... File chrome/browser/autocomplete/keyword_provider.cc (right): http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... chrome/browser/autocomplete/keyword_provider.cc:123: TemplateURLService* model = TemplateURLServiceFactory::GetForProfile(profile); On 2011/08/04 21:02:50, aaron.randolph wrote: > On 2011/08/04 20:07:53, Peter Kasting wrote: > > Nit: I wonder if there is a good way to make use of your new helper here. > Maybe > > this function should become non-static? > > I can make the helper take a Profile as an argument and then make it public and > static. That should allow it to be reused here. Though if it is just a static > function, is KeywordProvider the best place for it? Though to do that, the |model_| check would need to move back out to Observe and Start. Are you okay with that?
http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_match.h (right): http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_match.h:197: // If this is a keyword search match, |keyword| is the keyword text. On 2011/08/04 21:02:50, aaron.randolph wrote: > On 2011/08/04 20:07:53, Peter Kasting wrote: > > Nit: Maybe the explicit "If this match is a keyword provider match, or shows a > > keyword hint, ..."? > > I don't think that would be accurate because if this match shows a keyword hint > then |associated_keyword| should be set and |keyword| should be blank. That was what I was asking in my patch set 7 comments. I thought |keyword| is set here for anything that is a keyword or shows a keyword hint. If it's only set for things that are keywords, why do we need it? Can't we get that information from the |template_url| when we know the match is an |associated_keyword| of another match? http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... File chrome/browser/autocomplete/keyword_provider.cc (right): http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... chrome/browser/autocomplete/keyword_provider.cc:123: TemplateURLService* model = TemplateURLServiceFactory::GetForProfile(profile); On 2011/08/04 21:22:14, aaron.randolph wrote: > On 2011/08/04 21:02:50, aaron.randolph wrote: > > On 2011/08/04 20:07:53, Peter Kasting wrote: > > > Nit: I wonder if there is a good way to make use of your new helper here. > > Maybe > > > this function should become non-static? > > > > I can make the helper take a Profile as an argument and then make it public > and > > static. That should allow it to be reused here. Though if it is just a > static > > function, is KeywordProvider the best place for it? > > Though to do that, the |model_| check would need to move back out to Observe and > Start. Are you okay with that? No, that's precisely what I don't want. That's why I suggested making this function non-static.
http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_match.h (right): http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_match.h:197: // If this is a keyword search match, |keyword| is the keyword text. On 2011/08/04 21:31:42, Peter Kasting wrote: > On 2011/08/04 21:02:50, aaron.randolph wrote: > > On 2011/08/04 20:07:53, Peter Kasting wrote: > > > Nit: Maybe the explicit "If this match is a keyword provider match, or shows > a > > > keyword hint, ..."? > > > > I don't think that would be accurate because if this match shows a keyword > hint > > then |associated_keyword| should be set and |keyword| should be blank. > > That was what I was asking in my patch set 7 comments. I thought |keyword| is > set here for anything that is a keyword or shows a keyword hint. If it's only > set for things that are keywords, why do we need it? Can't we get that > information from the |template_url| when we know the match is an > |associated_keyword| of another match? In the places where |keyword| is used, we don't know that the match is an |associated_keyword| of another match. Plus, matches in the result set can have this set; it's not just for associated keywords. It's mainly used as a cache so that calls such as OpenMatch and OnPopupDataChanged are simpler. It replaces calls to GetKeywordText and, in most places, a conditional operator. http://codereview.chromium.org/6731036/diff/101001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/101001/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:589: canvas->drawLine( On 2011/08/04 20:07:53, Peter Kasting wrote: > Nit: This won't make a visible difference, but since you reserve > kNormalHorizontalEdgeThickness worth of width, I suggest using a rect-drawing > function here that draws a rect with that width, which will automatically do the > right thing if we change it. Drawing with a rect makes the line darker and narrower, which doesn't look as nice IMO. You can alleviate this somewhat by setting alpha to 50% but it still doesn't look the same. What I can do instead is set the stroke width to kNormalHorizontalEdgeThickness instead of hard coding it to 1, which should also do the right thing.
http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_match.h (right): http://codereview.chromium.org/6731036/diff/101001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_match.h:197: // If this is a keyword search match, |keyword| is the keyword text. On 2011/08/04 22:32:25, aaron.randolph wrote: > On 2011/08/04 21:31:42, Peter Kasting wrote: > > On 2011/08/04 21:02:50, aaron.randolph wrote: > > > On 2011/08/04 20:07:53, Peter Kasting wrote: > > > > Nit: Maybe the explicit "If this match is a keyword provider match, or > shows > > a > > > > keyword hint, ..."? > > > > > > I don't think that would be accurate because if this match shows a keyword > > hint > > > then |associated_keyword| should be set and |keyword| should be blank. > > > > That was what I was asking in my patch set 7 comments. I thought |keyword| is > > set here for anything that is a keyword or shows a keyword hint. If it's only > > set for things that are keywords, why do we need it? Can't we get that > > information from the |template_url| when we know the match is an > > |associated_keyword| of another match? > > > In the places where |keyword| is used, we don't know that the match is an > |associated_keyword| of another match. Plus, matches in the result set can have > this set; it's not just for associated keywords. It's mainly used as a cache so > that calls such as OpenMatch and OnPopupDataChanged are simpler. It replaces > calls to GetKeywordText and, in most places, a conditional operator. It sounds like this member deserves a block comment saying when and how this is set and what it's used for. For example, // For matches that correspond to valid substituting keywords ("search engines" that aren't the default engine, or extension keywords), this is the keyword. If this is set, then when displaying this match, the edit will use the "keyword mode" UI that shows a blue "Search <engine name>" chit before the user's typing. This should be set for any match that's an |associated_keyword| of a match in the main result list, as well as any other matches in the main result list that are direct keyword matches (e.g. if the user types in a keyword name and some search terms directly). http://codereview.chromium.org/6731036/diff/101001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/101001/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:589: canvas->drawLine( On 2011/08/04 22:32:25, aaron.randolph wrote: > On 2011/08/04 20:07:53, Peter Kasting wrote: > > Nit: This won't make a visible difference, but since you reserve > > kNormalHorizontalEdgeThickness worth of width, I suggest using a rect-drawing > > function here that draws a rect with that width, which will automatically do > the > > right thing if we change it. > > Drawing with a rect makes the line darker and narrower, which doesn't look as > nice IMO. Were you using FillRectInt()? Use that, not DrawRectInt(). > What I can do instead is set the stroke width to > kNormalHorizontalEdgeThickness instead of hard coding it to 1, which should also > do the right thing. I really want to find a way to do this with rectangles. For many reasons, our Skia line-drawing routines are broken and I'm trying to eliminate them entirely from the code. So even though setting a stroke width would work, I'd rather not do it.
http://codereview.chromium.org/6731036/diff/101001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/101001/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:589: canvas->drawLine( On 2011/08/04 23:13:16, Peter Kasting wrote: > On 2011/08/04 22:32:25, aaron.randolph wrote: > > On 2011/08/04 20:07:53, Peter Kasting wrote: > > > Nit: This won't make a visible difference, but since you reserve > > > kNormalHorizontalEdgeThickness worth of width, I suggest using a > rect-drawing > > > function here that draws a rect with that width, which will automatically do > > the > > > right thing if we change it. > > > > Drawing with a rect makes the line darker and narrower, which doesn't look as > > nice IMO. > > Were you using FillRectInt()? Use that, not DrawRectInt(). > > > What I can do instead is set the stroke width to > > kNormalHorizontalEdgeThickness instead of hard coding it to 1, which should > also > > do the right thing. > > I really want to find a way to do this with rectangles. For many reasons, our > Skia line-drawing routines are broken and I'm trying to eliminate them entirely > from the code. So even though setting a stroke width would work, I'd rather not > do it. Yes, I tried FillRectInt first and then various other calls. The main issue I'm seeing is that the rect drawing calls don't reflect the anti-alias flag, at least for pixel-wide rects, so you get a narrow, dark line, instead of a nice anti-aliased line. I can push up my current version (which also addresses the other comments) if you want to see what it looks like, or I can email you screenshots of the difference.
On 2011/08/05 00:52:26, aaron.randolph wrote: > the rect drawing calls don't reflect the anti-alias flag, at > least for pixel-wide rects, so you get a narrow, dark line, instead of a nice > anti-aliased line. I don't understand. How can a 1-px-wide straight vertical line be anti-aliased? There's no diagonals to suffer from aliasing. I can believe that the output looks different (e.g. if one routine is alpha-blending differently than another) but "anti-aliased" seems like a strange term. Maybe screenshots would help. Otherwise, perhaps tomorrow some time I can patch in this code and fiddle with it.
I uploaded a small update that addresses comments by pkasting. I also updated the separation between results and keywords to be kVerticalEdgeThickness (2) in order to support using rect drawing calls instead of line drawing calls. (The larger width, plus a 50% alpha, was necessary to make the output look the same.)
I uploaded a small update that changes the result/keyword divider to look more like the popup border while using FillRectInt.
http://codereview.chromium.org/6731036/diff/121001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/121001/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:573: 160); Where did this number come from? The code elsewhere uses k[Glass,Opaque]PopupAlpha. http://codereview.chromium.org/6731036/diff/121001/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:578: Nit: Extra newline
http://codereview.chromium.org/6731036/diff/121001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/121001/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:573: 160); On 2011/08/05 20:45:17, Peter Kasting wrote: > Where did this number come from? The code elsewhere uses > k[Glass,Opaque]PopupAlpha. I saw those but kGlassPopupAlpha is only ~94%, which didn't match the border to me. I couldn't find any other numbers and the popup border itself looks to be made up of bitmaps. So I experimented to find the percent that best matched the border.
http://codereview.chromium.org/6731036/diff/121001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/121001/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:573: 160); On 2011/08/05 20:56:58, aaron.randolph wrote: > On 2011/08/05 20:45:17, Peter Kasting wrote: > > Where did this number come from? The code elsewhere uses > > k[Glass,Opaque]PopupAlpha. > > I saw those but kGlassPopupAlpha is only ~94%, which didn't match the border to > me. I couldn't find any other numbers and the popup border itself looks to be > made up of bitmaps. So I experimented to find the percent that best matched the > border. OK, please test glass/opaque as well as high-contrast white-on-black (may need to restart Chrome) and then use a named constant with an explanatory comment.
http://codereview.chromium.org/6731036/diff/121001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/121001/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:573: 160); On 2011/08/05 21:02:31, Peter Kasting wrote: > On 2011/08/05 20:56:58, aaron.randolph wrote: > > On 2011/08/05 20:45:17, Peter Kasting wrote: > > > Where did this number come from? The code elsewhere uses > > > k[Glass,Opaque]PopupAlpha. > > > > I saw those but kGlassPopupAlpha is only ~94%, which didn't match the border > to > > me. I couldn't find any other numbers and the popup border itself looks to be > > made up of bitmaps. So I experimented to find the percent that best matched > the > > border. > > OK, please test glass/opaque as well as high-contrast white-on-black (may need > to restart Chrome) and then use a named constant with an explanatory comment. How about this? // An alpha value to apply to the AutocompleteResultView dim text color // that will help the resulting color (used for dividing result and keyword // views) match the popup border color. const SkAlpha kResultDividerAlpha = 170;
Uploaded a minor change that makes the alpha a constant and that has been tested against several themes, including glass, non-glass, classic, and several high contrast themes.
Still LGTM, but sky should OK before landing.
If the first entry in the omnibox has a keyword and you hit tab, then shift-tab doesn't take you out of keyword mode. The only way to get back is to hit tab again, then shift-tab. This seems wonky. I tried this with an instant complete behavior of INSTANT_COMPLETE_DELAYED and I didn't readily notice any problems. Of course tab no longer converts the gray text to selected, but that's expected. I'm out for a week, so Peter is going to handle the rest. The only one that I mention below that likely really needs to be addressed is the one at chrome/browser/autocomplete/autocomplete.cc:1013 . -Scott http://codereview.chromium.org/6731036/diff/82033/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/6731036/diff/82033/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.cc:1013: keyword_provider_->CreateAutocompleteMatch(keyword, input_))); Because we might copy old results do we always need to do this? Meaning should you have an if (!match->associated_keyword.get()) ? Also, do you need an else block for this 'if' that resets the associated_keyword to NULL? Lastly, can you expand the unit tests for coverage of this. http://codereview.chromium.org/6731036/diff/82033/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete.h (right): http://codereview.chromium.org/6731036/diff/82033/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.h:696: void PopulateMatchKeywordHints(); nit: seems like this method name should match what it's updating. Maybe UpdateAssociatedKeywords? http://codereview.chromium.org/6731036/diff/82033/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_match.h (right): http://codereview.chromium.org/6731036/diff/82033/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.h:135: void ComputeStrippedDestinationURL(); nit: Can you mention this is invoked internally and normally you need not invoke it. Having this method really makes me think this should no longer be a struct, but that's a separate issue. http://codereview.chromium.org/6731036/diff/82033/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.h:206: string16 keyword; Since the matches you care about have TemplateURL set, do we really need to cache the keyword too? Couldn't we ask the TemplateURL for the keyword() instead? If you need to ignore the default search engine AutocompleteController::PopulateMatchKeywordHints( could check if the TemplateURL corresponds to the default. http://codereview.chromium.org/6731036/diff/82033/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_popup_model.h (right): http://codereview.chromium.org/6731036/diff/82033/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_popup_model.h:19: enum MatchState { nit: description (it can be as simple as see match_state_ for details)? http://codereview.chromium.org/6731036/diff/82033/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/82033/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:263: if (i < AutocompleteResult::kMaxMatches) { Ick, how about two loops. But as I mentioned last time around I don't like pre-creating the max number of views and animations. As far as the animations go, could you use BoundsAnimator? It should work for this case. http://codereview.chromium.org/6731036/diff/82033/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:264: keyword_animations_.push_back(new ui::SlideAnimation(this)); Create one AnimationContainer and have all animations share it. http://codereview.chromium.org/6731036/diff/82033/chrome/browser/ui/views/aut... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.h (right): http://codereview.chromium.org/6731036/diff/82033/chrome/browser/ui/views/aut... chrome/browser/ui/views/autocomplete/autocomplete_result_view.h:9: #include <string> I don't think you need this include. If anything, add the include for base16.
On 2011/08/09 00:32:51, sky wrote: > If the first entry in the omnibox has a keyword and you hit tab, then shift-tab > doesn't take you out of keyword mode. The only way to get back is to hit tab > again, then shift-tab. This seems wonky. Yeah, that's true. We should at least make shift-tab in keyword mode toggle over to non-keyword mode. I wonder if this means we should go ahead and just make shift-tab traverse in exactly the opposite order as tab instead of skipping over the keywords.
http://codereview.chromium.org/6731036/diff/82033/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/6731036/diff/82033/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.cc:1013: keyword_provider_->CreateAutocompleteMatch(keyword, input_))); On 2011/08/09 00:32:51, sky wrote: > Because we might copy old results do we always need to do this? Meaning should > you have an if (!match->associated_keyword.get()) ? > > Also, do you need an else block for this 'if' that resets the associated_keyword > to NULL? > > Lastly, can you expand the unit tests for coverage of this. I don't think they are necessary as this function is only called on "fresh" matches, but that could always change and it doesn't hurt to have them. http://codereview.chromium.org/6731036/diff/82033/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete_match.h (right): http://codereview.chromium.org/6731036/diff/82033/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete_match.h:206: string16 keyword; On 2011/08/09 00:32:51, sky wrote: > Since the matches you care about have TemplateURL set, do we really need to > cache the keyword too? Couldn't we ask the TemplateURL for the keyword() > instead? If you need to ignore the default search engine > AutocompleteController::PopulateMatchKeywordHints( could check if the > TemplateURL corresponds to the default. The issue with |template_url| is it is only set on KeywordProvider matches when the keyword supports replacement. Since the |keyword| member is used for more than just the new functionality I added (I also changed OpenMatch calls to use it), I believe this would make calls that currently utilize the |keyword| member (e.g., OpenMatch) not function correctly.
On Mon, Aug 8, 2011 at 6:28 PM, <pkasting@chromium.org> wrote: > On 2011/08/09 00:32:51, sky wrote: >> >> If the first entry in the omnibox has a keyword and you hit tab, then > > shift-tab >> >> doesn't take you out of keyword mode. The only way to get back is to hit >> tab >> again, then shift-tab. This seems wonky. > > Yeah, that's true. Â We should at least make shift-tab in keyword mode toggle > over to non-keyword mode. Â I wonder if this means we should go ahead and > just > make shift-tab traverse in exactly the opposite order as tab instead of > skipping > over the keywords. > > http://codereview.chromium.org/6731036/ I like the symmetry of making shift-tab be the inverse of tab. -Scott
http://codereview.chromium.org/6731036/diff/82033/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/6731036/diff/82033/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.cc:1013: keyword_provider_->CreateAutocompleteMatch(keyword, input_))); On 2011/08/16 17:52:43, aaron.randolph wrote: > On 2011/08/09 00:32:51, sky wrote: > > Because we might copy old results do we always need to do this? Meaning should > > you have an if (!match->associated_keyword.get()) ? > > > > Also, do you need an else block for this 'if' that resets the > associated_keyword > > to NULL? > > > > Lastly, can you expand the unit tests for coverage of this. > > I don't think they are necessary as this function is only called on "fresh" > matches, but that could always change and it doesn't hurt to have them. How come you don't invoke this (PopuplateMatchKeywordHints) after CopyOldMatches?
http://codereview.chromium.org/6731036/diff/82033/chrome/browser/autocomplete... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/6731036/diff/82033/chrome/browser/autocomplete... chrome/browser/autocomplete/autocomplete.cc:1013: keyword_provider_->CreateAutocompleteMatch(keyword, input_))); On 2011/08/17 17:02:19, sky wrote: > On 2011/08/16 17:52:43, aaron.randolph wrote: > > On 2011/08/09 00:32:51, sky wrote: > > > Because we might copy old results do we always need to do this? Meaning > should > > > you have an if (!match->associated_keyword.get()) ? > > > > > > Also, do you need an else block for this 'if' that resets the > > associated_keyword > > > to NULL? > > > > > > Lastly, can you expand the unit tests for coverage of this. > > > > I don't think they are necessary as this function is only called on "fresh" > > matches, but that could always change and it doesn't hurt to have them. > > How come you don't invoke this (PopuplateMatchKeywordHints) after > CopyOldMatches? Honestly, because I forgot that besides setting associated keywords this was also ensuring there were no duplicates. It'll be changed in the next update. (Which is done except for the unit test of this method.)
On 2011/08/17 15:37:51, sky wrote: > On Mon, Aug 8, 2011 at 6:28 PM, <mailto:pkasting@chromium.org> wrote: > > On 2011/08/09 00:32:51, sky wrote: > >> > >> If the first entry in the omnibox has a keyword and you hit tab, then > > > > shift-tab > >> > >> doesn't take you out of keyword mode. The only way to get back is to hit > >> tab > >> again, then shift-tab. This seems wonky. > > > > Yeah, that's true. We should at least make shift-tab in keyword mode toggle > > over to non-keyword mode. I wonder if this means we should go ahead and > > just > > make shift-tab traverse in exactly the opposite order as tab instead of > > skipping > > over the keywords. > > > > http://codereview.chromium.org/6731036/ > > I like the symmetry of making shift-tab be the inverse of tab. > > -Scott I implemented this change and while I am a big fan of symmetry, actually using it strikes me as slightly odd. I think it has to do with the way it moves down the list on each TAB press, and then shoots to the right, and then moves down the list again. At least when moving down there is some sort of (keyword) hint for when it is going to move to the right. When moving back up and then suddenly going both up and to the right, without any hint that it will do so except for seeing the magnifying glass on the match above, it seems a little jarring. When you were discussing this a couple weeks ago I had an idea for an alternative layout that I forgot to mention. Instead of showing the keyword match to the right with an ellipsis, what if it was shown below it's associated match and indented? And also showed the full "Search X for <>" message? This way TAB/SHIFT+TAB would always move in a vertical direction, "Press TAB to search" would still work by default, and the keyword search would be much more obvious. For example, "ama" would show: ama - Google Search ------------------------- www.amazon.com - Amazon.com ------------------------- Search amazon.com for <enter query> ------------------------- amazon - Google Search -------------------------
On 2011/08/17 18:52:59, aaron.randolph wrote: > At least when moving down there is some sort of (keyword) hint > for when it is going to move to the right. When moving back up and then > suddenly going both up and to the right, without any hint that it will do so > except for seeing the magnifying glass on the match above, it seems a little > jarring. This was part of my fear. sky, please try using this patch and tell me what you think. > When you were discussing this a couple weeks ago I had an idea for an > alternative layout that I forgot to mention. Instead of showing the keyword > match to the right with an ellipsis, what if it was shown below it's associated > match and indented? Part of the reason for not going this route was the desire not to expand the dropdown's height or increase the visual scanning cost. In the worst case we could have several of these entries, and we're trying to keep the dropdown short and lightweight.
I've uploaded a new CL with the following main changes: - SHIFT+TAB movement now mirrors TAB movement. - If the first result has a keyword hint and is in keyword mode, SHIFT+TAB will exit keyword mode. - Replaced the SlideAnimation vector and logic in AutocompletePopupContentsView with BoundsAnimator. - Added a unit test for the |associated_keyword| creation method.
Just wanted to send out a reminder that I uploaded a new change list several weeks ago.
On 2011/09/20 23:03:25, aaron.randolph wrote: > Just wanted to send out a reminder that I uploaded a new change list several > weeks ago. Yep, I was actually testing it already :). I called in Glen, our fearless UI designer, and we walked through the behavior and appearance of the patch. We settled on the following changes: * Glen wants to make some mockups of drawing these rows more like we draw menu items that have submenus -- in other words, a line with associated tab-to-search engine might look like: O google.com > (here "O" is a globe and ">" is some kind of an arrow/caret icon; look at the wrench menu for examples of lines with these.) The selection in this case would cover the entire row, not stop before the arrow. Then when the state is toggled, we'd flip the arrow and animate it all the way leftward, also sliding in the new "Search google.com for..." text. The blue selection rect would continue to cover the entire line the whole time. When we reset the line we'd flip the arrow again and push it and the text back leftward, revealing the original text as we go. (All these details of the animation are my own invention, subject to change as Glen mocks this up.) * Added bonus: make hovering the arrow with the mouse light it up somehow, and then clicking toggle the state of the line. * Once this is done, revert shift-tab to my original spec: shift-tabbing from a line below this acts just like the up arrow currently does, instead of acting symmetrically with tab. Otherwise the behavior on this patch seems good to go. Obviously we need icons from Glen and the final behavioral details from his mocks. But even without those you can probably get this mostly implemented. I don't know how much of a change to the dropdown drawing code this implies; it seems like instead of having two result views for each line, we can maybe go back to one, and then just animate the actual layout of contents inside that result -- but maybe I'm wrong, I haven't thought hard about it.
Also I attached copy of your patch updated to the latest trunk on bug 57748 in case it saves you any time.
On 2011/09/21 00:15:20, Peter Kasting wrote: > On 2011/09/20 23:03:25, aaron.randolph wrote: > > Just wanted to send out a reminder that I uploaded a new change list several > > weeks ago. > > Yep, I was actually testing it already :). > > I called in Glen, our fearless UI designer, and we walked through the behavior > and appearance of the patch. We settled on the following changes: > > * Glen wants to make some mockups of drawing these rows more like we draw menu > items that have submenus -- in other words, a line with associated tab-to-search > engine might look like: > > O http://google.com > > > (here "O" is a globe and ">" is some kind of an arrow/caret icon; look at the > wrench menu for examples of lines with these.) The selection in this case would > cover the entire row, not stop before the arrow. > > Then when the state is toggled, we'd flip the arrow and animate it all the way > leftward, also sliding in the new "Search http://google.com for..." text. The blue > selection rect would continue to cover the entire line the whole time. When we > reset the line we'd flip the arrow again and push it and the text back leftward, > revealing the original text as we go. (All these details of the animation are > my own invention, subject to change as Glen mocks this up.) > > * Added bonus: make hovering the arrow with the mouse light it up somehow, and > then clicking toggle the state of the line. > > * Once this is done, revert shift-tab to my original spec: shift-tabbing from a > line below this acts just like the up arrow currently does, instead of acting > symmetrically with tab. > > Otherwise the behavior on this patch seems good to go. > > Obviously we need icons from Glen and the final behavioral details from his > mocks. But even without those you can probably get this mostly implemented. I > don't know how much of a change to the dropdown drawing code this implies; it > seems like instead of having two result views for each line, we can maybe go > back to one, and then just animate the actual layout of contents inside that > result -- but maybe I'm wrong, I haven't thought hard about it. That sounds good and I think it will simplify some things. I'll get started on the implementation. And thanks for the patch; it definitely helps.
On 2011/09/21 00:15:20, Peter Kasting wrote: > Obviously we need icons from Glen and the final behavioral details from his > mocks. Would it be possible to get these icons (including hover and pressed states) before I upload or should I upload it with placeholder icons? I have it working otherwise (including using the mouse to trigger the keyword.)
On 2011/10/12 01:41:46, aaron.randolph wrote: > On 2011/09/21 00:15:20, Peter Kasting wrote: > > Obviously we need icons from Glen and the final behavioral details from his > > mocks. > > Would it be possible to get these icons (including hover and pressed states) > before I upload or should I upload it with placeholder icons? I pinged Glen over IM about this.
I uploaded a new CL that implements the changes suggested by pkasting. Result selection highlight now covers the whole line, there is no more divider between match and keyword and no more ellipsis, SHIFT+TAB has been reverted to not expanding matches, and keyword mode can be activated with the mouse. The keyword activation button uses images I hacked together and they're strictly to show how the implementation works. They definitely need to be replaced with better, official ones. (I used a magnifying glass for the image because I thought that was a bit more intuitive than an arrow on what the result of clicking will be.) Additionally, pasting "keyword foo" will now display the "Press TAB to search keyword" hint and will correctly fill in "foo" when keyword mode is activated. (When I initially tested this in Chrome the hint didn't display there either. Is that by design?) This was necessary due to the new way keywords are displayed with this CL.
I'm sorry I've not gotten to this. I was unexpectedly OOO several days last week and am WebKit sheriff this week. I will try to get to this as soon as I can.
At long last I returned to this. Glen finally gave me some icons; I'll check them in for you soon (as soon as one of our tools guys fixes some gclient bugs) and then your change can use the real resource identifiers. This basically looks OK, almost everything below is a nit, except for one or two bits. http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:1016: last_result.default_match()->associated_keyword.get()))); Can the associated keyword actually differ in this case? http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:1047: match->associated_keyword.reset(NULL); Nit: NULL arg is unnecessary http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:1108: Nit: Unnecessary newline? http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete.h (right): http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.h:690: RedundantKeywordsIgnoredInResult); Nit: Indent even if possible http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_edit.cc (right): http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_edit.cc:891: keyword = match->keyword; Nit: Shorter: is_keyword_hint = match->associated_keyword.get() != NULL; keyword = (is_keyword_hint ? match->associated_keyword->keyword : match->keyword); http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_popup_model.cc (right): http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_popup_model.cc:104: string16 keyword = match.keyword; Nit: Shorter: const bool is_keyword_hint = match.associated_keyword.get() != NULL; string16 keyword(is_keyword_hint ? match.keyword : match.associated_keyword->keyword); Since you need this in two places I wonder if it makes sense to make this an accessor on AutocompleteMatch. http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_popup_model.cc:152: DCHECK_NE(selected_line_, kNoMatch); Nit: (expected, actual) http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_popup_model.h (right): http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_popup_model.h:18: // See selected_line_state_ for details Nit: Missing trailing period http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_unittest.cc (right): http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_unittest.cc:35: } // namespace Nit: For short namespaces like this let's just kill the newlines inside and skip the closing comment. http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_unittest.cc:118: void RunQuery(string16 query); Nit: const string16& http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_unittest.cc:210: controller_.reset(controller); Nit: Just combine these two lines http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_unittest.cc:332: struct test_data { Nit: Struct names use CamelCase. http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_unittest.cc:337: { ASCIIToUTF16("fo"), ASCIIToUTF16(""), false }, Nit: ASCIIToUTF16("") -> string16() (many places) http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_unittest.cc:340: { string16(), string16(), false }, The usage of "blank rows" in here to trigger the UpdateAssociatedKeywords call is a bit too clever. Instead it seems like you could just declare three instances of the test data struct, one with each set of cases; then you could repeatedly call a helper function that takes such a struct, populates the result set accordingly, and then runs your checks at the end. You can even use SCOPED_TRACE when doing this so you don't need to manually print the set number in the helper function. http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... File chrome/browser/autocomplete/keyword_provider.cc (right): http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... chrome/browser/autocomplete/keyword_provider.cc:167: string16 remaining_input = SplitReplacementStringFromInput(text, true); Nit: Can fold this into the next line http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... File chrome/browser/autocomplete/keyword_provider_unittest.cc (right): http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... chrome/browser/autocomplete/keyword_provider_unittest.cc:35: void RunTest(const char* input, const char* keyword); I'd prefer not to add a function with the same name that means something totally different from the one above. You can just inline this function's body into the GetKeywordForInput testcase: EXPECT_EQ(ASCIIToUTF16("aa"), kw_provider_->GetKeywordForText(ASCIIToUTF16("aa"))); ... (and similar) ... http://codereview.chromium.org/6731036/diff/151061/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): http://codereview.chromium.org/6731036/diff/151061/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1049: void ShiftTabRevertKeyword() { Nit: Instead of writing two tests for these it seems OK to just add the last two blocks here to the test above to save the initial setup code. http://codereview.chromium.org/6731036/diff/151061/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (left): http://codereview.chromium.org/6731036/diff/151061/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:265: v->GetPreferredSize().height()); Nit: This indentation was fine. http://codereview.chromium.org/6731036/diff/151061/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/151061/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:447: else if (!opt_in_view_) Nit: No else after return http://codereview.chromium.org/6731036/diff/151061/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/6731036/diff/151061/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:192: keyword_button_.reset(new views::ImageButton(this)); After talking with Glen some more we agreed that it would be better to drop the special mouse-handling on this icon and just make it a purely graphical indicator. Clicking on it will be no different than clicking anywhere else on the line. That should simplify some code. I think maybe this code didn't draw the appropriate background color behind the icon when the rest of the line was selected/hovered anyway?, so if so, that will get automatically fixed too. http://codereview.chromium.org/6731036/diff/151061/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:614: popup_model_->edit_model()->ClearKeyword( Nit: If one arm of a conditional spans more than one line, you need {}. You might be able to prevent this by factoring out all the popup_model_->edit_model() calls to a temp?
I've addressed all the nits, removed the mouse interaction, and fixed some bugs. All I have left is to integrate the new icons. Did you have any luck getting them checked in? If so, are they somewhere other than chrome/app/theme? http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/6731036/diff/151061/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:1016: last_result.default_match()->associated_keyword.get()))); On 2011/12/15 22:56:04, Peter Kasting wrote: > Can the associated keyword actually differ in this case? Yes, there were cases where the result set would update and the default match would gain an associated keyword, but since |fill_into_edit| was the same no notification that the default had updated was received. This resulted in the keyword hint not displaying correctly. I've updated the comment in the next CL.
On 2011/12/20 21:07:56, aaron.randolph wrote: > All I have left is to integrate the new icons. Did you have any luck getting > them checked in? If so, are they somewhere other than chrome/app/theme? I have been blocked by gcl <-> svn 1.7 bugs. maruel is supposedly fixing them, so I hope to have these checked in soon. There are two arrows and they'll be similar to other dropdown icons in that you'll have normal, selected, "dark" (used by GTK) and .pdf (used by Cocoa) versions. > On 2011/12/15 22:56:04, Peter Kasting wrote: > > Can the associated keyword actually differ in this case? > > Yes, there were cases where the result set would update and the default match > would gain an associated keyword, but since |fill_into_edit| was the same no > notification that the default had updated was received. This resulted in the > keyword hint not displaying correctly. I've updated the comment in the next CL. Huh, how did we get an async keyword without changing anything else about the match? Did this have something to do with extensions or something? I'm kind of mystified.
On 2011/12/21 05:52:16, Peter Kasting wrote: > > Huh, how did we get an async keyword without changing anything else about the > match? Did this have something to do with extensions or something? I'm kind of > mystified. I don't believe extensions were involved because I didn't have any installed. Here is what I recall of this case: - It would only be hit on the first result after the browser had been launched. All subsequent result updates performed as expected. - I type in a string that I know has a keyword, such as "ama" for amazon. - The first pass though SortAndCull would have 2 matches, IIRC. The first was a search match, and I believe the second was a history match. (I may have these backwards.) - UpdateAssociatedKeywords is called on this result set and no keyword is found for the search match. - The result set is updated with nav suggestions, I believe. These are merged in and UpdateAssociatedKeywords is called again. This time a keyword is found for the search match (same match as above). - The result view updates and a keyword icon is shown in the first match, but the "Press TAB to search" hint is not shown since default_match_changed is false. It's been a couple months so some of the details are a little hazy. I can try to replicate it later to get more exact details if necessary.
On 2011/12/21 05:52:16, Peter Kasting wrote: > On 2011/12/20 21:07:56, aaron.randolph wrote: > > All I have left is to integrate the new icons. Did you have any luck getting > > them checked in? If so, are they somewhere other than chrome/app/theme? > > I have been blocked by gcl <-> svn 1.7 bugs. maruel is supposedly fixing them, > so I hope to have these checked in soon. There are two arrows and they'll be > similar to other dropdown icons in that you'll have normal, selected, "dark" > (used by GTK) and .pdf (used by Cocoa) versions. As of today maruel fixed the svn issues, and I've checked in the new images in r116603. I also added resource identifiers so you should be able to "just use" these. For the keyword thing, I wonder if we really should write code here just to address the "loading the keyword DB on startup" case (very nice catch on spotting that one). I guess the only reason to not write this code is if we're worried that we'll do something bad if this code could make us occasionally send a notification that we shouldn't really send. I can't think of any such problems off the top of my head, so I guess this is fine. As a note in case you take longer than I do -- I'm deep in some refactoring work right now that is going to remove the TemplateURL* on AutocompleteMatch and replace it with a string "keyword". Obviously, this is similar to what you do in this change. The difference is that I'd be filling that field in for all cases where we used to have a non-NULL TemplateURL, whereas your change only does this in a subset of those cases. I guess this only matters if I manage to actually land this code soon, which doesn't look likely, unless you want to help me avoid merge pain by thinking through whether this change could be made to work correctly in that world (e.g. would we then need more fields on AutocompleteMatch?).
On 2012/01/06 01:03:27, Peter Kasting wrote: > As of today maruel fixed the svn issues, and I've checked in the new images in > r116603. I also added resource identifiers so you should be able to "just use" > these. Great! I'll start work on getting those integrated. > For the keyword thing, I wonder if we really should write code here just to > address the "loading the keyword DB on startup" case (very nice catch on > spotting that one). I guess the only reason to not write this code is if we're > worried that we'll do something bad if this code could make us occasionally send > a notification that we shouldn't really send. I can't think of any such > problems off the top of my head, so I guess this is fine. > Sounds good to me. I wasn't sure if that behavior was intentional, so I added the simple workaround instead. I'd be much happier not injecting workarounds, as I'm sure everyone else would be. > As a note in case you take longer than I do -- I'm deep in some refactoring work > right now that is going to remove the TemplateURL* on AutocompleteMatch and > replace it with a string "keyword". Obviously, this is similar to what you do > in this change. The difference is that I'd be filling that field in for all > cases where we used to have a non-NULL TemplateURL, whereas your change only > does this in a subset of those cases. I guess this only matters if I manage to > actually land this code soon, which doesn't look likely, unless you want to help > me avoid merge pain by thinking through whether this change could be made to > work correctly in that world (e.g. would we then need more fields on > AutocompleteMatch?). Would it make more sense for me to just make that refactoring a part of my patch? It's already halfway there, and would avoid potential merge issues. If not, I can work up what any potential sticking points there may be. What's the goal of the refactoring?
On 2012/01/06 01:58:08, aaron.randolph wrote: > Would it make more sense for me to just make that refactoring a part of my > patch? Probably not. Let's not delay your patch further. This is only a concern if your patch doesn't land any time soon. This particular bit of refactoring deals with a case where a TemplateURL is changed or deleted while autocompletion is running. In this case the pointers on the AutocompleteMatch objects point to either modified or dead objects, which is bad. This is probably extraordinarily rare; I'm fixing it because I spotted the theoretical issue, not because of an actual crash. As a precursor I'm trying to refactor a lot of machinery relating to TemplateURL to try and ensure that all TemplateURLs have keywords and that the keywords are unique (with the exception of extension keywords, which can overlap; for those cases there's a priority system that determines how to map keywords to TemplateURLs). This turns out to be surprisingly annoying.
I've uploaded a new patch that addresses the nits and uses the official icons. It also contains some bug fixes and updated unit tests. It also adds keyword DB loading to KeywordProvider creation as I thought that's where it made the most sense. For the template_url refactoring, I don't believe there will be any merge issues. Looking at how template_url is used in InstantController, AutocompleteEdit, and AutocompletePopupModel, I don't think any additional fields would need to be added to AutocompleteMatch if all references to template_url are replaced with calls to GetTemplateURLForKeyword (assuming that's what you had in mind). As an aside, should I delete some of the older patch sets?
Re-uploaded as I forgot an updated unit test.
Updated the patch to fix an issue with accepting a keyword when the popup is closed.
The main thing this still needs is to be patched in and tested by me/sky/glen. http://codereview.chromium.org/6731036/diff/187001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/6731036/diff/187001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:1015: last_result.default_match()->fill_into_edit)); I thought I talked myself to the conclusion that your addition of the associated_keyword check here was a good idea? http://codereview.chromium.org/6731036/diff/187001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:1027: string16 remaining_input; Nit: Declare this in the most local scope http://codereview.chromium.org/6731036/diff/187001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_match.h (right): http://codereview.chromium.org/6731036/diff/187001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_match.h:152: // |associated_keyword| is set then that value will be used. Nit: If I understand your comments correctly, I think the last two sentences can simply be removed -- the overall meaning doesn't change and the lack of implementation detail is fine for a header (and doesn't make it sound at first as if you're contradicting yourself or something, like the current text sort of does). http://codereview.chromium.org/6731036/diff/187001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_unittest.cc (right): http://codereview.chromium.org/6731036/diff/187001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_unittest.cc:32: const size_t num_results_per_provider = 3; Nit: Enclosing this constant in a namespace before was fine, I was just suggesting not also adding newlines and a trailing "// namespace" comment. While you're touching this file, this constant should probably be |kResultsPerProvider| or something like that. http://codereview.chromium.org/6731036/diff/187001/chrome/browser/autocomplet... File chrome/browser/autocomplete/keyword_provider.cc (right): http://codereview.chromium.org/6731036/diff/187001/chrome/browser/autocomplet... chrome/browser/autocomplete/keyword_provider.cc:63: GetTemplateURLService(); I'm worried about doing this. This can slow startup because this service has to load off disk. I think I'd rather take the tradeoff of leaving this as loaded-on-demand and having the DB not in memory for the first few characters. http://codereview.chromium.org/6731036/diff/187001/chrome/browser/autocomplet... chrome/browser/autocomplete/keyword_provider.cc:161: string16 remaining_input; Nit: This variable is dead. http://codereview.chromium.org/6731036/diff/187001/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): http://codereview.chromium.org/6731036/diff/187001/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1021: string16 text = UTF8ToUTF16(kSearchKeyword); Nit: Could be ASCIIToUTF16() http://codereview.chromium.org/6731036/diff/187001/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1043: location_bar_focus_view_id_)); Nit: Old indent was fine (2 places) http://codereview.chromium.org/6731036/diff/187001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/187001/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:323: const AutocompleteMatch& match = GetMatchAtIndex(i); Nit: Could roll this into a line below http://codereview.chromium.org/6731036/diff/187001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/6731036/diff/187001/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:124: animation_->SetDuration(500); I suspect this will be too slow, but I should patch it in locally and test. We try to make most of our animations happen very quickly, e.g. 120 ms or so. I would imagine this should probably be no larger than 300 ms. http://codereview.chromium.org/6731036/diff/187001/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:187: if (show_keyword) { Nit: No {} necessary http://codereview.chromium.org/6731036/diff/187001/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:293: IDR_OMNIBOX_TTS_KEYWORD_DARK; We don't actually use the DARK variants for GTK-views, just for GTK-non-views, so you can eliminate this #ifdef. (See the treatment of IDR_OMNIBOX_HTTP{_XXX}.)
Pushed up a new patch to address the nits. http://codereview.chromium.org/6731036/diff/187001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/6731036/diff/187001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:1015: last_result.default_match()->fill_into_edit)); On 2012/01/11 03:00:16, Peter Kasting wrote: > I thought I talked myself to the conclusion that your addition of the > associated_keyword check here was a good idea? Sorry, I misinterpreted your comment. I thought your first sentence was saying to add DB load code before here to address the start-up case, and all references to "this code" were referring to that (which is why I added the DB load code to the last update.)
On 2012/01/11 03:00:16, Peter Kasting wrote: > The main thing this still needs is to be patched in and tested by me/sky/glen. OK, glen and I tested this. Here's the feedback. (1) We're going to drop the left-facing arrow and just use the right-facing one. This should simplify things a little. (2) There are some visual problems because the icons I checked in weren't padded correctly. I'll change the icons, you don't need to worry about this. (3) Needs testing and fixing in RTL mode (launch with --lang=he): (a) Unlike the other omnibox icons, the arrow should be reversed in RTL. Views have some sort of "auto-flip in RTL" toggle so if you're using an ImageView or something you should be able to fix this easily. (b) The positioning of the icon and text has problems, especially when you've animated into keyword mode. There are probably multiple solutions to this, e.g. you can manually calculate everything correctly in both modes, or you can make the result view have a bunch of child views for the different icons and text and stuff and then try to get clever about how you auto-flip different views in the hierarchy. If you have problems with DCHECK failures about SanitizeString() you can just locally comment-out the DCHECKs. (4) There's a bug where if the dropdown is closed and you use tab/shift-tab to toggle keyword mode on and off, the dropdown no longer opens automatically at the same time. This should hopefully be an easy fix. (5) The SetDuration() call needs two changes: (a) It doesn't actually work unless you call SetSlideDuration() instead. (b) Let's change it from 500 ms to "width() / 4". This means you'll have to move the call to happen any time the dropdown changes size. The benefit of this is that the animation speed looks the same for all widths of the dropdown. Otherwise, Glen gave the "looks good, ship it" verbal sign-off. Hooray!
On 2012/01/11 22:38:54, Peter Kasting wrote: > > Otherwise, Glen gave the "looks good, ship it" verbal sign-off. Hooray! Great news!
On 2012/01/11 22:38:54, Peter Kasting wrote: > (2) There are some visual problems because the icons I checked in weren't padded > correctly. I'll change the icons, you don't need to worry about this. New icons in in r117354. Note that the resource IDs changed slightly. With these, a row that's in keyword mode should have both the icon and text looking appropriately centered (for the icon) or left-aligned (for the text) with the rows above and below.
I pushed out an update that addresses the issues found. The RTL issues were an easy fix - I just needed to change where I called GetMirroredX from. I also changed the icon to use an ImageView and added some infrastructure that I feel better supports switching the keyword icon from selected to not. I'm a big fan of the arrow always facing right, too. It resembles a prompt when in keyword mode, which has a nice symmetry with the keyword mode prompt.
http://codereview.chromium.org/6731036/diff/199006/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/6731036/diff/199006/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:1008: // updated if fill_into_edit differs. We don't check the URL as that may Nit: "...if fill_into_edit or associated_keyword differ. (The latter can change if we've just started Chrome and the keyword database finishes loading while processing this request.) ..." http://codereview.chromium.org/6731036/diff/199006/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_popup_model.cc (right): http://codereview.chromium.org/6731036/diff/199006/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_popup_model.cc:87: // We need to update |selected_line_| before calling OnPopupDataChanged(), so Nit: "We need to update |selected_line_state_| and |selected_line_| before calling InvalidateLine(), since it will check them to determine how to draw. We also need to update |selected_line_| before calling OnPopupDataChanged(), ..." http://codereview.chromium.org/6731036/diff/199006/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/199006/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:294: model_->selected_line_state() == AutocompletePopupModel::KEYWORD); Don't we need to add "IsSelectedIndex(line) &&" in front of this condition? Won't this code, as written, cause any keyword-containing line that's invalidated to flip to the same mode as the selected line? http://codereview.chromium.org/6731036/diff/199006/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/6731036/diff/199006/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:180: if (match.associated_keyword.get()) { There is a memory safety issue here, as well as a simplicity one. Memory-safety-wise, you need to do "keyword_icon_->set_parent_owned(false);" after creating it, because you own the object (via the scoped_ptr) rather than the view hierarchy owning it. Simplicity-wise, instead of creating this object in here, let's do it in the constructor. Then the code here can just say something like: if (match.associated_keyword.get()) { if (!keyword_icon_->parent()) AddChildView(keyword_icon_.get()); } else if keyword_icon_->parent()) { RemoveChildView(keyword_icon_.get()); } This also eliminates the need for the Invalidate() function. http://codereview.chromium.org/6731036/diff/199006/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:302: model_->IsSelectedIndex(model_index_) ? IDR_OMNIBOX_TTS_SELECTED : Nit: Break after '?' rather than ':'
http://codereview.chromium.org/6731036/diff/199006/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/6731036/diff/199006/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:180: if (match.associated_keyword.get()) { On 2012/01/12 22:59:59, Peter Kasting wrote: > There is a memory safety issue here, as well as a simplicity one. > > Memory-safety-wise, you need to do "keyword_icon_->set_parent_owned(false);" > after creating it, because you own the object (via the scoped_ptr) rather than > the view hierarchy owning it. > > Simplicity-wise, instead of creating this object in here, let's do it in the > constructor. Then the code here can just say something like: > > if (match.associated_keyword.get()) { > if (!keyword_icon_->parent()) > AddChildView(keyword_icon_.get()); > } else if keyword_icon_->parent()) { > RemoveChildView(keyword_icon_.get()); > } > > This also eliminates the need for the Invalidate() function. I'm not following how this eliminates the Invalidate function. I believe I still need that function to determine when to switch the ImageView between the normal icon and the selected icon? (I could do it in OnPaint but I don't like copying the image on every paint event, especially during an animation, when it only needs to be done once beforehand.)
Never give me an opportunity to re-review a file, I'll find more nits :P http://codereview.chromium.org/6731036/diff/199006/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/6731036/diff/199006/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:180: if (match.associated_keyword.get()) { On 2012/01/13 00:03:48, aaron.randolph wrote: > On 2012/01/12 22:59:59, Peter Kasting wrote: > > This also eliminates the need for the Invalidate() function. > > I'm not following how this eliminates the Invalidate function. I believe I > still need that function to determine when to switch the ImageView between the > normal icon and the selected icon? You're right, I missed the normal vs. selected part. Somehow the Invalidate() solution feels slightly wrong, like there must be a better place to hook precisely when we're selected or not, but I don't see one. http://codereview.chromium.org/6731036/diff/199006/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:302: model_->IsSelectedIndex(model_index_) ? IDR_OMNIBOX_TTS_SELECTED : Nit: (GetState() == SELECTED) seems fractionally cleaner (although if that's true the GetIcon() should probably make the same change) http://codereview.chromium.org/6731036/diff/199006/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:550: const SkBitmap* kw_icon = GetKeywordIcon(); Nit: Rather than calling GetKeywordIcon() here, it seems like we can just use |keyword_icon_|'s width and height in this function, because the ImageView should auto-size to its image's size (IIRC). That saves a line, and plus, if we ever make that view more than just an image, this code will all still work correctly. http://codereview.chromium.org/6731036/diff/199006/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:563: keyword_icon_->SetBounds(kw_x, (height() - kw_icon->height()) / 2, Nit: This can just be SetPosition().
http://codereview.chromium.org/6731036/diff/199006/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc (right): http://codereview.chromium.org/6731036/diff/199006/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_popup_contents_view.cc:294: model_->selected_line_state() == AutocompletePopupModel::KEYWORD); On 2012/01/12 22:59:59, Peter Kasting wrote: > Don't we need to add "IsSelectedIndex(line) &&" in front of this condition? > Won't this code, as written, cause any keyword-containing line that's > invalidated to flip to the same mode as the selected line? Yes, it's a bug, but not for any line. To flip state, I believe it would require a result that contained two matches with keywords where one had been selected and was in keyword mode, and the other was hovered over. With a single keyword result, if an unselected keyword line gets invalidated, selected line state will be NORMAL and ShowKeyword will be false, which is correct. http://codereview.chromium.org/6731036/diff/199006/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/6731036/diff/199006/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:550: const SkBitmap* kw_icon = GetKeywordIcon(); On 2012/01/13 00:36:41, Peter Kasting wrote: > Nit: Rather than calling GetKeywordIcon() here, it seems like we can just use > |keyword_icon_|'s width and height in this function, because the ImageView > should auto-size to its image's size (IIRC). Actually, it doesn't appear that ImageView sets the view's width and height to the image size, which seems a bit odd to me. It will return the image size if you call GetPreferredSize or GetImageSize or GetImage().width, however.
Uploaded a new patch to address the nits and updated the patch description.
http://codereview.chromium.org/6731036/diff/206001/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete.cc (right): http://codereview.chromium.org/6731036/diff/206001/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete.cc:1010: // finishes loading while processing this request.). We don't check the URL Nit: Extra period after paren http://codereview.chromium.org/6731036/diff/206001/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/6731036/diff/206001/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:551: const int kw_collapsed_size = keyword_icon_->width() + You're right, the ImageView doesn't actually size itself. I guess that means that we really should call keyword_icon_->SizeToPreferredSize() before this line. http://codereview.chromium.org/6731036/diff/206001/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:553: const int max_kw_x = bounds().width() - kw_collapsed_size; Nit: bounds().width() -> width() (3 places)
While testing the changes for the latest nits, I ran into a state where I'm not clear on the expected output. If I type "ama f" the first match is a search result, the second is a history match for amazon with a keyword. If I go to the second match and then hit tab, what should happen? A) Show keyword mode with the "f" populated in the text B) Show empty keyword mode I can see arguments for both but I think (A) presents more problems when trying to handle an immediate SHIFT+TAB due to text existing. Maybe when accepting a keyword, only set the text if the current match is the default or a search what you tuped?
On 2012/01/13 20:23:19, aaron.randolph wrote: > While testing the changes for the latest nits, I ran into a state where I'm not > clear on the expected output. If I type "ama f" the first match is a search > result, the second is a history match for amazon with a keyword. Can you give me some screenshots? I'm not clear on what this looks like. It seems like if the fill_into_edit on the second result has spaces in it (e.g. "amazon.com foo"), then it should not have an associated keyword at all. A match should only have a keyword hint if the match matches the keyword, without any further text. In the fill_into_edit is just "amazon.com", however, then there's no problem, because you can't "go to the second match" without changing the omnibox text to become the fill_into_edit, at which point the "f" that makes this confusing has disappeared.
On 2012/01/13 20:35:56, Peter Kasting wrote: > On 2012/01/13 20:23:19, aaron.randolph wrote: > > While testing the changes for the latest nits, I ran into a state where I'm > not > > clear on the expected output. If I type "ama f" the first match is a search > > result, the second is a history match for amazon with a keyword. > > Can you give me some screenshots? I'm not clear on what this looks like. > > It seems like if the fill_into_edit on the second result has spaces in it (e.g. > "amazon.com foo"), then it should not have an associated keyword at all. A > match should only have a keyword hint if the match matches the keyword, without > any further text. > > In the fill_into_edit is just "amazon.com", however, then there's no problem, > because you can't "go to the second match" without changing the omnibox text to > become the fill_into_edit, at which point the "f" that makes this confusing has > disappeared. That answered my question; I was overthinking it. Thanks. I'll upload the fixes shortly.
On 2012/01/13 20:35:56, Peter Kasting wrote: > It seems like if the fill_into_edit on the second result has spaces in it (e.g. > "amazon.com foo"), then it should not have an associated keyword at all. A > match should only have a keyword hint if the match matches the keyword, without > any further text. > Actually, I'd like to get clarification on this because there may be a bug with the keyword provider. If I paste "amazon.com foo" into the edit, the current behavior is to show a keyword (and associated hint) on the default search match. If I paste the same into the current version of Chrome, a keyword provider match is the second result. (It appears that match is getting suppressed in the patch due to the substituting keyword removal logic, which may be faulty.) I assume showing the keyword match is the desired behavior?
On 2012/01/13 21:52:39, aaron.randolph wrote: > If I paste "amazon.com foo" into the edit, the current > behavior is to show a keyword (and associated hint) on the default search match. > If I paste the same into the current version of Chrome, a keyword provider > match is the second result. We shouldn't be showing a keyword hint for the "Search what you typed" match for "amazon.com foo". The current version of Chrome handles things correctly. It sounds like somewhere we're calling the function that strips everything past the first space where we shouldn't be, and thus matching too greedily against keywords.
One final question (I hope.) If I have "amazon.com fo," I get these matches: 1) search provider match 2) history match, with associated keyword for amazon.com 3) keyword match for amazon.com with substitution area filled in. Considering there is a more specific keyword match in the results, should the history match have an associated keyword, even though it has higher relevance?
Uploaded a new patch that addresses the nits and fixes several bugs.
On 2012/01/14 00:17:28, aaron.randolph wrote: > One final question (I hope.) If I have "amazon.com fo," I get these matches: > > 1) search provider match > 2) history match, with associated keyword for http://amazon.com Is the fill_into_edit on this one "amazon.com" (or thereabouts)? > 3) keyword match for http://amazon.com with substitution area filled in. > > Considering there is a more specific keyword match in the results, should the > history match have an associated keyword, even though it has higher relevance? I don't know. I'm inclined to say the behavior you describe above is fine. I don't have a strong instinct for this. Luckily, it seems like this is likely to be something of an edge case?
http://codereview.chromium.org/6731036/diff/186007/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_edit.cc (right): http://codereview.chromium.org/6731036/diff/186007/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_edit.cc:653: view_->SetWindowTextAndCaretPos(window_text.c_str(), keyword_.length()); This isn't right, because we also need to ensure view_->TextChanged() (or, really, EmphasizeURLComponents()) gets called. I'm thinking about the possibility of fixing this by expanding SetWindowTextAndCaretPos() to optionally call UpdatePopup() and TextChanged() (with an arg to control each). There are two spots in OmniboxViewWin that could then drop their explicit TextChanged() calls (and one would drop its optional UpdatePopup() call). Changing callers in AutocompleteEditModel will be slightly trickier; most callers will not want either of these calls, but the code right here would want TextChanged() (and could drop the OnChanged() below; not sure if setting |is_keyword_hint_| would need to move up or not), and I think Revert() could pass |true| for calling TextChanged(), and then OmniboxViewWin::RevertAll() could drop its TextChanged() call and move the |saved_selection_for_focus_change_| update above the call to Revert(). (We'd need to test to ensure that move doesn't affect anything.) http://codereview.chromium.org/6731036/diff/186007/chrome/browser/ui/views/au... File chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc (right): http://codereview.chromium.org/6731036/diff/186007/chrome/browser/ui/views/au... chrome/browser/ui/views/autocomplete/autocomplete_result_view.cc:127: keyword_icon_->SizeToPreferredSize(); Ah, now I understand why you didn't need to do this call in Layout() -- GetKeywordIcon() will always return an icon of the same size. It's probably OK to rely on this behavior but we should perhaps add a comment in GetKeywordIcon() like: // NOTE: If we ever begin returning icons of varying size, then callers need to ensure that |keyword_icon_| is resized each time its image is reset.
On 2012/01/14 01:27:01, Peter Kasting wrote: > Is the fill_into_edit on this one "amazon.com" (or thereabouts)? > Yes. (I didn't realize the issue tracker automatically converts domains to URLs. At least, I assume that's where the http:// came from.) > I don't know. I'm inclined to say the behavior you describe above is fine. I > don't have a strong instinct for this. Luckily, it seems like this is likely to > be something of an edge case? I believe so, yes.
Uploaded a new patch that adds two flags to SetWindowTextAndCaretPos. http://codereview.chromium.org/6731036/diff/211059/chrome/browser/ui/gtk/omni... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/6731036/diff/211059/chrome/browser/ui/gtk/omni... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:650: TextChanged(); It looks like this could probably be removed and true passed to SetWindowTextAndCaretPos but I can't test it to verify.
I'll launch a try run for this stuff. http://codereview.chromium.org/6731036/diff/211059/chrome/browser/ui/gtk/omni... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/6731036/diff/211059/chrome/browser/ui/gtk/omni... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:650: TextChanged(); On 2012/01/14 04:25:34, aaron.randolph wrote: > It looks like this could probably be removed and true passed to > SetWindowTextAndCaretPos but I can't test it to verify. I looked at this with one of the GTK guys and neither of us are sure either. Since it's not a big deal let's just leave this as-is.
On 2012/01/18 18:41:31, Peter Kasting wrote: > I'll launch a try run for this stuff. > From looking at the try results, it seems it should be submitted instead with patch -p0?
On 2012/01/18 20:53:54, aaron.randolph wrote: > On 2012/01/18 18:41:31, Peter Kasting wrote: > > I'll launch a try run for this stuff. > > > > From looking at the try results, it seems it should be submitted instead with > patch -p0? Yeah, the tryserver instructions page was vague. Resubmitted in a way that will hopefully work better.
Uploaded a new CL that addresses Mac/Linux compile errors (autocomplete_popup_model.h/,cc, autocomplete_result_view.cc) and the failing unit test (history_url_provider_unittest.cc) The sync tests pass on my machine and I can't find a link between them and my CL. Not sure what happened with that try.
For the Linux and Mac tests, I can probably make them pass by removing the SHIFT+TAB clears keyword check. However, I've added comments to the spots I think need to be changed so both that test passes as is, and so TAB, SHIFT+TAB works correctly on those platforms. For the other failures, it looks like things unrelated to this patch? I'm not sure where to go from here. http://codereview.chromium.org/6731036/diff/229001/chrome/browser/ui/cocoa/om... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): http://codereview.chromium.org/6731036/diff/229001/chrome/browser/ui/cocoa/om... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:836: if (model_->is_keyword_hint()) Needs a check for SHIFT to clear keyword or logic to move up/down the results. (See chrome/browser/ui/views/omnibox/omnibox_view_win.cc: 2073-2083) http://codereview.chromium.org/6731036/diff/229001/chrome/browser/ui/cocoa/om... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:844: if (!IsCaretAtEnd()) { This if statement and body need to be removed. http://codereview.chromium.org/6731036/diff/229001/chrome/browser/ui/gtk/omni... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/6731036/diff/229001/chrome/browser/ui/gtk/omni... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1129: !(event->state & (GDK_CONTROL_MASK | GDK_SHIFT_MASK)); Needs SHIFT mask removed so that SHIFT+TAB can clear keyword or move up results. http://codereview.chromium.org/6731036/diff/229001/chrome/browser/ui/gtk/omni... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1724: // Trigger Tab to search behavior only when Tab key is pressed. Needs a check for SHIFT to clear keyword or logic to move up/down the results. (See chrome/browser/ui/views/omnibox/omnibox_view_win.cc: 2073-2083) http://codereview.chromium.org/6731036/diff/229001/chrome/browser/ui/gtk/omni... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1734: if (!handled) { This statement and body should be removed as this functionality is changed with this patch.
On 2012/01/19 23:13:36, aaron.randolph wrote: > For the Linux and Mac tests, I can probably make them pass by removing the > SHIFT+TAB clears keyword check. However, I've added comments to the spots I > think need to be changed so both that test passes as is, and so TAB, SHIFT+TAB > works correctly on those platforms. The only difficulty with this is that if we implement the behavioral change in the address bar, we'll also want to implement the graphical change in the dropdown so these platforms don't look confusingly broken. I talked to a GTK guy on the team who can probably port the graphical parts for GTK, but it seems harder to find someone on Mac. (You don't happen to have a Mac, do you?) Given that, maybe it makes sense to go ahead and make the relevant omnibox_view_gtk.cc changes, and then I'll get the GTK dropdown fixed after you check in, but to leave the Mac behavior unchanged and stick an #if in the test code so we don't test this new behavior on Mac yet? That way at least Mac would be internally consistent though it would no longer match the other platforms. For failures unrelated to this patch I wouldn't worry. We can check in if we're sure they're flakiness or some such.
Updated the CL to add traversal behavior to Mac and Linux, and updated the description to be clearer. I installed Ubuntu in a VM and borrowed a Mac to ensure the behavior works and the tests pass on each system. This was my first time developing on either system, so hopefully the changes I made make sense.
On 2012/01/23 04:48:43, aaron.randolph wrote: > Updated the CL to add traversal behavior to Mac and Linux I'm still concerned about whether it makes sense to change the Mac behavior when we don't have someone lined up to make the relevant changes to the omnibox dropdown on Mac. This didn't seem confusing in your testing? http://codereview.chromium.org/6731036/diff/228007/chrome/browser/ui/gtk/omni... File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right): http://codereview.chromium.org/6731036/diff/228007/chrome/browser/ui/gtk/omni... chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1735: handled = true; Nit: Factor this line down below the conditional since both arms have it. http://codereview.chromium.org/6731036/diff/228007/ui/base/keycodes/keyboard_... File ui/base/keycodes/keyboard_code_conversion_mac.mm (right): http://codereview.chromium.org/6731036/diff/228007/ui/base/keycodes/keyboard_... ui/base/keycodes/keyboard_code_conversion_mac.mm:34: { VKEY_BACKTAB /* 0x0A */, 0x21E4, '\031' }, Where do these values come from? (I noticed for example that some public Qt file uses "kTabCharCode" where you used \031.)
Actually, I didn't feel it was confusing at all, surprisingly. It almost made sense if you think about it in a certain way. Say I type in "foo" and see a keyword hint. I know if I press TAB it will enter keyword mode, because it tells me so. Once in keyword mode, there is no hint on what pressing TAB will do so I expect it to do the default: move to the next focusable input element. In this case, it moves to the second match in the results, which could be argued is the next focusable element. Maybe that's a bit weak, but when I was actually using it it wasn't jarring at all. Additionally, you mentioned not implementing the behavior since there is no one to do the graphics. I don't think that is an option because the new logic is tightly woven into the model now. All the behavior I added does is ensure up/down works like the other clients. When to display hints and how keyword mode is activated would still be based on the new logic, and it would be hard to filter that out for just the Mac, I think. http://codereview.chromium.org/6731036/diff/228007/ui/base/keycodes/keyboard_... File ui/base/keycodes/keyboard_code_conversion_mac.mm (right): http://codereview.chromium.org/6731036/diff/228007/ui/base/keycodes/keyboard_... ui/base/keycodes/keyboard_code_conversion_mac.mm:34: { VKEY_BACKTAB /* 0x0A */, 0x21E4, '\031' }, On 2012/01/24 19:14:44, Peter Kasting wrote: > Where do these values come from? (I noticed for example that some public Qt > file uses "kTabCharCode" where you used \031.) That would make sense. I got the other value from http://www.cocoadev.com/index.pl?AppleSpecificCodePoints. I could not find any apple-defined const for a backtab. When the tests sent a SHIFT+TAB, it wasn't getting converted to a backtab, so I saw this as the only way to go (with my limited knowledge).
On 2012/01/24 21:18:33, aaron.randolph wrote: > Actually, I didn't feel it was confusing at all, surprisingly. > > Additionally, you mentioned not implementing the behavior since there is no one > to do the graphics. I don't think that is an option because the new logic is > tightly woven into the model now. All the behavior I added does is ensure > up/down works like the other clients. When to display hints and how keyword mode > is activated would still be based on the new logic, and it would be hard to > filter that out for just the Mac, I think. OK. Sounds like we better go the route you took then. > http://codereview.chromium.org/6731036/diff/228007/ui/base/keycodes/keyboard_... > File ui/base/keycodes/keyboard_code_conversion_mac.mm (right): > > http://codereview.chromium.org/6731036/diff/228007/ui/base/keycodes/keyboard_... > ui/base/keycodes/keyboard_code_conversion_mac.mm:34: { VKEY_BACKTAB /* 0x0A */, > 0x21E4, '\031' }, > On 2012/01/24 19:14:44, Peter Kasting wrote: > > Where do these values come from? (I noticed for example that some public Qt > > file uses "kTabCharCode" where you used \031.) > > That would make sense. I got the other value from > http://www.cocoadev.com/index.pl?AppleSpecificCodePoints. I could not find any > apple-defined const for a backtab. When the tests sent a SHIFT+TAB, it wasn't > getting converted to a backtab, so I saw this as the only way to go (with my > limited knowledge). I asked one of our Mac folks to glance at this change just in case he has ideas I don't. LGTM once we can get some reasonable-looking try runs.
mac keycode changes look good to me. adding rsesek and thakis to review the cocoa/* changes.
shess is the mac omnibox man On Mac, tab should probably only cycle through all omnibox entries when "full keyboard access" is active in the system preferences.
Updated the CL to the latest revision to address the try failures, and addressed the nits.
On 2012/01/24 22:03:20, Nico wrote: > On Mac, tab should probably only cycle through all omnibox entries when "full > keyboard access" is active in the system preferences. After more testing with Nico, we think on Mac we should eliminate the "tab moves up and down" parts of this change entirely. I think it's more useful to have them, but it's also not at all a system-native behavior. I think we should leave the "backtab out of keyword search" bit in place. That still seems useful. So this just amounts to removing a little bit of the code in omnibox_view_mac.mm.
Updated the CL to remove up/down movement from the Mac version.
On 2012/01/24 22:55:27, aaron.randolph wrote: > Updated the CL to remove up/down movement from the Mac version. It doesn't look like this forced us to change any tests. Can we write a test that verifies tab/shift-tab traversing the dropdown on Linux/Windows?
On 2012/01/24 22:58:54, Peter Kasting wrote: > It doesn't look like this forced us to change any tests. Can we write a test > that verifies tab/shift-tab traversing the dropdown on Linux/Windows? Will do. Considering the functionality is explicitly being removed from Mac, should the test be defined out when run on the Mac or should it be marked as DISABLED?
On 2012/01/24 23:16:03, aaron.randolph wrote: > On 2012/01/24 22:58:54, Peter Kasting wrote: > > It doesn't look like this forced us to change any tests. Can we write a test > > that verifies tab/shift-tab traversing the dropdown on Linux/Windows? > > Will do. Considering the functionality is explicitly being removed from Mac, > should the test be defined out when run on the Mac or should it be marked as > DISABLED? I would do #if !defined(OS_MAC) or whatever the precise directive is, along with a comment about Mac intentionally not supporting this.
Updated the CL with a test for tab traversal and fixed the Mac failure.
Thanks for the test! LGTM, I'll kick off another try iteration tomorrow. http://codereview.chromium.org/6731036/diff/261098/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right): http://codereview.chromium.org/6731036/diff/261098/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:1079: size_t size = popup_model->result().size(); Nit: Optional form: for (size_t size = popup_model->result().size(); popup_model->selected_line() < (size - 1); old_selected_line = popup_model->selected_line()) { ... I personally like this because it scopes |size| to the loop. Up to you. (Variants usable below as well)
On 2012/01/25 05:53:05, Peter Kasting wrote: > Nit: Optional form: I'll change this later today. I may also expand the tests slightly.
Updated the traversal test to be more comprehensive.
Updated the revision to address the try failures. (Not sure why Authors was failing to merge. I had resolved the conflicts before uploading.)
Looks like there's an error in the linux_rel run on the latest version of the patch.
On 2012/01/27 17:26:21, Peter Kasting wrote: > Looks like there's an error in the linux_rel run on the latest version of the > patch. Could we run that try again and see if it was a fluke? The test "works on my machine," and the failing check is a bit weird.
On 2012/01/27 21:41:12, aaron.randolph wrote:
> On 2012/01/27 17:26:21, Peter Kasting wrote:
> > Looks like there's an error in the linux_rel run on the latest version of
the
> > patch.
>
> Could we run that try again and see if it was a fluke? The test "works on my
> machine," and the failing check is a bit weird.
Are you looking at the same check I am? The failure is that at line 1132, when
we've just hit tab again after tabbing into keyword mode, the "selected line" is
-1 ("no selection") instead of 1. This seems like a clear indicator that for
some reason in this case tab is changing focus. Maybe the IsOpen() call on
omnibox_view_gtk.cc line 1730 is failing for some reason?
(BTW, the reason I'm pushing back here is that even if the failure is flaky, I don't want to add a new test that's flaky; we should try and find and fix the source of the flakiness.)
On 2012/01/27 21:53:11, Peter Kasting wrote: > (BTW, the reason I'm pushing back here is that even if the failure is flaky, I > don't want to add a new test that's flaky; we should try and find and fix the > source of the flakiness.) Oh, I understand, and it's my intention to find a way for it to not be flaky. However, I was also assuming that it was losing focus and I've been trying various things to replicate that failure without much success. So I was curious if it was repeatable on the trybot consistently, and then maybe I could see what's different between it and my machine.
I asked the linux_rel bot to run again and also asked the linux bot (i.e. the debug bot) to try as well.
On 2012/01/27 22:24:30, Peter Kasting wrote: > I asked the linux_rel bot to run again and also asked the linux bot (i.e. the > debug bot) to try as well. Thanks! I updated the CL to the latest revision. Could you try it again, please?
Well, looks like it's consistent. However, it's interesting that NonSubstitutingKeywordTest also failed because the popup closed.
I was able to reproduce the failure after switching to the same window manager the trybots use. After a bit of digging, it appears the browser gets in a state where it keeps moving position, which then causes the popup to close. I've uploaded a fix for this. This update should also help with the fail we saw with NonSubstitutingKeywordTest. Additionally, PasteReplacingAll (http://crbug.com/80934) and DeleteItem (http://crbug.com/84420) may also be helped by this update. (They were failing on my machine until I made this change. Now they pass.) Changes are in omnibox_view_browsertest.cc, and in_process_browser_test.h/cc. I added a new function because other methods were either too early or too late for what I needed.
1) this cr is amazing 2) the OnBeforeShowBrowser() is probably used correctly; it should also be used widely in our gtk browser tests, but I am explicitly not holding this up on that because I can't think of an easy way to do that.
Changes LGTM. Filed and assigned bug 112041 regarding more detailed investigation of this function. I kicked off another try round. I also updated the change description to reference the test flakiness fix. Your finding this particular source of flakiness and fixing it just adds another notch in the "reasons Peter is really impressed with this CL and its author" column.
FYI, PopupAccelerators fails on Ubuntu Unity but passes on icewm. A gtk widget reference count check fails in owned_widget::Destroy. I looked into that while I was in there but didn't see an obvious place where a second reference was being added. (I am not a Linux dev, barely a user, so hopefully that makes sense.) On 2012/01/31 01:54:06, Peter Kasting wrote: > Your finding this particular source of flakiness and fixing it just adds another > notch in the "reasons Peter is really impressed with this CL and its author" > column. Ha, thanks! Just remember that if my resume ever crosses your desk. ;)
On 2012/01/31 02:57:27, aaron.randolph wrote: > FYI, PopupAccelerators fails on Ubuntu Unity but passes on icewm. A gtk widget > reference count check fails in owned_widget::Destroy. I looked into that while > I was in there but didn't see an obvious place where a second reference was > being added. (I am not a Linux dev, barely a user, so hopefully that makes > sense.) This is not your fault and is a sort of known issue. Gtk has a system where either the user or the desktop environment can load arbitrary bundles of code and override all sorts of stuff like categories in Objective-C on OSX. I personally want to get rid of the OwnedWidget CHECK because it is noisy and now that Unity is apparently the future and Ubuntu is using gtk module injection extensively, will never be right. Maybe I can convince the other linux devs with this. (crbug.com/54860) Also, I continue to be amazed.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aaron.randolph@gmail.com/6731036/227112
Presubmit check for 6731036-227112 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc,ui/base/keycodes/keyboard_codes_posix.h,ui/base/keycodes/keyboard_code_conversion_mac.mm,chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.h,chrome/browser/ui/cocoa/omnibox/omnibox_popup_view_mac.mm,chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm,chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h,chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc Presubmit checks took 6.2s to calculate.
erg, can you officially OWNERS LGTM for chrome/browser/ui/gtk? sail/rsesk, can one of you OWNERS LGTM chrome/browser/ui/cocoa? sky, can you OWNERS LGTM ui/base?
Rubber stamp LGTM
cocoa/ LGTM http://codereview.chromium.org/6731036/diff/227112/chrome/browser/ui/cocoa/om... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): http://codereview.chromium.org/6731036/diff/227112/chrome/browser/ui/cocoa/om... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:818: if (model_->popup_model()->selected_line_state() == nit: You can merge this with the previous if() using a &&
gtk lgtm
Aaron, if you want to get rsesek's last, trivial nit and resync to trunk, I'll check the commit box again. You only have a couple hours, though, before I'm gone until Friday.
On 2012/01/31 20:21:38, Peter Kasting wrote: > Aaron, if you want to get rsesek's last, trivial nit and resync to trunk, I'll > check the commit box again. You only have a couple hours, though, before I'm > gone until Friday. On it.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aaron.randolph@gmail.com/6731036/285005
Uploaded a fix for the mac compile failure.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aaron.randolph@gmail.com/6731036/282045
Try job failure for 6731036-282045 (retry) on linux_rel for step "ui_tests". It's a second try, previously, step "ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aaron.randolph@gmail.com/6731036/282045
Change committed as 120005 |
