1427317 - Enhance user interface to allow users to modify open search providers.

Status: RESOLVED FIXED

(Thunderbird :: Search, enhancement)

Description

[Matt]

Changes in TB 59 have removed the ability to add a non listed search provider.
In https://bugzilla.mozilla.org/show_bug.cgi?id=1427167#c5 a manual method to use the API to add the file is discussed.  But the process is hardly suitable for general users wanting to add their favourite search engine.

Firefox has implemented an extensive change to their user interface to accommodate the change, utilizing AMO.  While the AMO route is the simplest from a user perspective, I doubt Thunderbird has the resources at this time to match it.

I suggest we simply add an "add search provider" button to the options dialog beside the existing list of providers to be selected as "default" which opens a file picker so the user can point to the XML file they have downloaded.  While we are there I also think we should expose the open in a browser hidden pref to the user so they can select if the search results are to be opened in a Thunderbird tab, or my preferred option, a browser tab.

Comment 1

[Jorg K (CEST = GMT+2)]

Yes, this is a good idea. For the record from bug 1427167 comment #5, you simply need to execute, for example:
Services.search.addEngine("file://C:/mozilla-source/mozilla-central/mobile/locales/searchplugins/drae.xml", null, "", {});
but supply a success/failure object in the last argument, like so:
https://dxr.mozilla.org/mozilla-central/rev/c2daa4c032b7f23a708a89237940ac8c49bba6cf/browser/base/content/test/about/head.js#177

Volunteers? Maybe a job for Richard?

Comment 2

[Richard Marti (:Paenglab)]

Attachment: WIP

This is a rough patch which has two issues.

1. After adding a search engine, we should update the menulist to see the new engine. I tried with this.updateWebSearch() but this only doubled the entries. The update needs to remove the old entries and reread the list. But this is above my knowledge.

2. I added the link to the search engines on AMO. But here we have two problems. With the actual link under thunderbird there are no search engines found. But with the Firefox link the page says, we are not with a Firefox browser on this page and I can't download an engine (is this not the open web?). This issue may be for Andrej or Philipp.

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8939403 [details] [diff] [review]
WIP

Looks like we need to set some flags here so it won't get ignored.

Comment 4

[Jorg K (CEST = GMT+2)]

We should move this forward. Please add the this.updateWebSearch() again with a comment. I'll take a look. We get too many users wanting to add their own engines.

Comment 5

[Richard Marti (:Paenglab)]

Attachment: WIP: 1427317-searchEngines.patch

Okay, Updated to tip and used ATN addresses. ATN has no search engines actually.

Comment 6

[Friedrich Strohmaier]

Is this patch already implemented somewhere for beta testing?

Don't understand the meaning of the last phrase w/r of further action.

Comment 7

[Jorg K (CEST = GMT+2)]

Not implemented anywhere yet. So far the patch doesn't work.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1427317-searchEngines.patch - WIP - rebased

This almost works. Yes, it doubles up all the entries, but if you run "add from file" a second time, the one you previously added shows up.

We'd also need a remove button, I guess.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1427317-searchEngines.patch (v4)

OK, this works now. We can add a search engine from a file. It's not automatically made the default.

Now we need a remove button. Richard, can you add that. Looks like the code in this patch isn't from the current version of FF, so perhaps get the remove button from wherever this code came from.

Andrei, what do you think of the "Find more search engines" link. If gives zero results. Should we just remove that for the time being?

Comment 10

[Jorg K (CEST = GMT+2)]

Looking at the history of this bug, we really messed it up. This should have been landed in January 2018 in be included in TB 60 ESR. Now all we could do is use the 'browse.label' string in general.dtd for the "add from file". No idea where we would get a string for deletion from :-(

Comment 11

[Andrei [:sancus]]

Yes, I think removing the link makes sense.

As far as I know, AMO never supported search tools for Thunderbird. I could be wrong about that. Certainly, it does not seem possible to upload a search tool that is compatible with Thunderbird, they in general don't have any compatibility data associated with them, and they don't show up in the Thunderbird part of the site at all, even though they are in the database. I mistakenly thought they were deleted as thunderbird only addons, but they haven't been. They're just not visible because they only show for Firefox.

People could download the XML files from addons.mozilla.org and install them that way, I guess. I don't have time to fix this on ATN this week and honestly I doubt that I can get to it before sometime in December at the earliest. I'm not even sure how much time it would take to fix.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1427317-searchEngines.patch (v5)

This should do it. I removed the link and added a remove button which is disabled if there is only one search engine left.

Please check the access keys.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1427317-searchEngines.patch (vb)

Sorry, all the code related to the link can go now.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1427317-searchEngines.patch (v6)

Hmm, not quite ready yet. Looks like you can't add the default engines once you've deleted them; try adding bing.xml after deleting it.

So perhaps we need to disallow deletion of the default set. We can use Services.search.getDefaultEngines(). Of course we need to drive the remove button differently then. Or we just unhide the default engine when it's added again. Maybe users want to delete a default engine because they hate it.

Comment 15

[Jorg K (CEST = GMT+2)]

I'm hijacking this, my patch is ready. We've discussed this in the TB Council yesterday and need to move this forward.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1427317-searchEngines.patch (v7)

OK, I'm adding an add and a remove button. The remove button is disabled for default engines. That saves us having a restore button and more complicated logic to restore a removed default engine (since that's not really removed but only hidden).

