1045008 - Manage search Engine option in new tab is not working (broken) if search bar is removed

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Saheer Naduthodi]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446

Steps to reproduce:

1. Removed the search bar from the toolbar
2. Opened a new tab and tried to open "mange search engine" from the new tab search field.
3. it not opening any window
4. Tried by adding the search bar back again in the tool bar, its working fine.



Actual results:

No window or options to manage search engine is opened


Expected results:

It should open "manage search engine"  option

Comment 2

[[:Aleksej]]

Oops, that report seems to be about there being no way at all, not a broken one.

Comment 3

[[:Aleksej]]

Terminal shows:

A coding exception was thrown and uncaught in a Task.

Full message: TypeError: browserWin.BrowserSearch.searchBar is null
Full stack: this.ContentSearch._onMessageManageEngines@resource://app/modules/ContentSearch.jsm:157:5
this.ContentSearch._onMessage<@resource://app/modules/ContentSearch.jsm:119:13
TaskImpl_run@resource://gre/modules/Task.jsm:314:40
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7


Note that to open the menu on the page, you need to make sure an engine with an icon is selected, due to bug 1032324.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Good catch! ContentSearch.jsm must deal with BrowserSearch.searchBar being null (probably be just re-implementing search.xml's openManager itself rather than calling it).

Comment 5

[Alex Bardas :alexbardas]

Attachment: Manage search_engine_option_in_new_tab_is_not_working_if_search_bar_is_removed.diff

Added the functionality from search.xml.

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8467384 [details] [diff] [review]
Manage search_engine_option_in_new_tab_is_not_working_if_search_bar_is_removed.diff

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

::: browser/modules/ContentSearch.jsm
@@ +154,5 @@
>  
>    _onMessageManageEngines: function (msg, data) {
>      let browserWin = msg.target.ownerDocument.defaultView;
> +    let wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
> +             .getService(Components.interfaces.nsIWindowMediator);

Nit: put the dot at the end of the first line to match the style here.

@@ +157,5 @@
> +    let wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
> +             .getService(Components.interfaces.nsIWindowMediator);
> +    let window = wm.getMostRecentWindow("Browser:SearchManager");
> +
> +    if (window)

Nit: braces around the if-branch to match style.

Comment 7

[Alex Bardas :alexbardas]

Attachment: bug1045008_manage_search_engine_option_in_new_tab_is_not_working_if_search_bar_is_removed.diff

Fixed the style changes.

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 8468534 [details] [diff] [review]
bug1045008_manage_search_engine_option_in_new_tab_is_not_working_if_search_bar_is_removed.diff

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

Thanks.

Comment 9

[Alex Bardas :alexbardas]

https://tbpl.mozilla.org/?tree=Try&rev=525a1be55398

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Looks like the Try run got eaten somehow :(

Comment 11

[Drew Willcoxon :adw]

Ryan, what do you mean?  The tests didn't run on Windows XP for some reason, but other than that it looks fine (at least during the increasingly rare times that tbpl actually completely loads for me).  And besides that I'm confident that the patch is OK.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Weird, I'm not seeing any builds/tests on that Try push, even when I load it in self-serve.

https://secure.pub.build.mozilla.org/buildapi/self-serve/try/rev/525a1be55398

Comment 13

[Drew Willcoxon :adw]

Attachment: try server screenshot

Here's a screenshot to show I'm not crazy. :-)  I do see "Revision 525a1be55398 not found on branch try" on that self-serve link, though.  try and hg.m.o have been pretty crappy at least for me recently, so who knows.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Yeah, the recent infra issues have been a massive pain. Sorry for all the hassle :(
https://hg.mozilla.org/integration/fx-team/rev/0d125ed595c2

Comment 15

[Alex Bardas :alexbardas]

https://tbpl.mozilla.org/?tree=Try&rev=f07f889c09a9

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0d125ed595c2

Comment 17

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed using latest Nightly 2014-08-10 under Win 7 64-bit, Ubuntu 13.04 32-bit and Mac OSX 10.9.4.

Comment 18

[Alex Bardas :alexbardas]

https://tbpl.mozilla.org/?tree=Try&rev=55379fa54206

Comment 19

[Marco Mucci [:MarcoM]]

Hi Drew, can you provide a point value for this bug.

Comment 21

[:Gijs (he/him)]

Nope, bugzilla, I did not want to un-mentor Drew...

I do want to note that this affects 32 and 33, so I'm trying to get a patch together for uplift...

Comment 22

[:Gijs (he/him)]

Attachment: Beta patch

Comment 23

[:Gijs (he/him)]

Comment on attachment 8476735 [details] [diff] [review]
Beta patch

Approval Request Comment
[Feature/regressing bug #]: search on the new tab page (for Firefox 31)
[User impact if declined]: if the search bar is customized away, clicking the 'manage search engines' link doesn't work
[Describe test coverage new/current, TBPL]: baking on trunk, verified there, no automated test that I know of
[Risks and why]: low, very specific change
[String/UUID change made/needed]: none

Comment 24

[:Gijs (he/him)]

Comment on attachment 8468534 [details] [diff] [review]
bug1045008_manage_search_engine_option_in_new_tab_is_not_working_if_search_bar_is_removed.diff

Approval Request Comment: comment 23

Comment 25

[Lawrence Mandel [:lmandel] (use needinfo)]

I reviewed this bug with Gijs. Due to how late it is in the beta cycle and that this bug is understood to be an edge case, we're not going to take this in 32.

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8476735 [details] [diff] [review]
Beta patch

beta- See comment 25.

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Ouch - how did we end up dropping this? I guess I should have set some tracking flags in comment 4. Thanks for following up, Gijs.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f830a2623429

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

Petruta would you mind verifying the Aurora fix too? Thanks!

Comment 30

[Petruta Horea [:phorea], Desktop QA]

Using latest Aurora 33.0a2 (20140822004002) under Win 7 64-bit, Mac OSX 10.8.5 and Ubuntu 13.04 64-bit, I could use the "Manage search engine" option while the search bar was removed from the toolbar.