925517 - Remove filter on recorded search engine identifiers for Fennec

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Android, defect)

Description

[Mike Connor [:mconnor]]

See rationale in bug 925515 comment 0.

This should be straightforward, and should proceed once this change has been discussed and ratified.

Comment 1

[Richard Newman [:rnewman]]

mconnor: are you tracking this for a particular release?

Comment 2

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

Untested, but this should do the trick.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v2

Most of this is making SearchEngine and BrowserSearch not suck. We change the API for search observers to pass the engine around, and then compute a more stable string. The part that really addresses this bug is that that string is rarely just "other" (only in the case of fall-through keyword searches).

Comment 4

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

Comment on attachment 818177 [details] [diff] [review]
Proposed patch. v2

Throwing Brian on for feedback on the SearchEngine changes

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 818177 [details] [diff] [review]
Proposed patch. v2

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

::: mobile/android/base/home/SearchEngine.java
@@ +24,2 @@
>      public Bitmap icon;
> +    public List<String> suggestions = new ArrayList<String>();

Why not use private fields with getters/setters? 'suggestions' is especially awkward since it's a public field but also has a setter.

@@ +49,5 @@
> +        final String iconURI = getString(engineJSON, "iconURI");
> +        if (iconURI == null) {
> +            Log.w(LOG_TAG, "iconURI is null for search engine " + this.name);
> +        }
> +        this.icon = iconURI == null ? null : BitmapUtils.getBitmapFromDataURI(iconURI);

Nit: parentheses around 'iconURI == null'

Comment 6

[Richard Newman [:rnewman]]

(In reply to Brian Nicholson (:bnicholson) from comment #5)

> Why not use private fields with getters/setters? 'suggestions' is especially
> awkward since it's a public field but also has a setter.

Yeah, I only took it half-way to being a "proper" Java class, rather than an awful hybrid struct thing. Happy to go the rest of the way if nobody minds me sprawling this patch still further :)

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 818177 [details] [diff] [review]
Proposed patch. v2

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

Mostly in the right direction but I have a few questions.

I agree with Brian's setter comment.

Are we trying to capture the search engines from bookmark keywords too? We're currently not doing that (`BrowserApp.commitEditingMode`, `recordSearch(null, ...)`).

