1239694 - [gnome] firefox should have a search provider

Status: RESOLVED FIXED

(Firefox :: Shell Integration, enhancement, P5)

Description

[Scott Lawrence]

The gnome-shell search bar shows aggregated results from a collection of "search providers". It would be nice if firefox included a search provider, so that bookmarks, history, and the option to search with the default engine, showed up in gnome's search bar.

AFAIK, epiphany is the only browser to have a search provider. See [1] for the gnome bugtracker discussion explaining why that provider shouldn't just open the default browser. The important part (IMO) is that looking at bookmarks, rss feeds, history, and so forth can't be done in a generic manner.

[1] https://bugzilla.gnome.org/show_bug.cgi?id=720245

Comment 1

[Martin Stránský [:stransky] (ni? me)]

We can utilize the DBusRemoteService here to export search results via DBus. 

toolkit/components/remote/nsDBusRemoteService.* provides public OpenURL() method which enables open url by running Firefox instance. We may also implement the search provider DBus methods here to provide the search privider cabability.

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Looks like we need to implement a dearch provider D-Bus interface described at [1] which should be fairly easy.

[1] https://developer.gnome.org/SearchProvider/

Comment 3

[Martin Stránský [:stransky] (ni? me)]

Also I think we should provide the search only when Firefox is running so no external application would be needed for that.

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1239694 Implemenet Gnome search provider, r?jhorak

Implement org.gnome.Shell.SearchProvider2 D-Bus interface and enable it when
widget.gnome-search-provider.enabled pref is set, so this feature is disabled
by default.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c4b67f7d845231f830eae3a8de807da460635df

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1239694 Added gnome shell system wide config files, r?jhorak

Depends on D54334

Comment 7

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1239694 Use history icons with Gnome shell search provider, r?jhorak

Depends on D54334

Comment 8

[Martin Stránský [:stransky] (ni? me)]

There are some issues which need to be addressed:

• async history search
• correct behavior when there's no result returned from history search
• Fixed Bug 1625850

Comment 9

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1239694 Implemenet Gnome search provider, r?jhorak,mak

Implement org.gnome.Shell.SearchProvider2 D-Bus interface and enable it when
widget.gnome-search-provider.enabled pref is set, so this feature is disabled
by default.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a9caf80c895023c5d5089c4b5485b66a2943dd6

Comment 12

[Francesco Lodolo [:flod]]

# LOCALIZATION NOTE (gnomeSearchProviderSearch):
# Used for search by Gnome Shell activity screen, %s is a searched string.
gnomeSearchProviderSearch=Search the web for “%s”

I pointed out that %s is normally not correct. Martin's explanation:

%S does not work. IIUC %S means UTF16 and %s UTF8 encoding and gnome-shell/dbus use UTF8 so we can't use %S here.
There may be confusion here as gnomeSearchProviderSearch string is not used by Firefox but it's send over DBus which expects UTF8 only.

This kind of matches my memory of the issues we had in the past, so it seems wanted.

Having said that, do you expect any problem, at least in our l10n toolchain? I checked and compare-locales reports an ERROR if locales change from %s to %S, which is good. I remember we added checks for printf, no clue if that's relevant here (bug 1388789).

Comment 13

[Martin Stránský [:stransky] (ni? me)]

Francesco, I'll remove the %s from the translated string if that causes troubles. It's not necessary to have it there.

Comment 14

[Martin Stránský [:stransky] (ni? me)]

Okay, I'll fix the patch to use %S and then I can convert it to UTF8, there's no big deal here.

Comment 15

[Francesco Lodolo [:flod]]

