1292174 - Add Search Tile for Default Search Engine

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[Stephen Horlander [:shorlander]]

Because of the layout and UI differences between the awesome bar and the search bar results we should add the default search engine to the one-off footers list. 

Example from Activity Stream: https://cl.ly/3r1A1o2L342x

Comment 1

[Drew Willcoxon :adw]

Attachment: Bug 1292174 - Add Search Tile for Default Search Engine in urlbar.

Review commit: https://reviewboard.mozilla.org/r/69386/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69386/

Comment 2

[Drew Willcoxon :adw]

Pretty simple fix...

The one-off's contextmenu handler already checks whether the context engine is the current engine and disables the "set as current engine" menu item if so, so this patch doesn't even need to do that, nice.

Comment 3

[Florian Quèze [:florian]]

Comment on attachment 8778010 [details]
Bug 1292174 - Add Search Tile for Default Search Engine in urlbar.

https://reviewboard.mozilla.org/r/69386/#review66702

::: browser/components/search/content/search.xml:1233
(Diff revision 1)
>          <getter><![CDATA[
>            return this.getAttribute("compact") == "true";
>          ]]></getter>
>        </property>
>  
> +      <property name="includeCurrentEngine">

I don't think we need a getter/setter for this.

::: browser/components/search/content/search.xml:1428
(Diff revision 1)
>            let Preferences =
>              Cu.import("resource://gre/modules/Preferences.jsm", {}).Preferences;
>            let pref = Preferences.get("browser.search.hiddenOneOffs");
>            let hiddenList = pref ? pref.split(",") : [];
>  
>            let currentEngineName = Services.search.currentEngine.name;

Just:
let includeCurrentEngine = this.getAttribute("includecurrentengine");
before the loop seems simpler (and also we'll have only one getAttribute call instead of one per engine).

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8778010 [details]
Bug 1292174 - Add Search Tile for Default Search Engine in urlbar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69386/diff/1-2/

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8778010 [details]
Bug 1292174 - Add Search Tile for Default Search Engine in urlbar.

https://reviewboard.mozilla.org/r/69386/#review66732

Looks good thanks!

When testing this, it's a bit surprising that the default one-off isn't the first one displayed in the list, but I guess that's the expected behavior as it wouldn't make sense to display the one-offs in an order that doesn't match what the user has set in the Search preferences.

::: browser/components/search/content/search.xml:1417
(Diff revision 2)
>  
>            let currentEngineName = Services.search.currentEngine.name;
> -          let engines = Services.search.getVisibleEngines()
> -                                .filter(e => e.name != currentEngineName &&
> -                                             hiddenList.indexOf(e.name) == -1);
> +          let includeCurrentEngine = this.getAttribute("includecurrentengine");
> +          let engines = Services.search.getVisibleEngines().filter(e => {
> +            return (includeCurrentEngine || e.name != currentEngineName) &&
> +                   hiddenList.indexOf(e.name) == -1;

nit: touching this line seems like a good opportunity to replace the .indexOf(...) == -1 with a ! .includes(...)
(doesn't matter if you don't do it though)

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8778010 [details]
Bug 1292174 - Add Search Tile for Default Search Engine in urlbar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69386/diff/2-3/

Comment 7

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/6140083fa3c90e1bce6646785584e177e9cf665d
Bug 1292174 - Add Search Tile for Default Search Engine in urlbar. r=florian

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/6140083fa3c9

Comment 9

[Tanvir Rahman]

I have reproduced this bug with Nightly 51.0a1(2016-08-04) on Windows 10, 64 bit!

The Bug's fix is now verified on latest Nightly 51.0a1

Build ID 	20160811030201
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0

[bugday-20160810]

Comment 10

[Farhadur Reja Fahim]

I have reproduced this bug with Nightly 51.0a1 (2016-08-04) on Ubuntu 14.04, 64 bit!

The bug's fix is now verified on latest Nightly 51.0a1.

Nightly 51.0a1:
Build ID 	20160818030226
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0

[bugday-20160817]