1300201 - Move list of Firefox for Android search plugins to JSON

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect, P3)

Description

[Mike Kaply [:mkaply]]

In bug 1276739, we are moving to JSON files for search, replacing list.txt.

For browser, we will have one JSON file that is parsed at build time to create locale specific JSON files.

This bug is for the Fennec version of that.

Special work will be required for the search activity/widget

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Patch

This is the patch that is dependent on the desktop stuff going in first.

list.json still needs some tweaking, particularly for the UK, KK, etc.

Comment 2

[Mike Kaply [:mkaply]]

Attachment: Patch with new engine info

Now that everything has landed in desktop, this is ready to go for mobile.

The list.json is based on everything I know about the locales from:

https://l10n.mozilla-community.org/~flod/p12n/defaultengine_searchorder/?channel=aurora&product=mobile

The searchDefault is in because this file is being used on Firefox IOS as well and we need that information there. searchdefault is NOT being used by Fennec yet.

The build changes ended up being quite small.

Comment 3

[Francesco Lodolo [:flod]]

Comment on attachment 8807671 [details] [diff] [review]
Patch with new engine info

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

General approach is good, I probably need to do another pass after the first issues are addressed to double check the differences.

::: mobile/locales/search/list.json
@@ +25,5 @@
> +    "as": {
> +      "default": {
> +        "searchDefault": "Google",
> +        "visibleDefaultEngines": [
> +          "google", "yahoo-in", "bing", "google", "amazon-in", "wikipedia-as"

Double google

@@ +27,5 @@
> +        "searchDefault": "Google",
> +        "visibleDefaultEngines": [
> +          "google", "yahoo-in", "bing", "google", "amazon-in", "wikipedia-as"
> +        ]
> +      }      

Trailing spaces

@@ +45,5 @@
> +          "google", "bing", "amazondotcom", "azerdict", "duckduckgo", "wikipedia-az"
> +        ]
> +      }
> +    },
> +    "be": {

We should remove Belarusian (we just dropped the builds).

@@ +283,5 @@
> +    "fi": {
> +      "default": {
> +        "searchDefault": "Google",
> +        "visibleDefaultEngines": [
> +          "google", "twitter", "wikipedia-fi", "yahoo-fi", "amazondotcom"

I would expect this in a different order: google, amazondotcom, twitter, wikipedia-fi, yahoo-fi
I also have similar differences for other locales, but this is the one with most changes.

@@ +443,5 @@
> +    "kk": {
> +      "default": {
> +        "searchDefault": "Google",
> +        "visibleDefaultEngines": [
> +          "yandex", "google", "yahoo", "bing", "twitter", "wikipedia-kk"

Should this be google-nocodes?

@@ +719,5 @@
> +    "son": {
> +      "default": {
> +        "searchDefault": "Google",
> +        "visibleDefaultEngines": [
> +          "google", "yahoo-france", "bing", "amazon-france", "twitter", "wikipedia-fr"

This should be amazon-fr

@@ +775,5 @@
> +    "tr": {
> +      "default": {
> +        "searchDefault": "Google",
> +        "visibleDefaultEngines": [
> +          "google", "yandex-tr", "twitter", "wikipedia-tr"

Should this have yandex-tr first, and google-nocodes?
Otherwise we need to change kk to have google (or google-nocodes) as first.

@@ +907,5 @@
> +          "google-nocodes", "bing", "duckduckgo", "wikipedia-zh-TW"
> +        ]
> +      },
> +      "TW": {
> +        "searchDefault": "Google  ",

Extra spaces

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8807671 [details] [diff] [review]
Patch with new engine info

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

::: mobile/android/base/java/org/mozilla/gecko/search/SearchEngineManager.java
@@ +573,5 @@
>          if (in == null) {
>              return null;
>          }
>          final BufferedReader br = getBufferedReader(in);
> +        StringBuilder sb = new StringBuilder();

nit: 'final' here and further down (JSONObject, ..)

@@ +578,5 @@
> +        String line;
> +        try {
> +            while ((line = br.readLine()) != null) {
> +                sb.append(line);
> +            }

There's a helper method in FileUtils to read a stream into a string.

@@ +580,5 @@
> +            while ((line = br.readLine()) != null) {
> +                sb.append(line);
> +            }
> +        } catch (IOException e) {
> +            Log.e(LOG_TAG, "Error reading list.json", e);

In an error case we shouldn't continue here, I guess.

@@ +609,2 @@
>          }
>          return null;

The new version does not close the reader/stream anymore.

Comment 5

[Mike Kaply [:mkaply]]

> There's a helper method in FileUtils to read a stream into a string.

Unfortunately it requires a size:

public static String readStringFromInputStreamAndCloseStream(final InputStream inputStream, final int bufferSize)

Any thoughts on how best to handle that?

Comment 6

[Mike Kaply [:mkaply]]

> Should this be google-nocodes?

No. We only use google-nocodes in a region that has Yandex as the default.

When the build is used outside RU, KK, TR, UK, BE, it gets Google with codes.

But that should have had google first, not yandex.

fixing the rest

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Mike Kaply [:mkaply] from comment #5)
> > There's a helper method in FileUtils to read a stream into a string.
> 
> Unfortunately it requires a size:
> 
> public static String readStringFromInputStreamAndCloseStream(final
> InputStream inputStream, final int bufferSize)
> 
> Any thoughts on how best to handle that?

Do we know the file size? If not then reading in chunks between 10 - 100 kB should be okay. Previously you read the file line by line but we can easily read more at a time.

Comment 8

[Mike Kaply [:mkaply]]

> Do we know the file size? If not then reading in chunks between 10 - 100 kB should be okay. Previously you read the file line by line but we can easily read more at a time.

It differs between locales, so no.

Is it OK to hardcode a size in this case?

readStringFromInputStreamAndCloseStream isn't really designed to read in chunks

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Mike Kaply [:mkaply] from comment #8)
> > Do we know the file size? If not then reading in chunks between 10 - 100 kB should be okay. Previously you read the file line by line but we can easily read more at a time.
> 
> It differs between locales, so no.
> 
> Is it OK to hardcode a size in this case?
> 
> readStringFromInputStreamAndCloseStream isn't really designed to read in
> chunks

Yeah, it is okay to hardcode it to a size. Internally the method will use StringBuilder and this is only the size of characters that we will try to read from the reader and pass to the builder.

Comment 10

[Mike Kaply [:mkaply]]

Attachment: Bug 1300201 - Switch Fennec to use list.json.

Review commit: https://reviewboard.mozilla.org/r/118760/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118760/

Comment 11

[Francesco Lodolo [:flod]]

@Mike
Did you mean to not include list.json in your new patch? I assume the old patch should be marked as obsolete at this point, we also added a few locales in the meantime.

Comment 12

[Francesco Lodolo [:flod]]

From a quick check, besides the notes in comment 3 (still valid), these are the differences

Locale: ar
current hg: google, yahoo, bing, amazondotcom, twitter, wikipedia-ar
json:  google, wikipedia-ar, twitter

Locale: fa
There are differences.
current hg: google, yahoo, bing, wikipedia-fa
json:  google, wikipedia-fa, yahoo

Locale: he
There are differences.
current hg: google, yahoo, bing, amazondotcom, twitter, wikipedia-he
json:  google, twitter, wikipedia-he

Comment 13

[Mike Kaply [:mkaply]]

Comment on attachment 8845627 [details]
Bug 1300201 - Switch Fennec to use list.json.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118760/diff/1-2/

Comment 14

[Mike Kaply [:mkaply]]

No, I just forgot to hg add. This list has all the changes you've requested so far. (And I see that whitespace issue. I'll fix in the next update).

What are the new locales?

Comment 15

[Francesco Lodolo [:flod]]

Comment on attachment 8845627 [details]
Bug 1300201 - Switch Fennec to use list.json.

https://reviewboard.mozilla.org/r/118760/#review120986

Only a couple of details to fix. I've ignored cases where the first searchplugin is not the natural default, based on previous discussion in bug 1328713.

New locales are ar, fa, he, ru, but they did have some sort of searchplugins set up already. We're in the process of adding trs and zam, but they're not there yet, and you shouldn't bother for this migration.

::: mobile/locales/search/list.json:78
(Diff revision 2)
> +      }
> +    },
> +    "be": {
> +      "default": {
> +        "visibleDefaultEngines": [
> +          "google", "be.wikipedia.org", "bing", "tut.by", "yahoo", "yandex.by"

On second thoughts, it might be good to keep Belarusian, since they might come back soon like they did on desktop.

Let's drop 'tut.by' though, like we did there (see bug 1342747).

::: mobile/locales/search/list.json:242
(Diff revision 2)
> +      }
> +    },
> +    "fa": {
> +      "default": {
> +        "visibleDefaultEngines": [
> +          "google", "yahoo", "bing", "wikipedia-fa", "yahoo"

We shouldn't have yahoo here.

Comment 16

[Francesco Lodolo [:flod]]

For clarity, the r+ is on the json part, I left all the code to Sebastian ;-)

Comment 17

[Francesco Lodolo [:flod]]

We're adding trs (bug 1262869), in case you want to update your JSON

"trs": {
  "default": {
    "searchDefault": "Google",
    "visibleDefaultEngines": [
      "amazondotcom", "bing", "google", "twitter", "wikipedia-es", "yahoo-espanol"
    ]
  }
},

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8845627 [details]
Bug 1300201 - Switch Fennec to use list.json.

https://reviewboard.mozilla.org/r/118760/#review122458

::: mobile/android/base/java/org/mozilla/gecko/search/SearchEngineManager.java:581
(Diff revision 2)
> -        final BufferedReader br = getBufferedReader(in);
> -
> +        JSONObject json;
> +        try {
> +            json = new JSONObject(FileUtils.readStringFromInputStreamAndCloseStream(in, MAX_LISTJSON_SIZE));
> +        } catch (IOException e) {
> +            Log.e(LOG_TAG, "Error reading list.json", e);
> +            return null;
> +        } catch (JSONException e) {
> +            Log.e(LOG_TAG, "Error parsing list.json", e);
> +            return null;
> +        } finally {
> +            IOUtils.safeStreamClose(in);
> +        }

I guess this could be a helper in FileUtils/IOUtils too. We already have a helper method to read a JSON object from a file. That one could use a new readJSONObjectFromStream() method. But no reason to block landing this.

::: mobile/android/base/java/org/mozilla/gecko/search/SearchEngineManager.java:584
(Diff revision 2)
> +        } catch (IOException e) {
> +            Log.e(LOG_TAG, "Error reading list.json", e);
> +            return null;
> +        } catch (JSONException e) {
> +            Log.e(LOG_TAG, "Error parsing list.json", e);
> +            return null;

nit: If two (or more) exceptions are handled the same way then you can collapse them:

> } catch (IOException | JSONException e) {
>     ...
> }

Comment 19

[Mike Kaply [:mkaply]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3aede4b1edbfeb2536912fd27e0a9fdf521c0889
Bug 1300201 - Switch Fennec to use list.json. r=sebastian,r=flod

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3aede4b1edbf

Comment 21

[Mike Kaply [:mkaply]]

One of the things that had concerned me about this was multi locale because it's not something you can build easily locally.

I've just verified that multilocale is working, with list.json switching with the language.