873496 - Provider: searches

Status: RESOLVED FIXED

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

Description

[Richard Newman [:rnewman]]

Splitting this out from the original bug, because it spans a tree merge.

Comment 1

[Richard Newman [:rnewman]]

Ensure that we have filters for only partner engines.

Comment 2

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

Supersedes the draft patch in Bug 870110. I think a double-reporting bug just popped up this week, so need to check on that, but let's have a prelim review regardless!

Comment 3

[Chris Peterson [:cpeterson]]

Comment on attachment 751213 [details] [diff] [review]
Proposed patch. v1

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

LGTM with some questions and suggestions.

::: mobile/android/base/AwesomeBar.java
@@ +403,5 @@
> +     *        {@link BrowserHealthRecorder#SEARCH_LOCATIONS}.
> +     */
> +    private static void recordSearch(String identifier, String where) {
> +        BrowserHealthRecorder recorder = BrowserHealthRecorder.getInstance();
> +        Log.i("GeckoXXX", "Recording search: " + recorder + ", " + identifier + ", " + where);

You should probably replace "GeckoXXX" with this class' LOGTAG.

::: mobile/android/base/health/BrowserHealthRecorder.java
@@ +153,5 @@
>      public void handleMessage(String event, JSONObject message) {
>          try {
> +            if (event.equals(EVENT_KEYWORD_SEARCH)) {
> +                recordSearch(message.getString("identifier"), "bartext");
> +            }

Some people on our team prefer constant-first style string comparisons like `EVENT_KEYWORD_SEARCH.equals(event)` (because they are null-proof). I don't have a strong opinion. <:)

@@ +167,5 @@
> +    public static final String[] SEARCH_LOCATIONS = {
> +        "barkeyword",
> +        "barsuggest",
> +        "bartext",
> +    };

Does SEARCH_LOCATIONS need to be public? I don't see any other classes referencing it. They use hard coded names like "barkeyword".

@@ +170,5 @@
> +        "bartext",
> +    };
> +
> +    private void initializeSearchProvider(final EventDispatcher dispatcher) {
> +        this.storage.ensureMeasurementInitialized("org.mozilla.searches.counts", 3, new MeasurementFields() {

Where does the magic number 3 come from? SEARCH_LOCATIONS.length?

@@ +173,5 @@
> +    private void initializeSearchProvider(final EventDispatcher dispatcher) {
> +        this.storage.ensureMeasurementInitialized("org.mozilla.searches.counts", 3, new MeasurementFields() {
> +            @Override
> +            public Iterable<FieldSpec> getFields() {
> +                ArrayList<FieldSpec> out = new ArrayList<FieldSpec>();

You might consider using the ArrayList constructor with a capacity hint of SEARCH_LOCATIONS.length. If you think that is an ugly micro-optimization, then feel free to ignore this suggestion.

@@ +211,5 @@
> +            public void run() {
> +                Log.d(LOG_TAG, "Recording search: " + key + ", " + location +
> +                               " (" + day + ", " + env + ").");
> +                // TODO: memoize.
> +                final int searchField = storage.getField("org.mozilla.searches.counts", 3, location).getID();

Again, where does the magic number 3 come from?

Comment 4

[Richard Newman [:rnewman]]

> You should probably replace "GeckoXXX" with this class' LOGTAG.

Prelim patch :D


> Some people on our team prefer constant-first style string comparisons like
> `EVENT_KEYWORD_SEARCH.equals(event)` (because they are null-proof). I don't
> have a strong opinion. <:)

Yeah, I do too -- call it Friday afternoon inconsistency!


> Does SEARCH_LOCATIONS need to be public? I don't see any other classes
> referencing it. They use hard coded names like "barkeyword".

I generally like having descriptive/API constants be public, and in this case we have JavaDocs pointing to it from other classes.


> Where does the magic number 3 come from? SEARCH_LOCATIONS.length?

It's a version number. It's part of the FHR payload format. I'll lift that out into a named constant.


> You might consider using the ArrayList constructor with a capacity hint of
> SEARCH_LOCATIONS.length. If you think that is an ugly micro-optimization,
> then feel free to ignore this suggestion.

Ugly is beautiful!

Comment 5

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v2

Review comments addressed, tested more thoroughly. Carrying forward flag. Thanks, Chris!

Comment 6

[Richard Newman [:rnewman]]

Testing notes:

There are three ways to initiate a search in Fennec. You'll see log output for each.

* Type something in the awesomebar, hit enter.

  D/GeckoHealthRec( 1489): Recording search: google, bartext (15842, 2).

* Type something in the awesomebar, pick a search suggestion.

  I/GeckoAwesomeBar( 1489): Recording search: amazondotcom, barsuggest
  D/GeckoHealthRec( 1489): Recording search: amazondotcom, barsuggest (15842, 2).

* Sync a keyword search item, and type, e.g., "imdb shatner". (<https://support.mozilla.org/en-US/kb/how-search-from-address-bar>).

  I/GeckoAwesomeBar( 1489): Recording search: null, barkeyword
  D/GeckoHealthRec( 1489): Recording search: other, barkeyword (15842, 2).


After a pile of other patches land, you'll be able to see this stuff in your health report.

Comment 7

[Richard Newman [:rnewman]]

Attachment: Part 2: limit search counts to partners. v1

This isn't perfect, because it duplicates some content from desktop.

I also *manually* added wikipedia, because I'm not sure why it's not in our partner list.

I'm sure binary search on this array isn't the most efficient way to do this, but it isn't too bad.

Comment 8

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v3

Rebased.

Comment 9

[Richard Newman [:rnewman]]

Attachment: Part 2: limit search counts to partners. v2

Rebased part 2.

Comment 10

[Richard Newman [:rnewman]]

QA note: make sure the right thing happens when changing search defaults (Bug 730445).

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 751929 [details] [diff] [review]
Part 2: limit search counts to partners. v2

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

Not certain I'm the right person to review this, but it looks fine to me.

::: mobile/android/base/health/BrowserHealthRecorder.java
@@ +309,5 @@
>          "bartext",
>      };
>  
> +    // See services/healthreport/providers.jsm. Sorry for the duplication.
> +    // THIS LIST MUST BE SORTED.

With what search order?  Are all entries ASCII and sorted by C's strcmp or a particular ordering?  For example, I see that '-' < 'd', which suggests binary ordering, e.g. strcmp.

Comment 13

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7554b86cefc
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcf9daabbccb

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b7554b86cefc
https://hg.mozilla.org/mozilla-central/rev/bcf9daabbccb

Comment 15

[Richard Newman [:rnewman]]

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

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

User impact if declined: 
  No FHR in 23.

Testing completed (on m-c, etc.): 
  Baking on m-c for some time. QA overview written up; waiting for QA to execute. Manual testing.

Risk to taking this patch (and alternatives if risky): 
  Slim; it's pretty robust against failures, and later patches improve that still further.

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

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 751929 [details] [diff] [review]
Part 2: limit search counts to partners. v2

Approving for uplift as part of the body of work for FHR's first revision on Android.

Comment 17

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/23a61acd3139
https://hg.mozilla.org/releases/mozilla-aurora/rev/78a25df6a5b2