Closed Bug 1239694 Opened 5 years ago Closed 6 months ago

[gnome] firefox should have a search provider

Categories

(Firefox :: Shell Integration, enhancement, P5)

Product:
Firefox
Component:
Shell Integration
Platform:
All
Linux
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 77
Tracking Flags:
Tracking Status
firefox77 --- fixed

People

(Reporter: bytbox, Assigned: stransky)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Bug 1239694 Implemenet Gnome search provider, r?jhorak,mak
47 bytes, text/x-phabricator-request
Details | Review 
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
Assignee: nobody → stransky
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: stransky → ozoder
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.
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/
Also I think we should provide the search only when Firefox is running so no external application would be needed for that.
Assignee: ozoder → stransky
Type: defect → enhancement

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.

Depends on D54334

Attachment #9110949 - Attachment is obsolete: true
Attachment #9110911 - Attachment is obsolete: true
Attachment #9112849 - Attachment is obsolete: true
Priority: -- → P5
Depends on: 1625850

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

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.

Duplicate of this bug: 1629378
# 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).

Flags: needinfo?(l10n)

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

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

(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.

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.

Flags: needinfo?(l10n)

(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.

Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cee5051ad409
Implemenet Gnome search provider, r=jhorak,mak

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'
...
Flags: needinfo?(stransky)
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/30931766b1b1
Implemenet Gnome search provider, r=jhorak,mak

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

Status: NEW → RESOLVED
Closed: 6 months ago
status-firefox77: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 77
Depends on: 1634292
Depends on: 1634293
Depends on: 1634961
Severity: normal → N/A
You need to log in before you can comment on or make changes to this bug.