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

VERIFIED FIXED in Firefox 33

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: saheernaduthodi, Assigned: alexbardas, Mentored)

Tracking

({ux-implementation-level, ux-mode-error})

31 Branch
Firefox 34
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox31 wontfix, firefox32 wontfix, firefox33 verified, firefox34 verified)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
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

Updated

5 years ago
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 901315

Comment 2

5 years ago
Oops, that report seems to be about there being no way at all, not a broken one.
Status: RESOLVED → REOPENED
Component: Untriaged → Search
Ever confirmed: true
OS: Windows 7 → All
Resolution: DUPLICATE → ---
Summary: Manage search Engine option in new tab is not working if search bar is removed → Manage search Engine option in new tab is not working (broken) if search bar is removed

Comment 3

5 years ago
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.
Blocks: 962490
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).
Flags: firefox-backlog+
Hardware: x86_64 → All
Assignee

Updated

5 years ago
Assignee: nobody → abardas
Status: REOPENED → ASSIGNED
Assignee

Comment 5

5 years ago
Added the functionality from search.xml.
Attachment #8467384 - Flags: review?(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.
Attachment #8467384 - Flags: review?(adw) → review+
Assignee

Comment 7

5 years ago
Fixed the style changes.
Attachment #8467384 - Attachment is obsolete: true
Attachment #8468534 - Flags: review?(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.
Attachment #8468534 - Flags: review?(adw) → review+

Updated

5 years ago
Keywords: checkin-needed
Looks like the Try run got eaten somehow :(
Keywords: checkin-needed
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.
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
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
Flags: needinfo?(ryanvm)
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.
Yeah, the recent infra issues have been a massive pain. Sorry for all the hassle :(
https://hg.mozilla.org/integration/fx-team/rev/0d125ed595c2
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0d125ed595c2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Iteration: --- → 34.2
QA Whiteboard: [qa?]
QA Whiteboard: [qa?] → [qa+]
QA Contact: cornel.ionce
QA Contact: cornel.ionce → petruta.rasa
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Hi Drew, can you provide a point value for this bug.
Flags: needinfo?(adw)

Updated

5 years ago
Points: --- → 1
Flags: needinfo?(adw)

Updated

5 years ago
Mentor: adw
Duplicate of this bug: 1056254

Comment 21

5 years ago
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...
Mentor: adw

Comment 22

5 years ago
Posted patch Beta patchSplinter Review

Updated

5 years ago
Attachment #8468534 - Attachment is obsolete: true

Updated

5 years ago
Attachment #8468534 - Attachment is obsolete: false

Comment 23

5 years ago
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
Attachment #8476735 - Flags: review+
Attachment #8476735 - Flags: approval-mozilla-beta?

Comment 24

5 years ago
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
Attachment #8468534 - Flags: approval-mozilla-aurora?
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 on attachment 8476735 [details] [diff] [review]
Beta patch

beta- See comment 25.
Attachment #8476735 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
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.
Attachment #8468534 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Petruta would you mind verifying the Aurora fix too? Thanks!
Flags: needinfo?(petruta.rasa)
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.
Flags: needinfo?(petruta.rasa)
You need to log in before you can comment on or make changes to this bug.