After bug 530209 lands, tagged results will have both a tag icon and a star, even if bookmark behavior is off. We want to always show the tags (if any) for history results, and show the star only if the result is a bookmark and browser.urlbar.suggest.bookmark is true.

Marco: In bug 530209, you mentioned an intention to separate out the concepts of tags and bookmarks. Is there a bug for that?

This relies on the changes to head.js made in bug 1073339.

(In reply to Blair McBride [:Unfocused] from comment #1) > Marco: In bug 530209, you mentioned an intention to separate out the > concepts of tags and bookmarks. Is there a bug for that? bug 424160.

Updated the comment to mention the right bug (thanks Marco).

Comment on attachment 8527971 [details] [diff] [review] Patch v1.1 Review of attachment 8527971 [details] [diff] [review]: ----------------------------------------------------------------- r+ assuming the test passes with the change below, and also assuming Marco has already verified this is the correct approach. ::: browser/base/content/test/general/browser_autocomplete_tag_star_visibility.js @@ +1,4 @@ > +add_task(function*() { > + // This test is only relevant if UnifiedComplete is enabled. > + if (!Services.prefs.getBoolPref("browser.urlbar.unifiedcomplete")) > + return; nit: bracing single-line blocks is the style guideline for new code. @@ +25,5 @@ > + info("Test with suggest.bookmark=false"); > + Services.prefs.setBoolPref("browser.urlbar.suggest.bookmark", true); > + yield promiseAutocompleteResultPopup("tagtest"); > + result = gURLBar.popup.richlistbox.children[1]; > + is_element_visible(result._typeImage, "Type image should be hidden"); The preference should set to false and the check should be is_element_hidden.

Comment on attachment 8527971 [details] [diff] [review] Patch v1.1 Review of attachment 8527971 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/widgets/autocomplete.xml @@ +1694,5 @@ > + // If we're suggesting bookmarks, then treat tagged matches as > + // bookmarks for the star. > + // This should become less of a hack once the concepts of bookmarks > + // and tags are separated out in bug 424160. > + if (Services.prefs.getBoolPref("browser.urlbar.suggest.bookmark")) wouldn't this throw for anything that is not Firefox and thus doesn't have the above pref? I guess it might break stuff. You should at least try/catch it. Honestly I'm a little bit sad that we need to do this in autocomplete.xml, I was hoping we could do this from the search provider by setting "type" differently...

This ended up being a bit more involved, but I do prefer it.

Comment on attachment 8544498 [details] [diff] [review] Patch v2 Review of attachment 8544498 [details] [diff] [review]: ----------------------------------------------------------------- The production code looks good to me. Given how much the test cases in the file are similar to each other, I'd define those declaratively, using this pattern: let testcases = [ { suggestBookmarks: true, expectedType: "bookmark-tag", }, { suggestBookmarks: false, expectedType: "tag", }, // ...etc... { suggestBookmarks: false, expectedType: "tag", restrictionToken: "*", }, // ...etc... ]; let tagSequenceNumber = 1; for (let testcase of testcases) { info("Test case: " + JSON.serialize(testcase)); // ...add tag here, no need for separate function... // ...rest of the test... if ("restrictionToken" in testcase) { // ...you can use optional testcase parameters... } // ...no need to declare anything arbitrary in the testcase, // we can generate the value... tagSequenceNumber++; } This would likely make the test more maintainable in case something unrelated to what we're testing changes in the future (like how the panel is opened and closed). I also like the JSON serialization as a more precise description of the test case than an arbitrary text description.

Marco: I just fixed this up and pushed before realising you had needinfo'ed yourself - did you want to comment on something here? (In reply to :Paolo Amadini from comment #8) > Given how much the test cases in the file are similar to each other, I'd > define those declaratively, using this pattern: Oh, yes, good thought. > I also like the JSON serialization as a more precise description of the test > case than an arbitrary text description. However, I'm generally opposed to this in any test. It removes a simple description of that part of the test in the output, and removes a unique string that can be searched for in the source code, while adding noise that's generally only useful in situations when you need to look at the source code anyway. https://hg.mozilla.org/integration/fx-team/rev/c5feceed953a

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #9) > > I also like the JSON serialization as a more precise description of the test > > case than an arbitrary text description. > > However, I'm generally opposed to this in any test. It removes a simple > description of that part of the test in the output, and removes a unique > string that can be searched for in the source code, while adding noise > that's generally only useful in situations when you need to look at the > source code anyway. Different philosophies, I'm more for a minimalistic approach but this one is fine as well! Readable descriptions are less resilient to cut-and-paste, in fact I've seen descriptions checked in the tree not matching what the test does, on the other hand you might argue that when they don't match they're a clue that something is wrong.

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=1618236&repo=fx-team

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #9) > Marco: I just fixed this up and pushed before realising you had needinfo'ed > yourself - did you want to comment on something here? No I just wanted to remember to look at the change sometimes... there's no better way to create todo lists in bugzilla...

(In reply to Carsten Book [:Tomcat] from comment #11) > sorry had to back this out for test failures like > https://treeherder.mozilla.org/logviewer.html#?job_id=1618236&repo=fx-team this is due to the test being skipped and not having a todo(false, "Stop supporting old autocomplete components."); like the other skipped tests

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #12) > No I just wanted to remember to look at the change sometimes... there's no > better way to create todo lists in bugzilla... Ah, yes, I do the same - makes me realise how ambiguous it may look. (In reply to Marco Bonardo [::mak] (needinfo? me) from comment #13) > this is due to the test being skipped and not having a > todo(false, "Stop supporting old autocomplete components."); > like the other skipped tests *facepalm* I'm not liking that we're having all these UnifiedComplete-dependant tests that aren't run on the CI. So I've modified this to force-enable UnifiedComplete, and reset it when done. And I've filed bug 1119621. https://hg.mozilla.org/integration/fx-team/rev/44fb93810fc2

Verified fixed on Aurora 37.0a2 (2015-01-19) and Nightly 38.0a1 (2015-01-18), using Ubuntu 12.04 32-bit, Mac OS X 10.9.5 and Windows 8 32-bit.