Part of bug 951624. Searching via the current search engine is one of the possible actions of typing into the urlbar. currently that's not exposed anywhere in the UI, so is very opaque as to what's going to actually happen. So we want to add a heuristic and a new result type for when this will happen.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9cbff5ddc732 Note what you may think is an unexpected first match in test_tabmatches.js for "abc.com" - we get a searchengine result there, which becomes the expected visiturl result when the patch from bug 1067899 is applied.

Am investigating the test failure that showed up on that above Try run.

Fixed failed test from Try run. Seems I'd accidentally left in a decodeURI() in autocomplete.xml - test coverage now covers that because the code is better exercised with this patch.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=db1bcafbf03

Sorry for the churn - I missed a test failure as I thought it was in another bug. Only difference here is the one-line fix in browser_bug1003461-switchtab-override.js Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f828a23ba6f2

Comment on attachment 8492160 [details] [diff] [review] Patch v1.2 Review of attachment 8492160 [details] [diff] [review]: ----------------------------------------------------------------- The try run is still failing in the override test... there is nothing blocking on the coding side (apart the double notifyResults call that should be clarified) but you should figure out the purpose param with gavin and the favicon with ux. ::: browser/base/content/test/general/browser_action_searchengine.js @@ +1,4 @@ > +/** > + * Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ > + **/ nit: This header is wrong, the right one is 2 lines, please check https://www.mozilla.org/MPL/headers/ (PD is there as well) @@ +2,5 @@ > + * Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ > + **/ > + > + let gOriginalEngine; please fix indentation @@ +4,5 @@ > + **/ > + > + let gOriginalEngine; > + > +add_task(function* () { please check unified pref @@ +26,5 @@ > + return promiseClearHistory(); > + }); > + > + info("Wait for autocomplete") > + let searchDeferred = Promise.defer(); nit: we are starting seeing this waitForAutocomplete code path in more tests, maybe should be evaluated for head.js ::: browser/base/content/test/general/browser_autocomplete_a11y_label.js @@ +54,5 @@ > > + ok(gURLBar.popup.richlistbox.children.length > 1, "Should get at least 2 results"); > + let result = gURLBar.popup.richlistbox.children[1]; > + is(result.getAttribute("type"), "action switchtab", "Expect right type attribute"); > + is(result.label, expectedLabel, "Result a11y label should be as expected"); should this be wrapped by unified complete pref check? ::: browser/base/content/test/general/browser_bug1003461-switchtab-override.js @@ +42,1 @@ > EventUtils.synthesizeKey("VK_DOWN" , {}); ditto ::: browser/base/content/urlbarBindings.xml @@ +322,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(action.params.searchQuery); based on bug 1063530, I guess if here we should use the keyword purpose... I think you should try to ask Gavin about the purpose to use here. @@ +1028,5 @@ > break; > } > + case "searchengine": { > + let engine = Services.search.getEngineByName(action.params.engineName); > + let submission = engine.getSubmission(action.params.searchQuery); ditto ::: browser/locales/en-US/chrome/browser/places/places.properties @@ +92,5 @@ > switchtabResultLabel=Tab > +# LOCALIZATION NOTE (searchengineResultLabel) : > +# Noun used to describe the location bar autocomplete result type > +# to users with screen readers > +# See createResultLabel() in urlbarBindings.xml Ad I said in the other bug, I think doesn't make much sense to repeat the same localization note for each entry, it just makes this unreadable. I'd just put one note at the top. ::: browser/themes/linux/browser.css @@ +1508,5 @@ > padding: 0 3px; > } > > +richlistitem[type~="action"][actiontype="searchengine"] > .ac-title-box > .ac-site-icon { > + list-style-image: url("chrome://browser/skin/Search.png"); are we using the magnifier instead of the search engine icon? I wonder if we should rather use the search engine icon and append autocomplete-search.svg as we are doing for past searches, it would be more consistent. And more generally, I'm not sure if it's still sane to use Search.png vs autocomplete-search.svg please clarify with ux? The same is valid for all of the themes. ::: browser/themes/windows/browser.css @@ +1464,5 @@ > +} > + > +richlistitem[type~="action"][actiontype="searchengine"] > .ac-title-box > .ac-extra > .ac-comment { > + box-shadow: inset 0 0 1px 1px rgba(208,208,208,0.5); > + background-color: rgba(208,208,208,0.3); please check this has no issues with hi-contrast theme. ::: toolkit/components/places/UnifiedComplete.js @@ +866,5 @@ > + finalCompleteValue: this._trimmedOriginalSearchString, > + frecency: FRECENCY_SEARCHENGINES_DEFAULT, > + }); > + > + this.notifyResults(true); why is this needed? doesn't addMatch do it for you? if it's really really needed, then I'd prefer if addMatch would have an optional argument to enforce it internally. ::: toolkit/components/places/tests/unifiedcomplete/test_actions.js @@ +1,4 @@ > +/** > + * Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ > + **/ ditto on header @@ +6,5 @@ > +add_task(function* test_actions() { > + /* The following actions are already tested elsewhere: > + * switchtab - test_tabmatches.js > + * keyword - test_keyword_search_actions.js > + */ what about making this test specific for the search action then (by renaming it)?

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #6) > The try run is still failing in the override test... Bah, fixed the test in the wrong patch. > nit: we are starting seeing this waitForAutocomplete code path in more > tests, maybe should be evaluated for head.js Would you believe I already did this in bug 1066358 and forgot to update this patch to use it? :) > ::: browser/base/content/test/general/browser_autocomplete_a11y_label.js > should this be wrapped by unified complete pref check? The entire test is already gated on that. > based on bug 1063530, I guess if here we should use the keyword purpose... I > think you should try to ask Gavin about the purpose to use here. Oh, didn't know of that bug - thanks for mentioning it. Turns out it's stuck on figuring out some extra details, so I've filed bug 1071361 to handle integrating the two later. > are we using the magnifier instead of the search engine icon? > I wonder if we should rather use the search engine icon and append > autocomplete-search.svg as we are doing for past searches, it would be more > consistent. > And more generally, I'm not sure if it's still sane to use Search.png vs > autocomplete-search.svg Oh, autocomplete-search.svg is new to me. That landed sometime after I did this styling. Talked with phlsa about all this: * No search engine icon for this result, to distinguish it from other search results * But we do want an icon when we're matching on an alias in bug 1067896, to help with recognition * Change the string to be "QUERY — Search with ENGINE" > > +richlistitem[type~="action"][actiontype="searchengine"] > .ac-title-box > .ac-extra > .ac-comment { > > + box-shadow: inset 0 0 1px 1px rgba(208,208,208,0.5); > > + background-color: rgba(208,208,208,0.3); > > please check this has no issues with hi-contrast theme. It better have no issues - it's the emphasis styling we already use. But, turns out this is leftover from a previous version of the patch anyway - I'm not putting anything in ac-extra anymore. > > + this.notifyResults(true); > > why is this needed? doesn't addMatch do it for you? Oh, yes, indeed.