(In reply to Martin Stránský [:stransky] from comment #14)

Okay, I'll fix the patch to use %S and then I can convert it to UTF8, there's no big deal here.

I'd wait for Axel to chime in. I might be just overthinking this, and %s might be OK to use.

Comment 16

[Axel Hecht [:Pike]]

So, we'll need to change the l10n pieces here a bit. printf doesn't equal printf, and caching localized strings is not best practice.

Here's what I'd ask you to do:

Create a string bundle, cache that. Clear the cache on intl:app-locales-changed, https://firefox-source-docs.mozilla.org/intl/locale.html#events. Use bundle->FormatStringFromName.

Or cache the string, but also clear that cache on intl:app-locales-changed, but use nsTextFormatter.

Ending up in nsTextFormatter is important, as that's built to not crash on misplaced placeable parameters. And that's important as localizers love to get these wrong.

If you're going through FormatStringFromName, you'll want to use %S, if you're using nsTextFormatter, use whatever is appropriate. You'll find that it doesn't care much, it'll replace the format with the right option for the param you passed in.

Blowing away the cache will get you multi-locale support (more important on linux, too), nsTextFormatter gives you non-crashyness.

I would have loved to point directly to Fluent, but I don't think that using from C++ is a good idea before we have the localization classes outside of js.

Comment 17

[Martin Stránský [:stransky] (ni? me)]

(In reply to Axel Hecht [:Pike] from comment #16)

So, we'll need to change the l10n pieces here a bit. printf doesn't equal printf, and caching localized strings is not best practice.

Here's what I'd ask you to do:

Create a string bundle, cache that. Clear the cache on intl:app-locales-changed, https://firefox-source-docs.mozilla.org/intl/locale.html#events. Use bundle->FormatStringFromName.

Or cache the string, but also clear that cache on intl:app-locales-changed, but use nsTextFormatter.

Ending up in nsTextFormatter is important, as that's built to not crash on misplaced placeable parameters. And that's important as localizers love to get these wrong.

If you're going through FormatStringFromName, you'll want to use %S, if you're using nsTextFormatter, use whatever is appropriate. You'll find that it doesn't care much, it'll replace the format with the right option for the param you passed in.

Blowing away the cache will get you multi-locale support (more important on linux, too), nsTextFormatter gives you non-crashyness.

Thanks. As I have bundle->FormatStringFromName in place I'll add the intl:app-locales-changed observer to clear the case.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cee5051ad409
Implemenet Gnome search provider, r=jhorak,mak

Comment 19

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset cee5051ad409 (bug 1239694) for bustages complaining about dbus

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=cee5051ad4093393232950f16d42e50cc66d9c29&searchStr=linux%2Cbuild&tochange=766147895c754341bc633c879a4b7b34f675ce82

Backout link: https://hg.mozilla.org/integration/autoland/rev/766147895c754341bc633c879a4b7b34f675ce82

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=299966808&repo=autoland&lineNumber=16015

...
[task 2020-04-29T11:12:59.094Z] 11:12:59     INFO -  make[4]: Entering directory '/builds/worker/workspace/obj-build/xpcom/components'
[task 2020-04-29T11:12:59.102Z] 11:12:59     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ -std=gnu++17 -o StaticComponents.o -c  -I/builds/worker/workspace/obj-build/dist/stl_wrappers -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -ftrivial-auto-var-init=pattern -DDEBUG=1 -DMOZ_LAYOUT_DEBUGGER -DOS_POSIX=1 -DOS_LINUX=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/xpcom/components -I/builds/worker/workspace/obj-build/xpcom/components -I/builds/worker/workspace/obj-build/xpcom -I/builds/worker/checkouts/gecko/xpcom/base -I/builds/worker/checkouts/gecko/xpcom/build -I/builds/worker/checkouts/gecko/xpcom/ds -I/builds/worker/checkouts/gecko/chrome -I/builds/worker/checkouts/gecko/js/xpconnect/loader -I/builds/worker/checkouts/gecko/layout/build -I/builds/worker/checkouts/gecko/modules/libjar -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/checkouts/gecko/ipc/glue -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -Qunused-arguments -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Wunused-function -Wunused-variable -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-error=tautological-type-limit-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fcrash-diagnostics-dir=/builds/worker/artifacts -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -Os -fno-omit-frame-pointer -funwind-tables -Werror -I/builds/worker/checkouts/gecko/widget/gtk/compat-gtk3 -pthread -I/usr/include/gtk-3.0 -I/usr/include/atk-1.0 -I/usr/include/at-spi2-atk/2.0 -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/ -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/harfbuzz -I/usr/include/freetype2 -I/usr/include/pixman-1 -I/usr/include/libpng12 -I/usr/include/gtk-3.0/unix-print -fexperimental-new-pass-manager  -MD -MP -MF .deps/StaticComponents.o.pp   StaticComponents.cpp
[task 2020-04-29T11:12:59.103Z] 11:12:59     INFO -  In file included from StaticComponents.cpp:145:
[task 2020-04-29T11:12:59.103Z] 11:12:59     INFO -  In file included from /builds/worker/checkouts/gecko/xpcom/components/../../browser/components/shell/nsGNOMEShellService.h:14:
[task 2020-04-29T11:12:59.103Z] 11:12:59     INFO -  In file included from /builds/worker/checkouts/gecko/xpcom/components/../../browser/components/shell/nsGNOMEShellSearchProvider.h:11:
[task 2020-04-29T11:12:59.103Z] 11:12:59     INFO -  In file included from /builds/worker/workspace/obj-build/dist/include/mozilla/DBusHelpers.h:10:
[task 2020-04-29T11:12:59.103Z] 11:12:59     INFO -  /builds/worker/workspace/obj-build/dist/system_wrappers/dbus/dbus.h:3:15: fatal error: 'dbus/dbus.h' file not found
[task 2020-04-29T11:12:59.103Z] 11:12:59     INFO -  #include_next <dbus/dbus.h>
[task 2020-04-29T11:12:59.103Z] 11:12:59     INFO -                ^~~~~~~~~~~~~
[task 2020-04-29T11:12:59.104Z] 11:12:59     INFO -  1 error generated.
[task 2020-04-29T11:12:59.104Z] 11:12:59     INFO -  /builds/worker/checkouts/gecko/config/rules.mk:750: recipe for target 'StaticComponents.o' failed
[task 2020-04-29T11:12:59.104Z] 11:12:59    ERROR -  make[4]: *** [StaticComponents.o] Error 1
[task 2020-04-29T11:12:59.104Z] 11:12:59     INFO -  make[4]: Leaving directory '/builds/worker/workspace/obj-build/xpcom/components'
[task 2020-04-29T11:12:59.104Z] 11:12:59     INFO -  /builds/worker/checkouts/gecko/config/recurse.mk:74: recipe for target 'xpcom/components/target-objects' failed
[task 2020-04-29T11:12:59.104Z] 11:12:59    ERROR -  make[3]: *** [xpcom/components/target-objects] Error 2
[task 2020-04-29T11:12:59.104Z] 11:12:59     INFO -  make[3]: *** Waiting for unfinished jobs....
[task 2020-04-29T11:12:59.143Z] 11:12:59     INFO -  make[4]: Entering directory '/builds/worker/workspace/obj-build/media/libdav1d/asm'
...

Comment 20

[Martin Stránský [:stransky] (ni? me)]

Thanks, updated.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd332797255b8e62c0a454b3c12cef730ea78833

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30931766b1b1
Implemenet Gnome search provider, r=jhorak,mak

Comment 22

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/30931766b1b1