Seems to work. We missed the boat for TB 60 which is really annoying. Richard, could you please prepare an ESR 60 patch without the string changes. Instead of "add" and "remove" we could use iconic buttons "+" and "-", or some other icons which in indicate that. Alternatively, for "add" we could use the browse string.

Comment 17

[Jorg K (CEST = GMT+2)]

Hmm, looks like 'self' is not the right thing to use, although I've seen it used in similar occasions:
230:7  error  Read-only global 'self' should not be modified.  no-global-assign (eslint)

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9026638 [details] [diff] [review]
1427317-searchEngines.patch (v7)

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

Just clicking the "Add Search from file" crashes thunderbird for me

Re "self" - why do we we have a global self? That sounds pretty wrong.
You could probably just do .bind(this) though.

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 9026638 [details] [diff] [review]
1427317-searchEngines.patch (v7)

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

::: mail/locales/en-US/chrome/messenger/preferences/general.dtd
@@ +37,5 @@
>  <!ENTITY browse.label                     "Browse…">
>  <!ENTITY browse.accesskey                 "B">
>  
>  <!ENTITY defaultSearchEngine.label        "Default Search Engine">
> +<!ENTITY addSearchEngine.label            "Add Search from file">

Maybe we should phrase this "Add Open Search Provider", and maybe let the user enter a URL where we'd grab or autodiscover it? Adding from a file will be non-intutitive

Comment 20

[Jorg K (CEST = GMT+2)]

It crashes? That's surprising. I can add search engines from a file just fine and the same code appeared to the working for Richard when he started the patch. Can you tell why it's crashing? Hard for me to debug.

I'll find a solution for the 'self' issue.

As for the UI: We could do "Add Search Provider from file". The "from file" option was explicitly requested, see comment #0, last paragraph. Andrei suggests the same in comment #11. The people from bug 1427167 and bug 1427093 also want to add them from file. Don't let perfect be enemy of better ;-)

Comment 21

[Magnus Melin [:mkmelin]]

Surprising yes. Anyway, I think it would be much more user friendly to pop up a dialog saying "Please visit the website for your desired search engine", and we'd autodiscover from there. It should be potentially just < 10 lines of code.

Thread 1 "thunderbird" received signal SIGSEGV, Segmentation fault.
nsFilePicker::Open (this=0x7f3d0166acc0, aCallback=<optimized out>)
    at /home/magnus/Code/tb/mozilla/widget/gtk/nsFilePicker.cpp:383
383	    GTK_WINDOW(mParentWidget->GetNativeData(NS_NATIVE_SHELLWIDGET));
(gdb) bt
#0  0x00007f3d27b9162e in nsFilePicker::Open(nsIFilePickerShownCallback*)
    (this=0x7f3d0166acc0, aCallback=<optimized out>)
    at /home/magnus/Code/tb/mozilla/widget/gtk/nsFilePicker.cpp:383
#1  0x00007f3d2568d4f2 in NS_InvokeByIndex ()
    at /home/magnus/Code/tb/mozilla/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:106
#2  0x00007f3d25fe3fce in CallMethodHelper::Invoke() (this=<optimized out>)
    at /home/magnus/Code/tb/mozilla/js/xpconnect/src/XPCWrappedNative.cpp:1735
#3  0x00007f3d25fe3fce in CallMethodHelper::Call() (this=<optimized out>)
    at /home/magnus/Code/tb/mozilla/js/xpconnect/src/XPCWrappedNative.cpp:1268
#4  0x00007f3d25fe3fce in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (ccx=..., mode=<optimized out>)
    at /home/magnus/Code/tb/mozilla/js/xpconnect/src/XPCWrappedNative.cpp:1232
#5  0x00007f3d25fe53f0 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (cx=<optimized out>, argc=1, vp=<optimized out>)
    at /home/magnus/Code/tb/mozilla/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1020
