892094 - Create "Search" page in settings

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

To start on bug 891115, let's just make a basic "Search" page in settings that has the search suggestions checkbox, and a list of installed search engines.

To get a list of search engines from JS, you'll need to use the search service API. Here's how we currently do it in the addon manager:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutAddons.js#243

I'm cc'ing Chenxia to help advise on creating a new page in settings, since she's done most of the work on that.

To minimize the scope of this bug, let's work on the enable/disable/set at default dialog in a follow-up bug.

Comment 1

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #0)

> To get a list of search engines from JS, you'll need to use the search
> service API. Here's how we currently do it in the addon manager:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> aboutAddons.js#243

Actually, come to think of it, you should be able to just use the existing "SearchEngines:Get"/"SearchEngines:Data" messages to get the search engines in Java. See SearchEngines in browser.js:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6228

Comment 3

[Chris Kitching [:ckitching]]

Attachment: Adding search preferences screen that lists available search engines and moves search suggestions tickbox onto this new screen.

Here you go!

Comment 4

[:Margaret Leibovic]

Comment on attachment 773755 [details] [diff] [review]
Adding search preferences screen that lists available search engines and moves search suggestions tickbox onto this new screen.

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

I'd also like Chenxia to take a look at this, because she's the expert on the settings UI.

Comment 5

[Chris Kitching [:ckitching]]

Attachment: Adding search preferences screen that lists available search engines and moves search suggestions tickbox onto this new screen. Now with fewer nits.

Apologies for the spam. Discovered some nits to pick - preempting the reviewers.

Comment 6

[Chris Kitching [:ckitching]]

Attachment: Added support for tablets.

I omitted to include the new "Search" button in the tablet-specific layout file. Fixed.
Even more spam..

Comment 7

[Chenxia Liu [:liuche]]

Comment on attachment 773815 [details] [diff] [review]
Adding search preferences screen that lists available search engines and moves search suggestions tickbox onto this new screen. Now with fewer nits.

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

Make sure you've tested on all relevant versions (pre-v11, v11+ phone, tablet), and also clicked around the settings, etc, to make sure things don't crash.

Looks pretty good, r+ with comments.

