1029566 - Don't allow multiple addition of the same search engine through long tap

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Ioana Chiorean]

OS: Android 4.4.1, 3.2.1
Device: Nexus 4, Acer Iconia

I think we should reconsider 'Bug 779739 - No limitation on additional entries created for manually added search engines ' as we do this already for the addition from Page Menu

Steps: 
1. Go to bugzilla.mozilla.org
2. Long-tap in the search field to bring up the Context Menu. Choose "Add
Search Engine".
3. Repeat steps 1 and 2.
4. In the page bugzilla.mozilla.org long press on the search field - add seach engine
5. Repeat step 4. 

Expected Results:
- you shouldn't be able to add the search engine several times

Actual Results:
- only for the addition through page menu you are unable to add it several times as the option grays out

Note:
- I agree with Bug 779739 comment #3 ' I may want to add multiple Bugzilla engines that each correspond to a different Bugzilla query' but in this case the url for the search engine should be different -> different search engines

Comment 1

[Aaron Train [:aaronmt]]

Perhaps this can be mentored?

Comment 2

[amoghbl1]

Ok, I'm ready to take up work on this, I've already isolated all the code and would be able to come up with a patch in a bit. The aim is to check if an engine with the same formURL exists, if it does, a toast is shown which states that the engine already exists.

Comment 3

[Richard Newman [:rnewman]]

I wouldn't mind adding a check in Services.search, or SearchEngines.addEngine, that prevents this call from succeeding:

  http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7114

but I think we should cut this off by greying out the action bar item, or removing it altogether.


I suspect the right place to do that is in the SelectionHandler's filter:

  http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6851

but we might need to extend SelectionHandler to support the 'enabled' field if we want greyed out versus present at all.