::: mobile/android/base/BrowserApp.java
@@ +1492,5 @@
>       *        {@link BrowserHealthRecorder#SEARCH_LOCATIONS}.
>       */
> +    private static void recordSearch(SearchEngine engine, String where) {
> +        Log.i(LOGTAG, "Recording search: " +
> +                      ((engine == null) ? "null" : engine.name) +

nit: A slightly cleaner version of this code would be to create a sub-class of `SearchEngine` that represents a null search engine (i.e. `name = null` and `getTrackingIdentifier()` returns other), keep it around as a singleton, and replace the engine reference here if the engine == null.

@@ +1501,5 @@
>              message.put("type", BrowserHealthRecorder.EVENT_SEARCH);
>              message.put("location", where);
>              message.put("identifier", identifier);
>              GeckoAppShell.getEventDispatcher().dispatchEvent(message);
>          } catch (Exception e) {

What Exception are we trying to record here? Are we just trying to stop the browser from crashing because of FHR? If so, we should move the log statement into the `try` too.

::: mobile/android/base/health/BrowserHealthRecorder.java
@@ +705,5 @@
>       * Searches.
>       */
>  
>      public static final String MEASUREMENT_NAME_SEARCH_COUNTS = "org.mozilla.searches.counts";
> +    public static final int MEASUREMENT_VERSION_SEARCH_COUNTS = 5;

Don't forget to update the dev docs - it seems we're only at version 2.

https://docs.services.mozilla.com/healthreport/index.html#org-mozilla-searches-counts

@@ +748,3 @@
>       * @param location one of a fixed set of locations: see {@link #SEARCH_LOCATIONS}.
>       */
>      public void recordSearch(final String engine, final String location) {

In looking over the code, I was often confusing this recordSearch and BrowserApp.recordSearch. I'd prefer `engine` to be called `engineIdentifier` here since it helps show the difference and is more accurate to the variable's purpose.

::: mobile/android/base/home/SearchEngine.java
@@ +19,5 @@
> +public class SearchEngine {
> +    public static final String LOG_TAG = "GeckoSearchEngine";
> +
> +    public final String name;
> +    public final String identifier;

What is the distinction between the name and the identifier? And more importantly, are they both necessary anymore?

It seems like identifier is used to notate built-in search engines, but perhaps that could be replaced with a boolean value. Probably better as a followup though.

@@ +30,5 @@
>  
>      public SearchEngine(String name, String identifier, Bitmap icon) {
>          this.name = name;
>          this.identifier = identifier;
>          this.icon = icon;

You may want to add the same null checks in this constructor as you did for the JSONObject constructor.

Alternatively, it seems like this constructor may not be used so get rid of it.

::: mobile/android/base/home/SearchEngineRow.java
@@ +165,5 @@
>          // Update search engine reference
>          mSearchEngine = searchEngine;
>  
>          // Set the search engine icon (e.g., Google) for the row
> +        if (mSearchEngine.icon != null) {

Before this patch, if `mSearchEngine.icon == null`, `updateAndScaleImage` would eventually call `FaviconView.showDefaultFavicon()`. Is this no longer necessary?

Comment 8

[Richard Newman [:rnewman]]

(In reply to Michael Comella (:mcomella) from comment #7)

> Are we trying to capture the search engines from bookmark keywords too?
> We're currently not doing that (`BrowserApp.commitEditingMode`,
> `recordSearch(null, ...)`).

We have no good way to do so; these aren't really search engines per se. I would like to do so if someone can propose a good method!

> nit: A slightly cleaner version of this code would be to create a sub-class
> of `SearchEngine` that represents a null search engine (i.e. `name = null`
> and `getTrackingIdentifier()` returns other), keep it around as a singleton,
> and replace the engine reference here if the engine == null.

Prefer null checks to adding another class file :D

(This should be a very infrequent case, perhaps even impossible. I just like error detection code.)


> Don't forget to update the dev docs - it seems we're only at version 2.
> 
> https://docs.services.mozilla.com/healthreport/index.html#org-mozilla-
> searches-counts

---
Other notable differences from Version 2

    There is no default browser indicator on Android.
    Add-ons include a blocklistState attribute, as returned by AddonManager.
    Searches are now version 4, and are hierarchical: how the search was started (bartext, barkeyword, barsuggest), and then counts per provider.
---



> What is the distinction between the name and the identifier? And more
> importantly, are they both necessary anymore?

The identifier is typically the filename we ship this search engine as.

The name is the string shown in the UI.

The former is strictly more unique than the latter:

ja-JP-mac/browser/searchplugins/google-jp.xml:<ShortName>Google</ShortName>
ro/suite/searchplugins/google.xml:<ShortName>Google</ShortName>

We fall back to the name if we don't have an identifier, which is the case for engines that we don't ship on-disk. SearchEngine is also a more general class than just for FHR recording -- we need both fields for different purposes.

 
> Before this patch, if `mSearchEngine.icon == null`, `updateAndScaleImage`
> would eventually call `FaviconView.showDefaultFavicon()`. Is this no longer
> necessary?

I put this in because I had a typo (data.isNull("key") rather than data.isNull(key)), so I was getting nulls -- probably for the first time that any of this code has ever seen. I chased the nulls down the NPE path for a while.

Comment 9

[Richard Newman [:rnewman]]

> Before this patch, if `mSearchEngine.icon == null`,

Null check removed.


> What Exception are we trying to record here? Are we just trying to stop the
> browser from crashing because of FHR? If so, we should move the log
> statement into the `try` too.

The first log statement is safe. The exception log statement... well, that's chicken-egg, no?


> Don't forget to update the dev docs - it seems we're only at version 2.

I added a supplementary note to describe the change from v4 to v5.


> In looking over the code, I was often confusing this recordSearch and
> BrowserApp.recordSearch. I'd prefer `engine` to be called `engineIdentifier`
> here since it helps show the difference and is more accurate to the
> variable's purpose.

Done.


> Alternatively, it seems like this constructor may not be used so get rid of
> it.

Done.

Comment 10

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v3

Die, code, die.

Note that we use the tracking identifier (now named getEngineIdentifier) in updateAndScaleImage. It's strictly more unique than the name, so there's less likelihood of a collision producing the wrong icon.

Try passes. (Which is handy, considering that the search suggestions tests fail on my machine *without* my patch.)

Comment 11

[Richard Newman [:rnewman]]

Tested build manually. Search suggestions work as expected (clicking in-bar and in-suggestion), recording of searches works.

Also tested adding custom engines, which works:

10-17 22:40:34.402 D/GeckoHealthRec(21949): Recording search: other-Mycroft Project: Search Engine Plugins - Firefox IE Chrome, barsuggest (15996, 2).

Results appear in about:healthreport.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 818849 [details] [diff] [review]
Proposed patch. v3

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

(In reply to Richard Newman [:rnewman] from comment #8)
> Prefer null checks to adding another class file :D

IMO, a static class isn't so bad. :)

Overall, lgtm: r+. My comments are nits but I think it would be good if you addressed some of them.

::: mobile/android/base/home/SearchEngine.java
@@ +21,3 @@
>  
> +    public final String name;
> +    public final String identifier;

Why didn't you create getters/setters for these fields as well?

@@ +66,5 @@
> +    }
> +
> +    public boolean hasSuggestions() {
> +        List<String> s = this.suggestions;
> +        return (s != null) &&

When can this.suggestions be null? When suggestions are disabled? Perhaps we should make a comment to this effect in setSuggestions(), or on the variable declaration.

::: mobile/android/base/home/SearchEngineRow.java
@@ +172,5 @@
>          setDescriptionOnSuggestion(mUserEnteredTextView, mUserEnteredTextView.getText().toString());
>  
>          // Add additional suggestions given by this engine
>          final int recycledSuggestionCount = mSuggestionView.getChildCount();
> +        final Iterable<String> suggestions = mSearchEngine.getSuggestions();

nit: I'd inline the getter into the for each to save vertical space.

@@ +180,4 @@
>              final View suggestionItem;
>  
> +            // Reuse suggestion views from recycled view, if possible.
> +            if (suggestionCounter + 1 < recycledSuggestionCount) {

I know you didn't write this code but why are we offsetting by 1? Is it because the first child is the suggestion bubble for what the user currently typing? There should probably be a comment explaining this.

@@ +209,4 @@
>          }
>  
> +        // Hide extra suggestions that have been recycled.
> +        for (int i = suggestionCounter + 1; i < recycledSuggestionCount; i++) {

nit: `++i` because you already used `++var`. :)

Comment 13

[Richard Newman [:rnewman]]

(In reply to Michael Comella (:mcomella) from comment #12)

> > +    public final String name;
> > +    public final String identifier;
> 
> Why didn't you create getters/setters for these fields as well?

public final. These really *are* structy. Doing it this way gives Proguard less work to do :)

> When can this.suggestions be null? When suggestions are disabled? Perhaps we
> should make a comment to this effect in setSuggestions(), or on the variable
> declaration.

It's a mutable field, so I'm simply treating null as the empty list. Saves NPEs later.


> > +            // Reuse suggestion views from recycled view, if possible.
> > +            if (suggestionCounter + 1 < recycledSuggestionCount) {
> 
> I know you didn't write this code but why are we offsetting by 1? Is it
> because the first child is the suggestion bubble for what the user currently
> typing? There should probably be a comment explaining this.

Beats me. I elected not to spend the time to understand this code more than necessary to refactor it. Feel free to comment and clarify it in a follow-up :D


> > +        // Hide extra suggestions that have been recycled.
> > +        for (int i = suggestionCounter + 1; i < recycledSuggestionCount; i++) {
> 
> nit: `++i` because you already used `++var`. :)

Heh, I almost made that change (because I like preincrement), but decided to keep the blame as minimal as possible. Will change now that I know my reviewer is happy with me imposing my style on old code as I go :)

Comment 14

[Brian Nicholson (:bnicholson)]

(In reply to Michael Comella (:mcomella) from comment #12)
> I know you didn't write this code but why are we offsetting by 1? Is it
> because the first child is the suggestion bubble for what the user currently
> typing?

Yes, that's correct.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Brian Nicholson (:bnicholson) from comment #14)
> (In reply to Michael Comella (:mcomella) from comment #12)
> > I know you didn't write this code but why are we offsetting by 1? Is it
> > because the first child is the suggestion bubble for what the user currently
> > typing?
> 
> Yes, that's correct.

bug 929051 for a followup fix.

Comment 16

[Richard Newman [:rnewman]]

John clarified via email that this feature is approved to land.

Comment 17

[Richard Newman [:rnewman]]

https://hg.mozilla.org/services/services-central/rev/4e559dd64a13

Comment 18

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/4e559dd64a13

Comment 19

[Richard Newman [:rnewman]]

Comment on attachment 818849 [details] [diff] [review]
Proposed patch. v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  None.

User impact if declined: 
  Another release in which we're unable to address search engine hijacking using FHR data (Bug 925527). See Bug 925515 Comment 6.

Testing completed (on m-c, etc.): 
  Has been baking for a while.

Risk to taking this patch (and alternatives if risky): 
  Pretty slim. (In fact, I think it fixes some other bugs in search recording…)

String or IDL/UUID changes made by this patch:
  None.

Comment 20

[bhavana bajaj [:bajaj]]

(In reply to Richard Newman [:rnewman] from comment #19)
> Comment on attachment 818849 [details] [diff] [review]
> Proposed patch. v3
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
>   None.
> 
> User impact if declined: 
>   Another release in which we're unable to address search engine hijacking
> using FHR data (Bug 925527). See Bug 925515 Comment 6.
> 
> Testing completed (on m-c, etc.): 
>   Has been baking for a while.
> 
> Risk to taking this patch (and alternatives if risky): 
>   Pretty slim. (In fact, I think it fixes some other bugs in search
> recording…)
> 
> String or IDL/UUID changes made by this patch:
>   None.

Is there a way to verify this patch once landed ?

Comment 21

[Richard Newman [:rnewman]]

(In reply to bhavana bajaj [:bajaj] from comment #20)

> Is there a way to verify this patch once landed ?

Yes: make searches via text entry, picking a suggestion, or via a bookmark keyword. Do so for built-in and added engines (e.g., from the Mycroft Project). Verify that each engine and each method appears in about:healthreport, with the added engines appearing as "other-$Engine Name.$method".

Comment 22

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/be1b50532970