1265695 - Add pref "layout.accessiblecaret.extend_selection_for_phone_number" to all.js

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

This slip my review in bug 1235508. We need to add the pref 

  pref("layout.accessiblecaret.extend_selection_for_phone_number", true);

which defaults to false and some comment to all.js so that it could be flip easily on all platforms.

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Typo in comment 0. This is what should be added to all.js, which defaults to false.

pref("layout.accessiblecaret.extend_selection_for_phone_number", false);

Comment 2

[Mark Capella [:capella]]

This is a fairly simple bug for a new contributor (with basic Javascript skills) to learn the Mozilla development process. Estimated completion time should be easily less than a week.

While this bug completes a bit omitted earlier in an Android/mobile patch, it's results will be viewable in a basic desktop build.

You should be able to build Firefox on your desktop. See [0] for instructions if this is new to you, and ask questions on irc channels #introduction, or #mobile.


[0] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Comment 3

[mgsudhanva]

hello , i am a new contributor ,how do i start ?

Comment 4

[vinayak]

Hi, I would like to work on this.

Comment 5

[vinayak]

Attachment: Bug 1265695 - added a call to pref() function

I have added the required function call.
Could you please tell me a proper comment to add?

Comment 6

[Mark Capella [:capella]]

If you did a code search for existing uses of that pref [0], you'd have noticed the commented case where we're turning the pref ON [1].

What we're asking you to provide is a default OFF definition of the pref, which you've realized as your new definition correctly defines if as "false".

An appropriate comment (compared to the existing one) for the default OFF case might simply be like:
// Smart phone-number selection on long-press is not enabled by default.

[0] http://mxr.mozilla.org/mozilla-central/search?string=pref%28%22layout.accessiblecaret.extend_selection_for_phone_number&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central
[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/app/mobile.js?rev=4292da9df16b&mark=906-908#905

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

A quick step to verify of the correctness of the patch is:
1) Open about:config in the url bar in Firefox desktop browser. It'll show all the preferences in Firefox.
2) Search for the pref name you added.

If the pref is added correctly to all.js, you should be able to find it with the default value you set.

Comment 8

[vinayak]

Attachment: added a pref and necessary comment

tested it by running firefox desktop.

Comment 9

[Mark Capella [:capella]]

vinayak, this looks good. Please fix the commit message in the patch, re-attach and ask TYLin for review.

I'd approve, and while this is TextSelection related, this isn't close enough to my comfort zone.

See [0] for Checkin-comment rules.

[0] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment 10

[vinayak]

Attachment: Bug 1265695 - added a pref to disable smart phone-number selection on long-press by default. r=TYLin

Please review this

Comment 11

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8747426 [details] [diff] [review]
Bug 1265695 - added a pref to disable smart phone-number selection on long-press by default. r=TYLin

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

Thank you for the patch. It looks good to me.

Comment 12

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

BTW, the standard procedure for request a review for a patch is to change the review flag to r?. See [1] for the steps. 

And don't forget to set "checkin-needed" to the keywords field. It'll let sheriffs know that this bug has reviewed patches which need to be merged into the tree.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed

Comment 13

[Carsten Book [:Tomcat]]

needs rebasing

renamed 1265695 -> Bug1265695_v2.diff
applying Bug1265695_v2.diff
patching file modules/libpref/init/all.js
Hunk #1 FAILED at 4986
1 out of 1 hunks FAILED -- saving rejects to file modules/libpref/init/all.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug1265695_v2.diff

Comment 14

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

I'll help rebase the patch and land it.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1dea6c46f236

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1dea6c46f236