Closed Bug 852608 Opened 7 years ago Closed 6 years ago

Add support for OpenSearch. Firefox Mobile ignores <link rel="search">

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 28
Tracking Status
firefox28 --- verified
relnote-firefox --- 28+

People

(Reporter: bugs, Assigned: liuche)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 20 obsolete files)

2.79 KB, patch
liuche
: review+
Details | Diff | Splinter Review
5.79 KB, patch
liuche
: review+
Details | Diff | Splinter Review
14.01 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
So, after a bunch of hunting around for a way to add a number of search engines I wanted (google.fr, fr.wiktionary.org and fr.wikipedia.org)  I asked #mobile where cpeterson linked me to:
http://www.cpeterso.com/blog/2012/10/adding-custom-search-engines-to-firefox-for-android/

Which I appreciate...
But. That's search field specific.

A few objections
1) It has to occur to you that long touching in a site search field is what one should do.  I'd checked under "More" in the site nav where things like "Save as PDF" and "Request Desktop Site" and "Find in Page" are stashed, but it didn't occur to me I should be long touching the search field that I hardly ever used.
Heck, after he linked to his article, I still long touched several times on the URL bar thinking that must be where it was really hidden (I found several more functions by doing that that weren't in "More" either BTW).

2) The site search exposed by link rel="search" may not be the same as the search field you long touched on.  I'd just as soon have the "official" search the site admins think is useful for browser search engines,
even if it may well be that long touch will work just as well for the 3 cases I'd listed.

3) I occasionally toss opensearch XML on my site that I constructed for some sucky sites w/ truly horrible searches or lack thereof.  I've constructed these after untangling their interfaces, or just doing google "site:" searches.  I'm probably a rare case, but it'd be nice if this worked for mobile.  I've occasionally run into sites where someone made a custom opensearch xml like this for the use of others tho.
This sounds like a good request. We'll just have to figure out what UI we want to expose these search engines, since we don't have a search box the same way desktop does.
Eh. Just toss an option in the menu under More that says:
Add "Wiktionaire (fr)" as Search Engine.

It'll then get added to list of engines.
And since I now know that list is in Addons, I can go there to disable/remove it.