#6  0x00007f3d29159fec in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&)
    (cx=<optimized out>, native=<optimized out>, args=...)
    at /home/magnus/Code/tb/mozilla/js/src/vm/Interpreter.cpp:468
#7  0x00007f3d29159fec in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)
    (cx=0x7f3d1ed23800, args=..., construct=<optimized out>)
    at /home/magnus/Code/tb/mozilla/js/src/vm/Interpreter.cpp:560
#8  0x00007f3d29153da4 in js::CallFromStack(JSContext*, JS::CallArgs const&)
    (cx=<optimized out>, args=...)
    at /home/magnus/Code/tb/mozilla/js/src/vm/Interpreter.cpp:620
#9  0x00007f3d29153da4 in Interpret(JSContext*, js::RunState&)
    (cx=<optimized out>, state=...)
    at /home/magnus/Code/tb/mozilla/js/src/vm/Interpreter.cpp:3462
#10 0x00007f3d2914bd3d in js::RunScript(JSContext*, js::RunState&)
    (cx=0x7f3d1ed23800, state=...)
    at /home/magnus/Code/tb/mozilla/js/src/vm/Interpreter.cpp:447
#11 0x00007f3d2915a339 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)
    (cx=0x7f3d1ed23800, args=..., construct=<optimized out>)
    at /home/magnus/Code/tb/mozilla/js/src/vm/Interpreter.cpp:587
#12 0x00007f3d2915a819 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>)
    (cx=0x0, fval=..., thisv=..., args=..., rval=...)
    at /home/magnus/Code/tb/mozilla/js/src/vm/Interpreter.cpp:633
#13 0x00007f3d2946018f in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>)
    (cx=0x7f3d1ed23800, thisv=..., fval=..., args=..., rval=...)
    at /home/magnus/Code/tb/mozilla/js/src/jsapi.cpp:2988
#14 0x00007f3d26eb34b7 in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) (this=
    0x7f3d016b8680, cx=0x7f3d1ed23800, aThisVal=..., event=..., aRetVal=..., aRv=...)
--Type <RET> for more, q to quit, c to continue without paging-- 
    at /home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/dom/bindings/EventHandlerBinding.cpp:265
#15 0x00007f3d27321b71 in mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) (this=0x7f3d016b8680, thisVal=<optimized out>, event=
    ..., aRetVal=..., aRv=..., aExecutionReason=<optimized out>, aExceptionHandling=mozilla::dom::CallbackObject::eReportExceptions, aRealm=0x0)
    at /home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/EventHandlerBinding.h:363
#16 0x00007f3d2731d51e in mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) (this=<optimized out>, aEvent=0x7f3d014eb560)
    at /home/magnus/Code/tb/mozilla/dom/events/JSEventHandler.cpp:214
#17 0x00007f3d2730c3a1 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*)
    (this=0x7f3d05195ac0, aListener=0x7f3d051ad808, aDOMEvent=0x7f3d014eb560, aCurrentTarget=0x7f3d0a2bc310)
    at /home/magnus/Code/tb/mozilla/dom/events/EventListenerManager.cpp:1115
#18 0x00007f3d2730cc10 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) (this=<optimized out>, aPresContext=
    0x7f3d0a21d800, aEvent=<optimized out>, aDOMEvent=0x7ffc488ff5e0, aCurrentTarget=0x7f3d0a2bc310, aEventStatus=0x7ffc488ff5e8, aItemInShadowTree=<optimized out>) at /home/magnus/Code/tb/mozilla/dom/events/EventListenerManager.cpp:1317
#19 0x00007f3d27321085 in mozilla::EventListenerManager::HandleEvent(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool)
    (this=<optimized out>, aPresContext=0x7f3d014d93c8, aEvent=
    0x7f3d014d93c8, aDOMEvent=0x8, aCurrentTarget=0x7f3d014d93c8, aEventStatus=0x7ffc488ff5e8, aItemInShadowTree=<optimized out>)
    at /home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/dist/include/mozilla/EventListenerManager.h:390
#20 0x00007f3d27321085 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&)
    (this=<optimized out>, aVisitor=..., aCd=...)
    at /home/magnus/Code/tb/mozilla/dom/events/EventDispatcher.cpp:425
#21 0x00007f3d27305ee5 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&)
    (aChain=..., aVisitor=..., aCallback=0x0, aCd=...)
    at /home/magnus/Code/tb/mozilla/dom/events/EventDispatcher.cpp:642
#22 0x00007f3d27307452 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*)
    (aTarget=<optimized out>, aPresContext=0x7f3d0a21d800, aEvent=0x7f3d014eb6a0, aDOMEvent=0x7f3d014eb560, aEventStatus=0x7ffc488ff7f8, aCallback=0x0, aTargets=0x0) at /home/magnus/Code/tb/mozilla/dom/events/EventDispatcher.cpp:1164