Note that we already seem to have some code that prevents duplicate OpenSearch engines from being added:

  addOpenSearchEngine: function addOpenSearchEngine(engine) {
    Services.search.addEngine(engine.url, Ci.nsISearchEngine.DATA_XML, engine.iconURL, false, {
      onSuccess: function() {
        // Display a toast confirming addition of new search engine.
        NativeWindow.toast.show(Strings.browser.formatStringFromName("alertSearchEngineAddedToast", [engine.title], 1), "long");
      },

      onError: function(aCode) {
        let errorMessage;
        if (aCode == 2) {
          // Engine is a duplicate.
          errorMessage = "alertSearchEngineDuplicateToast";

Comment 4

[amoghbl1]

So grey it out or remove it based on the selected url? Also assign the bug to me?

(In reply to Richard Newman [:rnewman] from comment #3)
> I wouldn't mind adding a check in Services.search, or
> SearchEngines.addEngine, that prevents this call from succeeding:
> 
>  
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#7114
> 
> but I think we should cut this off by greying out the action bar item, or
> removing it altogether.
> 
> 
> I suspect the right place to do that is in the SelectionHandler's filter:
> 
>  
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#6851
> 
> but we might need to extend SelectionHandler to support the 'enabled' field
> if we want greyed out versus present at all.
> 
> 
> Note that we already seem to have some code that prevents duplicate
> OpenSearch engines from being added:
> 
>   addOpenSearchEngine: function addOpenSearchEngine(engine) {
>     Services.search.addEngine(engine.url, Ci.nsISearchEngine.DATA_XML,
> engine.iconURL, false, {
>       onSuccess: function() {
>         // Display a toast confirming addition of new search engine.
>        
> NativeWindow.toast.show(Strings.browser.
> formatStringFromName("alertSearchEngineAddedToast", [engine.title], 1),
> "long");
>       },
> 
>       onError: function(aCode) {
>         let errorMessage;
>         if (aCode == 2) {
>           // Engine is a duplicate.
>           errorMessage = "alertSearchEngineDuplicateToast";

Comment 5

[Richard Newman [:rnewman]]

(In reply to amoghbl1 from comment #4)
> So grey it out or remove it based on the selected url?

Yes.

> Also assign the bug to me?

We typically do that when you have a patch ready.

Also please note that you don't need to quote my entire reply; that creates a lot of noise in the bug. Only use Bugzilla's "reply" feature if you need to reply to some individual lines.

Comment 6

[amoghbl1]

hey rnewman, wouldn't people wonder why the button disappeared if we just stop showing it? Wouldn't it be better if we showed the button and if someone clicks on it, we can tell them that the search engine already exists?

Comment 7

[Richard Newman [:rnewman]]

(In reply to amoghbl1 from comment #6)
> hey rnewman, wouldn't people wonder why the button disappeared if we just
> stop showing it? Wouldn't it be better if we showed the button and if
> someone clicks on it, we can tell them that the search engine already exists?

That's probably a cultural or individual question.

For some people, it's more frustrating to complete a task and be told "couldn't finish, sorry".

For others, it's more frustrating to have a closed door.

Certainly we shouldn't ask the user to pick a name for the engine. I'd probably take a middle ground -- visually disable the button, and if tapped say that the engine already exists. But we'll see what's achievable.

Comment 8

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

For consistency, the other menu items (look in Page > *) will go disabled (greying it out) when the action is not possible. We should probably work the same way.

Comment 9

[amoghbl1]

Ok mfinkle, will grey out first, let's think about the toast later?

Comment 10

[amoghbl1]

Attachment: fix_1029566.patch

I had a long discussion with :MattN about this bug and realized that at this moment, only an initial fix for this can be made, this patch will check for engines added by the user and alert if the same engine is trying to be added. :MattN told me that the default search engines are special and hence, checking them for duplicates may be a little harder.

Comment 11

[amoghbl1]

Attachment: fix_1029566_v2.patch

this is the second path, it does the check for duplicate before the user enters a name for the search engine thereby cutting down one more step.

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8460885 [details] [diff] [review]
fix_1029566_v2.patch

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

::: mobile/android/chrome/content/browser.js
@@ +7083,5 @@
>            }
>        }
>      }
>  
> +    //Checking if the search engine is already saved by looking at the search url.

Nit: spacing and grammar.

  // Check if the search engine has already been saved
  // by looking at the search URL.

@@ +7084,5 @@
>        }
>      }
>  
> +    //Checking if the search engine is already saved by looking at the search url.
> +    var allEngines = Services.search.getEngines();

Use `let`, not `var`.

@@ +7086,5 @@
>  
> +    //Checking if the search engine is already saved by looking at the search url.
> +    var allEngines = Services.search.getEngines();
> +    let title = { value: (aElement.ownerDocument.title || docURI.host) };
> +    let name = title.value;

Flip these around and fix the names:

  let name = aElement.ownerDocument.title || docURI.host;
  let promptData = {value: name};

@@ +7088,5 @@
> +    var allEngines = Services.search.getEngines();
> +    let title = { value: (aElement.ownerDocument.title || docURI.host) };
> +    let name = title.value;
> +    for (let i = 0; i < allEngines.length; i++)
> +      if (allEngines[i].getSubmission("").uri.spec === formURL) {

for (let engine of allEngines) {
  if (engine.getSubmission("").uri.spec === formURL) {
     ...
     return;
  }
}

Comment 13

[amoghbl1]

Attachment: fix_1029566_v3.patch

Made all the changes pointed out by rnewman and some related variable name changes.

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 8461021 [details] [diff] [review]
fix_1029566_v3.patch

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

r+ with trailing whitespace removed in all three spots.

::: mobile/android/chrome/content/browser.js
@@ +7086,5 @@
>  
> +    // Checking if the search engine is already saved
> +    // by looking at the search url.
> +    let allEngines = Services.search.getEngines();
> +    

Nit: you've added trailing whitespace.

Comment 15

[amoghbl1]

Attachment: fix_1029566_v4.patch

Updated the patch removing the whitespaces.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

If the patch is pending review, it's not ready for checkin yet.

Comment 17

[:Margaret Leibovic]

Comment on attachment 8477873 [details] [diff] [review]
fix_1029566_v4.patch

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

I know rnewman was ready to give this an r+ before, but this is the first time I've seen this patch, so I have some opinions of my own :) Mostly I think this patch could contain fewer changes, so I'd like to see if we can make it simpler (and address the braces issue).

::: mobile/android/chrome/content/browser.js
@@ +7160,5 @@
> +    for (let engine of allEngines)
> +      if (engine.getSubmission("").uri.spec === formURL) {
> +        NativeWindow.toast.show(Strings.browser.formatStringFromName("alertSearchEngineDuplicateToast", [promptData.value], 1), "long");
> +        return;
> +    }

Please brace both your for statement and your if statement. Currently this close brace is aligned with the for statement, but it actually closes the if statement, and that's confusing. In general, we like to brace everything to avoid any confusion :) 

(This code was written before we felt as strongly about this, so that's why it's missing braces in some places.)

@@ +7166,4 @@
>      // prompt user for name of search engine
>      let promptTitle = Strings.browser.GetStringFromName("contextmenu.addSearchEngine2");
>      let title = { value: (aElement.ownerDocument.title || docURI.host) };
>      if (!Services.prompt.prompt(null, promptTitle, null, title, null, {}))

In the previous version of the patch you also updated this prompt title to use promptData.

I actually think the best fix would be to move the title declaration up above the new toast logic you added, then just use title.value for the toast. If you do this, you don't need to change the name logic down below.

Comment 18

[amoghbl1]

Thanks for the input margaret, I'll fix them up and resubmit.

Comment 19

[Brian Nicholson (:bnicholson)]

Comment on attachment 8477873 [details] [diff] [review]
fix_1029566_v4.patch

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

::: mobile/android/chrome/content/browser.js
@@ +7157,5 @@
> +    let name = aElement.ownerDocument.title || docURI.host;
> +    let promptData = {value: name};
> +
> +    for (let engine of allEngines)
> +      if (engine.getSubmission("").uri.spec === formURL) {

Does this comparison actually work? Doing a quick test, an empty submission from the bugzilla.mozilla.org quick search returns "https://bugzilla.mozilla.org/". The formURL, which corresponds to the <form> action, returns "https://bugzilla.mozilla.org/buglist.cgi".

But even this worked as intended, we don't want to simply compare the form URLs. As I mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=779739#c3, it's reasonable for a user to construct multiple search engines from the same form, which they can do by changing multiple form fields.

Take, for instance, the Bugzilla advanced search page [1]. The <form> action is always the same (https://bugzilla.mozilla.org/buglist.cgi), but you can create wildly different queries by changing the actual elements in that form. These elements are attached as parameters at [2], and have nothing to do with the formURL.

If you want to prevent true duplicates, you'll have to compare the actual search parameters from each form. I maintain that this might not be worth the effort considering <hidden> elements are often generated cookie data that change, but I guess there could be exceptions (e.g., the quick search form on the bugzilla.mozilla.org landing page is pretty minimal).

[1] https://bugzilla.mozilla.org/query.cgi?query_format=advanced
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=71315eef503a#7202

Comment 22

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.