1405236 - Custom Tabs Switch under Settings -> General

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Custom Tabs, enhancement)

Description

[Joe Cheng [:jcheng] (please needinfo)]

This is to add Custom Tabs Switch under Settings -> General and have it on by default

Comment 1

[Nevin Chen(Not active on Bugzilla)]

After discussing with Joe, the requirement is below
1. In Fennec, use code to enable CustomTab for all users in all channels.
2. We use Telemetry to track the users who disable this setting.

Action items will be 
1. Backout bug 1329152 again. So we can have translation string back to m-c.
2. Modify the code there so we can enable Custom-Tab by default.
3. Remove Switchboard related code. Since we'll treat it as a new feature and let users decide if they want to disable it or not.
4. If everything goes well, we'll request the uplift to beta. So this will be on 57 Release.
5. If unfortunately, we can't make it to 57, we'll ride the 58 train.


Hi snorp, walkingice, aryx
Please help advise before I start the action. Thank you!

Comment 2

[Francesco Lodolo [:flod]]

(In reply to Nevin Chen [:nechen] from comment #1)
> 1. Backout bug 1329152 again. So we can have translation string back to m-c.

You're not getting translations back, you're asking to localize these strings again, for the third time.
This would be the second backout for bug 1329152, wouldn't it? 

Please stop removing strings if you think that you'll need to back out a feature and need these strings again in the future. Remove code, or put it behind a flag, and file a bug to remove strings once it sticks.

This is far from OK.

Comment 3

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

Nevin, just tell me if I shall do the backout (or you will do it), and if yes, when you want it to get pushed.

Comment 4

[Nevin Chen(Not active on Bugzilla)]

Another suggestion is we don't backout the old patch. Just landed a new one (with new text) and ride the 58 train.
Hi Andreas, Joe
How do you think?

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Nevin Chen [:nechen] from comment #1)

This plan sounds fine to me, modulo the localization issues above.

Francesco, did we have translations for these strings at one point? If so, surely it should be possible to recover them?

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Whoops, mid-air broke my NI for :flod (comment #5)

Comment 7

[Francesco Lodolo [:flod]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #5)
> Francesco, did we have translations for these strings at one point? If so,
> surely it should be possible to recover them?

Each localization has its own repository, it would mean somehow recovering those translations from release (the only one that still has them) and manually commit them to ~90 repositories.

Comment 8

[Andreas Bovens [:abovens]]

I'm fine with reusing the old string, if that's easier (can you share what it was?). It would be nice to have it placed in Settings > General though.

Comment 9

[Nevin Chen(Not active on Bugzilla)]

According to comment 2 and comment 7, I suggest we land new string and ride the 58 train.

for the reference, old string is 
"Allow apps to open websites using a customized version of &brandShortName;"

Since in 57 we'll have Custom Tab turned on 100% via switchboard, it should be okay if we give this setting to users in 58.

Comment 10

[Joe Cheng [:jcheng] (please needinfo)]

"Allow apps to open websites using a customized version of &brandShortName;" looks fine to me
and we should place it under Settings > General to go along with all the other features switches

Comment 11

[Andreas Bovens [:abovens]]

This is probably OK for the initial release, but I'd like us to go with a better description string in a future release. 
"Allow apps to open web content in a quickly loading custom tab." is slightly better, I think.

Comment 12

[Wesly Huang (EPM)]

Based on above comments and F2F discussion w/ Joe & Nevin, let's land this change on 58 nightly with the new string suggested by Andreas, since we are a little bit late for 57. 

Nevin will land it by next week.

(In reply to Andreas Bovens from comment #11)
> This is probably OK for the initial release, but I'd like us to go with a
> better description string in a future release. 
> "Allow apps to open web content in a quickly loading custom tab." is
> slightly better, I think.

Comment 13

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1405236 - Custom Tabs Switch under Settings -> General.

Review commit: https://reviewboard.mozilla.org/r/188312/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188312/

Comment 14

[Julian Chu [:walkingice]]

Comment on attachment 8917300 [details]
Bug 1405236 - Custom Tabs Switch under Settings -> General.

https://reviewboard.mozilla.org/r/188312/#review193970

::: mobile/android/app/src/main/res/xml/preferences_general.xml:44
(Diff revision 1)
>                        android:defaultValue="true" />
>  
> +    <SwitchPreference android:key="android.not_a_preference.customtabs"
> +                      android:title="@string/pref_custom_tabs"
> +                      android:summary="@string/pref_custom_tabs_summary"
> +                      android:defaultValue="true"/>

In here we are re-using the same key for this preference. And in previous code the default value is `false` - user already persisted `false` for this preference if user never turn it on.

I guess this default value won't work as our expectation unless we change the key of preference.

::: mobile/android/base/locales/en-US/android_strings.dtd:293
(Diff revision 1)
>  
> +<!-- Custom Tabs is an Android API for allowing third-party apps to open URLs in a customized UI.
> +     Instead of switching to the browser it appears as if the user stays in the third-party app.
> +     For more see: https://developer.chrome.com/multidevice/android/customtabs -->
> +<!ENTITY pref_custom_tabs2 "Custom Tabs">
> +<!ENTITY pref_custom_tabs_summary4 "Allow apps to open web content in a quickly loading custom tab.">

I guess we should append 's'. (....loading custom tabs)?

Comment 15

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8917300 [details]
Bug 1405236 - Custom Tabs Switch under Settings -> General.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188312/diff/1-2/

Comment 16

[Nevin Chen(Not active on Bugzilla)]

ni wehuang so he knows the progress

Comment 17

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8917300 [details]
Bug 1405236 - Custom Tabs Switch under Settings -> General.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188312/diff/2-3/

Comment 18

[Julian Chu [:walkingice]]

Comment on attachment 8917300 [details]
Bug 1405236 - Custom Tabs Switch under Settings -> General.

https://reviewboard.mozilla.org/r/188312/#review195820

Comment 19

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8917300 [details]
Bug 1405236 - Custom Tabs Switch under Settings -> General.

Hi Francesco
Could you please help me review if I did the string correctly?

Comment 20

[Francesco Lodolo [:flod]]

Comment on attachment 8917300 [details]
Bug 1405236 - Custom Tabs Switch under Settings -> General.

https://reviewboard.mozilla.org/r/188312/#review196252

::: mobile/android/base/locales/en-US/android_strings.dtd:293
(Diff revision 3)
>  
> +<!-- Custom Tabs is an Android API for allowing third-party apps to open URLs in a customized UI.
> +     Instead of switching to the browser it appears as if the user stays in the third-party app.
> +     For more see: https://developer.chrome.com/multidevice/android/customtabs -->
> +<!ENTITY pref_custom_tabs2 "Custom Tabs">
> +<!ENTITY pref_custom_tabs_summary4 "Allow apps to open web content in a quickly loading custom tabs.">

"a quickly … tabs" seems like broken English to me, 

Either stick to the original "Allow apps to open web content in a quickly loading custom tab.", or remove the "a".

Comment 21

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8917300 [details]
Bug 1405236 - Custom Tabs Switch under Settings -> General.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188312/diff/3-4/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ed5bee95f79
Custom Tabs Switch under Settings -> General. r=walkingice

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/5ed5bee95f79

Comment 24

[Francesco Lodolo [:flod]]

One of our localizers correctly pointed out that these summaries don't have a closing period on Android. 

Both comment 9 and 10 didn't have one, not sure how we ended up with one.

Comment 25

[Nevin Chen(Not active on Bugzilla)]

Should I create a patch and remove the period?

Comment 26

[Francesco Lodolo [:flod]]

(In reply to Nevin Chen [:nechen] from comment #25)
> Should I create a patch and remove the period?

That's not really a question for me, it's a question for who's in charge of copy and internal consistency for Firefox for Android. I'd also like to see clear guidelines for that (if there aren't already), so that we can avoid this back and forth in the future.

Comment 27

[Sorina Florean [:sflorean]]

Tested on latest Nightly build (58.0a1 from 11-06) with Samsung Galaxy Note 4 (Android 5.0.1), Google Pixel (Android 8.0), Oneplus Two (Android 6.0.1) and Custom Tab option (enabled by default) with the description "Allow apps to open web content in a quickly loading custom tab" is displayed in Settings->General under Compact Tabs option. 
Mark 58 as verified.