892113 - Dialog to manage installed search providers

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

This will be follow-up work to bug 892094. We want to show a dialog when you tap on an installed search provider, giving you enable/disable/set as default options.

Comment 1

[Chris Kitching [:ckitching]]

I notice this dialog isn't in the pictures provided on Bug 891115 - I'd appreciate some input on how the layout in this dialog should be.
I guess we'll also want a "Delete" button for search providers?

Comment 2

[Ian Barlow (:ibarlow)]

Hi Chris, sure.

The three main items in the menu should be


  +-------------------------------+
  |                               |
  | Set as default                |
  |                               |
  +-------------------------------+
  |                               |
  | Disable                       |
  |                               |
  +-------------------------------+
  |                               |
  | Uninstall                     |
  |                               |
  +-------------------------------+


* "Set as default" should be greyed this out if already the default
* If an engine has been disabled, that menu item should read "Enable"

Comment 3

[Chris Kitching [:ckitching]]

Surely it is the case that this patch depends on the existence of the search settings screen, not that it blocks it (You could, after all, create the settings screen without these buttons doing anything (I just did, in fact :P )).
Or am I misunderstanding the use of the flags? (It seems that my patch here will depend on the one for 892094, so isn't this relationship a dependency, not a blockage?)

Comment 4

[Chris Kitching [:ckitching]]

Attachment: Part 1/3: The shiny new UI, not actually talking to Gecko.

A patch in three parts:

1) Add the new UI to display the default engine and enabledness of engines in the list; a prompt to remove, toggle enabledness, and uninstall engines.
2) Have the new UI components induce the appropriate configuration changes in Gecko's search engine service (Altering it as needed to make this work)
3) Remove the now-obsolete facility for changing default search engine (via the addons menu - that horrid kludge)

Included: Part the first.
You can tap shiny buttons and they'll lie horridly about actually changing your settings while not actually changing your settings. Useful for seeing how the user experience is expected to be (Feedback request in case Ian wants to give this a spin and provide comments about the way this works - if you're not in the mood for applying a patch and compiling Fennec I can provide a series of screenshots/a video illustrating how this feels in a few hours.)

Part the second - coming today..

Comment 5

[Chris Kitching [:ckitching]]

Note: The patches here build on the work done for Bug 892094 - it is necessary to apply that patch first.

Comment 6

[Ian Barlow (:ibarlow)]

Hi Chris, I'm not equipped to make builds. I'm happy to try a build if you make one, or screenshots/video are great too, thanks.

Comment 7

[Chris Kitching [:ckitching]]

No problem - here's an apk with the patches installed for you:
https://www.dropbox.com/s/q5e9s0ajpd6ir94/fennec-25.0a1.en-US.android-arm.apk
Enjoy!

Comment 8

[Chris Kitching [:ckitching]]

Attachment: Part 1/3: The shiny new UI, not actually talking to Gecko. V2 (BORK).

Regenerated patch that will apply atop the revised parent patch.

Comment 9

[Chris Kitching [:ckitching]]

Attachment: Part 1/3: The shiny new UI, not actually talking to Gecko. V3 - Regenerated against changes to parent patch. Without deleting it this time.

Comment 10

[Chris Kitching [:ckitching]]

Attachment: Part 1/3: The shiny new UI, not actually talking to Gecko. V4 - Minor bug fix

Uninstalling the default search engine now leads to a fallback being selected, instead of an undefined default engine.

Comment 11

[Chris Kitching [:ckitching]]

Attachment: Part 1/3: The new UI, sans actually talking to Gecko. V5 - Eliminated race condition with inflaters.

Occasionally the enabledness properties would be reset by the fact that GeckoPreferences called setEnabled(true) on all elements when it finishes setup. No longer.

Comment 12

[Chris Kitching [:ckitching]]

Attachment: Part 2/3: Communication with Gecko to save the changes made via the UI.

Causes the buttons added in the previous patch to actually have their results saved by the Gecko engine. We have a nice way of changing search providers! Yay!

Comment 13

[Chris Kitching [:ckitching]]

Attachment: Part 3/3: Eliminate the original method of changing search engine settings (Via the addons menu)

Removing the somewhat kludgeesque prior implementation. For now leaving the actual xml files used to define the bundled search engines as they are, as it is not entirely clear how best to refactor these (If we care to at all).
I note that the xml files in question actually include the favicons for the search engines inline - this is a somewhat wasteful method of encoding graphics - the apk could be made very slightly smaller if these were refactored out into seperate image files and loaded in the usual way.

Comment 14

[Chris Kitching [:ckitching]]

(We also probably don't care too much about entirely correctly incrementally removing things from the current addons management screen, since my next task is, essentially, to get rid of it)

In fact, with that in mind, it might be worthwhile to just ignore the third patch here for now. At some later point we can just delete the offending js file entirely.

Comment 15

[Ian Barlow (:ibarlow)]

This is looking good, Chris. Couple comments:

Menu should read "Set as default". Right now it says "Set default".

The default search provider should just say "Default" under it, not "Default search engine". 

Turning search suggestions on and off seems to blow away the "default" label in the search provider list. This shouldn't be happening.

Comment 16

[:Margaret Leibovic]

(In reply to Chris Kitching [:ckitching] from comment #3)
> Surely it is the case that this patch depends on the existence of the search
> settings screen, not that it blocks it (You could, after all, create the
> settings screen without these buttons doing anything (I just did, in fact :P
> )).
> Or am I misunderstanding the use of the flags? (It seems that my patch here
> will depend on the one for 892094, so isn't this relationship a dependency,
> not a blockage?)

We usually use "block" for follow-up work, meaning that the search settings screen is the whole feature, and this is part of it. It doesn't really matter much, but it does make it show up in the meta bug's dependency tree, which is useful. However, we could also just set this to block the meta bug so that it shows up there :)

Comment 17

[:Margaret Leibovic]

(In reply to Chris Kitching [:ckitching] from comment #14)
> (We also probably don't care too much about entirely correctly incrementally
> removing things from the current addons management screen, since my next
> task is, essentially, to get rid of it)
> 
> In fact, with that in mind, it might be worthwhile to just ignore the third
> patch here for now. At some later point we can just delete the offending js
> file entirely.

We should check with product to see if we're comfortable just shipping the search bit in settings without the add-ons bit. If we want to ship these features together, it would be best to leave about:addons alone, so that we can fall back to using it if we don't think we'll be able to ship in 25.

Comment 18

[:Margaret Leibovic]

Comment on attachment 776224 [details] [diff] [review]
Part 1/3: The new UI, sans actually talking to Gecko. V5 - Eliminated race condition with inflaters.

Oops, looks like you accidentally uploaded the part 2 patch twice here.

Comment 19

[:Margaret Leibovic]

Comment on attachment 776226 [details] [diff] [review]
Part 2/3: Communication with Gecko to save the changes made via the UI.

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

::: mobile/android/chrome/content/browser.js
@@ +6341,5 @@
> +        engine.hidden = false;
> +      break;
> +      case "SearchEngines:Disable":
> +        engine = this._extractEngineFromJSON(aData);
> +        engine.hidden = true;

Drive-by: I wonder if it would be worth combining these two messages to save on an observer. We could just make one "SearchEngines:Enable" message, and pass the enable/disable state as a boolean. Of course that would then require us to parse aData differently in this case.

Comment 20

[Chris Kitching [:ckitching]]

> ::: mobile/android/chrome/content/browser.js
> @@ +6341,5 @@
> > +        engine.hidden = false;
> > +      break;
> > +      case "SearchEngines:Disable":
> > +        engine = this._extractEngineFromJSON(aData);
> > +        engine.hidden = true;
> 
> Drive-by: I wonder if it would be worth combining these two messages to save
> on an observer. We could just make one "SearchEngines:Enable" message, and
> pass the enable/disable state as a boolean. Of course that would then
> require us to parse aData differently in this case.


I actually refactored from that to this for readability - are observers expensive, then, and something I should be trying to minimize?

(In reply to :Margaret Leibovic from comment #18)
> Comment on attachment 776224 [details] [diff] [review]
> Part 1/3: The new UI, sans actually talking to Gecko. V5 - Eliminated race
> condition with inflaters.
> 
> Oops, looks like you accidentally uploaded the part 2 patch twice here.


Drat.

Comment 21

[Chris Kitching [:ckitching]]

Attachment: Part 1/3: The new UI, sans actually talking to Gecko. V6 - Really eliminated race condition with inflaters.

Comment 22

[Chenxia Liu [:liuche]]

Comment on attachment 776504 [details] [diff] [review]
Part 1/3: The new UI, sans actually talking to Gecko. V6 - Really eliminated race condition with inflaters.

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

Is this the same patch as from bug 892094? If so, it doesn't need to be labeled 1/3, because it's technically not part of this bug. (If it is different, strip out the parts that have changed from the other bug.) In the future, just differentiate patch dependencies by specifying that this is a patch from bug <number> and it should be applied before patches for this bug.

Comment 23

[Chris Kitching [:ckitching]]

Comment on attachment 776504 [details] [diff] [review]
Part 1/3: The new UI, sans actually talking to Gecko. V6 - Really eliminated race condition with inflaters.

Yes, of course. Wow. Impressively stupid. Managed to convince myself that this patch was in fact two patches.

Comment 24

[Chris Kitching [:ckitching]]

Attachment: Part 1/3: New UI. V7 (Race condition with GekcoPreferences)

Ah! No! I'm not quite as much of an idiot as previously thought. There ARE three patches, but I uploaded the wrong one for number 1 previously.
So, in summary, there are:
1) Make the buttons work
2) Make the buttons actually talk to Gecko
3) Remove the old stuff.