BTW. On the subject of that Addons page.  That's a decent way to enable/disable search engines... But not so hot for determining order they should show up in dropdown.  I tried long-touch on "Wikipedia Integrated Search" to see if I would be able to drag it to the top of the Addon list to get it to show up first in list of suggested searches, but, all that was available was a single option "Disable" which is also available if I touch it to view it.
Oh. And, to be clear, you guys have search integrated w/ URL bar (which is why I still don't have google suggest turned on).
Sooo, obviously I'd expect new search engines to be in that list you guys show that prompts me to search on the words typed in the URL bar.
It'd be nice if I was prompted to turn on suggest, actually.  I might possibly maybe want it for fr.wiktionary.org - or at least, my desire to have my spelling clarified might outweigh privacy.
Actually, I'm a bit puzzled why you only prompt me to enable google suggest, and not, oh, suggest for Amazon or Twitter.
Does it only prompt for the first search in the list?  If so, all the more reason to allow me to reorganise them to put my preferred search at the top.
You're touching on a lot of different issues here. Let's just keep this bug about exposing open search support.

The "More" menuitem you're talking about doesn't exist on ICS and higher, and we generally don't like to just throw in new menuitems without some thought (the menu can really grow out of control), so it would be good to consider our options here before just adding a new menuitem, even if that's what we end up doing.

Selecting a different default search engine is being covered in bug 730445. We've talked about adding search suggestions for multiple search engines, but the implementations would be different (for example, bug 851969 was filed about amazon.com search suggestions). Feel free to file another bug about reordering the list of search engines, although I'm not sure if we'd want to go through the trouble of implementing that (the order comes from the search service, which is implemented in toolkit).
I am working on adding "feed" support (<link rel="alternate">) and have seen the search provider code (<link rel="search">) code too. For feeds I am planning to use the context menu for the URLBar. That is our "page menu" in Fennec. I think we'd do the same thing for search engines. Let's not add anything to the main menu. Not until we have a clear "page" submenu there.
Assignee: nobody → mark.finkle
Attached patch WIP v1 (obsolete) — Splinter Review
Not complete, but saving my place
Assignee: mark.finkle → liuche
Abandoned attempts to update search box to use link from opensearch xml; part 2 is for adding opensearch to contextmenu for urlbar.
Attachment #728692 - Attachment is obsolete: true
To expand on comment #7 re: search box: since Android currently supports "Add as Search Engine" for every form field, there's not a particularly clean way to determine which field matches the open search tag provided by page.

Desktop is slightly smarter about what form fields they support:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1486
related to search engine fields: bug 790898
Attachment #817913 - Attachment is obsolete: true
Attachment #819289 - Attachment is obsolete: true
Attachment #819288 - Attachment is obsolete: true
Attachment #821063 - Attachment description: Part 3: Add UI for adding a search engine. → WIP Part 3: Add UI for adding a search engine.
Attachment #821065 - Attachment description: Part 2: Support adding search engines from xml v2. → WIP Part 2: Support adding search engines from xml v2.
Attachment #821066 - Attachment description: Part 1: Support opensearch <rel="search"> tag v2. → WIP Part 1: Support opensearch <rel="search"> tag v2.
Still need to finish:
- handle cases to remove "Add engine" from context menu (new page, engine just added, engine already added)
- strings
Attachment #821066 - Attachment is obsolete: true
Attachment #823411 - Flags: feedback?(mark.finkle)
Attachment #823411 - Flags: feedback?(lucasr.at.mozilla)
Attachment #821065 - Attachment is obsolete: true
Attachment #823413 - Flags: feedback?(mark.finkle)
Attachment #823413 - Flags: feedback?(lucasr.at.mozilla)
Attachment #821063 - Attachment is obsolete: true
Attachment #823415 - Flags: feedback?(mark.finkle)
Attachment #823415 - Flags: feedback?(lucasr.at.mozilla)
This is almost there. The biggest problem is that the engine name is not removed from the urlbar context menu when you leave the page. I tried listening for pagehide/pageshow events as a cue to hide the contextmenu item, but those fire after the DOMLinkAdded events, so that doesn't really work.
(In reply to Chenxia Liu [:liuche] from comment #19)
> This is almost there. The biggest problem is that the engine name is not
> removed from the urlbar context menu when you leave the page. I tried
> listening for pagehide/pageshow events as a cue to hide the contextmenu
> item, but those fire after the DOMLinkAdded events, so that doesn't really
> work.

It looks like desktop clears out the browser.engines (and browser.feeds) in the onStateChange where the NETWORK | START bits are set:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3705

For fennec that is in here:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3629
Comment on attachment 823411 [details] [diff] [review]
Part 1: support opensearch <rel="search"> tag v3

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

Looks good, just wonder if using title to match engines is a good enough way to go here.

::: mobile/android/chrome/content/browser.js
@@ +3474,5 @@
>              };
>              sendMessageToJava(json);
>            } catch (e) {}
> +        } else if (list.indexOf("[search]" != -1)) {
> +          let type = target.type && target.type.toLowerCase();

What if target.type is undefined? You'll end up with type being undefined, no? This might cause problems on type.replace() call below.

nit: I'd add an empty line here.

@@ +3476,5 @@
>            } catch (e) {}
> +        } else if (list.indexOf("[search]" != -1)) {
> +          let type = target.type && target.type.toLowerCase();
> +          // Replace all starting or trailing spaces or spaces before "*;" globally w/ "".
> +          type = type.replace(/^\s+|\s*(?:;.*)?$/g, "");

nit: add here.

@@ +3478,5 @@
> +          let type = target.type && target.type.toLowerCase();
> +          // Replace all starting or trailing spaces or spaces before "*;" globally w/ "".
> +          type = type.replace(/^\s+|\s*(?:;.*)?$/g, "");
> +          // Check that type matches opensearch.
> +          let isEngine = (type == "application/opensearchdescription+xml");

isOpenSearch?

@@ +3479,5 @@
> +          // Replace all starting or trailing spaces or spaces before "*;" globally w/ "".
> +          type = type.replace(/^\s+|\s*(?:;.*)?$/g, "");
> +          // Check that type matches opensearch.
> +          let isEngine = (type == "application/opensearchdescription+xml");
> +          if (isEngine && target.title && /^(?:https?|ftp):/i.test(target.href)) {

I wonder if using title to match engines is reliable enough? Maybe something like a URL would be better? Not sure how feasible this would be though.

@@ +3480,5 @@
> +          type = type.replace(/^\s+|\s*(?:;.*)?$/g, "");
> +          // Check that type matches opensearch.
> +          let isEngine = (type == "application/opensearchdescription+xml");
> +          if (isEngine && target.title && /^(?:https?|ftp):/i.test(target.href)) {
> +            let debugEngine = { title: target.title, href: target.href };

Leftover?

@@ +6559,5 @@
>    PREF_SUGGEST_ENABLED: "browser.search.suggest.enabled",
>    PREF_SUGGEST_PROMPTED: "browser.search.suggest.prompted",
>  
>    init: function init() {
> +    Services.obs.addObserver(this, "SearchEngines:Add", false);

It seems like this belongs to the next patch.

@@ +6574,5 @@
>      this._contextMenuId = NativeWindow.contextmenus.add(contextName, filter, this.addEngine);
>    },
>  
>    uninit: function uninit() {
> +    Services.obs.removeObserver(this, "SearchEngines:Add");

Same here, move it to the next patch.
Attachment #823411 - Flags: feedback?(lucasr.at.mozilla) → feedback+
Comment on attachment 823413 [details] [diff] [review]
Part 2: Support adding search engines from xml v3

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

::: mobile/android/chrome/content/browser.js
@@ +6698,5 @@
> +  addOpensearchEngine: function addOpensearchEngine(aData) {
> +    let engine = JSON.parse(aData);
> +
> +    if (Services.search.getEngineByName(engine.title)) {
> +      NativeWindow.toast.show(Strings.browser.formatStringFromName("alertSearchEngineExistsToast", [engine.title], 1), "short");

Edge case, right?

@@ +6703,5 @@
> +      return;
> +    }
> +
> +    // We only ever handle opensearch here.
> +    let typeXML = Components.interfaces.nsISearchEngine.DATA_XML;

You can do Ci.nsISearchEngine.DATA_XML. Why not just inline this in the method call?

@@ +6706,5 @@
> +    // We only ever handle opensearch here.
> +    let typeXML = Components.interfaces.nsISearchEngine.DATA_XML;
> +
> +    // Display a toast confirming addition of new search engine.
> +    let notification_cb = { onSuccess: function() NativeWindow.toast.show(Strings.browser.formatStringFromName("alertSearchEngineAddedToast", [engine.title], 1), "long") };

nit: we don't usually use underline in variable names in our js code.

It would be nice if you created a local util function to make these calls look shorter. Something like:

function formatStringFromName(...) {
    Strings.browser.formatStringFromName(...)
}

@@ +6707,5 @@
> +    let typeXML = Components.interfaces.nsISearchEngine.DATA_XML;
> +
> +    // Display a toast confirming addition of new search engine.
> +    let notification_cb = { onSuccess: function() NativeWindow.toast.show(Strings.browser.formatStringFromName("alertSearchEngineAddedToast", [engine.title], 1), "long") };
> +    Services.search.addEngine(engine.url, typeXML, engine.iconURL, false, notification_cb);

I'd prefer something like:

Services.search.addEngine(engine.url, typeXML, engine.iconURL, false, {
  onSuccess: function() {
  }
});

Just a personal-taste thing though. Not a big deal.
Attachment #823413 - Flags: feedback?(lucasr.at.mozilla) → feedback+
Comment on attachment 823415 [details] [diff] [review]
Part 3: Add UI fro adding a search engine v2

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

::: mobile/android/base/BrowserApp.java
@@ +740,5 @@
>              return true;
>          }
>  
> +        if (itemId == R.id.add_search_engine) {
> +            Tab tab = Tabs.getInstance().getSelectedTab();

final

@@ +745,5 @@
> +            if (tab != null) {
> +                Tab.SearchEngine engine = tab.getSearchEngine();
> +                if (engine != null) {
> +                    try {
> +                        JSONObject message = new JSONObject();

final

@@ +748,5 @@
> +                    try {
> +                        JSONObject message = new JSONObject();
> +                        message.put("title", engine.mTitle);
> +                        message.put("url", engine.mUrl);
> +                        message.put("iconURL", engine.mIconUrl);

nit: add empty line here.

@@ +754,5 @@
> +                    } catch (JSONException e) {
> +                        Log.e(LOGTAG, "Error making SearchEngine:Add message.", e);
> +                        return true;
> +                    }
> +                    tab.clearSearchEngine();

Shouldn't you only clear the engine if you've successfully added the engine?

::: mobile/android/base/BrowserToolbar.java
@@ +352,5 @@
>                          menu.findItem(R.id.add_to_launcher).setVisible(false);
>                      }
>                      if (!tab.getFeedsEnabled()) {
>                          menu.findItem(R.id.subscribe).setVisible(false);
>                      }

nit: add empty line here.

@@ +353,5 @@
>                      }
>                      if (!tab.getFeedsEnabled()) {
>                          menu.findItem(R.id.subscribe).setVisible(false);
>                      }
> +                    Tab.SearchEngine searchEngine = tab.getSearchEngine();

final

@@ +354,5 @@
>                      if (!tab.getFeedsEnabled()) {
>                          menu.findItem(R.id.subscribe).setVisible(false);
>                      }
> +                    Tab.SearchEngine searchEngine = tab.getSearchEngine();
> +                    MenuItem menuItem = menu.findItem(R.id.add_search_engine);

final

@@ +356,5 @@
>                      }
> +                    Tab.SearchEngine searchEngine = tab.getSearchEngine();
> +                    MenuItem menuItem = menu.findItem(R.id.add_search_engine);
> +                    if (searchEngine != null) {
> +                        menuItem.setTitle(menuItem.getTitle().toString().replace("%s", searchEngine.mTitle));

Android's formatted strings are probably a more robust approach. Have a look at:
http://developer.android.com/guide/topics/resources/string-resource.html#FormattingAndStyling

::: mobile/android/base/Tab.java
@@ +87,5 @@
>  
> +    public static class SearchEngine {
> +        public String mTitle;
> +        public String mUrl;
> +        public String mIconUrl;

Make all the members 'final' so that SearchEngine instances are immutable.

@@ +285,5 @@
> +    public SearchEngine getSearchEngine() {
> +        return mSearchEngine;
> +    }
> +
> +    public void addSearchEngine(String title, String url, String iconUrl) {

Isn't this more like a setSearchEngine?

::: mobile/android/base/Tabs.java
@@ +421,5 @@
>              } else if (event.equals("Tab:Select")) {
>                  selectTab(tab.getId());
>              } else if (event.equals("Content:LocationChange")) {
>                  tab.handleLocationChange(message);
> +            } else if (event.equals("Content:NewSearch")) {

Maybe 'Link:Opensearch' would be more consistent and clear?
Attachment #823415 - Flags: feedback?(lucasr.at.mozilla) → feedback+
Nits addressed, comments inline.

(In reply to Lucas Rocha (:lucasr) from comment #21)
> Comment on attachment 823411 [details] [diff] [review]
> Part 1: support opensearch <rel="search"> tag v3
> 
> Review of attachment 823411 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, just wonder if using title to match engines is a good enough way
> to go here.
> 

Unfortunately, the nsISearchService only supports matching engines by name, and I added a comment referencing bug 335102.

> ::: mobile/android/chrome/content/browser.js
> @@ +3474,5 @@
> >              };
> >              sendMessageToJava(json);
> >            } catch (e) {}
> > +        } else if (list.indexOf("[search]" != -1)) {
> > +          let type = target.type && target.type.toLowerCase();
> 
> What if target.type is undefined? You'll end up with type being undefined,
> no? This might cause problems on type.replace() call below.
> 

Thanks for pointing this out - I was using code from desktop but looking into it, even if the type is undefined, String.replace first calls .toString() on the object if it's not a String type, so this will still work.

> @@ +3479,5 @@
> > +          // Replace all starting or trailing spaces or spaces before "*;" globally w/ "".
> > +          type = type.replace(/^\s+|\s*(?:;.*)?$/g, "");
> > +          // Check that type matches opensearch.
> > +          let isEngine = (type == "application/opensearchdescription+xml");
> > +          if (isEngine && target.title && /^(?:https?|ftp):/i.test(target.href)) {
> 
> I wonder if using title to match engines is reliable enough? Maybe something
> like a URL would be better? Not sure how feasible this would be though.

I'm not entirely sure what you mean here - is this regarding the same problem of differentiating by url vs name (which nsISearchEngine doesn't support yet) or is this regarding the check for whether something is an (opensearch) engine?
Attachment #823411 - Attachment is obsolete: true
Attachment #823411 - Flags: feedback?(mark.finkle)
(In reply to Lucas Rocha (:lucasr) from comment #22)
> Comment on attachment 823413 [details] [diff] [review]
> Part 2: Support adding search engines from xml v3
> 
> Review of attachment 823413 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +6698,5 @@
> > +  addOpensearchEngine: function addOpensearchEngine(aData) {
> > +    let engine = JSON.parse(aData);
> > +
> > +    if (Services.search.getEngineByName(engine.title)) {
> > +      NativeWindow.toast.show(Strings.browser.formatStringFromName("alertSearchEngineExistsToast", [engine.title], 1), "short");
> 
> Edge case, right?

You're right - I removed this, because we should definitely handle this.

> 
> @@ +6703,5 @@
> > +      return;
> > +    }
> > +
> > +    // We only ever handle opensearch here.
> > +    let typeXML = Components.interfaces.nsISearchEngine.DATA_XML;
> 
> You can do Ci.nsISearchEngine.DATA_XML. Why not just inline this in the
> method call?

Done.

> 
> @@ +6706,5 @@
> > +    // We only ever handle opensearch here.
> > +    let typeXML = Components.interfaces.nsISearchEngine.DATA_XML;
> > +
> > +    // Display a toast confirming addition of new search engine.
> > +    let notification_cb = { onSuccess: function() NativeWindow.toast.show(Strings.browser.formatStringFromName("alertSearchEngineAddedToast", [engine.title], 1), "long") };
> 
> nit: we don't usually use underline in variable names in our js code.
> 
> It would be nice if you created a local util function to make these calls
> look shorter. Something like:
> 
> function formatStringFromName(...) {
>     Strings.browser.formatStringFromName(...)
> }

I took a look at this; Strings.browser lazy-loads the browser.properties strings, and since the assignment to Strings isn't hoisted, using Strings.browser in a helper function (formatStringFromName) didn't work. I'm not sure if there's a better way to wrap a lazy-loaded string bundle in a top-level function in JS?

> @@ +6707,5 @@
> > +    let typeXML = Components.interfaces.nsISearchEngine.DATA_XML;
> > +
> > +    // Display a toast confirming addition of new search engine.
> > +    let notification_cb = { onSuccess: function() NativeWindow.toast.show(Strings.browser.formatStringFromName("alertSearchEngineAddedToast", [engine.title], 1), "long") };
> > +    Services.search.addEngine(engine.url, typeXML, engine.iconURL, false, notification_cb);
> 
> I'd prefer something like:
> 
> Services.search.addEngine(engine.url, typeXML, engine.iconURL, false, {
>   onSuccess: function() {
>   }
> });
> 
> Just a personal-taste thing though. Not a big deal.

Done, thanks for the JS style suggestions! I haven't written enough JS to have a feel for the style, so I appreciate any and all style pointers.
Attachment #823413 - Attachment is obsolete: true
Attachment #823413 - Flags: feedback?(mark.finkle)
I'm going to finish the nested context menu work for displaying a list of search engines addressing part 3, because a lot of that is getting ripped out of Java (and moved into Gecko like the feeds stuff).
(In reply to Chenxia Liu [:liuche] from comment #24)

> > I wonder if using title to match engines is reliable enough? Maybe something
> > like a URL would be better? Not sure how feasible this would be though.
> 
> I'm not entirely sure what you mean here - is this regarding the same
> problem of differentiating by url vs name (which nsISearchEngine doesn't
> support yet) or is this regarding the check for whether something is an
> (opensearch) engine?

Right. I think "by name" is the best we have.
Comment on attachment 823415 [details] [diff] [review]
Part 3: Add UI fro adding a search engine v2

An alternate proposal:

You are sending all the meta data from JS to Java. But then we need to send it back to JS when the user wants to add the search engine.

Instead, why not do something like the Feeds code. The feeds are kept in the JS side in the Tab.browser.feeds array. We send a ping to Java to let the menu know that we have at least 1 feed. That will enable the "Subscribe to Page" menu. Tapping the menu sends a message to JS, which just subscribes to Tab.browser.feeds[0] if there is only 1 feed, or shows a Prompt if there are more than 1 feed.

We could do the same for search engines. Less code on the Java side.

f- just to start the discussion
Attachment #823415 - Flags: feedback?(mark.finkle) → feedback-
Comment on attachment 824807 [details] [diff] [review]
Part 1: support opensearch <rel="search"> tag v4

># HG changeset patch
># Parent 855da6d8a327c277e9653911b3244c3229134363
>Bug 852608 - Part 1: Support opensearch <rel="search"> tag.
>
>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+            // Broadcast message about a new search engine.
>+            let newEngineMessage = {
>+              type: "Content:NewSearch",

"Link:OpenSearch" for consistency.

>+              tabID: this.id,
>+              title: target.title,
>+              url: target.href,
>+              iconURL: iconURL

Why not add these to Tab.browser.engines, like we do for feeds?
Absolutely! I actually have some WIP patches on top the two I updated, doing exactly that. Thank you for pointing me towards the feeds code in comment #20, I hadn't thought to look there initially.
Attachment #824807 - Attachment is obsolete: true
Attachment #824810 - Attachment is obsolete: true
Attachment #823415 - Attachment is obsolete: true
Attachment #825599 - Attachment is obsolete: true
Attachment #826176 - Flags: review?(lucasr.at.mozilla)
Attachment #826176 - Flags: feedback?(mark.finkle)
Attachment #825600 - Attachment is obsolete: true
Attachment #826177 - Flags: review?(lucasr.at.mozilla)
Attachment #826177 - Flags: feedback?(mark.finkle)
Attachment #825601 - Attachment is obsolete: true
Attachment #826178 - Flags: review?(lucasr.at.mozilla)
Attachment #826178 - Flags: feedback?(mark.finkle)
These patches handle adding search engines exposed by the OpenSearch standard via menus exposed by long-tapping the urlbar.

These don't handle re-adding an engine exposed by a page if the user removes the engine from Search Settings and then does not refresh the page. This will be handled by a follow-up.

One feature I'd like to add after this patch series lands is displaying a search engine in the list of search shortcuts if the current page exposes it as an OpenSearch engine (without adding the engine to list of user-added search engines), perhaps right under the default engine.
Comment on attachment 826176 [details] [diff] [review]
Part 1: Support opensearch search tag v6

Just as I imagined it! I hope that means it's right :)
Attachment #826176 - Flags: feedback?(mark.finkle) → feedback+
Comment on attachment 826177 [details] [diff] [review]
Part 2: Suport add OpenSearch engines v6

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

>+  addOpenSearchEngine: function addOpenSearchEngine(engine) {
>+    Services.search.addEngine(engine.url, Ci.nsISearchEngine.DATA_XML, engine.iconURL, false, {
>+      onSuccess: function() {
>+                   // Display a toast confirming addition of new search engine.
>+                   NativeWindow.toast.show(Strings.browser.formatStringFromName("alertSearchEngineAddedToast", [engine.title], 1), "long")

You only need to indent these lines 1-indent
Attachment #826177 - Flags: feedback?(mark.finkle) → feedback+
Comment on attachment 826178 [details] [diff] [review]
Part 3: Add UI for adding an OpenSearch engine v4

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

>                     if (!tab.getFeedsEnabled()) {
>                         menu.findItem(R.id.subscribe).setVisible(false);
>                     }
>+
>+                    if (tab.getOpenSearchEnginesVisible()) {
>+                        menu.findItem(R.id.add_search_engine).setVisible(true);
>+                    } else {
>+                        menu.findItem(R.id.add_search_engine).setVisible(false);
>+                    }

I could be wrong, but since the menu has visible set to false, you only need to make it visible, not make it hidde. Which is why I think the Feeds one works.

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

>     private boolean mFeedsEnabled;

>+    private boolean mOpenSearchEnginesVisible;

Naming thought: mOpenSearchVisible
                getOpenSearchVisible
                setOpenSearchVisible

Also, since Feeds and OpenSearch are the same pattern, could you rename:
  mFeedsEnabled -> mFeedsVisible
  getFeedsEnabled -> getFeedsVisible
  setFeedsEnabled -> setFeedsVisible

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

>+            } else if (event.equals("Link:OpenSearch")) {
>+                boolean visible = message.getBoolean("visible");
>+                tab.setOpenSearchEnginesVisible(visible);

Just my OCD, but you could move this down to where Link:Feed is handled, keeping them together.

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!ENTITY contextmenu_add_search_engine2 "Add a search engine…">

* Why the "2"?
* We are still using title case for menus (bug filed) so let's keep doing that. And no ellipsis.

"Add a Search Engine"

>diff --git a/mobile/android/base/resources/menu/titlebar_contextmenu.xml b/mobile/android/base/resources/menu/titlebar_contextmenu.xml

>+    <item android:id="@+id/add_search_engine"
>+          android:title="@string/contextmenu_add_search_engine"
>+          android:visible="false"/>

Ian might have a different viewpoint, but for now, let's insert the menu right below the Subscribe menu

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

>+      // Clear page-specific opensearch engines and feeds for a new request.
>+      if (aStateFlags & Ci.nsIWebProgressListener.STATE_START && aRequest && aWebProgress.isTopLevel) {
>+          this.browser.engines = null;
>+          this.browser.feeds = null;
>+      }

This seems like it should work


Overall, this looks like the right direction.
Attachment #826178 - Flags: feedback?(mark.finkle) → feedback+
(In reply to Mark Finkle (:mfinkle) from comment #40)
This would have been a nice use case for Bug 853700. :)
(In reply to Wesley Johnston (:wesj) from comment #41)
> (In reply to Mark Finkle (:mfinkle) from comment #40)
> This would have been a nice use case for Bug 853700. :)

It still could be. Although, the whole GeckoView thing is making me rethink some of my earlier opinions on doing all (or most) of our core logic in JS.
(In reply to Lucas Rocha (:lucasr) from comment #23)
> Comment on attachment 823415 [details] [diff] [review]
> Part 3: Add UI fro adding a search engine v2
> 
> Review of attachment 823415 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +740,5 @@
> >              return true;
> >          }
> >  
> > +        if (itemId == R.id.add_search_engine) {
> > +            Tab tab = Tabs.getInstance().getSelectedTab();
> 
> final
> 
> @@ +745,5 @@
> > +            if (tab != null) {
> > +                Tab.SearchEngine engine = tab.getSearchEngine();
> > +                if (engine != null) {
> > +                    try {
> > +                        JSONObject message = new JSONObject();
> 
> final

I can make these final, but for consistency, I should probably change the other context menu items too - what do you think?
Attachment #826177 - Attachment is obsolete: true
Attachment #826177 - Flags: review?(lucasr.at.mozilla)
Attachment #826871 - Flags: review?(lucasr.at.mozilla)
Renamed and restructured per comments.
Attachment #826178 - Attachment is obsolete: true
Attachment #826178 - Flags: review?(lucasr.at.mozilla)
Attachment #826873 - Flags: review?(lucasr.at.mozilla)
(In reply to Mark Finkle (:mfinkle) from comment #40)
> Comment on attachment 826178 [details] [diff] [review]
> Part 3: Add UI for adding an OpenSearch engine v4
> 
> >diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java
> 
> >                     if (!tab.getFeedsEnabled()) {
> >                         menu.findItem(R.id.subscribe).setVisible(false);
> >                     }
> >+
> >+                    if (tab.getOpenSearchEnginesVisible()) {
> >+                        menu.findItem(R.id.add_search_engine).setVisible(true);
> >+                    } else {
> >+                        menu.findItem(R.id.add_search_engine).setVisible(false);
> >+                    }
> 
> I could be wrong, but since the menu has visible set to false, you only need
> to make it visible, not make it hidde. Which is why I think the Feeds one
> works.

I wanted to remove the "Add a Search Engine" when all the opensearch engines had been added, so I think we do need to explicitly hide this context menu item.

> >diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd
> 
> >+<!ENTITY contextmenu_add_search_engine2 "Add a search engine…">
> 
> * Why the "2"?
> * We are still using title case for menus (bug filed) so let's keep doing
> that. And no ellipsis.
> 
> "Add a Search Engine"
> 

Whoops, fixed! The ellipsis was to hint that there was a nested context menu, but I guess feeds doesn't do that either. Might be worth considering though.
Blocks: 934591
Blocks: 934596
Comment on attachment 826176 [details] [diff] [review]
Part 1: Support opensearch search tag v6

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

Nice.

::: mobile/android/chrome/content/browser.js
@@ +3482,5 @@
>                tabID: this.id
>              };
>              sendMessageToJava(json);
>            } catch (e) {}
> +        } else if (list.indexOf("[search]" != -1)) {

nit: it would be great if this gigantic switch was factored out into smaller methods. Follow-up?

@@ +3492,5 @@
> +          // Check that type matches opensearch.
> +          let isOpenSearch = (type == "application/opensearchdescription+xml");
> +          if (isOpenSearch && target.title && /^(?:https?|ftp):/i.test(target.href)) {
> +            let visibleEngines = Services.search.getVisibleEngines();
> +            // XXX Engines are currently identified by name, but this can be changed when

Is the 'XXX' actually needed here? :-)

@@ +3493,5 @@
> +          let isOpenSearch = (type == "application/opensearchdescription+xml");
> +          if (isOpenSearch && target.title && /^(?:https?|ftp):/i.test(target.href)) {
> +            let visibleEngines = Services.search.getVisibleEngines();
> +            // XXX Engines are currently identified by name, but this can be changed when
> +            // engines are identified by URL (see bug 335102).

nit: Engines...

@@ +3501,5 @@
> +            }
> +
> +            if (this.browser.engines) {
> +              // This engine has already been handled, do nothing.
> +              if (this.browser.engines.some(function(e) e.url == target.href))

nit: I'd add {}'s here to make the code easier to read with the bested if's.

@@ +3525,5 @@
> +            // Broadcast message that this tab contains search engines.
> +            let newEngineMessage = {
> +              type: "Link:OpenSearch",
> +              tabID: this.id,
> +              visible: true

What's this 'visible' about?
Attachment #826176 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 826871 [details] [diff] [review]
Part 2: Support OpenSearch engines v7

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

Cool.

::: mobile/android/chrome/content/browser.js
@@ +6742,5 @@
> +    }).setSingleChoiceItems(engines.map(function(e) {
> +      return { label: e.title };
> +    })).show((function(data) {
> +      if (data.button == -1)
> +        return;

nit: empty line here.
Attachment #826871 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 826873 [details] [diff] [review]
Part 3: Add UI for adding an OpenSearch engine v5

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

Looks good.

::: mobile/android/base/BrowserToolbar.java
@@ +373,5 @@
>                          menu.findItem(R.id.share).setVisible(false);
>                          menu.findItem(R.id.add_to_launcher).setVisible(false);
>                      }
> +
> +                    if (!tab.getFeedsVisible()) {

I wonder why feeds doesn't explicitly show the menu item just like you're doing with opensearch? Do they behave differently?

@@ +381,5 @@
> +                    if (tab.getOpenSearchVisible()) {
> +                        menu.findItem(R.id.add_search_engine).setVisible(true);
> +                    } else {
> +                        menu.findItem(R.id.add_search_engine).setVisible(false);
> +                    }

What about:
menu.findItem(R.id.add_search_engine).setVisible(tab.hasOpenSearch())

::: mobile/android/base/Tab.java
@@ +42,5 @@
>      private String mTitle;
>      private Bitmap mFavicon;
>      private String mFaviconUrl;
>      private int mFaviconSize;
> +    private boolean mFeedsVisible;

I'd probably have done this variable renaming in a separate patch btw.

@@ +43,5 @@
>      private Bitmap mFavicon;
>      private String mFaviconUrl;
>      private int mFaviconSize;
> +    private boolean mFeedsVisible;
> +    private boolean mOpenSearchVisible;

I wonder if hasFeeds and hasOpenSearch would be a better name?

@@ +237,5 @@
>      public synchronized String getFaviconURL() {
>          return mFaviconUrl;
>      }
>  
> +    public boolean getFeedsVisible() {

Then this would be hasFeeds()

@@ +241,5 @@
> +    public boolean getFeedsVisible() {
> +        return mFeedsVisible;
> +    }
> +
> +    public boolean getOpenSearchVisible() {

...and hasOpenSearch()

@@ +391,5 @@
>          mFaviconUrl = null;
>          mFaviconSize = 0;
>      }
>  
> +    public void setFeedsVisible(boolean feedsVisible) {

setHasFeeds()?

@@ +395,5 @@
> +    public void setFeedsVisible(boolean feedsVisible) {
> +        mFeedsVisible = feedsVisible;
> +    }
> +
> +    public void setOpenSearchVisible(boolean openSearchVisible) {

setHasOpenSearch?

@@ +624,5 @@
>          }
>  
>          setContentType(message.getString("contentType"));
>          clearFavicon();
> +        setFeedsVisible(false);

Reset open search too?

::: mobile/android/chrome/content/browser.js
@@ +3687,5 @@
>  
> +      // Clear page-specific opensearch engines and feeds for a new request.
> +      if (aStateFlags & Ci.nsIWebProgressListener.STATE_START && aRequest && aWebProgress.isTopLevel) {
> +          this.browser.engines = null;
> +          this.browser.feeds = null;

I'd have done this first for feeds in a separate patch. Not a big deal though.
Attachment #826873 - Flags: review?(lucasr.at.mozilla) → review+
Blocks: 935259
Carrying over r+.

(In reply to Lucas Rocha (:lucasr) from comment #47)
> Comment on attachment 826176 [details] [diff] [review]
> Part 1: Support opensearch search tag v6
> 
> Review of attachment 826176 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice.
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +3482,5 @@
> >                tabID: this.id
> >              };
> >              sendMessageToJava(json);
> >            } catch (e) {}
> > +        } else if (list.indexOf("[search]" != -1)) {
> 
> nit: it would be great if this gigantic switch was factored out into smaller
> methods. Follow-up?

Filed bug 935259.

> 
> @@ +3501,5 @@
> > +            }
> > +
> > +            if (this.browser.engines) {
> > +              // This engine has already been handled, do nothing.
> > +              if (this.browser.engines.some(function(e) e.url == target.href))
> 
> nit: I'd add {}'s here to make the code easier to read with the bested if's.

I ended up sticking the innermost function into braces as well, and added some more braces for the inner if.

> 
> @@ +3525,5 @@
> > +            // Broadcast message that this tab contains search engines.
> > +            let newEngineMessage = {
> > +              type: "Link:OpenSearch",
> > +              tabID: this.id,
> > +              visible: true
> 
> What's this 'visible' about?

Added a comment clarifying. Basically, reusing this message to handle showing/hiding the open search context menu item (once all the opensearch engines exposed have been added, we'll want to send a message with "visible: false").
Attachment #826176 - Attachment is obsolete: true
Attachment #827665 - Flags: review+
Addressed nits, carrying over r+.
Attachment #826871 - Attachment is obsolete: true
Attachment #827710 - Flags: review+
Handled the renaming of methods and variables - I think your names are better.

One more review bounce, because I realized that while I cleared the engine list on page change, I never sent the message to hide the context menu option.

(In reply to Lucas Rocha (:lucasr) from comment #49)
> Comment on attachment 826873 [details] [diff] [review]
> Part 3: Add UI for adding an OpenSearch engine v5
> 
> Review of attachment 826873 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/BrowserToolbar.java
> @@ +373,5 @@
> >                          menu.findItem(R.id.share).setVisible(false);
> >                          menu.findItem(R.id.add_to_launcher).setVisible(false);
> >                      }
> > +
> > +                    if (!tab.getFeedsVisible()) {
> 
> I wonder why feeds doesn't explicitly show the menu item just like you're
> doing with opensearch? Do they behave differently?
> 

I believe that since feeds are handled by a feed subscriber (e.g., defunct Google Reader), we have no knowledge of whether the feeds have actually been subscribed to, and what visible/hidden state we should display - so we always display available feeds. For search engines however, we know when we've added an OpenSearch engine, and it's nice to hide the option if all available engines have already been added.

> 
> I'd probably have done this variable renaming in a separate patch btw.
> 

Thanks, I'll keep that in mind next time!
Attachment #826873 - Attachment is obsolete: true
Attachment #827711 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 827711 [details] [diff] [review]
Part 3: Add UI for adding OpenSearch engine v6

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

Nice.

::: mobile/android/base/Tab.java
@@ +396,5 @@
> +        mHasFeeds = hasFeeds;
> +    }
> +
> +    public void setHasOpenSearch(boolean hasOpenSearch) {
> +        mHasOpenSearch= hasOpenSearch;

nit: space before =

@@ +624,5 @@
>          }
>  
>          setContentType(message.getString("contentType"));
>          clearFavicon();
> +        setHasFeeds(false);

No need to reset hasOpenSearch?
Attachment #827711 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #53)
> Comment on attachment 827711 [details] [diff] [review]
> Part 3: Add UI for adding OpenSearch engine v6
> 
> Review of attachment 827711 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice.
> 
> ::: mobile/android/base/Tab.java
> @@ +396,5 @@
> > +        mHasFeeds = hasFeeds;
> > +    }
> > +
> > +    public void setHasOpenSearch(boolean hasOpenSearch) {
> > +        mHasOpenSearch= hasOpenSearch;
> 
> nit: space before =

whoops! fixed :)
> 
> @@ +624,5 @@
> >          }
> >  
> >          setContentType(message.getString("contentType"));
> >          clearFavicon();
> > +        setHasFeeds(false);
> 
> No need to reset hasOpenSearch?

When I added logging it looked like the onLocationChange event was a subset of the page requests/progress events (which I added handling for in browser.js - sending a message to Java to clear the search engine contextmenu item), but just handling onLocationChange wasn't sufficient.


I'm not sure why feeds clearing is handled here, tbh. The behavior of clearing fields on new page loads was actually handled in browser.xml [1] (which worked but wasn't a very visible place for it); now that we handle clearing feeds to the new request/progress block in browser.js, we could probably remove this onLocationChange code for feeds, actually.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/bindings/browser.xml#550
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 28
wooo! ♥ ♥ ♥
Looking forward to trying this in tomorrow's nightly!
Status: RESOLVED → VERIFIED
Flags: in-moztrap?(fennec)
Depends on: 937769
Depends on: 939802
No longer depends on: 939802
Depends on: 1017903
You need to log in before you can comment on or make changes to this bug.