Closed Bug 755228 Opened 8 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: General, enhancement)

All
Android
enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 33
Tracking Status
firefox30 --- affected
firefox31 --- affected
firefox32 --- affected
firefox33 --- affected

People

(Reporter: aryx, Assigned: amoghbl1)

Details

Attachments

(1 file, 1 obsolete file)

2.25 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
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.
This is still the same text. 
More for inconsistency the Page Menu has the 'Add a Search Engine'
Version: Firefox 14 → Trunk
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?
Attached patch fix_755228.patch (obsolete) — Splinter Review
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.
Attachment #8456565 - Flags: review?(nalexander)
Attachment #8456565 - Flags: review?(michael.l.comella)
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.
Attachment #8456565 - Flags: review?(nalexander)
Attachment #8456565 - Flags: review?(michael.l.comella)
Attachment #8456565 - Flags: feedback+
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).
Assignee: nobody → amoghbl1
Status: NEW → ASSIGNED
Attached patch new patchSplinter Review
Deleted the old string as suggested by nalexander.
Attachment #8456565 - Attachment is obsolete: true
Attachment #8456570 - Flags: review?(nalexander)
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 on attachment 8456570 [details] [diff] [review]
new patch

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

Looks good!
Attachment #8456570 - Flags: review?(nalexander) → review+
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 <amoghbl1@gmail.com>"

but the tree is closed :(
Keywords: checkin-needed
Whiteboard: [sheriff instructions in comment 9]
https://hg.mozilla.org/integration/fx-team/rev/2899ce243882
Keywords: checkin-needed
Whiteboard: [sheriff instructions in comment 9] → [sheriff instructions in comment 9][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2899ce243882
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [sheriff instructions in comment 9][fixed-in-fx-team] → [sheriff instructions in comment 9]
Target Milestone: --- → Firefox 33
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!
Whiteboard: [sheriff instructions in comment 9]
Hey nalexander , yes, this is my first patch! Will look into the links you've given me! Thanks :)
You need to log in before you can comment on or make changes to this bug.