Closed Bug 1405236 Opened 4 years ago Closed 4 years ago

Custom Tabs Switch under Settings -> General

Categories

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

enhancement
Not set
normal

Tracking

(firefox58 verified)

VERIFIED FIXED
Firefox 58
Tracking Status
firefox58 --- verified

People

(Reporter: jcheng, Assigned: cnevinchen)

Details

(Whiteboard: [FNC][SPT58.3][INT])

Attachments

(1 file)

This is to add Custom Tabs Switch under Settings -> General and have it on by default
Flags: needinfo?(cnevinchen)
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!
Flags: needinfo?(walkingice0204)
Flags: needinfo?(snorp)
Flags: needinfo?(cnevinchen)
Flags: needinfo?(aryx.bugmail)
(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.
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.
Flags: needinfo?(aryx.bugmail)
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?
Flags: needinfo?(jcheng)
Flags: needinfo?(abovens)
(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?
Whoops, mid-air broke my NI for :flod (comment #5)
Flags: needinfo?(snorp) → needinfo?(francesco.lodolo)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Flags: needinfo?(francesco.lodolo)
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.
Flags: needinfo?(abovens)
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.
Flags: needinfo?(abovens)
"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
Flags: needinfo?(jcheng)
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.
Flags: needinfo?(abovens)
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.
Flags: needinfo?(cnevinchen)
Assignee: nobody → cnevinchen
Flags: needinfo?(cnevinchen)
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)?
Flags: needinfo?(walkingice0204)
ni wehuang so he knows the progress
Flags: needinfo?(wehuang)
Comment on attachment 8917300 [details]
Bug 1405236 - Custom Tabs Switch under Settings -> General.

https://reviewboard.mozilla.org/r/188312/#review195820
Attachment #8917300 - Flags: review?(walkingice0204) → review+
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?
Attachment #8917300 - Flags: review?(francesco.lodolo)
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".
Attachment #8917300 - Flags: review?(francesco.lodolo)
Pushed by nechen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ed5bee95f79
Custom Tabs Switch under Settings -> General. r=walkingice
https://hg.mozilla.org/mozilla-central/rev/5ed5bee95f79
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Flags: needinfo?(wehuang)
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.
Should I create a patch and remove the period?
Flags: needinfo?(francesco.lodolo)
(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.
Flags: needinfo?(francesco.lodolo)
Whiteboard: [FNC][SPT58.3][INT]
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.
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.