::: mobile/android/base/GeckoAppShell.java
@@ +2195,5 @@
>      public static void registerEventListener(String event, GeckoEventListener listener) {
>          sEventDispatcher.registerEventListener(event, listener);
>      }
>  
> +    public static EventDispatcher getEventDispatcher() {

It looks like this change is already in the m-c: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#2211

Make sure your patch applies to the most recent version of m-c or m-i.

::: mobile/android/base/Makefile.in
@@ +135,5 @@
>    ProfileMigrator.java \
>    Prompt.java \
>    PromptInput.java \
>    PromptService.java \
> +  preferences/SearchPreferenceCategory.java \

I know the sqlite dir stuff is up here too, but this should be alphabetized after case, so move this down to where the gfx/* and widget/* files are.

::: mobile/android/base/preferences/SearchPreferenceCategory.java
@@ +30,5 @@
> +    }
> +
> +    public SearchPreferenceCategory(Context context) {
> +        super(context);
> +        init();

Can this be done in onAttachedToActivity?

@@ +41,5 @@
> +    @Override
> +    protected void onAttachedToActivity() {
> +        super.onAttachedToActivity();
> +
> +        // Request list of search engines from Gecko...

No need to use ellipses here.

@@ +55,5 @@
> +
> +                // Create an element in this PreferenceCategory for each engine.
> +                for (int i = 0; i < engines.length(); i++) {
> +                    Preference engine = new Preference(getContext());
> +                    JSONObject engineJSON = engines.getJSONObject(i);

Nit: you call engingJSON.getString("name") twice - just save it as a variable.

@@ +56,5 @@
> +                // Create an element in this PreferenceCategory for each engine.
> +                for (int i = 0; i < engines.length(); i++) {
> +                    Preference engine = new Preference(getContext());
> +                    JSONObject engineJSON = engines.getJSONObject(i);
> +                    engine.setTitle(engineJSON.getString("name"));

I would be inclined to try/catch in here, so that if your first search engine pref *dies*, at least the other prefs have a chance of being added.

@@ +58,5 @@
> +                    Preference engine = new Preference(getContext());
> +                    JSONObject engineJSON = engines.getJSONObject(i);
> +                    engine.setTitle(engineJSON.getString("name"));
> +                    engine.setKey(engineJSON.getString("name"));
> +                    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) {

What does this look like on an older version? You might want to ask Ian if he wants some generic icon, or if he's ok with no icon. Add a screenshot to the bug and needinfo him.

Also, BitmapDrawable is present in v1, so add a comment explaining the version check. (I'd be curious to know.)

@@ +59,5 @@
> +                    JSONObject engineJSON = engines.getJSONObject(i);
> +                    engine.setTitle(engineJSON.getString("name"));
> +                    engine.setKey(engineJSON.getString("name"));
> +                    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) {
> +                        BitmapDrawable drawable = new BitmapDrawable(Bitmap.createScaledBitmap(BitmapUtils.getBitmapFromDataURI(engineJSON.getString("iconURI")), sIconSize, sIconSize, false));

Break this line into intermediate variable assignments for clarity (and because it's really long).

@@ +63,5 @@
> +                        BitmapDrawable drawable = new BitmapDrawable(Bitmap.createScaledBitmap(BitmapUtils.getBitmapFromDataURI(engineJSON.getString("iconURI")), sIconSize, sIconSize, false));
> +                        engine.setIcon(drawable);
> +                    }
> +                    addPreference(engine);
> +                    //TODO: Add event listener here for tapping on each element. Produce a dialog to provide options.

Add a reference to bug 892113 in the comment here.

@@ +66,5 @@
> +                    addPreference(engine);
> +                    //TODO: Add event listener here for tapping on each element. Produce a dialog to provide options.
> +                }
> +            } catch (JSONException e) {
> +                Log.e("Fennec", "Error getting search engine JSON", e);

See other classes for how to use Log. We use a LOGTAG string that matches the classname (as long as it's under 20 char because otherwise Android crashes the activity) so the log message is useful to someone unfamiliar to the class. E.g., don't use Fennec - it's way too broad.

::: mobile/android/base/resources/values/dimens.xml
@@ +76,5 @@
>      <dimen name="validation_message_margin_top">6dp</dimen>
>      <dimen name="forward_default_offset">-13dip</dimen>
>      <dimen name="addressbar_offset_left">32dp</dimen>
> +
> +    <dimen name="searchpreferences_icon_size">32dp</dimen>

Move this to the list of dimens above in alphabetical order.

::: mobile/android/base/resources/xml-v11/preferences_customize_tablet.xml
@@ +22,5 @@
>              android:title="@string/pref_import_android"
>              android:positiveButtonText="@string/bookmarkhistory_button_import"
>              android:negativeButtonText="@string/button_cancel"
>              android:persistent="false" />
>  

Noticed you added the search provider in the most recent version of the patch.

::: mobile/android/base/resources/xml/preferences_search.xml
@@ +9,5 @@
> +                  android:enabled="false">
> +
> +    <CheckBoxPreference android:key="browser.search.suggest.enabled"
> +                        android:title="@string/pref_search_suggestions"
> +                        android:defaultValue="true"

The default value should match the default Gecko preference, which I believe is "false". You can check any Gecko preference value manually in about:config (non-bold indicates default), or in mobile/android/app/mobile.js.

Things are getting a little complicated because some preferences are Java-only (e.g., healthreport), but that's only because those modules are Java-only, and not backed by any Gecko code. Therefore, we eliminated the Gecko preference altogether. These are preferences that are named "android.not_a_preference.*".

@@ +12,5 @@
> +                        android:title="@string/pref_search_suggestions"
> +                        android:defaultValue="true"
> +                        android:persistent="false" />
> +
> +    <org.mozilla.gecko.preferences.SearchPreferenceCategory android:key="android.not_a_preference.datareporting.preferences"

You don't necessarily need a key here, and it definitely should not be android.not*.datareporting.preferences. Unless this is a datareporting preference?

Basically, we added keys to certain categories because sometimes they modules they provide UI for are not built. For example, we have flags to determine whether or not to build telemetry, healthreport, and crash reporter - if none of those are built, then we don't want to display the buttons for them, and we *also* want to remove the category. The android:key for these categories gives us a way to remove it.

AFAIK, we always build search preferences, so there's no need to assign a key to that category.

Comment 8

[Chenxia Liu [:liuche]]

Comment on attachment 774122 [details] [diff] [review]
Added support for tablets.

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

Make sure you've tested on all relevant versions (pre-v11, v11+ phone, tablet), and also clicked around the settings, etc, to make sure things don't crash.
Send it back to me after addressing comments, and it should be an r+.

::: mobile/android/base/GeckoAppShell.java
@@ +2195,5 @@
>      public static void registerEventListener(String event, GeckoEventListener listener) {
>          sEventDispatcher.registerEventListener(event, listener);
>      }
>  
> +    public static EventDispatcher getEventDispatcher() {

It looks like this change is already in the m-c: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#2211

::: mobile/android/base/Makefile.in
@@ +135,5 @@
>    ProfileMigrator.java \
>    Prompt.java \
>    PromptInput.java \
>    PromptService.java \
> +  preferences/SearchPreferenceCategory.java \

I know the sqlite dir stuff is up here too, but this should be alphabetized after case, so move this down to where the gfx/* and widget/* files are.

::: mobile/android/base/preferences/SearchPreferenceCategory.java
@@ +21,5 @@
> +    private static int sIconSize;
> +
> +    public SearchPreferenceCategory(Context context, AttributeSet attrs, int defStyle) {
> +        super(context, attrs, defStyle);
> +        init();

Can this be done in onAttachedToActivity?

@@ +41,5 @@
> +    @Override
> +    protected void onAttachedToActivity() {
> +        super.onAttachedToActivity();
> +
> +        // Request list of search engines from Gecko...

No need to use ellipses here.

@@ +56,5 @@
> +                // Create an element in this PreferenceCategory for each engine.
> +                for (int i = 0; i < engines.length(); i++) {
> +                    Preference engine = new Preference(getContext());
> +                    JSONObject engineJSON = engines.getJSONObject(i);
> +                    engine.setTitle(engineJSON.getString("name"));

Nit: you call engingJSON.getString("name") twice - just save it as a variable.

I would be inclined to try/catch within the for loop, so that if your first search engine pref *dies*, at least the other prefs have a chance of being added.

@@ +58,5 @@
> +                    Preference engine = new Preference(getContext());
> +                    JSONObject engineJSON = engines.getJSONObject(i);
> +                    engine.setTitle(engineJSON.getString("name"));
> +                    engine.setKey(engineJSON.getString("name"));
> +                    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) {

What does this look like on an older version? You might want to ask Ian if he wants some generic icon, or if he's ok with no icon. Add a screenshot to the bug and needinfo him.
Also, BitmapDrawable is present in v1, so add a comment explaining the version check. (I for one would be curious to know.)

@@ +59,5 @@
> +                    JSONObject engineJSON = engines.getJSONObject(i);
> +                    engine.setTitle(engineJSON.getString("name"));
> +                    engine.setKey(engineJSON.getString("name"));
> +                    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) {
> +                        BitmapDrawable drawable = new BitmapDrawable(Bitmap.createScaledBitmap(BitmapUtils.getBitmapFromDataURI(engineJSON.getString("iconURI")), sIconSize, sIconSize, false));

Break this line into intermediate variable assignments for clarity (and because it's really long).

@@ +63,5 @@
> +                        BitmapDrawable drawable = new BitmapDrawable(Bitmap.createScaledBitmap(BitmapUtils.getBitmapFromDataURI(engineJSON.getString("iconURI")), sIconSize, sIconSize, false));
> +                        engine.setIcon(drawable);
> +                    }
> +                    addPreference(engine);
> +                    //TODO: Add event listener here for tapping on each element. Produce a dialog to provide options.

Add a reference to bug 892113 in the comment here.

@@ +66,5 @@
> +                    addPreference(engine);
> +                    //TODO: Add event listener here for tapping on each element. Produce a dialog to provide options.
> +                }
> +            } catch (JSONException e) {
> +                Log.e("Fennec", "Error getting search engine JSON", e);

See other classes for how to use Log. We use a LOGTAG string that matches the classname (as long as it's under 20 char because otherwise Android crashes the activity) so the log message is useful to someone unfamiliar to the class. E.g., don't use Fennec - it's way too broad.

::: mobile/android/base/resources/values/dimens.xml
@@ +76,5 @@
>      <dimen name="validation_message_margin_top">6dp</dimen>
>      <dimen name="forward_default_offset">-13dip</dimen>
>      <dimen name="addressbar_offset_left">32dp</dimen>
> +
> +    <dimen name="searchpreferences_icon_size">32dp</dimen>

Move this to the list of dimens above in alphabetical order.

::: mobile/android/base/resources/xml/preferences_search.xml
@@ +9,5 @@
> +                  android:enabled="false">
> +
> +    <CheckBoxPreference android:key="browser.search.suggest.enabled"
> +                        android:title="@string/pref_search_suggestions"
> +                        android:defaultValue="true"

The default value should match the default Gecko preference, which I believe is "false". You can check any Gecko preference value manually in about:config (non-bold indicates default), or in mobile/android/app/mobile.js.
Things are getting a little complicated because some preferences are Java-only (e.g., healthreport), but that's only because those modules are Java-only, and not backed by any Gecko code. Therefore, we eliminated the Gecko preference altogether. These are preferences that are named "android.not_a_preference.*".

@@ +12,5 @@
> +                        android:title="@string/pref_search_suggestions"
> +                        android:defaultValue="true"
> +                        android:persistent="false" />
> +
> +    <org.mozilla.gecko.preferences.SearchPreferenceCategory android:key="android.not_a_preference.datareporting.preferences"

You don't necessarily need a key here, and it definitely should not be android.not*.datareporting.preferences. Unless this is a datareporting preference?
Basically, we added keys to certain categories because sometimes they modules they provide UI for are not built. For example, we have flags to determine whether or not to build telemetry, healthreport, and crash reporter - if none of those are built, then we don't want to display the buttons for them, and we *also* want to remove the category. The android:key for these categories gives us a way to remove it.
AFAIK, we always build search preferences, so there's no need to assign a key to that category.

Comment 9

[:Margaret Leibovic]

Comment on attachment 774122 [details] [diff] [review]
Added support for tablets.

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

I agree with all of Chenxia's comments. You can just let her review your final patch :)

Comment 10

[Chris Kitching [:ckitching]]

Attachment: Applying review-imposed improvements.

> ::: mobile/android/base/Makefile.in
> @@ +135,5 @@
> >    ProfileMigrator.java \
> >    Prompt.java \
> >    PromptInput.java \
> >    PromptService.java \
> > +  preferences/SearchPreferenceCategory.java \
> 
> I know the sqlite dir stuff is up here too, but this should be alphabetized
> after case, so move this down to where the gfx/* and widget/* files are.

Sure - but there seem to be other files in this makefile which don't follow the rule (BrowserDB.java, for instance). Do we care enough to make a cleanup patch?


> ::: mobile/android/base/preferences/SearchPreferenceCategory.java
> @@ +21,5 @@
> > +    private static int sIconSize;
> > +
> > +    public SearchPreferenceCategory(Context context, AttributeSet attrs, int defStyle) {
> > +        super(context, attrs, defStyle);
> > +        init();
> 
> Can this be done in onAttachedToActivity?

The call to init can (It was necessary to add these three annoying constructors because otherwise the Android system seems to fail to inflate the view as it attempts, and fails, to use one of them), but then we would end up doing it every time the menu is opened. That said, I suppose that is preferable to doing it on app startup.


> @@ +58,5 @@
> > +                    Preference engine = new Preference(getContext());
> > +                    JSONObject engineJSON = engines.getJSONObject(i);
> > +                    engine.setTitle(engineJSON.getString("name"));
> > +                    engine.setKey(engineJSON.getString("name"));
> > +                    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) {
> 
> What does this look like on an older version? You might want to ask Ian if
> he wants some generic icon, or if he's ok with no icon. Add a screenshot to
> the bug and needinfo him.
> Also, BitmapDrawable is present in v1, so add a comment explaining the
> version check. (I for one would be curious to know.)

The part here that isn't available is the setIcon() method on the Preference. Since there'd be no point creating a BitmapDrawable if we can't use it, the whole shaboodle is inside the version check.
It looks essentially the same, but without the icons. I'll attach a screenshot shortly.
 
> ::: mobile/android/base/resources/xml/preferences_search.xml
> @@ +9,5 @@
> > +                  android:enabled="false">
> > +
> > +    <CheckBoxPreference android:key="browser.search.suggest.enabled"
> > +                        android:title="@string/pref_search_suggestions"
> > +                        android:defaultValue="true"
> 
> The default value should match the default Gecko preference, which I believe
> is "false". You can check any Gecko preference value manually in
> about:config (non-bold indicates default), or in
> mobile/android/app/mobile.js.

Interesting - I just pinched this block unchanged from its former location - looks like we found a new bug! This explains why I observed the tickbox changing itself on the really slow phone.

Comment 11

[Chris Kitching [:ckitching]]

Attachment: Old phone screenshot

Screenshot on an old phone.

Comment 12

[Chris Kitching [:ckitching]]

Attachment: New phone screenshot

Newer phone.. (Supports icons - Excitement!)

Comment 13

[Chris Kitching [:ckitching]]

Attachment: Tablet screenshot

Screenshot from tablet. Swankier layout, and icons to boot.

Comment 14

[Chris Kitching [:ckitching]]

Attachment: A wild space had appeared.

Comment 15

[Chris Kitching [:ckitching]]

Attachment: I accidentially a method body.

Comment 16

[Chenxia Liu [:liuche]]

Comment on attachment 774350 [details] [diff] [review]
I accidentially a method body.

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

Great, fix the 2 little nits, and then r+ with that.

In the future, please version your patches (v1, etc) - if you look at the list of obsolete patches, it's not immediately obvious from the patch descriptions what the progression is. Sorry about being nitpicky, but you can blame rnewman.

::: mobile/android/base/preferences/SearchPreferenceCategory.java
@@ +41,5 @@
> +
> +    @Override
> +    protected void onAttachedToActivity() {
> +        super.onAttachedToActivity();
> +        sIconSize = getContext().getResources().getDimensionPixelSize(R.dimen.searchpreferences_icon_size);

Ok, I think we should actually go with your initial work and do this in init() - no need to fetch the icon size every single time we attach. Number o times is probably the same because we never detach the category from the activity, but we definitely will not assign the iconSize more times in the constructor than in onAttachedToActivity. Sorry about making you change it.

@@ +69,5 @@
> +
> +                    Preference engine = new Preference(getContext());
> +                    engine.setTitle(engineName);
> +                    engine.setKey(engineName);
> +                    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) {

Add a comment here explaining that setIcon is not supported in pre-v11.

Comment 17

[Chenxia Liu [:liuche]]

Oh also, I think it's standard for the patch message to match the bug name exactly. So in this case, simply "Bug 892094 - Create "Search" page in settings. r=<reviewer>".

Comment 18

[Ian Barlow (:ibarlow)]

This is looking good Chris. 

Does this bug include adding "Default" under whichever engine has been set as the default?

Also seeing some inconsistencies with text alignment and icon size across phones and tablets. Tablets are using the right icon size, 'show search suggestions' should be aligned to the left.

Comment 19

[Chris Kitching [:ckitching]]

Attachment: V3 - Nit correction.

Final patch addressing the nitpicks.

Ian: The icon size thing is extremely strange. On all platforms the icon used is the one obtained from Gecko's search engine service for the engine. The icon is then scaled to 32dp. Shouldn't it be the case that this should produce identically sized icons? (dp are density-corrected pixels, right?).
Very strange. Further investigation needed on that one, methinks.

On the topic of text alignment on tablets - that one seems to be a quirk of the way Android draws Preference elements on the different platforms. Compare, for example, the text placement of "Show search suggestions" with that of "Double tap to reflow text" on the "Display" menu - these are the same. The problem is that this is not consistent with the alignment of things in the search provider items.
Why Android draws the two differently by default is sort of weird. I guess it handles things with icons somewhat differently to things without icons. That said, I am rapidly learning that attempting to apply sanity to the Android UI libraries is a futile task.
I believe that to shift the text to the left it would be necessary to create a new class inheriting from Preference with new rules about drawing things - is it a sufficiently big deal to warrant a mildly nontrivial amount of work?

This bug doesn't include displaying "Default" on the default engine. See bug 892113 for that and somewhat more (Patch due there extremely soon - it's starting to look somewhat usable. Yay!)

One final thing - your drawing included a sort of nifty little picture and tip showing how to add additional engines from the address bar. Could you provide an appropriately formatted edition of the little diagram image to be used to create that? It does seem a nice touch to have.

Comment 20

[Chenxia Liu [:liuche]]

Comment on attachment 775544 [details] [diff] [review]
V3 - Nit correction.

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

::: mobile/android/base/preferences/SearchPreferenceCategory.java
@@ +17,5 @@
> +import org.mozilla.gecko.gfx.BitmapUtils;
> +import org.mozilla.gecko.util.GeckoEventListener;
> +
> +public class SearchPreferenceCategory extends PreferenceCategory implements GeckoEventListener {
> +    public static final String LOGTAG = "SearchPreferenceCategory";

This tag is too long - the maximum length for Android log tags is 23 characters [1]. If you actually try to log anything with this (try it by sticking in a test Log.d somewhere), you'll get an IllegalArgumentException, and your activity will crash (ref: bug 850058). Maybe switch it to SearchPrefCategory.

...because Android.

[1] http://developer.android.com/reference/android/util/Log.html#isLoggable%28java.lang.String,%20int%29

@@ +56,5 @@
> +            try {
> +                engines = data.getJSONArray("searchEngines");
> +            } catch (JSONException e) {
> +                Log.e(LOGTAG, "Unable to decode search engine data from Gecko.");
> +                e.printStackTrace();

No stack traces in production code - just log it.

Comment 21

[Chenxia Liu [:liuche]]

needinfo?-ing Ian to address Chris's comment #19.

> One final thing - your drawing included a sort of nifty little picture and tip 
> showing how to add additional engines from the address bar. Could you provide an 
> appropriately formatted edition of the little diagram image to be used to create 
> that? It does seem a nice touch to have.

Good ask - I think bug 892113 is a better place to put this.

Comment 22

[Chenxia Liu [:liuche]]

(In case this wasn't clear, the rest of comment #19 has several other points to address, Ian.)

Comment 23

[Chris Kitching [:ckitching]]

Attachment: V4 - Android logtag insanity correction.

(In reply to Chenxia Liu [:liuche] from comment #20)
> ::: mobile/android/base/preferences/SearchPreferenceCategory.java
> @@ +17,5 @@
> > +import org.mozilla.gecko.gfx.BitmapUtils;
> > +import org.mozilla.gecko.util.GeckoEventListener;
> > +
> > +public class SearchPreferenceCategory extends PreferenceCategory implements GeckoEventListener {
> > +    public static final String LOGTAG = "SearchPreferenceCategory";
> 
> This tag is too long - the maximum length for Android log tags is 23
> characters [1]. If you actually try to log anything with this (try it by
> sticking in a test Log.d somewhere), you'll get an IllegalArgumentException,
> and your activity will crash (ref: bug 850058). Maybe switch it to
> SearchPrefCategory.
> 
> ...because Android.
> 
> [1]
> http://developer.android.com/reference/android/util/Log.
> html#isLoggable%28java.lang.String,%20int%29

Wow. That's my-apartment-building's-freight-elevator levels of stupid.

Fixed the new nits.

Comment 24

[Chenxia Liu [:liuche]]

Comment on attachment 776011 [details] [diff] [review]
V4 - Android logtag insanity correction.

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

Chris, thanks for doing so many review iterations. One last thing with the Logging, and string changes.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +72,5 @@
>  <!ENTITY pref_category_display "Display">
>  <!ENTITY pref_category_privacy_short "Privacy">
>  <!ENTITY pref_category_vendor "&vendorShortName;">
>  <!ENTITY pref_category_datareporting "Data choices">
> +<!ENTITY pref_category_installed_search_providers "Installed search providers">

I'd prefer "search engines" instead "search providers." It may not be *technically* correct, but it sounds less confusing/scary. You'll also have to update the entity name and the strings.xml.in reference.

::: mobile/android/base/preferences/SearchPreferenceCategory.java
@@ +55,5 @@
> +            JSONArray engines;
> +            try {
> +                engines = data.getJSONArray("searchEngines");
> +            } catch (JSONException e) {
> +                Log.e(LOGTAG, "Unable to decode search engine data from Gecko.");

Include the exception as the third arg to Log.e().

@@ +80,5 @@
> +                    }
> +                    addPreference(engine);
> +                    // TODO: Bug 892113 - Add event listener here for tapping on each element. Produce a dialog to provide options.
> +                } catch (JSONException e) {
> +                    Log.e(LOGTAG, "JSONException parsing engine at index "+i);

Nit: spaces around "+" operator, and also include the exception as the third argument to Log (http://developer.android.com/reference/android/util/Log.html#e%28java.lang.String,%20java.lang.String,%20java.lang.Throwable%29)

Comment 25

[Chris Kitching [:ckitching]]

Attachment: V5 - Further nitting.

Comment 26

[Chenxia Liu [:liuche]]

Attachment: v6 - unbitrot and use hg rename/copy

Patch looks good.

I tried applying it and it looks like it has bitrotted [1].

However! That reminds me that instead of copying files, we should be using hg rename and hg copy, so that we can preserve hg blame.

This is an unbitrotted patch (that also adds the restore-session pref to tablet, which bnicholson missed) that uses hg rename and hg copy. (Note in the diff, instead of "new file" for each of the customize files, they are now a copy or rename.

Have a look-over, make sure I didn't do anything stupid.

[1] http://hg.mozilla.org/mozilla-central/annotate/e5d74eebd0e2/mobile/android/base/resources/xml/preferences_customize.xml

Comment 27

[Chris Kitching [:ckitching]]

Ah. Whoops.
Your patch looks great. I should pull more often, it seems.

Comment 28

[Ian Barlow (:ibarlow)]

(In reply to Chris Kitching [:ckitching] from comment #19)
> Created attachment 775544 [details] [diff] [review]
> V3 - Nit correction.
> 
> Final patch addressing the nitpicks.
> 
> Ian: The icon size thing is extremely strange. On all platforms the icon
> used is the one obtained from Gecko's search engine service for the engine.
> The icon is then scaled to 32dp. Shouldn't it be the case that this should
> produce identically sized icons? (dp are density-corrected pixels, right?).
> Very strange. Further investigation needed on that one, methinks.

I'm not sure what you're asking me here. Given Android's wide range of pixel densities, we donn't really rely on pixels to define the size of an icon because it would vary wildly from device to device. I guess my point is that in some screenshots the icons are tiny and in some they are big -- but they should be big everywhere.

> Why Android draws the two differently by default is sort of weird. I guess
> it handles things with icons somewhat differently to things without icons.
> That said, I am rapidly learning that attempting to apply sanity to the
> Android UI libraries is a futile task.
> I believe that to shift the text to the left it would be necessary to create
> a new class inheriting from Preference with new rules about drawing things -
> is it a sufficiently big deal to warrant a mildly nontrivial amount of work?

Heh. Ok, let's leave it for now and I'll file a followup bug to investigate. Not the top priority right now. 
 
> This bug doesn't include displaying "Default" on the default engine. See bug
> 892113 for that and somewhat more (Patch due there extremely soon - it's
> starting to look somewhat usable. Yay!)

Awesome!

> One final thing - your drawing included a sort of nifty little picture and
> tip showing how to add additional engines from the address bar. Could you
> provide an appropriately formatted edition of the little diagram image to be
> used to create that? It does seem a nice touch to have.

Ill put that together for you very soon, yes.

Comment 29

[Ian Barlow (:ibarlow)]

Chris, in the interest of moving things along, you can also land this without the tip and file a follow-up bug for the tip graphic, and cc me on it.

Comment 30

[:Margaret Leibovic]

(In reply to Chenxia Liu [:liuche] from comment #17)
> Oh also, I think it's standard for the patch message to match the bug name
> exactly. So in this case, simply "Bug 892094 - Create "Search" page in
> settings. r=<reviewer>".

Disagree! I think it's best to have the commit message describe what the patch is doing. In this case the two are very similar, but in other cases, the bug summary can be describing a defect that doesn't actually relate to the commit that's landing. This is me being pedantic here :)

Comment 31

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fe88df5c376

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5fe88df5c376

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android 2.2 robocop-2 failures.
https://hg.mozilla.org/mozilla-central/rev/0888e29c83a3

https://tbpl.mozilla.org/php/getParsedLog.php?id=25342225&tree=Mozilla-Inbound

Comment 34

[Chenxia Liu [:liuche]]

Attachment: Include test fix in testSettingsMenuItems

I forgot to ask you to update the test, Chris, so this is a quick fix to the robocop tests to correctly reflect the settings menu.

(I also added restore-session to the test while I was there, for bug 801412.)

Comment 35

[Chris Kitching [:ckitching]]

Comment on attachment 776713 [details] [diff] [review]
Include test fix in testSettingsMenuItems

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

Test seems to work, nothing else has been changed. Seems good.

Comment 36

[Chenxia Liu [:liuche]]

Relanded with tests fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1028e173331

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f1028e173331

Comment 38

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 25.0a1( 2013-07-18)
Device: LG Nexus 4
OS: Android 4.1.1

Comment 39

[:Margaret Leibovic]

Now that the follow-up work to this missed the merge, we should back this out of aurora.

Comment 40

[Chenxia Liu [:liuche]]

Attachment: Patch: back out search [aurora]

With the backout patch, I'm going to keep the Customize menu that was added, and just replace "Search Settings" with the previous "Show search suggestions", because later patches have added settings into the Customize menu.

Try build here: https://tbpl.mozilla.org/?tree=Try&rev=4b068fa81b82

Chris, can you make sure this removes the things you changed for this bug and that I didn't miss anything?

Comment 41

[Chenxia Liu [:liuche]]

Comment on attachment 786403 [details] [diff] [review]
Patch: back out search [aurora]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Backing out because the rest of Search settings did not make 25 (bug 892113, bug 898151, bug 895423)
User impact if declined: Incomplete search settings feature
Testing completed (on m-c, etc.): Backout patch on try: https://tbpl.mozilla.org/?tree=Try&rev=4b068fa81b82
Risk to taking this patch (and alternatives if risky): low - leaving in the UI changes that have been landed on top of this patch.
String or IDL/UUID changes made by this patch: Removal of two strings that landed with this feature

Comment 42

[Chris Kitching [:ckitching]]

Comment on attachment 786403 [details] [diff] [review]
Patch: back out search [aurora]

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

Seems to be a perfect complement of the patch that landed - this should do the job.

Comment 43

[Chenxia Liu [:liuche]]

Backout patch is all green on try, and manually verified that the changes are backed out in the try build, on both tablet and phone.

Comment 44

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

Why the a- for backing out the patch from Aurora? Do we need to remove the feature, but keep the strings?

Comment 45

[Chenxia Liu [:liuche]]

Attachment: Patch: back out search [aurora] v2 - no string changes

This is the same approval request, but without string removal.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Backing out because the rest of Search settings did not make 25 (bug 892113, bug 898151, bug 895423)
User impact if declined: Incomplete search settings feature
Testing completed (on m-c, etc.): Backout patch on try: https://tbpl.mozilla.org/?tree=Try&rev=4b068fa81b82
Risk to taking this patch (and alternatives if risky): low - leaving in the UI changes that have been landed on top of this patch.
String or IDL/UUID changes made by this patch: none, left the strings added by this feature in

Comment 46

[Chenxia Liu [:liuche]]

Needinfo-ing akeybl, for comment #44 - just to clarify, this is approval request for a backout of an incomplete feature, not uplifting a new feature that removes strings.

Comment 47

[Alex Keybl [:akeybl]]

Comment on attachment 787902 [details] [diff] [review]
Patch: back out search [aurora] v2 - no string changes

Sounds good

Comment 48

[Chenxia Liu [:liuche]]

Backout landed on aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/6a1a48886f7e

Comment 49

[Paul Feher]

Verified fixed on:
Firefox 26 Beta 1 (2013-10-26)
Device: LG Optimus 4X
OS: Android 4.1.2