725213 - Add search engines from text input fields

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Brian Nicholson (:bnicholson)]

Currently, the only way to add a search engine to the AwesomeScreen is by installing a search add-on. We can make search engines much easier to install by allowing users to turn any input box into a search engine.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: (1/2) refactor search engines in BrowserApp

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: (2/2) add search engines from text input

This patch allows a user to add a search engine from any text input field's context menu.

Example:
- Navigate to https://bugzilla.mozilla.org
- Long press on the search box
- Name the engine "Bugzilla"
- Type a bug number in the AwesomeScreen search
- Click Bugzilla

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Let's get a build to Madhava to try out.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 595328 [details] [diff] [review]
(2/2) add search engines from text input

If you had the favicon in browser.js, you could do all of this in JS. You could use a promptservice prompt dialog to get the name.

I don't like the back-n-forth between JS and Java for this. I'd like to see if we could find a way to keep this in JS. We could use JS sqlite query on the browser.db IMAGES table to find the favicon for this page.

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: screenshots

giving r? to madhava to confirm design shown in screenshots

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: (2/2) add search engines from text input v2

moved favicon/prompt logic to js

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: (2/2) add search engines from text input v3

minor change that removes the searchEngines.defaultName string and instead uses the hostname of the document URL (when there is no page title)

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 605252 [details] [diff] [review]
(2/2) add search engines from text input v3

>diff --git a/mobile/android/base/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java

>-            if (drawable != null)
>-                faviconView.setImageDrawable(drawable);
>+            faviconView.setImageDrawable(drawable);

Why are we dropping the check? getDrawableFromDataURI still can return null, right?

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+  addEngine: function addEngine(aElement) {

>+        Services.search.addEngineWithDetails(name, favicon, null, null, method, formURL);
>+        let engine = Services.search.getEngineByName(name);
>+        engine.wrappedJSObject._queryCharset = charset;

Is the _queryCharset ever saved by the search engine? I suppose it has to be saved to work.

r+ but leave the null check unless you can explain why it's not needed.

Comment 9

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f48345d39a26
http://hg.mozilla.org/integration/mozilla-inbound/rev/4434a5371bbc

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/f48345d39a26
https://hg.mozilla.org/mozilla-central/rev/4434a5371bbc

Comment 11

[Cristian Nicolae (:xti)]

Add Search Engine option was added on the latest Nightly build. Closing bug as verified fixed on:

Firefox 18.0a1 (2012-09-19)
Device: Galaxy Note
OS: Android 4.0.4

Comment 12

[Florian Quèze [:florian]]

(In reply to Mark Finkle (:mfinkle) from comment #8)

> >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> 
> >+  addEngine: function addEngine(aElement) {
> 
> >+        Services.search.addEngineWithDetails(name, favicon, null, null, method, formURL);
> >+        let engine = Services.search.getEngineByName(name);
> >+        engine.wrappedJSObject._queryCharset = charset;
> 
> Is the _queryCharset ever saved by the search engine? I suppose it has to be
> saved to work.

Relying on wrappedJSObject to access private properties of the engine isn't clean. In the future when something is missing, please consider extending the idl interface.