#23 0x00007f3d27308d4b in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*)
--Type <RET> for more, q to quit, c to continue without paging--
aPresContext=0x7f3d0a21d800, aEventStatus=0x7ffc488ff7f8) at /home/magnus/Code/tb/mozilla/dom/events/EventDispatcher.cpp:1245
#24 0x00007f3d27cf3162 in mozilla::PresShell::HandleDOMEventWithTarget(nsIContent*, mozilla::dom::Event*, nsEventStatus*) (this=0x7f3d0a2aa000, aTargetContent=
    0x7f3d0a2bc310, aEvent=0x7f3d014eb560, aStatus=0x7ffc488ff7f8) at /home/magnus/Code/tb/mozilla/layout/base/PresShell.cpp:8129
#25 0x00007f3d2662ea61 in nsContentUtils::DispatchXULCommand(nsIContent*, bool, mozilla::dom::Event*, nsIPresShell*, bool, bool, bool, bool, unsigned short)
    (aTarget=0x7f3d0a2bc310, aTrusted=<optimized out>, aSourceEvent=0x0, aShell=0x7f3d0a2aa000, aCtrl=false, aAlt=<optimized out>, aShift=<optimized out>, aMeta=<optimized out>, aInputSource=1)
    at /home/magnus/Code/tb/mozilla/dom/base/nsContentUtils.cpp:6441
#26 0x00007f3d27edfed3 in nsButtonBoxFrame::MouseClicked(mozilla::WidgetGUIEvent*) (this=<optimized out>, aEvent=0x7ffc488ffde0) at /home/magnus/Code/tb/mozilla/layout/xul/nsButtonBoxFrame.cpp:215
#27 0x00007f3d27edfdd1 in nsButtonBoxFrame::HandleEvent(nsPresContext*, mozilla::WidgetGUIEvent*, nsEventStatus*) (this=
    0x7f3d0be74010, aPresContext=0x7f3d0a21d800, aEvent=0x7ffc488ffde0, aEventStatus=0x7ffc488ffae8) at /home/magnus/Code/tb/mozilla/layout/xul/nsButtonBoxFrame.cpp:171
#28 0x00007f3d27306157 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) (aChain=..., aVisitor=..., aCallback=0x7ffc488ffcf0, aCd=...) at /home/magnus/Code/tb/mozilla/dom/events/EventDispatcher.cpp:707
#29 0x00007f3d27307452 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) (aTarget=<optimized out>, aPresContext=0x7f3d0a21d800, aEvent=0x7ffc488ffde0, aDOMEvent=0x0, aEventStatus=0x7ffc488ffed4, aCallback=0x7ffc488ffcf0, aTargets=0x0)
    at /home/magnus/Code/tb/mozilla/dom/events/EventDispatcher.cpp:1164
#30 0x00007f3d27cf3053 in mozilla::PresShell::DispatchEventToDOM(mozilla::WidgetEvent*, nsEventStatus*, nsPresShellEventCB*)
    (this=0x7f3d0a2aa000, aEvent=0x7ffc488ffde0, aStatus=0x7ffc488ffed4, aEventCB=<optimized out>) at /home/magnus/Code/tb/mozilla/layout/base/PresShell.cpp:7995
#31 0x00007f3d27cf13bc in mozilla::PresShell::HandleEventInternal(mozilla::WidgetEvent*, nsEventStatus*, bool, nsIContent*)
    (this=0x7f3d0a2aa000, aEvent=0x7ffc488ffde0, aStatus=0x7ffc488ffed4, aIsHandlingNativeEvent=false, aOverrideClickTarget=0x0) at /home/magnus/Code/tb/mozilla/layout/base/PresShell.cpp:7733
#32 0x00007f3d27cf0714 in mozilla::PresShell::HandleEventWithTarget(mozilla::WidgetEvent*, nsIFrame*, nsIContent*, nsEventStatus*, bool, nsIContent**, nsIContent*)
    (this=0x7f3d0a2aa000, aEvent=0x7ffc488ffde0, aFrame=
    0x56445c79857a <mozilla::TimeStamp::Now(bool)+42>, aContent=0x7f3d0a2bc310, aStatus=0x7ffc488ffed4, aIsHandlingNativeEvent=<optimized out>, aTargetContent=0x0, aOverrideClickTarget=0x0)
    at /home/magnus/Code/tb/mozilla/layout/base/PresShell.cpp:7518
