755228 - 'Add Search Engine' label on text input fields should be 'Add as Search Engine'

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Firefox mobile 14.0b1 build 3, Android 4.0.4 (stock), Google Nexus S

If you tap a text input field long, you get the context menu offering 'Add Search Engine'. Please change it to 'Add as Search Engine', indicating that it's not a native search engine, indicating it's more likely to break.

Steps to reproduce:
1. Go to http://www.mozilla.org/en-US/ and tap the mail sign up field at the bottom long.

Comment 1

[Ioana Chiorean]

This is still the same text. 
More for inconsistency the Page Menu has the 'Add a Search Engine'

Comment 2

[amoghbl1]

Well, 'Add a Search Engine' seems to be valid as there may be multiple search engines on one page, the 'Add Search Engine' being made 'Add as Search Engine' makes sense though, can I take up work on this?

Comment 3

[amoghbl1]

Attachment: fix_755228.patch

This patch creates a new string named addSearchEngine2, this has been done in order to alert the localization team. This can be changed to replace the old string if that is seen to be suitable.

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8456565 [details] [diff] [review]
fix_755228.patch

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

Technically, this is fine, modulo one nit.  I'm investigating whether the change is desired.

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +200,3 @@
>  contextmenu.saveImage=Save Image
>  contextmenu.setImageAs=Set Image As
>  contextmenu.addSearchEngine=Add Search Engine

Remove the old value, since according to mxr, you've replaced all uses.

Comment 5

[Nick Alexander :nalexander [he/him]]

OK, let's just go with this.  Post a new patch with the nit addressed, update the commit message with r=nalexander, and post a try build (if you have access to try).

Comment 6

[amoghbl1]

Attachment: new patch

Deleted the old string as suggested by nalexander.

Comment 7

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

Comment on attachment 8456565 [details] [diff] [review]
fix_755228.patch


>diff -r 7883d8e9f210 mobile/android/locales/en-US/chrome/browser.properties
> contextmenu.addSearchEngine=Add Search Engine
>+contextmenu.addSearchEngine2=Add as Search Engine

Why are we keeping the old string?

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8456570 [details] [diff] [review]
new patch

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

Looks good!

Comment 9

[Nick Alexander :nalexander [he/him]]

Sheriff: the commit message could use some light formatting (not required, but nice).

The username appears malformed; I would have landed with

hg qref --user "amoghbl1 <[e-mail]>"

but the tree is closed :(

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/2899ce243882

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2899ce243882

Comment 12

[Nick Alexander :nalexander [he/him]]

Thanks, tomcat!

amoghbl1: Bugzilla said this was your first patch; is that correct?  (I feel like I've been chatting with you on IRC for quite a while.)  If so, congratulations!

From here, you might be interested in Bug 1029566 and Bug 1029548.  They're both related; the former is really valuable.

Thanks for your contribution, amoghbl1!

Comment 13

[amoghbl1]

Hey nalexander , yes, this is my first patch! Will look into the links you've given me! Thanks :)