The awesomebar list should always include one item at the top that is selected (highlighted) while typing and shows action that is going to happen when pressing enter. This will not only make the behavior of the awesomebar more transparent, it will also help people discover that you can actually search from that bar as well.

Wouldn't this break the common "Type+Down+Enter" URL entering behavior? Or would you just suggest we ignore the first "Down" in that case? This is also currently not possible because the first entry in the popup doesn't always match the inline autocompleted value. How would we address that problem?

(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #1) > Wouldn't this break the common "Type+Down+Enter" URL entering behavior? Or > would you just suggest we ignore the first "Down" in that case? > > This is also currently not possible because the first entry in the popup > doesn't always match the inline autocompleted value. How would we address > that problem? It is specifically designed not to break that behavior. We are adding one new item at the beginning of the list, but since it is always selected (while typing), pushing the down arrow will still get you to the (now second) item you'd expect. Right now, the action that happens from just typing and pressing enter is just not visible at all, which is especially bad since most of the time that invisible action is searching. This is the problem we're trying to fix here.

Interactive spec: http://phlsa.github.io/ff-search-prompt

Marco: Have been told this is top priority, so picking this up for iteration now I'm back from conferences.

Added to Iteration 33.3

Marco: I have vague recollections of this being related to your plan for unified autocomplete. Am I crazy?

Oh, yes, this builds on top of unified autocomplete, and is thus dependant on it.

Work in progress. Functionality is there: * First item is always selected (almost, see below) * Detect URL * Detect keyword * Use search engine Together with bug 693808, it's now clear what will actually happen when you type something like "mozilla" into the URL bar \o/ Still to do: * Finish making it non-ugly * Fix edge case in always having an item selected * Tests

Did some of this land in some other bug? The first result now seems to sometimes show and select a previously typed entry when typing then hitting enter quickly.

Nope, I'm not aware of any related changes that have landed recently.

Sorry for these side comments, but I suppose it's relevant as if you didn't expect these changes, someone else is landing something similar already?. The top is nightly clean profile where "g" shows google.com as the first result.

(In reply to Ed Lee :Mardak from comment #12) > Created attachment 8463230 [details] > "g" on nightly vs aurora > > Sorry for these side comments, but I suppose it's relevant as if you didn't > expect these changes, someone else is landing something similar already?. > The top is nightly clean profile where "g" shows google.com as the first > result. You should ask Marco Bonardo to have further details, but this change is simply due to the fact that the unified autocomplete has been enabled on Nightly recently (bug 1040335). I've also noticed that the search providers now appear in the awesomebar.

(In reply to Guillaume C. [:ge3k0s] from comment #13) > You should ask Marco Bonardo to have further details, but this change is > simply due to the fact that the unified autocomplete has been enabled on > Nightly recently (bug 1040335). I've also noticed that the search providers > now appear in the awesomebar. Yeah, more specifically this is a new feature, Search Provider Top Suggestion, from bug 958188.

Oh, duh, yes. What comment 13 and comment 14 said. This bug is actually working on top of all that.

Ok, so, turns out there was significantly more to do that I thought earlier. But it's now interacting much nicer with all the new items, and cooperating with autofill and other recent additions (eg, bug 1034381). Still some work to do, namely: * Cleanup recent work * Marked todo items * Copy theme work over to osx/linux * More test coverage But, putting this up to get some feedback from Marco (pretty please).

Comment on attachment 8467721 [details] [diff] [review] WiP v2 Review of attachment 8467721 [details] [diff] [review]: ----------------------------------------------------------------- Some questions about the behavior: 1. if we autoFill a search engine (like I type "goo" and google.com is the first result coming from the searchProvider) do we select the first popup entry? If we don't select it we might be breaking habits (we insert a fake autoFill entry pushing down what was the top entry before) 2. if we don't autoFill and the input is not a host or a url, what happens? do we select the first entry? if so, is the first entry special? if we select the top entry but it's not special, down+enter would break habits by going to the second entry. To sum up, in any case where we set defaultIndex we should preselect it, same with actions. In the other cases we should select the top entry only if it's a "special" made up entry, or we will break habits. Just by looking at the patch it's not clear what happens, I think adding some general comments on the expected behavior would help following the code. It looks like a good approach. ::: browser/base/content/urlbarBindings.xml @@ +155,5 @@ > + } else if (action.type == "searchengine") { > + returnValue = action.params.input; > + } else if (action.type == "visiturl") { > + returnValue = action.params.url; > + } nit: might use a switch with coalesced cases @@ +297,5 @@ > + } else if (action.type == "keyword") { > + url = action.params.url; > + } else if (action.type == "searchengine") { > + let engine = Services.search.getEngineByName(action.params.engineName); > + let submission = engine.getSubmission(actin.params.searchQuery); typo: actin @@ +931,5 @@ > + <parameter name="aIndex"/> > + <parameter name="aMaxRow"/> > + <body><![CDATA[ > + if (aMaxRow < 0) > + return -1; this conflicts with "this will now never return -1" comment @@ +936,5 @@ > + > + var newIdx = aIndex + (aReverse?-1:1)*aAmount; > + if (aReverse && aIndex == -1 || newIdx > aMaxRow && aIndex != aMaxRow) > + newIdx = aMaxRow; > + else if (!aReverse && aIndex == -1 || newIdx < 0 && aIndex != 0) please add parenthesis to make the precedence explicit @@ +944,5 @@ > + aIndex = aMaxRow; > + else if (newIdx > aMaxRow && aIndex == aMaxRow) > + aIndex = 0; > + else > + aIndex = newIdx; I suspect some of this code might be simplified using a modulo operator... but I'm not sure what's the expected behavior, should this loop continuously? Fwiw I was trying this code that should be looping properly, but looks like you want a different behavior here? A comment would help figuring ohw the code should behave. if (aReverse && aIndex == -1) aIndex = aMaxRow; else if (!aReverse && aIndex == -1) aIndex = 0; aIndex = Math.abs( aIndex + (aReverse ? -1 : 1) * aAmount ) % (aMaxRow + 1); ::: toolkit/components/places/UnifiedComplete.js @@ +651,5 @@ > let queries = [ this._adaptiveQuery, > this._switchToTabQuery, > this._searchQuery ]; > > + let initialResultTypes = new Set(); why is this a set? I'm not sure I get the purpose of this, you seem to only check its size... @@ +661,5 @@ > } > > + if (this._enableActions && this._searchTokens.length > 0) { > + // If it's not a bookmarked keyword, then it may be a search engine > + // with an alias - which works like a keyword. should it be an else if? if it's a keyword looks like we still don't need to do this... @@ +695,5 @@ > + if (this.pending && this._searchTokens.length == 1 && > + initialResultTypes.size == 0) { > + // This is added as a frecency match, and therefore isn't guaranteed to be > + // the first match in the final result - and thus not an "initial" result. > + yield this._matchSearchEngineUrl(); if it's the first result we autoFill it... what differs from other results? shouldn't it still be selected? @@ +775,5 @@ > + yield this._addSearchEngineMatch(engine, query); > + return true; > + }, > + > + _matchCurrentSearchEngine: function* () { this one looks unused

Hey, if you want a preliminary ui-review here, could you upload a build somewhere? Given that there's been some questions around the behavior, that might be good thing to do. Applying the patch fails for me.

Blair, do you have an updated/rebased WIP patch? We're trying to put together some demos for the Firefox Townhall next week, and this might contribute to a "search" demo (even if it's unpolished).

Not one that currently works :\ I'm having to rewrite a section due to other changes. When do you want it by?

No worries - I think we won't end up needing to demo this given the other stuff we have, so no rush. I am interested in trying a build next week if you have one, though!

(In reply to Marco Bonardo [:mak] (Away 15-31 Aug) from comment #19) > Comment on attachment 8467721 [details] [diff] [review] > WiP v2 > > Review of attachment 8467721 [details] [diff] [review]: > ----------------------------------------------------------------- > > Some questions about the behavior: > 1. if we autoFill a search engine (like I type "goo" and google.com is the > first result coming from the searchProvider) do we select the first popup > entry? If we don't select it we might be breaking habits (we insert a fake > autoFill entry pushing down what was the top entry before) > 2. if we don't autoFill and the input is not a host or a url, what happens? > do we select the first entry? if so, is the first entry special? if we > select the top entry but it's not special, down+enter would break habits by > going to the second entry. With this, the first entry is guaranteed to be special - so we don't break down+enter. We go through a list of possible special cases in the following order: (1) keywords (this._keywordQuery) (2) search engine by alias (3) search engine domains (4) inline completion (this._hostQuery or this._urlQuery) (5) plain url (ie, can be navigated to as-is) (6) current search engine Only one of these can be in the results list. (6) is a catch-all and will always match if none of the others do. > I suspect some of this code might be simplified using a modulo operator... > but I'm not sure what's the expected behavior, should this loop continuously? > > Fwiw I was trying this code that should be looping properly, but looks like > you want a different behavior here? A comment would help figuring ohw the > code should behave. That method is copied from autocomplete.xml - with only a minor modification (added a comment to this effect). So I'm wary about deviating unnecessarily from the original. > why is this a set? I'm not sure I get the purpose of this, you seem to only > check its size... Please assume that's a boolean :) I had made it a Set temporarily to help me debug.

Comment on attachment 8471521 [details] [diff] [review] Patch v1 And a Try build for Philipp: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-b140f2ba8ddf

*sigh* And then I upload a patch that's missing files. New Try build: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-edb5da36bc12/

(In reply to Blair McBride [:Unfocused] from comment #24) > > I suspect some of this code might be simplified using a modulo operator... > > but I'm not sure what's the expected behavior, should this loop continuously? > > > > Fwiw I was trying this code that should be looping properly, but looks like > > you want a different behavior here? A comment would help figuring ohw the > > code should behave. > > That method is copied from autocomplete.xml - with only a minor modification > (added a comment to this effect). So I'm wary about deviating unnecessarily > from the original. But this is already a modification of the original and off-hand I can't tell if it's acting as expected, cause I don't know what's expected. I did some testing the this code you have was not giving me the results I was expecting (unfortunately I didn't write down those cases). Thus my question.

This works for me. What works is that the first guess is selected. And I like the more distinctive background of the selected choice. And because the first is selected, a single tab gets to the second, which is often the first remembered URL containing the guessed domain, or containing the string if it doesn't match a remembered domain name. (In the broken state, the first guess would be visited by enter as if it were selected, but tab would then select the first guess again to be visited). (I use tab - easier to reach - but down arrow works the same way). Thanks.

By the way, IMHO, this is much better than Chrome, which essentially forces you to navigate through a google search to get the the remembered url.

I've noticed a few issue while testing the try build. Firstly the awesomebar frequently shows a scrollbar even if not necessary. Sometimes the awesome even shows an item partially (note this would be nice to get rid of this scrollbar in all cases, but that's a different bug). The second issue is more annoying. STR : 1. type "face" in the awesomebar. 2. Awesomebar autocompletes to facebook.com 3. Hit backspace to have "face" again in the awesomebar. CR : the first entry is still visit facebook.com ER : the first entry change to search google for "face"

The mockup also shows a behaviour that is closer to what Google Chrome does. I.e. if domain.net is the first entry, the second entry is search for domain.net an then there are all the other entries.

(In reply to Guillaume C. [:ge3k0s] from comment #32) > The mockup also shows a behaviour that is closer to what Google Chrome does. > I.e. if domain.net is the first entry, the second entry is search for > domain.net an then there are all the other entries. The second entry in many cases for us will be an adaptive result, it really depends on browsing habits. in a new profile might do that since there's no data.

(In reply to Guillaume C. [:ge3k0s] from comment #31) > The second issue is more annoying. > STR : > 1. type "face" in the awesomebar. > 2. Awesomebar autocompletes to facebook.com > 3. Hit backspace to have "face" again in the awesomebar. > > CR : the first entry is still visit facebook.com > > ER : the first entry change to search google for "face" I agree, this should be fixed

Comment on attachment 8471529 [details] [diff] [review] Patch v1.1 Review of attachment 8471529 [details] [diff] [review]: ----------------------------------------------------------------- It looks quite good, not yet r+ cause I still have some questions and I think another pass won't hurt. We're really close to ship it! I should be able to do that last pass before 16, but in case I should be away already, paolo, mano or enn could likely help. I think you might want to add a test (or modify an existing test) for the docshell changes. And you should try to fix the bug reported in comment 31 (test welcome!) Some things I don't like very much: 1. not all code paths go through _addMatch 2. Raw use of Services.search in UnifiedComplete, I think it should be abstracted away by PlacesSearchAutocompleteProvider 3. added dependency of toolkit/content on Places ::: browser/base/content/urlbarBindings.xml @@ +939,5 @@ > + <body><![CDATA[ > + if (aMaxRow < 0) > + return -1; > + > + var newIdx = aIndex + (aReverse?-1:1)*aAmount; nit: let @@ +1013,5 @@ > > // Check if this is meant to be an action > let action = this.mInput._parseActionUrl(url); > if (action) { > + //XXXunf Should centralise this. no XXX comments, please. if it's planned this should be a // TODO (bug 123456): do something. Btw, looks like the 2 instances of this are for different purposes and return different stuff (for example keyword), so I'm not sure the code can be shared. @@ +1056,2 @@ > label += " " + this._bundle.GetStringFromName(aType + "ResultLabel"); > + } catch (e) {} just for added clarity: } catch (e) { // Undefined result label, do nothing. } ::: browser/locales/en-US/chrome/browser/places/places.properties @@ +87,5 @@ > bookmarkResultLabel=Bookmark > +# LOCALIZATION NOTE (searchengineResultLabel) : > +# Noun used to describe the location bar autocomplete result type > +# to users with screen readers > +# See createResultLabel() in urlbarBindings.xml while here could you please fix the trailing space across all of the "See createResultLabel() in urlbarBindings.xml" rows (both old and new). ::: browser/themes/linux/browser.css @@ +1502,5 @@ > +richlistitem[type~="action"][actiontype="searchengine"] > .ac-title-box > .ac-site-icon { > + list-style-image: url("chrome://browser/skin/Search.png"); > +} > + > +@media (min-resolution: 2dppx) { do we support hi dpi in the linux theme? I can't find any 2dppx rule in it. The rule looks unneeded for now. ::: browser/themes/linux/jar.mn @@ +69,5 @@ > skin/classic/browser/privatebrowsing-mask.png > skin/classic/browser/reload-stop-go.png > skin/classic/browser/searchbar.css > + skin/classic/browser/Search.png (../shared/Search.png) > + skin/classic/browser/Search@2x.png (../shared/Search@2x.png) ditto ::: browser/themes/osx/browser.css @@ +2116,5 @@ > -moz-image-region: rect(22px, 32px, 44px, 0); > } > + > + .autocomplete-richlistbox[actiontype="searchengine"] .ac-site-icon { > + list-style-image: url("chrome://browser/skin/Search@2x.png"); what is this rule covering? ::: browser/themes/windows/browser.css @@ +1456,5 @@ > +richlistitem[type~="action"][actiontype="searchengine"] > .ac-title-box > .ac-site-icon { > + list-style-image: url("chrome://browser/skin/Search.png"); > +} > + > +@media (min-resolution: 2dppx) { on windows usually we uses 1.25dppx.. even if we are not really doing hi-dpi yet (bug 1023600) http://mxr.mozilla.org/mozilla-central/search?string=min-resolution&find=windows ::: docshell/base/nsDefaultURIFixup.cpp @@ +384,5 @@ > info->mFixupCreatedAlternateURI = MakeAlternateURI(info->mFixedURI); > } > > + // If there is no relavent dot in the host, do we require the domain to > + // be whitelisted? please get review on the docshell changes from smaug or bz. Gijs recently worked on this so he might be a good target for a feedback? too. @@ +389,5 @@ > + if (info->mFixedURI) { > + nsAutoCString asciiHost; > + if (NS_SUCCEEDED(info->mFixedURI->GetAsciiHost(asciiHost)) && > + !asciiHost.IsEmpty()) { > + brace on newline and no empty line (see this code module coding style) @@ +390,5 @@ > + nsAutoCString asciiHost; > + if (NS_SUCCEEDED(info->mFixedURI->GetAsciiHost(asciiHost)) && > + !asciiHost.IsEmpty()) { > + > + uint32_t dotLoc = uint32_t(asciiHost.FindChar('.')); nit: I wonder if RFindChar might be faster on average, depends on asciiHost form (I'd expect something like domain.com) @@ +995,5 @@ > } > } > } > > +bool nsDefaultURIFixup::IsDomainWhitelisted(nsDefaultURIFixupInfo* aFixupInfo) { brace on newline @@ +1002,5 @@ > + if (aFixupInfo->mFixedURI && > + NS_SUCCEEDED(aFixupInfo->mFixedURI->GetAsciiHost(asciiHost)) && > + !asciiHost.IsEmpty()) { > + > + // Check if this domain is whitelisted as an actual this code module has if braces on a new line (likely cause indentation clashes with the if conditions), I think you should respect that, thus remove the empty newline. see the other ifs in the surrounding. @@ +1008,5 @@ > + // NB: any processing of the host here should stay in sync with > + // code in the front-end(s) that set the pref. > + nsAutoCString pref("browser.fixup.domainwhitelist."); > + > + uint32_t dotLoc = uint32_t(asciiHost.FindChar('.')); ditto on RFindChar ::: toolkit/components/places/UnifiedComplete.js @@ +642,5 @@ > // wait for the initialization of PlacesSearchAutocompleteProvider first. > yield PlacesSearchAutocompleteProvider.ensureInitialized(); > > + // For any given search, we run many queries/heuristics: > + // 1) keywords (this._keywordQuery) do we really run this query before anything else? It doesn't look like... If we'd run it as first query this would be a reason to r-. autoFill must be able to kick-in as soon as possible cause it fights with the user input, anything before it should not cause IO. To me looks like inline query still runs before keyword here: we unshift keyword to the queries array as the first entry, but we executeCached inline first. This list should respect the actual *execution* order, not the order we manage stuff in code. @@ +660,5 @@ > + // enabled, the first result is always a special result (resulting from one > + // of the queries between (1) and (6) inclusive). As such, the UI is > + // expected to auto-select the first result when actions are enabled. If the > + // first result is an inline completion result, that will also be the > + // default result and therefore be autofilled. but will inline result be selected by default even if actions are disabled? it should be, but the comment is unclear about that. @@ +667,5 @@ > let queries = [ this._adaptiveQuery, > this._switchToTabQuery, > this._searchQuery ]; > > + let hasFirstResult = false; please comment about how it's supposed to be used, just above it, for future reference. @@ +706,5 @@ > // to avoid overloading the connection. > let [ query, params ] = this._hostQuery; > + yield conn.executeCached(query, params, row => { > + hasFirstResult = true; > + this._onResultRow(row); now that these 2 paths do the same, I wonder if we should include all of the if/if/else madness into _shouldautofill, make it instead return the query to use or null, and here just have: let autofillQuery = this._autofillQuery; if (this.pending && autofillQuery) { let [ query, params ] = autofillQuery; yield conn.executeCached(query, params, row => { hasFirstResult = true; this._onResultRow(row); }); } @@ +774,5 @@ > + }, > + > + _matchAliasedSearchEngine: function* () { > + if (this._searchTokens.length == 0) > + return false; this check is already done outside, and you are not adding similar protection to _matchSearchEngineUrl. IF you think this protection might be useful, should be consistent in all methods expecting a given tokens size. @@ +776,5 @@ > + _matchAliasedSearchEngine: function* () { > + if (this._searchTokens.length == 0) > + return false; > + > + let engine = Services.search.getEngineByAlias(this._searchTokens[0]); I'd prefer if PlacesSearchAutocompleteProvider would form a layer between autocomplete and Services.search... would it be possible to move the search matching code to new methods there? For example it might have a buildMatchForAlias(alias, searchTerms) that builds the match. @@ +787,5 @@ > + return true; > + }, > + > + _matchCurrentSearchEngine: function* () { > + let engine = Services.search.currentEngine; ditto for PlacesSearchAutocompleteProvider, could provide a buildMatchForToken @@ +797,5 @@ > + > + yield this._addSearchEngineMatch(engine, query); > + }, > + > + _addSearchEngineMatch: function* (engine, query) { and I'd prefer if these would run through the common _addMatch... would that be complicated? I'd prefer to keep all additions in a single method. @@ +844,5 @@ > + url: uri.spec, > + input: this._originalSearchString, > + }); > + > + this._result.appendMatch(value, again, would it be possible to go through the common _addMatch? @@ +857,5 @@ > + }, > + > + _makeActionURL: function (action, params) { > + let url = "moz-action:" + action + "," + JSON.stringify(params); > + return NetUtil.newURI(url).spec; doesn't need to be a method, please make into an helper function at the top of the file (there are other helpers there already). ::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js @@ +281,5 @@ > + let url = "moz-action:" + action + "," + JSON.stringify(params); > + return NetUtil.newURI(url); > +} > + > +let gMockSearchService = null; so, I appreciate all of the effort put here, though, is a mock service really needed? I was guessing we might rather use the official search service, but add our own special test engine to it, indeed we also have a bug filed cause tests are failing in comm-central (that has the search service but doesn't have google, amazon and such). My fear is that mock service might be more expensive to maintain in future, than just adding fake engines to search service. ::: toolkit/components/places/tests/unifiedcomplete/test_actions.js @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +add_task(function* test_actions() { > + /* The following actionsd are already tested elsewhere: typo: actionsd ::: toolkit/content/widgets/autocomplete.xml @@ +1093,2 @@ > return; > + } what about break instead of return and later if (this._currentIndex < matchCount) { // yield after each batch of items so that typing the url bar is responsive setTimeout(function (self) { self._appendCurrentResult(); }, 0, this); } so there one single call to onResultsAdded. @@ +1269,5 @@ > + if (panel.createResultLabel) { > + let types = new Set(this.getAttribute("type").split(" ")); > + types.delete("action"); > + > + return panel.createResultLabel(title, url, [...types].join("")); I don't understand why the need to create a Set to just make an array out of it... let types = [t for each (t in this.getAttribute("type").split(" ")) if (t != "action")]; @@ +1422,5 @@ > + <body> > + <![CDATA[ > + // Get rid of all previous text > + while (aDescriptionElement.hasChildNodes()) > + aDescriptionElement.removeChild(aDescriptionElement.firstChild); nit: or aDescriptionElement.firstChild.remove(); @@ +1488,5 @@ > // keyword searches, however, the title ellipsis should not take up > // space when hidden. Setting the hidden property accomplishes that. > this._titleOverflowEllipsis.hidden = false; > > + let types = new Set(type.split(/\s+/)); please be consistent, you splitted on " " above, make them both use either the regex or the space. @@ +1505,5 @@ > + > + let sourceStr = "]]>&action.searchEngine.label;<![CDATA["; > + title = this._generateEmphasisPairs(sourceStr, [ > + [action.params.engineName, false], > + [action.params.searchQuery, true] nit: indent by 2 from ( @@ +1513,5 @@ > + url = action.params.url; > + > + let sourceStr = "]]>&action.visitURL.label;<![CDATA["; > + title = this._generateEmphasisPairs(sourceStr, [ > + [trimURL(url), true], ditto @@ +1516,5 @@ > + title = this._generateEmphasisPairs(sourceStr, [ > + [trimURL(url), true], > + ]); > + > + let favicons = Components.classes["@mozilla.org/browser/favicon-service;1"] ugh, this should be #ifdef MOZ_PLACES at a minimum... I'm not really happy to add a Places dependency on toolkit/content... Why can't we pass an icon out of UnifiedComplete.js? @@ +1523,5 @@ > + .getService(Components.interfaces.nsIIOService2); > + let uri = ios.newURI(url, null, null); > + favicons.getFaviconURLForPage(uri, faviconURI => { > + if (faviconURI) > + this.setAttribute("image", faviconURI.spec); should probably use getFaviconLinkForIcon so we read the cached one... @@ +1540,3 @@ > const TITLE_SEARCH_ENGINE_SEPARATOR = " \u00B7\u2013\u00B7 "; > > + [title, url] = title.split(TITLE_SEARCH_ENGINE_SEPARATOR); just as a side note, with paolo we were wondering whether instead of using a dumb separator in titles we could make parsed search results be actions, and pass the required info in the action json object. @@ +1558,5 @@ > + > + types.delete("autofill"); > + } > + > + type = [...types].join(" "); you sure to join on " "? later we seem to compare it with unspaced string values @@ +1620,5 @@ > + this._setUpEmphasisedSections(this._title, title); > + else > + this._setUpDescription(this._title, title, !emphasiseTitle); > + > + this._setUpDescription(this._url, url, !emphasiseUrl); I don't understand the double _setUpDescription if we hit the else case, is not the second one overwriting the first one?

Was looking to try this out, but it looks like this was bitrotten by bug 1051830.

Smaug, would you mind looking at the docshell changes?

Comment on attachment 8474475 [details] [diff] [review] Patch v2 (In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #36) > Was looking to try this out, but it looks like this was bitrotten by bug > 1051830. Yea, suffering a bit with bitrot on this one. Try the latest patch. Also, a build will turn up in: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/bmcbride@mozilla.com-ea76f5db9263/

(In reply to Marco Bonardo [:mak] (Away 15-31 Aug) from comment #35) > And you should try to fix the bug reported in comment 31 (test welcome!) I don't see a way to handle that with the current autocomplete API, so I've filed bug 1054914. > ::: browser/base/content/urlbarBindings.xml > @@ +1013,5 @@ > > > > // Check if this is meant to be an action > > let action = this.mInput._parseActionUrl(url); > > if (action) { > > + //XXXunf Should centralise this. > > Btw, looks like the 2 instances of this are for different purposes and > return different stuff (for example keyword), so I'm not sure the code can > be shared. Not so much shared, but centralized. At the moment the implementation for actions is spread around a few places in urlbarBindings.xml - making it more difficult to work with than it should be. Filed bug 1054816 for this one. > do we support hi dpi in the linux theme? I can't find any 2dppx rule in it. > The rule looks unneeded for now. We do support HiDPI on Linux, but our theme doesn't make use of it. I've been adding HiDPI support on Linux for everything I touch, but so far everything has been in /shared/. The more we can do now, the less work it will be later. > ::: docshell/base/nsDefaultURIFixup.cpp > nit: I wonder if RFindChar might be faster on average, depends on asciiHost > form (I'd expect something like domain.com) No, that'd give us the wrong result in cases like "domain.com." - we want to know if there's a dot that isn't at the end. (See bug 1042519) > > + // For any given search, we run many queries/heuristics: > > + // 1) keywords (this._keywordQuery) > > do we really run this query before anything else? It doesn't look like... > If we'd run it as first query this would be a reason to r-. autoFill must be > able to kick-in as soon as possible cause it fights with the user input, > anything before it should not cause IO. > > To me looks like inline query still runs before keyword here: we unshift > keyword to the queries array as the first entry, but we executeCached inline > first. This list should respect the actual *execution* order, not the order > we manage stuff in code. Ah, yes. Clarified in comment. > but will inline result be selected by default even if actions are disabled? > it should be, but the comment is unclear about that. Yes. Clarified in comment. > > const TITLE_SEARCH_ENGINE_SEPARATOR = " \u00B7\u2013\u00B7 "; > > > > + [title, url] = title.split(TITLE_SEARCH_ENGINE_SEPARATOR); > > just as a side note, with paolo we were wondering whether instead of using a > dumb separator in titles we could make parsed search results be actions, and > pass the required info in the action json object. I'd been thinking the same :) Filed bug 1054800. > @@ +1620,5 @@ > > + this._setUpEmphasisedSections(this._title, title); > > + else > > + this._setUpDescription(this._title, title, !emphasiseTitle); > > + > > + this._setUpDescription(this._url, url, !emphasiseUrl); > > I don't understand the double _setUpDescription if we hit the else case, is > not the second one overwriting the first one? Er, no? First is for the title, second is for the URL. Or am I misunderstanding you?

Comment on attachment 8474475 [details] [diff] [review] Patch v2 I've started looking into the patch, but there are a lot of things to consider and the review will likely be finished tomorrow or the day after. Would you prefer for me to post comments while I look at various portions, or do you prefer one single list at the end? The CSS changes look good to me, but I'm not testing the patch on all platforms, so I assume you have tested them already or they will be tested as part of the UI-review.

Comment on attachment 8474475 [details] [diff] [review] Patch v2 Very nice! There are a few styling and wording changes I'd like to make, but those are material for a separate bug. Only one reason for the r-: Please use the white version of the magnifying glass when the row is selected. Let me know if you don't yet have an asset for that.

Comment on attachment 8474475 [details] [diff] [review] Patch v2 > if (info->mFixedURI && aFixupFlags & FIXUP_FLAGS_MAKE_ALTERNATE_URI) { > info->mFixupCreatedAlternateURI = MakeAlternateURI(info->mFixedURI); > } > >+ // If there is no relavent dot in the host, do we require the domain to relevant >+ // be whitelisted? >+ if (info->mFixedURI) >+ { >+ nsAutoCString asciiHost; >+ if (NS_SUCCEEDED(info->mFixedURI->GetAsciiHost(asciiHost)) && >+ !asciiHost.IsEmpty()) >+ { { should be in the previous line. Same also elsewhere. (I know the code isn't consistent about that, but let's use the right coding style for new code) >+ uint32_t dotLoc = uint32_t(asciiHost.FindChar('.')); >+ >+ if ((dotLoc == uint32_t(kNotFound) || dotLoc == asciiHost.Length() - 1) && >+ aFixupFlags & FIXUP_FLAG_REQUIRE_WHITELISTED_HOST) >+ { >+ if (IsDomainWhitelisted(info)) So IsDomainWhitelisted will do another info->mFixedURI->GetAsciiHost. Could we avoid that. >+ { >+ info->mPreferredURI = info->mFixedURI; >+ } >+ } >+ else >+ { >+ info->mPreferredURI = info->mFixedURI; >+ } >+ >+ return NS_OK; >+ } You look for '.' even if you might not use that information for anything in case FIXUP_FLAG_REQUIRE_WHITELISTED_HOST isn't set. And why you need to have asciiHost in order to set mPreferredURI. So, perhaps check first aFixupFlags & FIXUP_FLAG_REQUIRE_WHITELISTED_HOST and then do the other stuff. > > let testcases = [ >- ["http://www.mozilla.org", "http://www.mozilla.org/", null, false, false], >- ["http://127.0.0.1/", "http://127.0.0.1/", null, false, false], >- ["file:///foo/bar", "file:///foo/bar", null, false, false], >- ["://www.mozilla.org", "http://www.mozilla.org/", null, false, true], >- ["www.mozilla.org", "http://www.mozilla.org/", null, false, true], >- ["http://mozilla/", "http://mozilla/", "http://www.mozilla.com/", false, false], >- ["http://test./", "http://test./", "http://www.test./", false, false], >- ["127.0.0.1", "http://127.0.0.1/", null, false, true], >- ["1234", "http://1234/", "http://www.1234.com/", true, true], >- ["host/foo.txt", "http://host/foo.txt", "http://www.host.com/foo.txt", false, true], >- ["mozilla", "http://mozilla/", "http://www.mozilla.com/", true, true], >- ["test.", "http://test./", "http://www.test./", true, true], >- [".test", "http://.test/", "http://www..test/", true, true], >- ["mozilla is amazing", null, null, true, true], >- ["", null, null, true, true], >- ["[]", null, null, true, true] >+ ["http://www.mozilla.org", "http://www.mozilla.org/", null, false, false, false, false], >+ ["http://127.0.0.1/", "http://127.0.0.1/", null, false, false, false, false], >+ ["file:///foo/bar", "file:///foo/bar", null, false, false, false, false], >+ ["://www.mozilla.org", "http://www.mozilla.org/", null, false, true, false, false], >+ ["www.mozilla.org", "http://www.mozilla.org/", null, false, true, false, false], >+ ["http://mozilla/", "http://mozilla/", "http://www.mozilla.com/", false, false, false, false], >+ ["http://test./", "http://test./", "http://www.test./", false, false, false, false], >+ ["127.0.0.1", "http://127.0.0.1/", null, false, true, false, false], >+ ["1234", "http://1234/", "http://www.1234.com/", true, true, true, false], >+ ["host/foo.txt", "http://host/foo.txt", "http://www.host.com/foo.txt", false, true, true, false], >+ ["mozilla", "http://mozilla/", "http://www.mozilla.com/", true, true, true, false], >+ ["test.", "http://test./", "http://www.test./", true, true, true, false], >+ [".test", "http://.test/", "http://www..test/", true, true, false, false], >+ ["mozilla is amazing", null, null, true, true, false, false], >+ ["", null, null, true, true, false, false], >+ ["[]", null, null, true, true, false, false], >+ ["http://whitelisted/", "http://whitelisted/", "http://www.whitelisted.com/", false, false, true, true], > ]; This is getting hard to understand. Could you at least document before 'let testcases' what each item in these arrays mean.

Comment on attachment 8474475 [details] [diff] [review] Patch v2 Review of attachment 8474475 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/test/unit/test_nsDefaultURIFixup_info.js @@ +76,5 @@ > > function run_test() { > for (let [testInput, expectedFixedURI, alternativeURI, > + expectKeywordLookup, expectProtocolChange, affectedByWhitelist, > + inWhitelist] of testcases) { To follow-up on Olli's comment, I believe using an object destructuring assignment and updating the test data accordingly would make this test more readable. You can also omit some items from the test data (the variables will contain "undefined").

One of the main concerns I found while reviewing this patch is that, apparently, we're skewing the FHR counters for searches initiated by the location bar: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDefaultURIFixup.cpp#466 As the comment mentions, the counter increase shouldn't be done there in the first place. Now that bug 693808 introduced the getFixupURIInfo method, we should probably use it to return the engine name, and audit the callers of createFixupURI so that they increase the counters where needed. I recommend doing this as a separate dependent bug, before this one lands, so that we can verify it separately. Also, can you please explain the purpose of the changes to the fixup whitelist? Can they be separated into their own bug that can land earlier?

Blair, the more I look into this, the more I think we should break this bug down into smaller chunks. I tested the patch and it has a buggy behavior, especially with backspacing, but where exactly things are going wrong is hard to tell. Type "t:" to see that it incorrectly says "Visit http://www.google.com/search?q=t:...", and when deleting characters, occasionally it displays "Visit moz-action:...". This bug should have been something apparent in the code review, but it wasn't, as the code is becoming too much complicated. You filed one related backspacing bug as a follow-up, since you said we need to modify the autocomplete API for that. However, since we know we're going to need it anyways before releasing, maybe we should figure out which new features we need in the API first, rather than rushing a half-complete solution that we already know requires more work. Heh, this bug is marked "hard" for a reason :-( Smaller patches will also be easier for me to review while Marco is away. We should also feel free to investigate how to modify/refactor the autocomplete API as we see fit at this point. Reasonably, major changes will not land anyways before the work week in September, but we can already work on them. Related, we're also planning visual design and styling improvements for all of our autocomplete popups. I've already posted a heads up on dev.platform that this refactoring is happening, so the SeaMonkey and Thunderbird teams will likely fork autocomplete code and won't need to adapt to each individual change (unless they choose to keep in sync, with their own resources). In other words, we can do any change as long as it does not break mozilla-central's TBPL tests.

Comment on attachment 8474475 [details] [diff] [review] Patch v2 Canceling review request while we figure out how to proceed here.

(In reply to :Paolo Amadini from comment #43) > To follow-up on Olli's comment, I believe using an object destructuring > assignment and updating the test data accordingly would make this test more > readable. Oh, yes! That vastly improves it, thanks. (In reply to :Paolo Amadini from comment #44) > I recommend doing this as a separate dependent bug, before this one lands, > so that we can verify it separately. Filed bug 1057166. > Also, can you please explain the purpose of the changes to the fixup > whitelist? It's so we can fixup a URL for a visitsite action result, without getting back a keyword search - which we want in a different type of result. > Can they be separated into their own bug that can land earlier? Filed bug 1057186.

(In reply to :Paolo Amadini from comment #45) > Blair, the more I look into this, the more I think we should break this bug > down into smaller chunks. Yea, this ended up being a lot bigger and complex than expected. I originally had it separated out, but combined everything when I found that separation was causing more problems than it was solving. I think now I should be able to separate it out again now that everything is there, I've refactored it to be more modular, and I better understand the bigger picture. > I tested the patch and it has a buggy behavior, especially with backspacing, > but where exactly things are going wrong is hard to tell. Type "t:" to see > that it incorrectly says "Visit http://www.google.com/search?q=t:...", Oh, bah. Non-Windows bug. I'm on Windows, which interprets "t:" as a file path - other OSes won't. > and > when deleting characters, occasionally it displays "Visit moz-action:...". > This bug should have been something apparent in the code review, but it > wasn't, as the code is becoming too much complicated. Huh. I think this is exposing an existing bug that just wasn't easy to reproduce previously. > You filed one related backspacing bug as a follow-up, since you said we need > to modify the autocomplete API for that. However, since we know we're going > to need it anyways before releasing, maybe we should figure out which new > features we need in the API first, rather than rushing a half-complete > solution that we already know requires more work. Fair enough. > Heh, this bug is marked "hard" for a reason :-( Glad it's not just me then... > In other words, we > can do any change as long as it does not break mozilla-central's TBPL tests. Oh, this is most excellent news!

(In reply to Blair McBride [:Unfocused] from comment #47) > > Also, can you please explain the purpose of the changes to the fixup > > whitelist? > > It's so we can fixup a URL for a visitsite action result, without getting > back a keyword search - which we want in a different type of result. I see how this can be useful in general to offer both a search and a navigation item. My question is, if getFixupURIInfo with the existing flags returned a keyword search, wouldn't we currently ignore and not display the navigation item anyways? In other words, wouldn't it be enough to call getFixupURIInfo with the current flags and style the result according to the type of result returned?

(In reply to :Paolo Amadini from comment #49) > My question is, if getFixupURIInfo with the existing flags returned a > keyword search, wouldn't we currently ignore and not display the navigation > item anyways? In other words, wouldn't it be enough to call getFixupURIInfo > with the current flags and style the result according to the type of result > returned? We couldn't use it - a keyword search can contain POST data, which we can't pass through autocomplete. And we'd still need to reach into the search service to get the name of the engine used, and still fixup the input string separately for display (we could do both of those, but it's inefficient). I think bug 1057531 might help consolidate this in the future.

As an affected user, I notice that this bug has become entangled with a discussion of the whole implementation in this area. BUT - the initial subject effects users in a rather direct way. We learn habits, sequences of key presses that get repetitive tasks done. There is a web site I visit, which, given my history, is reached by ctrl-l, ds, tab (or is it tab tab!), enter. You see the problem here. With no knowledge of the total implementation story, I ask: Can a (possibly temporary) fix for the subject issue be provided, with a new bug for the overhaul of the whole system?

(In reply to Marc Auslander from comment #51) > As an affected user, I notice that this bug has become entangled with a > discussion of the whole implementation in this area. > > BUT - the initial subject effects users in a rather direct way. We learn > habits, sequences of key presses that get repetitive tasks done. There is a > web site I visit, which, given my history, is reached by ctrl-l, ds, tab (or > is it tab tab!), enter. You see the problem here. > > With no knowledge of the total implementation story, I ask: > > Can a (possibly temporary) fix for the subject issue be provided, with a new > bug for the overhaul of the whole system? I'm not sure if I understand you correctly, but if this is about changing keyboard habits: This feature shouldn't make you change your habits at all (see comment #2)

Comment 2 says, if I understand correctly, that fixing this bug preserves the current behavior in which the first tab or down arrow takes you to the second item on the list - the first now being identical to the contents of the url bar. But today, in nightly, that is NOT the case, since the first item is a copy of the url bar but is not selected. So comment 2 says that fixing this bug preserves key press behavior. But the bug doesn't appear to be on the road to being fixed quickly, And it appears to this outsider that the issues delaying it are quite global and hard. Thus the request for a temporary fix for the specific issue, and a new bug for the larger issues.

(In reply to Marc Auslander from comment #53) > Comment 2 says, if I understand correctly, that fixing this bug preserves > the current behavior in which the first tab or down arrow takes you to the > second item on the list - the first now being identical to the contents of > the url bar. But today, in nightly, that is NOT the case, since the first > item is a copy of the url bar but is not selected. > > So comment 2 says that fixing this bug preserves key press behavior. > > But the bug doesn't appear to be on the road to being fixed quickly, And it > appears to this outsider that the issues delaying it are quite global and > hard. Thus the request for a temporary fix for the specific issue, and a > new bug for the larger issues. Ah, I see. There's bug 1045985 about fixing that.

Blair, we've been assuming this is the fix we want for bug 1045985, but I've not been able to getting around to testing this for various reasons. Bug 1045985 is tracking 34+, and it seems to me that given the dependencies here, we're quite unlikely to get this in to 34. I think we need to explore other options there. Does that match your understanding?

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #55) > Bug 1045985 is tracking 34+, and it seems to me that given the dependencies > here, we're quite unlikely to get this in to 34. I think we need to explore > other options there. Does that match your understanding? Yes, that sounds right.

Documenting in one place for reference (mostly mak's benefit), the dependent bugs apply in the following order: Bug 1065303 - Prepare autocomplete.xml/UnifiedComplete for adding new special result types and heuristics. Bug 1066358 - Improve how keyword autocomplete results are displayed Bug 1067888 - Add autocomplete result type for searching via current search engine Bug 1067896 - Add autocomplete result type for searching via alias of a search engine Bug 1067899 - Add autocomplete result type for arbitrary URLs Bug 1067903 - Autoselect first autocomplete result when it's guaranteed to be a special result

Ehrm, should this be INVALID/WONTFIX? No patch landed for this bug, it instead being broken out into separate bugs?

This breaks the "Enter Selects" extension (https://addons.mozilla.org/en-US/firefox/addon/enter-selects/). Now, hitting enter will always run this new entry instead of the original entry (the one that you press down to get).

Thanks Trev, I probably won't update Enter Selects until these related bugs have settled. For example, bug 1067903 might make it so "down enter" goes to the adaptive result (unless the adaptive result is the same as the special result).

(In reply to Justin Dolske [:Dolske] from comment #58) > Ehrm, should this be INVALID/WONTFIX? No patch landed for this bug, it > instead being broken out into separate bugs? it's a breakdown bug...