#33 0x00007f3d272e7869 in mozilla::EventStateManager::InitAndDispatchClickEvent(mozilla::WidgetMouseEvent*, nsEventStatus*, mozilla::EventMessage, nsIPresShell*, nsIContent*, AutoWeakFrame, bool, nsIContent*)
    (aMouseUpEvent=0x7ffc48900370, aStatus=0x7ffc488ffff4, aMessage=mozilla::EventMessage::eMouseClick, aPresShell=0x7f3d0a2aa000, aMouseUpContent=0x7f3d0a2bc310, aCurrentTarget=..., aNoContentDispatch=<optimized out>, aOverrideClickTarget=0x0) at /home/magnus/Code/tb/mozilla/dom/events/EventStateManager.cpp:5033
#34 0x00007f3d272e79e1 in mozilla::EventStateManager::DispatchClickEvents(nsIPresShell*, mozilla::WidgetMouseEvent*, nsEventStatus*, nsIContent*, nsIContent*)
    (this=<optimized out>, aPresShell=0x7f3d0a2aa000, aMouseUpEvent=0x7ffc48900370, aStatus=0x7ffc488ffff4, aMouseUpContent=0x7f3d0a2bc310, aOverrideClickTarget=0x0)
    at /home/magnus/Code/tb/mozilla/dom/events/EventStateManager.cpp:5150
#35 0x00007f3d272e5ab4 in mozilla::EventStateManager::PostHandleMouseUp(mozilla::WidgetMouseEvent*, nsEventStatus*, nsIContent*) (this=
    0x7f3d1202e700, aMouseUpEvent=<optimized out>, aStatus=0x7ffc489002dc, aOverrideClickTarget=0x0) at /home/magnus/Code/tb/mozilla/dom/events/EventStateManager.cpp:5088
#36 0x00007f3d272e4c34 in mozilla::EventStateManager::PostHandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIFrame*, nsEventStatus*, nsIContent*)
    (this=<optimized out>, aPresContext=<optimized out>, aEvent=<optimized out>, aTargetFrame=<optimized out>, aStatus=<optimized out>, aOverrideClickTarget=<optimized out>)
    at /home/magnus/Code/tb/mozilla/dom/events/EventStateManager.cpp:3405
#37 0x00007f3d27cf14dc in mozilla::PresShell::HandleEventInternal(mozilla::WidgetEvent*, nsEventStatus*, bool, nsIContent*)
    (this=0x7f3d0a2aa000, aEvent=0x7ffc48900370, aStatus=0x7ffc489002dc, aIsHandlingNativeEvent=true, aOverrideClickTarget=0x0) at /home/magnus/Code/tb/mozilla/layout/base/PresShell.cpp:7753
#38 0x00007f3d27cf02bf in mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*)
    (this=<optimized out>, aFrame=0x7f3d13d98020, aEvent=<optimized out>, aDontRetargetEvents=false, aEventStatus=0x7ffc489002dc) at /home/magnus/Code/tb/mozilla/layout/base/PresShell.cpp:7343
#39 0x00007f3d27b1c306 in nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) (this=<optimized out>, aEvent=0x7ffc48900370, aView=<optimized out>, aStatus=0x7ffc489002dc)
    at /home/magnus/Code/tb/mozilla/view/nsViewManager.cpp:812
#40 0x00007f3d27b1c0e4 in nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) (this=<optimized out>, aEvent=0x7ffc48900370, aUseAttachedEvents=<optimized out>)
    at /home/magnus/Code/tb/mozilla/view/nsView.cpp:1141
#41 0x00007f3d27b5cfa4 in nsWindow::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) (this=<optimized out>, aEvent=0x7f3d014d93c8, aStatus=@0x7ffc48900328: nsEventStatus_eIgnore)
    at /home/magnus/Code/tb/mozilla/widget/gtk/nsWindow.cpp:581
#42 0x00007f3d27b24497 in nsBaseWidget::DispatchInputEvent(mozilla::WidgetInputEvent*) (this=0x7f3d1780b000, aEvent=0x7ffc48900370) at /home/magnus/Code/tb/mozilla/widget/nsBaseWidget.cpp:1241
#43 0x00007f3d27b64c5b in nsWindow::OnButtonReleaseEvent(_GdkEventButton*) (this=0x7f3d1780b000, aEvent=0x7f3d014eb600) at /home/magnus/Code/tb/mozilla/widget/gtk/nsWindow.cpp:2865
#44 0x00007f3d27b6943c in button_release_event_cb(_GtkWidget*, _GdkEventButton*) (widget=<optimized out>, event=0x7f3d014eb600) at /home/magnus/Code/tb/mozilla/widget/gtk/nsWindow.cpp:5866
#45 0x00007f3d2ef802cb in  () at /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#46 0x00007f3d2df7eb6d in g_closure_invoke () at /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#47 0x00007f3d2df918f3 in  () at /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#48 0x00007f3d2df99f43 in g_signal_emit_valist () at /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0

