Closed Bug 1292174 Opened 3 years ago Closed 3 years ago

Add Search Tile for Default Search Engine

Categories

(Firefox :: Search, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 51
Tracking Status
firefox51 --- verified

People

(Reporter: shorlander, Assigned: adw)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

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
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.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Attachment #8778010 - Flags: review?(florian) → review-
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).
Priority: -- → P1
Whiteboard: [fxsearch]
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/
Attachment #8778010 - Flags: review- → review?(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)
Attachment #8778010 - Flags: review?(florian) → review+
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/
https://hg.mozilla.org/integration/fx-team/rev/6140083fa3c90e1bce6646785584e177e9cf665d
Bug 1292174 - Add Search Tile for Default Search Engine in urlbar. r=florian
https://hg.mozilla.org/mozilla-central/rev/6140083fa3c9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
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]
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]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.