Remove filter on recorded search engine identifiers for Fennec

RESOLVED FIXED in Firefox 27

Status

Firefox Health Report
Client: Android
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mconnor, Assigned: rnewman)

Tracking

(Blocks: 1 bug, {verifyme})

unspecified
Firefox 28
All
Android
verifyme
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox27 fixed, firefox28 fixed)

Details

(Whiteboard: [qa+])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
See rationale in bug 925515 comment 0.

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

Comment 1

4 years ago
mconnor: are you tracking this for a particular release?
OS: All → Android
Summary: remove filter on search recording for Fennec → Remove filter on recorded search engine identifiers for Fennec
(Assignee)

Comment 2

4 years ago
Created attachment 817265 [details] [diff] [review]
Proposed patch. v1

Untested, but this should do the trick.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
Created attachment 818177 [details] [diff] [review]
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).
Attachment #817265 - Attachment is obsolete: true
Attachment #818177 - Flags: review?(michael.l.comella)
Comment on attachment 818177 [details] [diff] [review]
Proposed patch. v2

Throwing Brian on for feedback on the SearchEngine changes
Attachment #818177 - Flags: feedback?(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'
Attachment #818177 - Flags: feedback?(bnicholson) → feedback+
(Assignee)

Comment 6

4 years ago
(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 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?
Attachment #818177 - Flags: review?(michael.l.comella) → review-
(Assignee)

Comment 8

4 years ago
(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.
(Assignee)

Comment 9

4 years ago
> 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.
(Assignee)

Comment 10

4 years ago
Created attachment 818849 [details] [diff] [review]
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.)
Attachment #818177 - Attachment is obsolete: true
Attachment #818849 - Flags: review?(michael.l.comella)
(Assignee)

Comment 11

4 years ago
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 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`. :)
Attachment #818849 - Flags: review?(michael.l.comella) → review+
(Assignee)

Comment 13

4 years ago
(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 :)
(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.
(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.
(Assignee)

Updated

4 years ago
Blocks: 929051
(Assignee)

Comment 16

4 years ago
John clarified via email that this feature is approved to land.
(Assignee)

Comment 17

4 years ago
https://hg.mozilla.org/services/services-central/rev/4e559dd64a13
Whiteboard: [fixed in services][qa+]
Target Milestone: --- → Firefox 28
(Assignee)

Comment 18

4 years ago
https://hg.mozilla.org/mozilla-central/rev/4e559dd64a13
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services][qa+] → [qa+]
(Assignee)

Comment 19

4 years ago
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.
Attachment #818849 - Flags: approval-mozilla-aurora?

Updated

4 years ago
Attachment #818849 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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 ?

Updated

4 years ago
Flags: needinfo?(rnewman)
(Assignee)

Comment 21

4 years ago
(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".
(Assignee)

Comment 22

4 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/be1b50532970
status-firefox27: --- → fixed
Flags: needinfo?(rnewman)
Keywords: verifyme
status-firefox28: --- → fixed
(Assignee)

Updated

4 years ago
Blocks: 913743
(Assignee)

Updated

4 years ago
Depends on: 961526
You need to log in before you can comment on or make changes to this bug.