Comment 22

[Jorg K (CEST = GMT+2)]

Hmm, I tell "New to Bugzilla" people to not SPAM comments with debug, and here we have SPAM from the TB module owner :-(

Seriously, the first few lines to "#1  0x00007f3d2568d4f2 in NS_InvokeByIndex ()" would have sufficed, from that we can see that the file picker crashed somehow.

I think an "add from file" is needed, I have XML files with search engine definitions and it's apparently easy to get them from AMO. As I said, don't let perfect be enemy of better. Once this lands, feel free to add your additional 10 lines, the framework will then be in place.

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: 1427317-searchEngines.patch (v8)

Crash fixed by added init call. Self problem solved by adding 'let'.

We already have two authors here, so Magnus, if you want to add 10 lines that let people get the search engine from some web site, add it to the patch, submit a part 2 or do it in a new bug.

Comment 24

[Richard Marti (:Paenglab)]

Comment on attachment 9026672 [details] [diff] [review]
1427317-searchEngines.patch (v8)

Yes, keep it simple and use the "Add Search from file". If we could add a link then oka, but asking for a link is too complicated.

I tested the patch under Linux too and it doesn't crash.

Comment 25

[Matt]

Is there a build of this that I can try out... having just had the google search disappear I am lost as to how to get it back.

Comment 26

[Jorg K (CEST = GMT+2)]

No, there isn't. Sadly this is not waiting for Magnus since he didn't like our solution.

Comment 27

[Magnus Melin [:mkmelin]]

Attachment: bug1427317_opensearch_discover.patch

This is adding the search engine by entering a search engine URL, which is something a normal user could figure out, with some luck. The OpenSearch support seems way worse than I thought though... but you can add bugzilla, wikipedia, and company internal searches - so at least it's flexible. The bugzilla entry could actually be somewhat useful for bugmail processing.

Comment 28

[Magnus Melin [:mkmelin]]

The patch from comment 27 works, but not if the preferences pane is loaded through about:preferences, since  I'm using a normal fetch() - which is blocked by not having CSP for this (still) XUL page. 

I think bug 965637 will enable having a CSP for it, but nonetheless, I'm somewhat confused about the purpose of the CSP (or rather lack of such) blocking this fetch, given this is all chrome code. Chris, is that on purpose?

Yes, I could use other ways to fetch(), but I'd prefer not to.

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: bug1427317_opensearch_discover.patch (rebased)

I think you should not allow removal of the default engines since it might be hard to restore them, see my patch. Add works with a file: URL, so that's OK.

Comment 30

[Matt]

While I could see issues that might be created by removing a last search provider,  I think allowing users to remove the ones that they do not want would hurt nothing.  I am unlikely to ever use a search on amazon.com.  I have never made a purchase there and can not see that changing.  Twitter is about as useful as horns on a table so I can see users wanting to clean up that mess.  

Having said that I particularly like Magnus idea of entering a URL.  Getting the XML file is really a messy process, with most providers offering it but not offering it in a way fireox will offer to download it,  leaving you to scrape the display screen with a copy and paste to make you own text file.  This is unnecessarily complex as opensearch offer discover-ability from the base URL.  We should be using that.

Given we default to Bing as a provider and still have not managed to add Google or DuckDuckGo we are really not helping our cause by making the process an more difficult than it has to be.

Comment 31

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Magnus Melin [:mkmelin] from comment #28)
> The patch from comment 27 works, but not if the preferences pane is loaded
> through about:preferences, since  I'm using a normal fetch() - which is
> blocked by not having CSP for this (still) XUL page. 
> 
> I think bug 965637 will enable having a CSP for it, but nonetheless, I'm
> somewhat confused about the purpose of the CSP (or rather lack of such)
> blocking this fetch, given this is all chrome code. Chris, is that on
> purpose?
> 
> Yes, I could use other ways to fetch(), but I'd prefer not to.

Sorry, I can't completely follow what you mean. If
a) CSP is blocking a load that it shouldn't, then please provide log from the web console (please also check browser console) and if possible the CSP for that page.
b) there is no CSP on something where we should have a CSP, then please let me know where.

Within bug 965637 we are moving the CSP from the Principal into the client, but that should not affect anything that's currently in the tree. So if it's blocked currently, it will be blocked in the future and vice versa.

Comment 32

[Jorg K (CEST = GMT+2)]

