1202372 - remove the UITour dead code related to the old searchbar UI.

Status: RESOLVED FIXED

(Firefox :: Tours, defect)

Description

[Florian Quèze [:florian]]

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 1

[Florian Quèze [:florian]]

Attachment: Patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7126f7928389

Comment 2

[Matthew N. [:MattN]]

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.

Comment 3

[Florian Quèze [:florian]]

Attachment: Patch v2

(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.

Comment 4

[Alex Gibson [:agibson]]

(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

Comment 5

[[github robot]]

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

Comment 6

[Matthew N. [:MattN]]

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 7

[Matthew N. [:MattN]]

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".

Comment 8

[Alex Gibson [:agibson]]

(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?

Comment 9

[Florian Quèze [:florian]]

(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.

Comment 10

[Alex Gibson [:agibson]]

(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 11

[Florian Quèze [:florian]]

Attachment: Patch v3

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8662379 [details] [diff] [review]
Patch v3

Review of attachment 8662379 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/815dc4143c96

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/815dc4143c96