And apparently backed out: https://hg.mozilla.org/integration/fx-team/rev/be50c9825144

Try came up clean after a test fix (I messed up during a last-second change), so: https://hg.mozilla.org/integration/fx-team/rev/dcd7ac495365

Reverted for failures in browser_action_searchengine.js: https://tbpl.mozilla.org/php/getParsedLog.php?id=48761222&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=48768492&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=48778888&tree=Fx-Team remote: https://hg.mozilla.org/integration/fx-team/rev/16eb452e62ba

Testing performed on Nightly 35.0a1 (2014-09-28) using Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 LTS 32-bit. Acceptance criteria for this patch: - A "<keyword> - Search with <engine>" entry is displayed for a specific search keyword typed in the awesomebar. - The "<keyword> - Search with <engine>" also includes the display of a loop icon. Potential issues: 1. [All platforms] "<keyword> - Search with Google" is displayed despite using a different search engine. * note: confirmed this with multiple search engines and also checked the 'browser.search.defaultenginename' pref * screenshot: http://i.imgur.com/qOL2mnc.png 2. [All platforms] The search keyword is missing from "- Search with <engine>" if the suggestions pane has been dismissed before pressing the <enter> key. * screenshot: http://i.imgur.com/lziGDnW.png 3. [All platforms] The "<keyword> - Search with <engine>" entry is not displayed if the keyword is typed in a private window. * screenshot: http://i.imgur.com/qsYohcm.png Blair, what's your take on these?

(In reply to Andrei Vaida, QA [:avaida] from comment #14) > Blair, what's your take on these? 3 is bug 1075450. Could you file bugs for the others?

(In reply to Blair McBride [:Unfocused] from comment #15) > (In reply to Andrei Vaida, QA [:avaida] from comment #14) > > Blair, what's your take on these? > > 3 is bug 1075450. Could you file bugs for the others? Filed Bug 1075532 and Bug 1075549. Since these issues will be tracked separately, I'm marking this one as verified. Testing details for this patch are available in Comment 14.