Richard, this is not progressing well. Once big point of critique is that TB doesn't allow adding search providers. I'd like to ship something like "1427317-searchEngines.patch (v8)" minus the string changes in TB 60.5 at the end of January 2019.

I suggest to rebase the patch for c-esr60 and replace the "Add" and "Remove" buttons with "+" and "-".
getElementById("bundlePreferences").getString("searchEngine ...") would just be replaced by "". Not ideal, but working.

Could you look into this or should I do the rebasing first?

Comment 33

[Magnus Melin [:mkmelin]]

Why the rush? It never had the ability in the 10 yrs it featured search engines. They are just as possible to add now as they always were.

Comment 34

[Jorg K (CEST = GMT+2)]

I wish you were right, but you are wrong. Please read comment #0 to know why we are here. The hacky manual method doesn't work anymore, see bug 1427167 comment #0.

I came to this bug today since it was reiterated on the Council mailing list that the lack of DuckDuckGo and Google is still very much criticised (bug 1427133), hence what you call "rush".

There's also the concept of retrofitting much needed functionality into the current ESR release, which fortunately we're doing now, for example with WeTransfer and OWL. I know people who are still on TB 52 since their add-ons doen't work on TB 60. So how many people to you think will stick to TB 60 once TB 68 comes around?

May I suggest that you implement the "bells and whistles" solution on trunk and ESR 68 and let me manage ESR 60 in the meantime.

Comment 35

[Jorg K (CEST = GMT+2)]

Attachment: 1427317-searchEngines.patch - ESR 60 version without string changes

Let's see whether it works:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=21dc057e2e375d647dafd35b73309a785d70e4d2

Comment 36

[Jorg K (CEST = GMT+2)]

Attachment: 1427317-searchEngines.patch - ESR 60 version without string changes (v2)

