Closed Bug 870110 Opened 6 years ago Closed 6 years ago

Search system changes to support FHR searches provider

Categories

(Firefox for Android :: General, defect, P1)

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 23

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
These propagate the search engine identifier into Java, and also notify Java when a keyword search occurs.
Attachment #747623 - Flags: review?
Attached patch FHR provider (obsolete) — Splinter Review
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.
Depends on: 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`.
Attachment #747623 - Flags: review? → review+
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
Whiteboard: [leave open]
Target Milestone: --- → Firefox 23
Attached patch FHR provider, v2 (obsolete) — Splinter Review
Slightly lower bar than usual. Depends on storage.
Attachment #747624 - Attachment is obsolete: true
Attachment #747704 - Flags: review?(nalexander)
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.
Blocks: 873496
Morphing this. See Bug 873496.
No longer blocks: 868442
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
No longer depends on: 858742
Resolution: --- → FIXED
Summary: Provider: searches → Search system changes to support FHR searches provider
Whiteboard: [leave open]
Component: Client: Android → General
Product: Firefox Health Report → Firefox for Android
Version: unspecified → Trunk
Attachment #747704 - Attachment is obsolete: true
Attachment #747704 - Flags: review?(nalexander)
You need to log in before you can comment on or make changes to this bug.