But, in my haste, I accidentially uploaded the patch from 892116 in place of 1.

Right. Okay. Fine. Yay. That's how it's supposed to be.

Comment 25

[Chenxia Liu [:liuche]]

A quick drive-by after playing with the applied patches:

- Add the string changes from Ian's comment 15
- Bug: when I disable all the search engines and then re-eanble the previous default, it remains grayed out
- Disabling all the search engines causes 404 when typing words into the urlbar. That probably indicates that you shouldn't be able to disable all the search engines.

Comment 26

[Chenxia Liu [:liuche]]

Ian: I think the enable-disable-uninstall actions for search engines are really confusing, especially since there isn't explicit context for what enabling/disabling a search engine means, and where this would be. What do you think about something like "Show/Hide in search options" (choices?) and "Remove"?

Comment 27

[Chris Kitching [:ckitching]]

Attachment: New UI. V8 - Strings corrected. The last search engine cannot be removed or disabled.

Since the last engine is always the default at that point, and since we want to be unable to either disable or uninstall the final engine, the problem described is simply solved by declining to show the prompt when only one engine is left. (As all the buttons on it would be disabled anyway)
Is this behaviour okay, or does anyone have a better idea?

Comment 28

[Ian Barlow (:ibarlow)]

(In reply to Chenxia Liu [:liuche] from comment #26)
> Ian: I think the enable-disable-uninstall actions for search engines are
> really confusing, especially since there isn't explicit context for what
> enabling/disabling a search engine means, and where this would be. What do
> you think about something like "Show/Hide in search options" (choices?) and
> "Remove"?

That's a good point. I suppose that language is carry-over from when Search lived inside of Add-ons, which makes even less sense now that we are separating them. Your suggestion for strings is certainly clearer, let's do it.

Comment 29

[Chris Kitching [:ckitching]]

Attachment: Part 2/3: Communication with Gecko to save the changes made via the UI. V2 - Fixing bitrot.

Part 2/3: Communication with Gecko to save the changes made via the UI. V2: Fighting bitrot.

Comment 30

[Chris Kitching [:ckitching]]

Attachment: Part 1/3: New UI. V9 - Strings corrected. The last search engine cannot be removed or disabled. Unbitrotted

Comment 31

[Chris Kitching [:ckitching]]

Attachment: Part 1/3: New UI. V10 - Adding latest string changes.

Added the suggested changes to the labels for "Enabled"/"Disabled".

To my mind, "Show in search options" is a little confusing, too, because it might be taken to mean "Show in the options menu for search" - where we currently are - and it then responds by greying out.
I don't think this label makes it perfectly clear what disabling it really does (Stops it from showing up in the list of search engine possibilities that appear as you are typing something in the address bar).

It's certainly substantially clearer than "Enable"/"Disable" - I just wonder if we could do even better?

Comment 32

[Chris Kitching [:ckitching]]

Attachment: Part 2/3: Communication with Gecko to save the changes made via the UI. V3 - Adding correct patch..

Comment 33

[Chris Kitching [:ckitching]]

Attachment: Part 1/3: New UI. V11 - Preventing the "Uninstallation" of bundled search engines.

The uninstall option is now greyed out for bundled engines - Gecko won't allow us to actually uninstall these anyway.

Comment 34

[Chenxia Liu [:liuche]]

Comment on attachment 777996 [details] [diff] [review]
Part 1/3: New UI. V10 - Adding latest string changes.

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

Nice job! This is looking very good (lots of nits).

In general, your code is doing the right thing, but try to be more clear about variable names and more concise/clear about comments. There are some places where variables and comments are confusing rather than as helpful as they could be.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +154,5 @@
>  <!ENTITY pref_vendor_faqs "FAQs">
>  <!ENTITY pref_vendor_feedback "Give feedback">
> +<!ENTITY pref_search_set_default "Set as default">
> +<!ENTITY pref_search_default "Default">
> +<!ENTITY pref_search_enable "Show in search options">

I think you might have a point for strings - looking at this in the UI, the context is unclear. What do you think about something like "Show (in) search results" and "Hide (from) search results"?
Nit: update the entity names to reflect the string, both here and in strings.xml.in.

@@ +156,5 @@
> +<!ENTITY pref_search_set_default "Set as default">
> +<!ENTITY pref_search_default "Default">
> +<!ENTITY pref_search_enable "Show in search options">
> +<!ENTITY pref_search_disable "Hide from search options">
> +<!ENTITY pref_search_disabled "Disabled">

I think this string is ok, but it's not very consistent with the "Show/hide" changes we've made. Give it a thought (although I'm fine with keeping it the same).

@@ +157,5 @@
> +<!ENTITY pref_search_default "Default">
> +<!ENTITY pref_search_enable "Show in search options">
> +<!ENTITY pref_search_disable "Hide from search options">
> +<!ENTITY pref_search_disabled "Disabled">
> +<!ENTITY pref_search_uninstall "Uninstall">

I find the idea of "uninstalling" a search engine still weird, so how about "Remove"?

::: mobile/android/base/preferences/SearchEnginePreference.java
@@ +21,5 @@
> +import org.mozilla.gecko.R;
> +import org.mozilla.gecko.gfx.BitmapUtils;
> +import org.mozilla.gecko.util.ThreadUtils;
> +
> +// Represents an element in the list of search engines on the preferences menu.

Use /** **/ style javadocs for comments about classes. The rest of the code is pretty lax, but let's start making new comments better!

@@ +23,5 @@
> +import org.mozilla.gecko.util.ThreadUtils;
> +
> +// Represents an element in the list of search engines on the preferences menu.
> +public class SearchEnginePreference extends Preference {
> +    // Constants representing the indices in the button array of the AlertDialog of the three buttons.

Conciseness: // Indices of AlertDialog buttons.

@@ +24,5 @@
> +
> +// Represents an element in the list of search engines on the preferences menu.
> +public class SearchEnginePreference extends Preference {
> +    // Constants representing the indices in the button array of the AlertDialog of the three buttons.
> +    public static final int SET_DEFAULT_BUTTON_POSITION = 0;

Just my preference, but I'd prefer these to all start with the same prefix. Like INDEX_SET_DEFAULT, INDEX_TOGGLE_ENABLE, INDEX_REMOVE perhaps?

@@ +28,5 @@
> +    public static final int SET_DEFAULT_BUTTON_POSITION = 0;
> +    public static final int TOGGLE_ENABLEDNESS_BUTTON_POSITION = 1;
> +    public static final int UNINSTALL_BUTTON_POSITION = 2;
> +
> +    // 'twould be nice to have this as static, but the structure just isn't there to reliably have it deallocated.

More formal style for comments.

@@ +29,5 @@
> +    public static final int TOGGLE_ENABLEDNESS_BUTTON_POSITION = 1;
> +    public static final int UNINSTALL_BUTTON_POSITION = 2;
> +
> +    // 'twould be nice to have this as static, but the structure just isn't there to reliably have it deallocated.
> +    // To avoid constantly using the resource manager, and also to avoid keeping them in memory after we stop caring.. (Static would be nice..)

same for style.

@@ +30,5 @@
> +    public static final int UNINSTALL_BUTTON_POSITION = 2;
> +
> +    // 'twould be nice to have this as static, but the structure just isn't there to reliably have it deallocated.
> +    // To avoid constantly using the resource manager, and also to avoid keeping them in memory after we stop caring.. (Static would be nice..)
> +    public String DISABLE_LABEL;

Same prefix; also, since we've been changing the strings, make sure the string names are somewhat consistent with the strings themselves.

@@ +33,5 @@
> +    // To avoid constantly using the resource manager, and also to avoid keeping them in memory after we stop caring.. (Static would be nice..)
> +    public String DISABLE_LABEL;
> +    public String ENABLE_LABEL;
> +    public String IS_DEFAULT_TEXT;
> +    // The span element used to colour text grey for disabled elements

Nit: period.

@@ +36,5 @@
> +    public String IS_DEFAULT_TEXT;
> +    // The span element used to colour text grey for disabled elements
> +    private ForegroundColorSpan disabledSpan;
> +
> +    // Does this object represent the currently configured default search engine?

I think these variables are named in an obvious way and don't really need the comments.

Also, in the future, only use declarative comments - these read like TODO/verify to me.

@@ +38,5 @@
> +    private ForegroundColorSpan disabledSpan;
> +
> +    // Does this object represent the currently configured default search engine?
> +    private boolean isDefaultEngine;
> +    // Is this engine hidden?

same

@@ +41,5 @@
> +    private boolean isDefaultEngine;
> +    // Is this engine hidden?
> +    private boolean isHiddenEngine;
> +
> +    // The strings used as labels in the prompt that appears when this item is tapped.

Nit: just "Labels for search options prompt" or something. Concise!

@@ +55,5 @@
> +    private SpannableString titleSpannable;
> +
> +    private SearchPreferenceCategory parentCategory;
> +
> +    public SearchEnginePreference(Context context, SearchPreferenceCategory backreference) {

A javadoc comment here, describing what "backreference" is. Could also just rename it to "parentCategory".

@@ +71,5 @@
> +        setOnPreferenceClickListener(new OnPreferenceClickListener() {
> +            @Override
> +            public boolean onPreferenceClick(Preference preference) {
> +                SearchEnginePreference sPref = (SearchEnginePreference) preference;
> +                //Access and display the dialog configured on the preference object.

Nit: space between // and comment.

What does "configured on the preference object" mean? Perhaps "configured from."

@@ +84,5 @@
> +        ENABLE_LABEL = res.getString(R.string.pref_search_enable);
> +        IS_DEFAULT_TEXT = res.getString(R.string.pref_search_default);
> +
> +        // Set up default dialog items.
> +        dialogItems = new String[] {res.getString(R.string.pref_search_set_default),

Nit: spaces between {} and inner contents.

@@ +92,5 @@
> +        // Spannable used for expressing enabledness.
> +        disabledSpan = new ForegroundColorSpan(res.getColor(R.color.text_color_primary_disable_only));
> +    }
> +
> +    // Configure this object to display the properties of a Gecko search engine object.

// Configure this Preference object from the Gecko search engine JSON object.

@@ +93,5 @@
> +        disabledSpan = new ForegroundColorSpan(res.getColor(R.color.text_color_primary_disable_only));
> +    }
> +
> +    // Configure this object to display the properties of a Gecko search engine object.
> +    public void setSearchEngine(JSONObject o) throws JSONException {

setSearchEngineFromJSON, perhaps? Also, name o differently, to geckoEngineJSON or something.

@@ +110,5 @@
> +            setIcon(drawable);
> +        }
> +    }
> +
> +    // Specify if this engine is the currently configured default one or not (Shows subtitle text identifying as such)

Make this more concise; nits: punctuation, capitalization.
Maybe javadoc it?
/**
    Set this engine's default state to <code>isDefault</code> and update Preference display.
...
**/

@@ +112,5 @@
> +    }
> +
> +    // Specify if this engine is the currently configured default one or not (Shows subtitle text identifying as such)
> +    public void setIsDefaultEngine(boolean isDefault) {
> +        isDefaultEngine = isDefault;

this.isDefaultEngine = isDefault to be more clear.

@@ +124,5 @@
> +            }
> +        }
> +    }
> +
> +    // Configure display for the hiddenness of this element - If hidden grey out and show "Disabled".

Nit: capitalization.

@@ +126,5 @@
> +    }
> +
> +    // Configure display for the hiddenness of this element - If hidden grey out and show "Disabled".
> +    public void setIsHiddenEngine(boolean hidden) {
> +        isHiddenEngine = hidden;

this.isHiddenEngine

@@ +128,5 @@
> +    // Configure display for the hiddenness of this element - If hidden grey out and show "Disabled".
> +    public void setIsHiddenEngine(boolean hidden) {
> +        isHiddenEngine = hidden;
> +
> +        // Update the button labels for "Enable"/"Disable" depending on enabledness state.

Update comment to match "Show/Hide".

@@ +158,5 @@
> +        // Due to Andrid's lazy loading of buttons, we can't actually configure the enabledness of the buttons until the
> +        // prompt is shown, so we do this inside the onShowListener. But first - build it!
> +
> +        // If we are the only engine left, then we are the default engine, and none of the options on this menu can do anything.
> +        if (parentCategory.isLastEngineStanding(this)) {

I'd almost rather this be made not clickable, and maybe pop up a toast explaining why, but we should see what Ian thinks about it. (I know, that makes it more complicated...)

@@ +167,5 @@
> +        builder.setTitle(getTitle().toString());
> +        builder.setItems(dialogItems, new DialogInterface.OnClickListener() {
> +            // Forward the various events that we care about to the container class for handling.
> +            @Override
> +            public void onClick(DialogInterface dialog, int which) {

indexClicked, instead of which?

@@ +172,5 @@
> +                hideDialog();
> +                switch (which) {
> +                    case SET_DEFAULT_BUTTON_POSITION:
> +                        parentCategory.setDefault(SearchEnginePreference.this);
> +                    break;

Indent: Fennec style aligns break; with previous line. Do the same for the other cases.

@@ +178,5 @@
> +                        parentCategory.toggleEnabledness(SearchEnginePreference.this);
> +                    break;
> +                    case UNINSTALL_BUTTON_POSITION:
> +                        parentCategory.uninstall(SearchEnginePreference.this);
> +                    break;

Add a default: with a Log.w.

@@ +181,5 @@
> +                        parentCategory.uninstall(SearchEnginePreference.this);
> +                    break;
> +                }
> +            }
> +        });

Nit: newline.

@@ +193,5 @@
> +            @Override
> +            public void run() {
> +                dialog = builder.create();
> +                dialog.setOnShowListener(new DialogInterface.OnShowListener() {
> +                    // Called when the dialog is shown (And we're finally able to manipulate button enabledness)

Nit: period.

@@ +217,5 @@
> +        });
> +    }
> +
> +    // Having shown the dialog, the lazily-created button elements therein will have been spawned by Android, so we can
> +    // now muck about with them as we want.

More formal comment style.

@@ +219,5 @@
> +
> +    // Having shown the dialog, the lazily-created button elements therein will have been spawned by Android, so we can
> +    // now muck about with them as we want.
> +    private void configureShownDialog() {
> +        // If we are the default engine, disable the "Set default" button.

Update comment to reflect strings.

@@ +221,5 @@
> +    // now muck about with them as we want.
> +    private void configureShownDialog() {
> +        // If we are the default engine, disable the "Set default" button.
> +        TextView defaultButton = (TextView) dialog.getListView().getChildAt(SET_DEFAULT_BUTTON_POSITION);
> +        // Disable "Set default" button if we are already the default.

same

::: mobile/android/base/preferences/SearchPreferenceCategory.java
@@ +1,1 @@
>  package org.mozilla.gecko.preferences;

Add a license here.

@@ +22,1 @@
>      }

Nit: while you're here, add a newline.

@@ +31,5 @@
>      @Override
>      protected void onAttachedToActivity() {
>          super.onAttachedToActivity();
>  
> +        // Useful for ensuring the default engine is always at the top of the list.

"Ensure default engine is at top of list."

@@ +44,5 @@
>      public void handleMessage(String event, final JSONObject data) {
>          if (event.equals("SearchEngines:Data")) {
>              JSONArray engines;
> +            JSONObject defaultEngine;
> +            final String defaultEngineName;

Comment here:
// Get engines array from JSON, and also fetch the default engine name from first item in array.

or something along those lines.

@@ +49,3 @@
>              try {
>                  engines = data.getJSONArray("searchEngines");
> +                if (engines.length() == 0) {

Should there ever be no engines? Perhaps Log.w here, "No search engines."

@@ +63,5 @@
>                  try {
>                      JSONObject engineJSON = engines.getJSONObject(i);
>                      final String engineName = engineJSON.getString("name");
>  
> +                    SearchEnginePreference engine = new SearchEnginePreference(getContext(), this);

Lots of "engine" in this section here - let's be more explicit and name this enginePreference.

@@ +64,5 @@
>                      JSONObject engineJSON = engines.getJSONObject(i);
>                      final String engineName = engineJSON.getString("name");
>  
> +                    SearchEnginePreference engine = new SearchEnginePreference(getContext(), this);
> +                    engine.setSearchEngine(engineJSON);

I'd prefer this to be named explicitly too - setSearchEnginFromJSON or something.

@@ +68,5 @@
> +                    engine.setSearchEngine(engineJSON);
> +                    if (engineName.equals(defaultEngineName)) {
> +                        engine.setIsDefaultEngine(true);
> +                        defaultEngineReference = engine;
> +                    }

Nit: newline.

@@ +73,5 @@
> +                    engine.setOnPreferenceClickListener(new OnPreferenceClickListener() {
> +                        @Override
> +                        public boolean onPreferenceClick(Preference preference) {
> +                            SearchEnginePreference sPref = (SearchEnginePreference) preference;
> +                            //Access and display the dialog configured on the preference object.

Nit: space between // and comment.
Also, "configured on the preference object"? Can you make that a little more clear?

@@ +88,5 @@
>          }
> +        GeckoAppShell.unregisterEventListener("SearchEngines:Data", this);
> +    }
> +
> +    boolean isLastEngineStanding(SearchEnginePreference engine) {

More formal method name: isLastEnabledEngine

@@ +91,5 @@
> +
> +    boolean isLastEngineStanding(SearchEnginePreference engine) {
> +        if (engine.getIsHiddenEngine()) {
> +            return false;
> +        }

Nit: newline.

@@ +92,5 @@
> +    boolean isLastEngineStanding(SearchEnginePreference engine) {
> +        if (engine.getIsHiddenEngine()) {
> +            return false;
> +        }
> +        int prefs = getPreferenceCount();

Nit: numPrefs.

@@ +94,5 @@
> +            return false;
> +        }
> +        int prefs = getPreferenceCount();
> +        for (int i = 0; i < prefs; i++) {
> +            SearchEnginePreference thisEngine = (SearchEnginePreference) getPreference(i);

thisEngine is confusing - either the "engine" argument should be "thisEngine," or "thisEngine" should be "aEngine".

@@ +104,5 @@
> +    }
> +
> +    private void setFallbackDefaultEngine() {
> +        // Set the default engine to an arbitrary available engine. Useful if the default is removed or disabled.
> +        int prefs = getPreferenceCount();

Nit: numPrefs.

@@ +106,5 @@
> +    private void setFallbackDefaultEngine() {
> +        // Set the default engine to an arbitrary available engine. Useful if the default is removed or disabled.
> +        int prefs = getPreferenceCount();
> +        for (int i = 0; i < prefs; i++) {
> +            SearchEnginePreference thisEngine = (SearchEnginePreference) getPreference(i);

same: use aEngine.

@@ +116,5 @@
> +    }
> +
> +    // Methods called by tapping items on the submenus for each search engine.
> +    public void toggleEnabledness(SearchEnginePreference engine) {
> +        boolean isHidden = !engine.getIsHiddenEngine();

I understand what you're doing here, but as it's written, this is a confusing line because it's taking "isHidden" and setting it to NOT "getIsHiddenEngine()". You should rename isHidden to something like "newHiddenState" so that line doesn't look like a blatant bug.

@@ +128,5 @@
> +
> +    public void uninstall(SearchEnginePreference engine) {
> +        removePreference(engine);
> +        if (engine == defaultEngineReference) {
> +            // If they're deleting their default engine, get them a new default engine..

Nit: remove double ..

::: mobile/android/chrome/content/browser.js
@@ +6373,5 @@
>        suggestEngine = engine.name;
>        suggestTemplate = engine.getSubmission("__searchTerms__", "application/x-suggestions+json").uri.spec;
>      }
>  
> +    // By convention, the currently configured default engine is at position zero in searchEngines

Nit: period.

Comment 35

[Chris Kitching [:ckitching]]

Attachment: Part 1/3: New UI. V12 - Nits on toast.

> ::: mobile/android/base/preferences/SearchEnginePreference.java
> @@ +21,5 @@
> > +import org.mozilla.gecko.R;
> > +import org.mozilla.gecko.gfx.BitmapUtils;
> > +import org.mozilla.gecko.util.ThreadUtils;
> > +
> > +// Represents an element in the list of search engines on the preferences menu.
> 
> Use /** **/ style javadocs for comments about classes. The rest of the code
> is pretty lax, but let's start making new comments better!

Ah! I had no idea Javadoc comments were desired. Javadoc comments will be provided.

> @@ +158,5 @@
> > +        // Due to Andrid's lazy loading of buttons, we can't actually configure the enabledness of the buttons until the
> > +        // prompt is shown, so we do this inside the onShowListener. But first - build it!
> > +
> > +        // If we are the only engine left, then we are the default engine, and none of the options on this menu can do anything.
> > +        if (parentCategory.isLastEngineStanding(this)) {
> 
> I'd almost rather this be made not clickable, and maybe pop up a toast
> explaining why, but we should see what Ian thinks about it. (I know, that
> makes it more complicated...)

Toasts added - Good idea.

> @@ +49,3 @@
> >              try {
> >                  engines = data.getJSONArray("searchEngines");
> > +                if (engines.length() == 0) {
> 
> Should there ever be no engines? Perhaps Log.w here, "No search engines."

You can't ever have no engines - Gecko won't allow you to delete the bundled engines.

Comment 36

[Chris Kitching [:ckitching]]

Attachment: Part 2/3: Communication with Gecko to save the changes made via the UI. V4 - Further nit removal

Comment 37

[Chenxia Liu [:liuche]]

Comment on attachment 778770 [details] [diff] [review]
Part 1/3: New UI. V12 - Nits on toast.

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

Sorry this took so long! This is looking great. Some nits, and if you could roll a build and link it so Ian can see the strings (needinfo him), we can get land this.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +158,5 @@
> +<!ENTITY pref_search_shortcuts_enable "Show in search shortcuts">
> +<!ENTITY pref_search_shortcuts_disable "Hide from search shortcuts">
> +<!ENTITY pref_search_disabled "Disabled">
> +<!ENTITY pref_search_remove "Remove">
> +<!ENTITY pref_search_last_toast "You can\'t remove or disable your last search engine.">

Let's check with Ian about this string. Can you roll a build for him to test this out?

::: mobile/android/base/preferences/SearchEnginePreference.java
@@ +41,5 @@
> +
> +    // Cache labels to avoid repeated use of the resource system.
> +    public String LABEL_SHORTCUTS_HIDE;
> +    public String LABEL_SHORTCUTS_SHOW;
> +    public String LABEL_IS_DEFAULT;

nit: newline

@@ +50,5 @@
> +    private boolean mIsDisabledEngine;
> +    private boolean mIsImmutableEngine;
> +
> +    // Dialog element labels.
> +    private String[] mDialogItems;

nit: remove newline

@@ +140,5 @@
> +        }
> +    }
> +
> +    /**
> +     * Set if this object's UI should show that this engine is disabled (Disabled)

Nit: "Disabled" in quotes

@@ +159,5 @@
> +        } else {
> +            mDialogItems[INDEX_ENABLEDNESS_BUTTON] = LABEL_SHORTCUTS_HIDE;
> +            // Reset colour of the title of this search engine.
> +            titleSpannable.removeSpan(disabledSpan);
> +            // In the almost-always true event that we're not a disabled default engine...

This does not apply anymore, right? Default engines can no longer be disabled.

@@ +207,5 @@
> +                    case INDEX_UNINSTALL_BUTTON:
> +                        mParentCategory.uninstall(SearchEnginePreference.this);
> +                        break;
> +                    default:
> +                        Log.wtf(LOGTAG, "Selected index out of range.");

Heh, as tempting as it is to use Log.wtf is, just use Log.warn please.

@@ +224,5 @@
> +            @Override
> +            public void run() {
> +                mDialog = builder.create();
> +                mDialog.setOnShowListener(new DialogInterface.OnShowListener() {
> +                    // Called when the dialog is shown (And we're finally able to manipulate button enabledness).

Nit: lowercase the "And"

@@ +263,5 @@
> +        if (mIsDefaultEngine) {
> +            defaultButton.setEnabled(false);
> +            // Failure to unregister this listener leads to tapping the button dismissing the dialog without doing anything.
> +            defaultButton.setOnClickListener(null);
> +        }

Nit: newline

@@ +265,5 @@
> +            // Failure to unregister this listener leads to tapping the button dismissing the dialog without doing anything.
> +            defaultButton.setOnClickListener(null);
> +        }
> +        if (mIsImmutableEngine) {
> +            // Prevent the uninstallation of bundled engines (Gecko forbids this).

Nit: move this comment one line up.

::: mobile/android/base/preferences/SearchPreferenceCategory.java
@@ +143,5 @@
> +     * Called from {@link org.mozilla.gecko.preferences.SearchEnginePreference}.
> +     * @param engine The engine to have enabledness toggled.
> +     */
> +    public void toggleEnabledness(SearchEnginePreference engine) {
> +        // Our new disabled state...

Nit: comment is unnecessary, as you've renamed the variable better.

::: mobile/android/chrome/content/browser.js
@@ +6364,3 @@
>      let searchEngines = engineData.map(function (engine) {
> +      let isImmutable = false;
> +      for (let i = 0; i < immutableEngines.length; i++) {

Can you use indexOf instead?

@@ +6410,2 @@
>    observe: function observe(aSubject, aTopic, aData) {
> +    if (aTopic == "SearchEngines:GetVisible") {

My one concern is the tradeoff between more message observers vs passing/parsing JSON. These patches definitely go in the "more messages" direction - I guess for now at least, since I don't know the relatibe cost of either, that should be fine.

Comment 38

[Chenxia Liu [:liuche]]

Comment on attachment 778771 [details] [diff] [review]
Part 2/3: Communication with Gecko to save the changes made via the UI. V4 - Further nit removal

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

Heh, much shorter patch. Two nits, then r+.

::: mobile/android/base/preferences/SearchPreferenceCategory.java
@@ +140,5 @@
> +     * Helper method to send a particular event string to Gecko with an associated engine name.
> +     * @param engine The engine to which the event relates.
> +     * @param event The type of event to send.
> +     */
> +    private void sendGeckoEngineEvent(SearchEnginePreference engine, String event) {

Nit: event as arg 1, and engine as arg 2. Stylistically, this seems more consistent with other event calls, and it makes more sense to send it as method: args form.

::: mobile/android/chrome/content/browser.js
@@ +6429,5 @@
> +      break;
> +      case "SearchEngines:Get":
> +        // Return a list of all engines, including "Hidden" ones.
> +        Services.search.init(this._handleSearchEnginesGetAll.bind(this));
> +      break;

Nit: align break with content lines, here and below.

Comment 39

[Chenxia Liu [:liuche]]

Comment on attachment 776228 [details] [diff] [review]
Part 3/3: Eliminate the original method of changing search engine settings (Via the addons menu)

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

Remove the unused strings - I think these are in locale/en-US/chrome/, and make sure there aren't others.

Comment 40

[Chenxia Liu [:liuche]]

Finally, push these patches to try and make sure there aren't any broken tests - or fix them if there are.

Comment 41

[Chris Kitching [:ckitching]]

Attachment: Part 1/3: New UI. V13 - Additional further nit removal, plus some polishing.

Comment 42

[Chris Kitching [:ckitching]]

Attachment: Part 2/3: Communication with Gecko to save the changes made via the UI. V5 - Additional further nit removal

Comment 43

[Chris Kitching [:ckitching]]

Attachment: Part 3/3: Eliminate the original method of changing search engine settings (Via the addons menu) V2 - Removing redundant strings.

Comment 44

[Chris Kitching [:ckitching]]

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=327ab519bf32

Comment 45

[Chenxia Liu [:liuche]]

Wonderful, the try push is green, so once you get some final patches up, we can land this and let it sit on nightly for a few days before merge.

Comment 46

[Chenxia Liu [:liuche]]

Oh, just noticed that you already have the cleaned up patches up.

Can you make a build for Ian to run, and needinfo him for that? We just need to run these changes (string, etc) by him.

Also include in the needinfo comment a list of the UI changes that need some looking over (strings, removing all the engines + toast, etc).

Comment 47

[Chris Kitching [:ckitching]]

Ian, here's a build with the current patches applied for your perusal:
https://www.dropbox.com/s/q5e9s0ajpd6ir94/fennec-25.0a1.en-US.android-arm.apk

What's your opinion on the changes we made to the menu labels? (The "Show/hide from search shortcuts" thing).
Do you like what currently happens when you go to remove or disable your last remaining search engine? (It shows a toast and tells you this isn't allowed)
Any other thoughts?

Comment 48

[Ian Barlow (:ibarlow)]

Overall, the new strings look good to me.

However, I don't get why "Remove" is greyed out in all of these options... I see that if I add another engine like Duckduckgo I can remove that one though. Makes for pretty confusing experience.

Are we trying to solve for the case of what happens when you remove all your search engines? Maybe we can be smarter about it than not allowing people to change things in an inconsistent way.

Perhaps if I remove all my pre-installed search engines a row appears in settings that reads "Reinstall default search engines". Tapping it opens a context menu with checkboxes:


        +--------------------------------------+
        |                                      |
        | Reinstall default search engines     |
        |                                      |
        +--------------------------------------+
        |+-+                                   |
        ||X|  Google                           |
        |+-+                                   |
        +--------------------------------------+
        |+-+                                   |
        ||X|  Amazon                           |
        |+-+                                   |
        +--------------------------------------+
        |+-+                                   |
        ||X|  Twitter                          |
        |+-+                                   |
        +--------------------------------------+
        |+-+                                   |
        ||X|  Wikipedia                        |
        |+-+                                   |
        +------------------+-------------------+
        |                  |                   |
        |     Cancel       |        OK         |
        |                  |                   |
        +------------------v-------------------+

Comment 49

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

I'm pretty sure "Remove" is disabled because it is not possible to remove a built-in search provider. We should probably not even show the "Remove" menu for those.

IMO, "Show/Hide from search shortcuts" is confusing and will translate (l10n) poorly. I'd be happier with the old "Enable/Disable" strings.

Comment 50

[Chenxia Liu [:liuche]]

Comment on attachment 782347 [details] [diff] [review]
Part 3/3: Eliminate the original method of changing search engine settings (Via the addons menu) V2 - Removing redundant strings.

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

Looks good, r+ - sorry for taking so long!

Comment 51

[Chris Kitching [:ckitching]]

(In reply to Mark Finkle (:mfinkle) from comment #49)
> I'm pretty sure "Remove" is disabled because it is not possible to remove a
> built-in search provider. We should probably not even show the "Remove" menu
> for those.
> 
> IMO, "Show/Hide from search shortcuts" is confusing and will translate
> (l10n) poorly. I'd be happier with the old "Enable/Disable" strings.

I also think "Enable/Disable" are simpler, but they don't make it entirely clear what this actually DOES. What does it mean to "Disable" Google? Disabling Google sounds like a particularly odd thing to do. Does this mean I can't use Google any more? This creates its own flavour of confusion, just perhaps one that we aren't susceptible to.

You are also correct about why "Remove" is disabled - Gecko does not allow removal of bundled search engines, period. I guess there's some licensing rule behind this or something, but that's the way it is. You can of course remove engines you add yourself. It would indeed probably be less confusing to not show the "Remove" option here.

On the topic of the "Reinstall default search engines" thing - if that were implemented, it would once again be possible to disable/remove all search engines. In this situation, when you type something that isn't a URL that can be navigated to into the AwesomeBar you get an error page, instead of it doing anything more useful.
Isn't it the case that the sort of person who would do this is probably exactly the sort of person we want to keep from doing this, because they're doing it by mistake because they're confused. (I doubt the existence of people who genuinely want to be able to do this).


So, given the shortness of time, we need to come to a decision about:

Strings: Keep them as they are, or revert to "Enable/Disable"?
Removal: Delete the "Remove" option from the menu for all bundled engines?
Removal: Do we really want the ability to pseudo-uninstall all bundled engines as Ian suggests, enabling you to run Fennec with no search providers at all?

Sort of nowish.

Comment 52

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

We started talking about this on IRC. I notice that desktop does not have "enable/disable" actions for Search Engines. Only "Remove" and reordering is supported.

I like removing the "Enable/Disable" search engine actions from Fennec. That leaves "Remove". Removing engines that I installed seems safe. To add the engine back I need to re-install it. Removing built-in engines means "hiding" them since we can't really remove them. No big deal, but how to we get them back? Desktop uses a "Restore defaults" button. We could add that too, but I think we start to push beyond what we can get done in 3 days.

Chrome on Android does not allow the user to remove built-in search engines. I could live with that compromise for Fx25, maybe longer.

I propose:
* Remove the "Enable/Disable" actions from all engines
* Only show a "Remove" action if it's a search engine the user installed.

Thoughts?

Comment 53

[Chris Kitching [:ckitching]]

(In reply to Mark Finkle (:mfinkle) from comment #52)
> We started talking about this on IRC. I notice that desktop does not have
> "enable/disable" actions for Search Engines. Only "Remove" and reordering is
> supported.
> 
> I like removing the "Enable/Disable" search engine actions from Fennec. That
> leaves "Remove". Removing engines that I installed seems safe. To add the
> engine back I need to re-install it. Removing built-in engines means
> "hiding" them since we can't really remove them. No big deal, but how to we
> get them back? Desktop uses a "Restore defaults" button. We could add that
> too, but I think we start to push beyond what we can get done in 3 days.
> 
> Chrome on Android does not allow the user to remove built-in search engines.
> I could live with that compromise for Fx25, maybe longer.
> 
> I propose:
> * Remove the "Enable/Disable" actions from all engines
> * Only show a "Remove" action if it's a search engine the user installed.
> 
> Thoughts?

This sounds distinctly doable - the only objection I can think of is that we might want to have the "hide" feature, since re-adding engines on mobile is more fiddly than on the desktop. (A sort of plausible use-case is I might want to try out a new search engine before ultimately returning to my earlier one - I'd rather not have to uninstall and reinstall the original engine when doing this.)

This suggestion now makes it once again possible to remove every search engine. In this state, Fennec gives you an error page if you input a non-URL into the address bar.
Are you sure you want that to be possible?
Or did you want to keep the toast that prevents you from deleting the last one left?

Comment 54

[Chris Kitching [:ckitching]]

I'm sorry, I wasn't clear - my above comment refers to your upper, more ideal solution. I was not commenting on your compromise solution (Which also sounds entirely straightforward)

Comment 55

[Chris Kitching [:ckitching]]

Attachment: Foldable: Remove "Remove" button for bundled engines.

So since there seems to be uncertainty about where we're going with this, I've taken the liberty of implementing some patches-for-the-patches on this topic. They fit between parts 2 and 3 already presented.
When a final decision is made I shall fold the appropriate patches. I suspect this approach makes the lives of the reviewers better in the meantime (And makes it easier for people to muck about with patches and try out different approaches).

This post: Patch removing the "Remove" button from bundled engines.

Comment 56

[Chris Kitching [:ckitching]]

Attachment: Foldable : Remove enable/disable buttons.

Patch removing the "Enable/Disable" buttons entirely.

Comment 57

[Chris Kitching [:ckitching]]

Attachment: Foldable: Can delete bundled engies. Can restore such engines when all erased.

Patch, stacked on the prior two foldables, which replaces the ability to delete bundled search engines. When all bundled engines are deleted, a special element appears which, when tapped, restores the bundled engines.

So, for Finkle's proposed compromise we fold the first two patches. For the extended can-also-delete-defaults solution we fold all three.

Builds of these to follow for possible perusal by Ian.

Comment 58

[Chris Kitching [:ckitching]]

Build with all foldables:
https://www.dropbox.com/s/9q2rjm2dtu0ztc8/allFolded.apk

Build with the first two foldables:
https://www.dropbox.com/s/gdr9itam04mfsp3/twoFolded.apk

Build with just the first foldable:
https://www.dropbox.com/s/yiaxwnqoil049sd/oneFolded.apk

If we go for the approach of being-allowed-to-delete-bundled-engines-and-then-restore-them-with-a-button, is the implementation provided sufficient, or shall I enhance it with the checkbox as Ian suggested above?
What if I delete some, but not all of the default providers? Should there be a way of getting them back without having to counter-intuitively delete the remainder to enable the special entry?
How do we deal with the no-search-engines-exist state which would then become possible?


Personally, I preferred the implementation as it was before this latest round of changes. I liked the ability to toggle engines on/off without removing them from the menu. Not being able to remove the bundled ones was not a bother to me - the ideal combination for me would probably be just to hide the "Remove" button from the bundled  ones and be done with it. But then, I'm strange. :P

Comment 59

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

Ian and I chatted on IRC. For now, we want to go with the minimum proposal in comment 52:
* Remove the "Enable/Disable" actions from all engines
* Only show a "Remove" action if it's a search engine the user installed.

I think that means we want to review attachment 783628 [details] [diff] [review] and attachment 783629 [details] [diff] [review]. We can evaluate adding the additional functionality later, but these two attachments should be enough to safely ship this feature in Fx25.

Comment 60

[:Margaret Leibovic]

(In reply to Chris Kitching [:ckitching] from comment #53)
> (In reply to Mark Finkle (:mfinkle) from comment #52)
> > We started talking about this on IRC. I notice that desktop does not have
> > "enable/disable" actions for Search Engines. Only "Remove" and reordering is
> > supported.
> > 
> > I like removing the "Enable/Disable" search engine actions from Fennec. That
> > leaves "Remove". Removing engines that I installed seems safe. To add the
> > engine back I need to re-install it. Removing built-in engines means
> > "hiding" them since we can't really remove them. No big deal, but how to we
> > get them back? Desktop uses a "Restore defaults" button. We could add that
> > too, but I think we start to push beyond what we can get done in 3 days.
> > 
> > Chrome on Android does not allow the user to remove built-in search engines.
> > I could live with that compromise for Fx25, maybe longer.
> > 
> > I propose:
> > * Remove the "Enable/Disable" actions from all engines
> > * Only show a "Remove" action if it's a search engine the user installed.
> > 
> > Thoughts?
> 
> This sounds distinctly doable - the only objection I can think of is that we
> might want to have the "hide" feature, since re-adding engines on mobile is
> more fiddly than on the desktop. (A sort of plausible use-case is I might
> want to try out a new search engine before ultimately returning to my
> earlier one - I'd rather not have to uninstall and reinstall the original
> engine when doing this.)

You can do this by just choosing to do the search with that engine sometimes. Having an extra engine installed is pretty unobtrusive. I feel like we just have those enable/disable options in the current implementation because that's what we have for add-ons (and search engines are in the add-ons manager), so we shouldn't feel tied to that.

Comment 61

[Chris Kitching [:ckitching]]

(In reply to Mark Finkle (:mfinkle) from comment #59)
> Ian and I chatted on IRC. For now, we want to go with the minimum proposal
> in comment 52:
> * Remove the "Enable/Disable" actions from all engines
> * Only show a "Remove" action if it's a search engine the user installed.
> 
> I think that means we want to review attachment 783628 [details] [diff] [review]
> [review] and attachment 783629 [details] [diff] [review]. We can evaluate
> adding the additional functionality later, but these two attachments should
> be enough to safely ship this feature in Fx25.

Okay. This means the only item on the menu for bundled search engines will be "Set as default". Do we still want to prompt the user with a one-item popup menu in this case, or should it set as default as soon as you tap? (Creating an inconsistency with engines you've added yourself)

Comment 62

[:Margaret Leibovic]

(In reply to Chris Kitching [:ckitching] from comment #61)
> (In reply to Mark Finkle (:mfinkle) from comment #59)
> > Ian and I chatted on IRC. For now, we want to go with the minimum proposal
> > in comment 52:
> > * Remove the "Enable/Disable" actions from all engines
> > * Only show a "Remove" action if it's a search engine the user installed.
> > 
> > I think that means we want to review attachment 783628 [details] [diff] [review]
> > [review] and attachment 783629 [details] [diff] [review]. We can evaluate
> > adding the additional functionality later, but these two attachments should
> > be enough to safely ship this feature in Fx25.
> 
> Okay. This means the only item on the menu for bundled search engines will
> be "Set as default". Do we still want to prompt the user with a one-item
> popup menu in this case, or should it set as default as soon as you tap?
> (Creating an inconsistency with engines you've added yourself)

I think this sounds fine, especially for a first version. At this point, I don't think we have the time to think about a different interaction pattern for this case.

Comment 63

[Chris Kitching [:ckitching]]

Attachment: Part 1/3: New UI. V14 - Remove enable/disable functionality, remove disable button from bundled engines.

Okay then, generated new versions of the two affected patches here...

Comment 64

[Chris Kitching [:ckitching]]

Attachment: Part 2/3: Communication with Gecko to save the changes made via the UI. V6 - Removing enable/disable support.

Just pruning the enable/disable buttons..

Comment 65

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

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

> > Okay. This means the only item on the menu for bundled search engines will
> > be "Set as default". Do we still want to prompt the user with a one-item
> > popup menu in this case, or should it set as default as soon as you tap?
> > (Creating an inconsistency with engines you've added yourself)
> 
> I think this sounds fine, especially for a first version. At this point, I
> don't think we have the time to think about a different interaction pattern
> for this case.

Right. It's fine if there is only "Set as default" on the prompt for built-in engines.

Comment 66

[Chenxia Liu [:liuche]]

Comment on attachment 784730 [details] [diff] [review]
Part 1/3: New UI. V14 - Remove enable/disable functionality, remove disable button from bundled engines.

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

A few more nits, and then should be good.

Also of note, versioning of the patches was very helpful! I learned about the nifty interdiff feature, which lets me diff between two versions of patches.

::: mobile/android/base/preferences/SearchEnginePreference.java
@@ +33,5 @@
> +    public static int sIconSize;
> +
> +    // Indices in button array of the AlertDialog of the three buttons.
> +    public static final int INDEX_SET_DEFAULT_BUTTON = 0;
> +    public static final int INDEX_UNINSTALL_BUTTON = 1;

Possibly update this to REMOVE.

@@ +40,5 @@
> +    public String LABEL_IS_DEFAULT;
> +
> +    // Specifies if this engine is configured as the default search engine.
> +    private boolean mIsDefaultEngine;
> +    // Specifies if this engine is one of the ones bundled with the app, which cannot delete.

"which cannot be deleted"

@@ +95,5 @@
> +        SpannableString titleSpannable = new SpannableString(engineName);
> +        mIsImmutableEngine = geckoEngineJSON.getBoolean("immutable");
> +
> +        if (mIsImmutableEngine) {
> +            // Delete the "Remove" option from the menu...

Nit: remove ellipsis.

@@ +146,5 @@
> +            });
> +            return;
> +        }
> +
> +        // If there are no enabled items on the menu, abort.

I don't think this comment reflects what you are doing. Rather, something about default-immutable engines having no actions.

Is there a way to avoid displaying the click highlight action? We could also file this as a follow-up, or handle it in the next iteration.

::: mobile/android/base/preferences/SearchPreferenceCategory.java
@@ +115,5 @@
> +            setDefault(aEngine);
> +        }
> +    }
> +
> +    // Methods called by tapping items on the submenus for each search engine...

Nit: Remove ellipsis.

::: mobile/android/chrome/content/browser.js
@@ +6457,5 @@
>        return {
>          name: engine.name,
>          identifier: engine.identifier,
> +        iconURI: (engine.iconURI ? engine.iconURI.spec : null),
> +        hidden: engine.hidden,

Is it possible to hide engines anymore? Are they not just removed?

Comment 67

[Chenxia Liu [:liuche]]

Comment on attachment 784732 [details] [diff] [review]
Part 2/3: Communication with Gecko to save the changes made via the UI. V6 - Removing enable/disable support.

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

Almost there. Also, make sure that all the patches apply on top of each other. I think Part 3 needs a trivial unbitrot to apply.

::: mobile/android/base/preferences/SearchPreferenceCategory.java
@@ +125,5 @@
> +        JSONObject json = new JSONObject();
> +        try {
> +            json.put("engine", engine.getTitle());
> +        } catch (JSONException e) {
> +            Log.e(LOGTAG, "JSONException creating enabledness toggle message for Gecko.", e);

Is enabledness still relevant? Or maybe you mean a different word?

::: mobile/android/chrome/content/browser.js
@@ +6420,5 @@
>  
>    init: function init() {
>      Services.obs.addObserver(this, "SearchEngines:Get", false);
>      Services.obs.addObserver(this, "SearchEngines:GetVisible", false);
> +    Services.obs.addObserver(this, "SearchEngines:Enable", false);

Doesn't this series of patches get rid of Enable/Disable?

@@ +6423,5 @@
>      Services.obs.addObserver(this, "SearchEngines:GetVisible", false);
> +    Services.obs.addObserver(this, "SearchEngines:Enable", false);
> +    Services.obs.addObserver(this, "SearchEngines:Disable", false);
> +    Services.obs.addObserver(this, "SearchEngines:SetDefault", false);
> +    Services.obs.addObserver(this, "SearchEngines:Uninstall", false);

Whichever you decide (remove, uninstall) for Part 1, make sure this is consistent.

@@ +6510,5 @@
> +    let engine;
> +    switch(aTopic) {
> +      case "SearchEngines:GetVisible":
> +        Services.search.init(this._handleSearchEnginesGetVisible.bind(this));
> +      break;

Style: break should be aligned with case content. Perhaps update your IDE style rules?

@@ +6515,5 @@
> +      case "SearchEngines:Get":
> +        // Return a list of all engines, including "Hidden" ones.
> +        Services.search.init(this._handleSearchEnginesGetAll.bind(this));
> +        break;
> +      case "SearchEngines:Enable":

Same as above: do we still need enable/disable?

@@ +6537,5 @@
> +        engine.hidden = false;
> +        Services.search.removeEngine(engine);
> +        break;
> +      default:
> +        dump("Unexpected message type observed: "+aTopic);

Nit: spaces around + operator.

Comment 68

[Chris Kitching [:ckitching]]

Attachment: Part 1/3: New UI. V15 - Nits and nomenclature.

(In reply to Chenxia Liu [:liuche] from comment #66)
> Is there a way to avoid displaying the click highlight action? We could also
> file this as a follow-up, or handle it in the next iteration.

Yes, but this would require reordering this collection of related bugs - a followup after landing this chunk of stuff would be considerably easier (And a miniscule patch to review etc.)

> ::: mobile/android/chrome/content/browser.js
> @@ +6457,5 @@
> >        return {
> >          name: engine.name,
> >          identifier: engine.identifier,
> > +        iconURI: (engine.iconURI ? engine.iconURI.spec : null),
> > +        hidden: engine.hidden,
> 
> Is it possible to hide engines anymore? Are they not just removed?

This one is sort of annoying. When we tell Gecko to remove a bundled (immutable) engine it responds by hiding it for us. I chose to make the real picture of what's in Gecko's data structures available to Java, should we ever want to rejig the frontend.
(Notably, if we ever want to be able to remove the default engines and then restore them later then sending all the engines is necessary for the restoration button to work. I think.)


Fifteenth time lucky, perhaps?

Comment 69

[Chenxia Liu [:liuche]]

Comment on attachment 785183 [details] [diff] [review]
Part 1/3: New UI. V15 - Nits and nomenclature.

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

Great! Done with this patch! (Be sure to file the follow-ups, please.)

Comment 70

[Chris Kitching [:ckitching]]

Attachment: Part 2/3: Communication with Gecko to save the changes made via the UI. V7 - Removing dead code. Nits.

Well spotted! I left all the dead JS code behind.

Comment 71

[Chenxia Liu [:liuche]]

Comment on attachment 785197 [details] [diff] [review]
Part 2/3: Communication with Gecko to save the changes made via the UI. V7 - Removing dead code. Nits.

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

Looks good!

Comment 72

[Chris Kitching [:ckitching]]

Attachment: Part 3/3: Eliminate the original method of changing search engine settings (Via the addons menu) V3 - Fighting bitrot.

Comment 73

[Chris Kitching [:ckitching]]

Attachment: Part 1/3: New UI. V16 - fx-team-ifying.

Comment 74

[Chris Kitching [:ckitching]]

Attachment: Part 2/3: Communication with Gecko to save the changes made via the UI. V8 fx-team-ification

Comment 75

[Chris Kitching [:ckitching]]

Attachment: Part 3/3: Eliminate the original method of changing search engine settings (Via the addons menu) V4 - fx-team-ification

Comment 76

[Chenxia Liu [:liuche]]

Landed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/aa054b0f118b

Comment 77

[Tim Taubert [:ttaubert] (inactive)]

Backed out for turning rc2 tests permanently orange:

https://hg.mozilla.org/integration/fx-team/rev/50247b78a831

Comment 78

[Chris Kitching [:ckitching]]

This is, in retrospect, not surprising - the testAddSearchEngine test is completely wrong for this new search engine management system, so clearly it would fail.

It is, however, very surprising that this:
https://tbpl.mozilla.org/?tree=Try&rev=327ab519bf32
was all green. It was not the newest version of the patch, but it included the critical changes that I would have expected would cause this test to fail.. (Namely, deleting all the old search engine management code)

Is this an "Intermittent green"?

It would be nice if I could manage to actually land something without it exploding shortly thereafter at least once during my internship.

Time for a robocop test patch, then. Onward!

Comment 79

[Chris Kitching [:ckitching]]

Attachment: Part 0/3: String changes only.

Since the problem here seems to be the test's fault, I'm seperating the string changes from this patch set into its own patch which we can hopefully land in time to not miss the boat on internationalisation. (I guess this is the right procedure for this scenario?)
It is my hope that the remainder can be uplifted in due course. It would be irritating to have to postpone this until FF 26.

Comment 80

[Chris Kitching [:ckitching]]

Attachment: Part 1/3: New UI. V17 - Eliminate string changes.

Comment 81

[Chenxia Liu [:liuche]]

Unfortunately, it doesn't quite work this way - in order to localize a string, localizers must be able to see the string it UI to understand its context.

Chris, there seem to be a couple of outcomes:
1) If you can fix the test to handle the search settings change correctly, please push to try as soon as you get a fix and I can review and land the patches tomorrow.

2) If for some reason, you can't fix the test, you should at least update the patches for the other two bugs so that those can land - the "search engine tip" bug might be slightly dependent on this one (you probably should be able to delete a search engine you added), but the icon size one is definitely independent. Perhaps also add a patch to disable clicks on the existing search engines list, so there isn't confusing UI (e.g., clicking registers but does nothing).

3) If all else fails, no worries - we'll definitely get this into 26!

Comment 82

[Chris Kitching [:ckitching]]

I see. Thanks for the info about the localisation procedures.

I believe I have now found the problem - this test uses a slightly odd way of parsing JSON. This caused my addition of an extra field to the JSON objects sent to Java from Gecko listing the search engines to lead to the test ending up with a nonexistent field called "true,name" (It would combine the value of the previous field with the name of this field)
Subsequently, it would count how many "name" fields it finds, and use this to tell how many search engines there were. It got upset when the values before and after adding an engine were the same (Zero, that is).

I'll have a patch through try by the time sane people are awake. I appreciate your help in landing this.

Comment 83

[Chris Kitching [:ckitching]]

Attachment: Part 4/3: Correct the way in which testAddSearchEngine parses JSON to prevent test failures.

This appears to have corrected the problem - it is no longer reproducible locally, and a try push is underway at:
https://tbpl.mozilla.org/?tree=Try&rev=a558b4fbb728
Provided that all goes green we're in business.

The problem was caused by BaseTest.parseEventData - a limited JSON parser. This method is only used by the search engine test, is unnecessary, and is fundamentally flawed in implementation - so I'm removing it with this patch (As far as I can tell there's nothing this function can do that org.json.* couldn't do better, so to prevent some future test-writer from using this dodgy one it seems wise to get rid of it)
The fix is just to replace the use of this method with the standard JSON parsing libraries.

You may notice that the getSearchEngineNames method is used exclusively to count the engines at this time - this was the structure of the original implementation, and I think leaving it this way makes it much more obvious how to extend to test to cover more of the test system. (It'd be a pretty small job to expand it to cover all I recently added - followup bug, perhaps?)

The code in this test was extremely messy. I'm also including a supplemental patch to make it less confusing - review and include it if you want - it's not necessary, but since I wrote extra comments to get my head round the code it seems possibly useful to submit a cleaned-up edition for future benefit.

Comment 84

[Chris Kitching [:ckitching]]

Attachment: Part 5/3: Cleanup code in testAddSearchEngine.

Part five of the increasingly inaccurately named search preferences trilogy.

Optional extra patch to make the code in this test less incomprehensible.

Comment 85

[Chris Kitching [:ckitching]]

Alas, the run went orange, but for a different reason. testAddonManager, this time. It is not clear why these failures were not observed in the earlier try run.
Time to patch testAddonManager, then...

Comment 86

[Chris Kitching [:ckitching]]

It appears this problem is a regression caused by botching the patch splitting. Submitted a try run with the complete patches:
https://tbpl.mozilla.org/?tree=Try&rev=2cf15eb5e16f

This ought to turn green. Let's see.

Comment 87

[Chenxia Liu [:liuche]]

Chris, can you update the patches to reflect the patches are currently reflected in this try build, so if it greens, I know what to review? And if the current patches also match the ones applied in the try push, just let me know that.

Comment 88

[Chenxia Liu [:liuche]]

Comment on attachment 785500 [details] [diff] [review]
Part 4/3: Correct the way in which testAddSearchEngine parses JSON to prevent test failures.

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

r+ with nits.

::: mobile/android/base/tests/testAddSearchEngine.java.in
@@ +5,5 @@
>  import android.view.View;
>  import android.widget.ListAdapter;
>  import android.widget.ListView;
>  import java.util.ArrayList;
> +import android.util.Log;

Move this up to the rest of the android.* imports.

@@ +44,5 @@
> +        try {
> +            // Parse the data to get the number of searchengines.
> +            searchEngines = getSearchEnginesNames(eventData);
> +        } catch (JSONException e) {
> +            mAsserter.ok(false, "Fatal exception in testAddSearchEngine while decoding JSON search engine string from Gecko prior to addition of new engine.", e.toString());

Can you use the Assert.fail method with mAsserter?

@@ +87,5 @@
> +        try {
> +            // Parse the data to get the number of searchengines
> +            searchEngines = getSearchEnginesNames(eventData);
> +        } catch (JSONException e) {
> +            mAsserter.ok(false, "Fatal exception in testAddSearchEngine while decoding JSON search engine string from Gecko after adding of new engine.", e.toString());

Same, use mAsserter.fail. (See javadocs for Assert.)

Comment 89

[Chenxia Liu [:liuche]]

The try build is failing on a missing resource.

Comment 90

[Chris Kitching [:ckitching]]

(In reply to Chenxia Liu [:liuche] from comment #89)
> The try build is failing on a missing resource.

So I see. This one is proving less simple than expected. The resource it complains about is never referenced from anywhere in our codebase. I suspect this problem is created by the favicon size increasing patch - perhaps android is doing something deranged in secret that has it request a nonexistent resource.

But.. The resource in question is builtin and has existed since Android 1.0. Googling around suggests this error is a madness Android sometimes spits at you when you do certain flavours of slightly stupid thing. Investigation continues.(In reply to Chenxia Liu [:liuche] from comment #87)


> Chris, can you update the patches to reflect the patches are currently
> reflected in this try build, so if it greens, I know what to review? And if
> the current patches also match the ones applied in the try push, just let me
> know that.

Yes, the patches in the try push match the current ones here. Apologies for the renaming. Slightly beside the point now it went orange again.
At least it's differently broken now. That's usually progress.

Comment 91

[Chenxia Liu [:liuche]]

Comment on attachment 785501 [details] [diff] [review]
Part 5/3: Cleanup code in testAddSearchEngine.

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

r+, tiny nits. Fold this patch into the one before it, since this is only comments and very minor changes.

::: mobile/android/base/tests/testAddSearchEngine.java.in
@@ +33,3 @@
>          waitForText("Browser Blank Page 01");
>  
> +        // Get the searchengine data by clicking the awesomebar - this Gecko to send Java the list

Accidentally a word.

@@ +63,4 @@
>          mAsserter.dumpLog("Long Clicking at width = " + String.valueOf(width) + " and height = " + String.valueOf(height));
>          mSolo.clickLongOnScreen(width,height);
>          if (!waitForText("Add Search Engine")) {
> +            // TODO: clickLongOnScreen does not always work - known Robotium issue - . Clicking a second time seems to work.

Get rid of the extra period, if you're going to touch this line anyways.

@@ +147,5 @@
> +                            Class searchEngineRow = classLoader.loadClass("org.mozilla.gecko.SearchEngineRow");
> +                            for (int i = 0; i < adapter.getCount(); i++ ) {
> +                                View item = view.getChildAt(i);
> +                                if (searchEngineRow.isInstance(item)) {
> +                                    searchEngineCount++;

All these lines look identical on visual inspection. Has something changed...?

Comment 92

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/7bbaba1b712f
https://hg.mozilla.org/integration/fx-team/rev/b92db71dc525

Comment 93

[Chris Kitching [:ckitching]]

No problem - I'll take care of the nits in due course. Always good to get cleanup patches landed!

(In reply to Chenxia Liu [:liuche] from comment #91)
> 
> @@ +147,5 @@
> > +                            Class searchEngineRow = classLoader.loadClass("org.mozilla.gecko.SearchEngineRow");
> > +                            for (int i = 0; i < adapter.getCount(); i++ ) {
> > +                                View item = view.getChildAt(i);
> > +                                if (searchEngineRow.isInstance(item)) {
> > +                                    searchEngineCount++;
> 
> All these lines look identical on visual inspection. Has something
> changed...?

These lines were indented one level further than they should have been.

Comment 94

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7bbaba1b712f
https://hg.mozilla.org/mozilla-central/rev/b92db71dc525

Comment 95

[u421692]

Test case created in moztrap:
https://moztrap.mozilla.org/manage/cases/?filter-suite=407