I removed the old searchbar UI in bug 1119250, but left most of the UI Tour pieces there, because I'm less familiar with this code, and didn't want to block the large search code removal on figuring out how to clean up UI Tour. There's also some dead test code in browser/components/uitour/test/browser_UITour.js.

Comment on attachment 8659229 [details] [diff] [review] Patch Review of attachment 8659229 [details] [diff] [review]: ----------------------------------------------------------------- I thought we made the switch to Yahoo with the new UI and that's when we used this code? ::: browser/components/uitour/UITour.jsm @@ -2078,5 @@ > > - // We only allow matching based on a search engine's identifier - this gives > - // us a non-changing ID and guarentees we only match against app-provided > - // engines. > - getSearchEngineTarget(aWindow, aIdentifier) { If you're getting rid of the ability to target a search engine then you need to also have getConfiguration("availableTargets"); reflect that. Please remove getAvailableSearchEngineTargets and its reference otherwise the method would be lying IIUC. The downside is that I think the page will no longer know the available search engines to set as the default but we should add an API on getConfiguration for that specifically then.

(In reply to Matthew N. [:MattN] from comment #2) > I thought we made the switch to Yahoo with the new UI and that's when we > used this code? This code is touching the old UI (which I removed), not the new one. I think this was used when we introduced DDG in Firefox 33.1 (it was written in bug 1068284 which was uplifted to 33). The UITour code for the new UI is the code from bug 1101999.

(In reply to Florian Quèze [:florian] [:flo] from comment #3) > This code is touching the old UI (which I removed), not the new one. I think > this was used when we introduced DDG in Firefox 33.1 (it was written in bug > 1068284 which was uplifted to 33). The UITour code for the new UI is the > code from bug 1101999. As far as I understand this is correct. Fwiw, the particular tour page that was shown for 33.1 is no longer in production and was removed from bedrock in these two commits: https://github.com/mozilla/bedrock/pull/3173 https://github.com/mozilla/bedrock/pull/2923

Commits pushed to master at https://github.com/mozilla/bedrock https://github.com/mozilla/bedrock/commit/3c1b1fd9051d81cd9a1831506a540b81a825438e [bug 1202372] Remove references to old searchbar UI from UITour docs https://github.com/mozilla/bedrock/commit/a4b2c4ff268bfc455d0fe2e666ce7d472af798f0 Merge pull request #3311 from alexgibson/update-uitour-docs [bug 1202372] Remove references to old searchbar UI from UITour docs

So Alex, do you think it's fine that we remove the search engine names from availableTargets? Florian is adding a new option to getConfiguration to get this information. I kinda would rather remove this ability if we aren't planning on using it but I suspect it's possible there will be more search engine changes as there is focus on that area still.

Comment on attachment 8660657 [details] [diff] [review] Patch v2 Review of attachment 8660657 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. I want to look again after this change though and we'll see what Alex says about availableTargets ::: browser/components/uitour/UITour.jsm @@ +1823,5 @@ > break; > case "availableTargets": > this.getAvailableTargets(aMessageManager, aWindow, aCallbackID); > break; > + case "availableSearchEngines": IMO this should simply be "search" like we do for "loop". Then it can also include a "searchEngineIdentifier" property (for backwards compatibility) like "selectedSearchEngine" returns and we can alias that to the new API. I would have had "selectedSearchEngine" implemented as "search" in the first place but I wasn't flagged for review IIRC. I'm trying to keep the getConfiguration options general so we don't need to keep adding new supported arguments and because these APIs are async so it's better if a tour page depends on fewer requests to keep the page responsive since some of the APIs calls are needed before displaying content. Note to please keep the cases alphabetical when you move this to "search".

(In reply to Matthew N. [:MattN] (behind on mail) from comment #6) > So Alex, do you think it's fine that we remove the search engine names from > availableTargets? Florian is adding a new option to getConfiguration to get > this information. I kinda would rather remove this ability if we aren't > planning on using it but I suspect it's possible there will be more search > engine changes as there is focus on that area still. We already have `selectedSearchEngine` as an option for getConfiguration, which was added when we did the Yahoo! /whatsnew page for the new search UI (this does only return the search engine that's selected as default however). Having a list of all search engines returned could be useful in the future, but I imagine the primary use case would be to have them available as highlight targets, since we can already get/set the default with existing functions. So maybe this new function is not quite as useful? For now I would say it's probably best to leave adding/changing functionality, until we have a real use case for something we need to achieve?

(In reply to Alex Gibson [:agibson] from comment #8) > Having a list of all search engines returned could be useful in > the future, but I imagine the primary use case would be to have them > available as highlight targets, since we can already get/set the default > with existing functions. The list of available engine identifiers is for use with the set default function. Without such a list, you can't really be sure the engine you want to set as the default exists. The list of default engines differs based on the locale, and browser version.

(In reply to Florian Quèze [:florian] [:flo] from comment #9) > (In reply to Alex Gibson [:agibson] from comment #8) > > > Having a list of all search engines returned could be useful in > > the future, but I imagine the primary use case would be to have them > > available as highlight targets, since we can already get/set the default > > with existing functions. > > The list of available engine identifiers is for use with the set default > function. > Without such a list, you can't really be sure the engine you want to set as > the default exists. The list of default engines differs based on the locale, > and browser version. This is a good point. We've only used this functionality in a situation where we knew a particular search engine was available (e.g. Yahoo for en-US), but in the future this may well not be the case.

Comment on attachment 8662379 [details] [diff] [review] Patch v3 Review of attachment 8662379 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!