That didn't quite work. Looks like addEngine() has different parameters in ESR 60. Compare:
https://dxr.mozilla.org/mozilla-esr60/source/browser/base/content/test/about/head.js#176
Services.search.addEngine(url, null, "", false, {
to
https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/browser/base/content/test/about/head.js#234
Services.search.addEngine(url, "", false, {

Here we go again:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5baac692ba6a4b15440f30af47f38b31083be28b

Comment 37

[Jorg K (CEST = GMT+2)]

Comment on attachment 9033719 [details] [diff] [review]
1427317-searchEngines.patch - ESR 60 version without string changes (v2)

This works now. The UI isn't brilliant, but better than nothing.

Comment 38

[Richard Marti (:Paenglab)]

Comment on attachment 9033719 [details] [diff] [review]
1427317-searchEngines.patch - ESR 60 version without string changes (v2)

This looks good as far as the UI is limited with the string freeze. It's not a completely new UI as we use the '+' and '-' buttons already in Message Filters.

Shouldn't we get this bug landed also in Daily and beta? But the patch for this needs a rebasing.

Comment 39

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #38)
> Shouldn't we get this bug landed also in Daily and beta? But the patch for
> this needs a rebasing.
I don't know. The patch for trunk is awaiting review for more than a month now since Magnus had a better/different idea which is stalled. I'll rebase it now, then Magnus can decide. As you know, he sees no need to rush this when there is :-(

Comment 40

[Jorg K (CEST = GMT+2)]

Attachment: 1427317-searchEngines.patch (v8) - rebased

Magnus, can you please let us know how to proceed here?

Comment 41

[Jorg K (CEST = GMT+2)]

Attachment: 1427317-searchEngines.patch - ESR 60 version without string changes (v2b)

I didn't like the big buttons, so Richard suggested to use
  style="min-width: 3em;"

I've tried it in the inspector and 2.5em seems best.

Comment 42

[Jorg K (CEST = GMT+2)]

TB 60.5 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/bf7ab364e1d6fda638493767c605c59db36d8ea0

Comment 43

[Jorg K (CEST = GMT+2)]

TB 65 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/0fc0c5dd8de5573e8951af5e00fa6ed821f66670
Mix of the ESR patch and the trunk patch.

Comment 44

[Jorg K (CEST = GMT+2)]

Attachment: 1427317-searchEngines.patch (v9)

Grrrr, currentEngine was renamed to defaultEngine in bug 1510936 :-(

Comment 45

[Jorg K (CEST = GMT+2)]

TB 65 beta 2, follow-up, rename currentEngine to defaultEngine:
https://hg.mozilla.org/releases/comm-beta/rev/636f744e5be4cc85e4c8438b8b87dbe5c3e8beee

Comment 46

[Magnus Melin [:mkmelin]]

Comment on attachment 9033804 [details] [diff] [review]
1427317-searchEngines.patch (v9)

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

Since there's nothing more permanent than a temporary fix, I'd rather not take this on trunk. 
It doesn't even give any clue about what the xml file might be so an end user could not figure this out without reading the source code. Really bad UI in other words.
Then on the other hand, even if this feature is as requested as you say, I dare say it'd be just better to add the few missing engines instead. That's what the users really are after.

Anyway, the patch adding by URL does work (after all) and has little disadvantages. I'll finish it soon.

::: mail/locales/en-US/chrome/messenger/preferences/preferences.properties
@@ +57,5 @@
>  soundFilesDescription=Sound Files
>  
> +#### Search Engine Picker
> +searchEnginePickerTitle=Choose Seach Engine
> +searchEngineType=Seach Engine

missing some r's

Comment 47

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin [:mkmelin] from comment #46)
> Then on the other hand, even if this feature is as requested as you say, I
> dare say it'd be just better to add the few missing engines instead. That's
> what the users really are after.
That would be useful, too: Bug 1427133. However, there are soooo many search engines around that adding those two won't make everyone happy.

Comment 48

[Jorg K (CEST = GMT+2)]

Doing bug 1427133 now. Happy? If users already find the search engines they want, they won't have to use the "really bad UI".

Comment 49

[Jorg K (CEST = GMT+2)]

TB 66 beta:
https://treeherder.mozilla.org/#/jobs?repo=comm-beta&revision=b5802d8134e455897eac08e7750f2139ef3335aa
Merged patch of what went into TB 65 beta.

Magnus, when can we expect this to be landed. Too late now for TB 66 beta, so I need to uplift the solution you don't like again :-(

Comment 50

[Jorg K (CEST = GMT+2)]

(Posted the wrong link)
TB 66 beta:
https://hg.mozilla.org/releases/comm-beta/rev/b5802d8134e455897eac08e7750f2139ef3335aa

Magnus, when can we expect this to be landed. Too late now for TB 67 beta, so I need to uplift the solution you don't like again :-( - And it needs heavy rebasing due to bug 1492475 / bug 1524593.

Comment 51

[Jorg K (CEST = GMT+2)]

Attachment: 1427317-searchEngines-67.patch

This applies to trunk at version 68 on 2019-03-23 and beta 67.

Magnus, you will have to rebase your patch accordingly. Note that engines are now compared by name, like here:
https://searchfox.org/mozilla-central/diff/481ae95c00e75189f71159fc61c9a1af771d112e/browser/components/extensions/parent/ext-search.js#57

Comment 52

[Jorg K (CEST = GMT+2)]

Attachment: 1427317-searchEngines-67.patch (v2)

Played around with this some more, this works, too.
I'm not sure what the difference is between the attribute and the async getter/setter:
https://searchfox.org/mozilla-central/rev/4eaf7c637a303e0f7adcf1ae45064b2d4bef9eb0/toolkit/components/search/nsISearchService.idl#450

Comment 53

[Jorg K (CEST = GMT+2)]

TB 67 beta:
https://hg.mozilla.org/releases/comm-beta/rev/4fa819fdd79f2e0f046cec2a0cbdcb77e6c8c7d5

Comment 54

[Jorg K (CEST = GMT+2)]

Attachment: 1427317-searchEngines-68.patch (v10)

This is like the TB 67 version but with the strings. I also found a bug in TB 60 and TB 67 for which I'll push a fix in order not to forget.

Comment 55

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-beta/rev/bdede91aa0536b97ca94721cf1c1ea7755c4d070
https://hg.mozilla.org/releases/comm-esr60/rev/f050afa30f8001e5e33ca16bf002bd57fa9f209e

Comment 56

[Richard Marti (:Paenglab)]

Comment on attachment 9059090 [details] [diff] [review]
1427317-searchEngines-68.patch (v10)

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

::: mail/locales/en-US/chrome/messenger/preferences/preferences.properties
@@ +56,5 @@
>  soundFilePickerTitle=Choose Sound
>  
> +#### Search Engine Picker
> +searchEnginePickerTitle=Choose Search Engine
> +searchEngineType=Search Engine

Where are this strings used? I don't find them in the code and also see them not in the file picker.

Comment 57

[Jorg K (CEST = GMT+2)]

Attachment: 1427317-searchEngines-68.patch (v10b)

Sigh, those strings got lost due to the various versions of the patch. Here they are again.

Edit: See interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=9059090&action=interdiff&newid=9059203&headers=1

Comment 58

[Richard Marti (:Paenglab)]

Comment on attachment 9059203 [details] [diff] [review]
1427317-searchEngines-68.patch (v10b)

Thanks.

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/af02a4451ba2
Allow addition of search engines from XML file and removal in general pane in preferences. r=Paenglab DONTBUILD