These propagate the search engine identifier into Java, and also notify Java when a keyword search occurs.

Depends on search system changes *and* a bunch of FHR stuff that hasn't landed yet, so putting this here for illustration.

Depends on patches in Bug 858742.

Comment on attachment 747623 [details] [diff] [review] Search system changes. v1 Review of attachment 747623 [details] [diff] [review]: ----------------------------------------------------------------- LGTM with some suggestions. ::: mobile/android/base/AwesomeBar.java @@ +398,2 @@ > } > + Remove trailing whitespace. @@ +403,5 @@ > + return; > + } > + > + final String search = URLEncoder.encode(keywordSearch); > + openUrlAndFinish(keywordUrl.replace("%s", search), "", true); I know the old code had two calls to openUrlAndFinish(), but these are so close together and only have one parameter difference, they would be consolidated. Maybe something like this? String searchUrl = (keywordUrl != null) ? keywordUrl.replace("%s", URLEncoder.encode(keywordSearch)) : url; openUrlAndFinish(searchUrl, "", true); ::: mobile/android/base/SearchEngine.java @@ +6,5 @@ > +package org.mozilla.gecko; > + > +import java.util.ArrayList; > + > +import android.graphics.Bitmap; In Fennec code, we order our imports by mozilla, android, com/net/org/etc, then java. ::: mobile/android/base/awesomebar/AllPagesTab.java @@ +652,1 @@ > String iconURI = engineJSON.getString("iconURI"); Is engineJSON.has("identifier") ever false? This code assumes engineJSON always has an "iconURI" element and browser.js changes below look like they always add `identifier: engine.identifier`. ::: mobile/android/chrome/content/browser.js @@ +1316,2 @@ > this.selectedTab.userSearch = aData; > + Remove trailing whitespace. @@ +1317,5 @@ > + > + let engine = aSubject.QueryInterface(Ci.nsISearchEngine); > + sendMessageToJava({ > + type: "Search:Keyword", > + engine: engine.identifier, I think we should rename the `engine` field to `identifier`. We call it `identifier` in the _handleSearchEnginesGet() function below and the FHR patch's recordSearch() method also calls its parameter `identifier`.

Comment on attachment 747623 [details] [diff] [review] Search system changes. v1 Review of attachment 747623 [details] [diff] [review]: ----------------------------------------------------------------- Mostly nits and me rambling. ::: mobile/android/base/AwesomeBar.java @@ +398,2 @@ > } > + ws @@ +405,5 @@ > + > + final String search = URLEncoder.encode(keywordSearch); > + openUrlAndFinish(keywordUrl.replace("%s", search), "", true); > + } > + }); I'm a little confused why all this is in here. It does look more readable though @@ +410,3 @@ > } > > + private void openSearchAndFinish(String url, SearchEngine engine) { I'd just merge this function with onSearch above. I don't see anyone else using it. ::: mobile/android/base/GeckoApp.java @@ +1634,5 @@ > registerEventListener("Update:Check"); > registerEventListener("Update:Download"); > registerEventListener("Update:Install"); > registerEventListener("PrivateBrowsing:Data"); > + registerEventListener("Search:Keyword"); GeckoApp is probably the wrong place to listen for this (FHR can listen all by iteself in fact..., can't it?) ::: mobile/android/base/SearchEngine.java @@ +24,5 @@ > + this.identifier = identifier; > + this.icon = icon; > + this.suggestions = new ArrayList<String>(); > + } > +} I don't really love exposing this to the world, but we haven't done a great job namespacing this stuff either... I'd say lets put it in the awesomebar folder so that someday we remember to fix it along with the other classes in there, but we're probably killing those guys soon anyway in favor of an about:home folder.... so, I guess this is fine. I'd be fine with making it a subclass in AwesomeBar too though. ::: mobile/android/base/awesomebar/AllPagesTab.java @@ +647,5 @@ > JSONObject engineJSON = engines.getJSONObject(i); > String name = engineJSON.getString("name"); > + String identifier = engineJSON.has("identifier") ? > + engineJSON.getString("identifier") : > + null; engineJSON.optString("identifier", null); you sure you want it to be optional? ::: mobile/android/chrome/content/browser.js @@ +1311,5 @@ > break; > > case "keyword-search": > + // This assumes that the user can only perform a keyword search on the > + // selected tab. We don't wrap lines in js. @@ +1316,2 @@ > this.selectedTab.userSearch = aData; > + ws @@ +1317,5 @@ > + > + let engine = aSubject.QueryInterface(Ci.nsISearchEngine); > + sendMessageToJava({ > + type: "Search:Keyword", > + engine: engine.identifier, Use identifier to be consistent with the other message

(In reply to Wesley Johnston (:wesj) from comment #5) > I'm a little confused why all this is in here. It does look more readable > though I switched it to early return. > I'd just merge this function with onSearch above. I don't see anyone else > using it. wfm. Was being minimally intrusive, but happy to clean as I go :) > GeckoApp is probably the wrong place to listen for this (FHR can listen all > by iteself in fact..., can't it?) Yeah, I guess it can (albeit with a dependency on GeckoAppShell). For the record, mind, the thing that would do the listening is just an object that GeckoApp owns. > about:home folder.... so, I guess this is fine. I'd be fine with making it a > subclass in AwesomeBar too though. I started doing that, then Utils, then said "bah". We kinda want a "structures" namespace :) There's a lot of "get this landed" attitude in this, as I'm sure you've noticed. > engineJSON.optString("identifier", null); you sure you want it to be > optional? Oh, a corner of the JavaDoc I didn't see. Neat. I should probably just remove the guard altogether. > Use identifier to be consistent with the other message I knew you guys would pick up on that ;)

Comments addressed: https://hg.mozilla.org/integration/mozilla-inbound/rev/797f695d4cb6

Slightly lower bar than usual. Depends on storage.

Comment on attachment 747623 [details] [diff] [review] Search system changes. v1 With the changes to openUserEnteredAndFinish, I want to make sure QA is testing user entered navigation: * type URL -> goes to URL * type two words -> search for the two words using default engine * bookmark and keyword, then type keyword -> loads the bookmarked url (and any other litmus tests I am missing)

N.B., I need to add a filter here for partner search providers, just as we do on desktop, to avoid over-reporting.

Morphing this. See Bug 873496.