Closed Bug 1045008 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Search, defect)

31 Branch
defect
Not set
normal
Points:
1

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.2
Tracking Status
firefox31 --- wontfix
firefox32 --- wontfix
firefox33 --- verified
firefox34 --- verified

People

(Reporter: saheernaduthodi, Assigned: alexbardas, Mentored)

References

Details

(Keywords: ux-implementation-level, ux-mode-error)

Attachments

(3 files, 1 obsolete file)

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
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 901315
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
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: nobody → abardas
Status: REOPENED → ASSIGNED
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+
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+
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)
Attached image 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.
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: 6 years ago6 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)
Points: --- → 1
Flags: needinfo?(adw)
Mentor: adw
Duplicate of this bug: 1056254
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
Attached patch Beta patchSplinter Review
Attachment #8468534 - Attachment is obsolete: true
Attachment #8468534 - Attachment is obsolete: false
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 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.