Move list of Firefox for Android search plugins to JSON

RESOLVED FIXED in Firefox 55

Status

()

Firefox for Android
Search Activity
P3
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: mkaply, Assigned: mkaply)

Tracking

(Blocks: 3 bugs)

Trunk
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 affected, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

a year ago
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
(Assignee)

Updated

a year ago
Blocks: 1300209
Priority: -- → P3
(Assignee)

Updated

a year ago
Blocks: 1302815
(Assignee)

Comment 1

a year ago
Created attachment 8796588 [details] [diff] [review]
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.
(Assignee)

Comment 2

a year ago
Created attachment 8807671 [details] [diff] [review]
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.
Attachment #8796588 - Attachment is obsolete: true
Attachment #8807671 - Flags: review?(s.kaspari)
Attachment #8807671 - Flags: feedback?(francesco.lodolo)
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
Attachment #8807671 - Flags: feedback?(francesco.lodolo) → feedback+
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.
Attachment #8807671 - Flags: review?(s.kaspari) → feedback+
(Assignee)

Comment 5

a year ago
> 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?
(Assignee)

Comment 6

a year ago
> 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
(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.
(Assignee)

Comment 8

11 months ago
> 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
(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.

Updated

10 months ago
Summary: Move list.txt over to JSON once bug 1276739 is in → Move list of Firefox for Android searchplugins to to JSON

Updated

10 months ago
Blocks: 1324045

Updated

10 months ago
Summary: Move list of Firefox for Android searchplugins to to JSON → Move list of Firefox for Android search plugins to JSON
(Assignee)

Updated

9 months ago
Blocks: 1332006
Comment hidden (mozreview-request)
@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.
Flags: needinfo?(mozilla)
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 hidden (mozreview-request)
(Assignee)

Comment 14

8 months ago
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?
Flags: needinfo?(mozilla)

Comment 15

8 months ago
mozreview-review
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.
Attachment #8845627 - Flags: review?(francesco.lodolo) → review+
For clarity, the r+ is on the json part, I left all the code to Sebastian ;-)
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

7 months ago
mozreview-review
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) {
>     ...
> }
Attachment #8845627 - Flags: review?(s.kaspari) → review+
(Assignee)

Comment 19

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

Comment 20

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3aede4b1edbf
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Attachment #8807671 - Attachment is obsolete: true
Depends on: 1347830
(Assignee)

Comment 21

7 months ago
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.
You need to log in before you can comment on or make changes to this bug.