Closed Bug 1129443 Opened 10 years ago Closed 10 years ago

Add item in Settings to manage tab queue

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Android
defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: antlam, Assigned: mhaigh)

References

Details

Attachments

(1 file, 1 obsolete file)

The user may want to configure certain things about this feature like disabling/enabling it, etc. We should leave this under an item in Settings for the time being.
Summary: Add item in Settings to manage open-in-background → Add item in Settings to manage tab queue
Assignee: nobody → mhaigh
Depends on: 1132185
This patch uses build flags introduced in the patch attached to bug 1132185 (not landed at the time of submitting this patch)
Attachment #8563466 - Flags: review?(liuche)
Comment on attachment 8563466 [details] [diff] [review] Add item in Settings to manage tab queue Review of attachment 8563466 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/preferences/GeckoPreferences.java @@ +661,4 @@ > } else if (pref instanceof PanelsPreferenceCategory) { > mPanelsPreferenceCategory = (PanelsPreferenceCategory) pref; > } > + if(!(AppConstants.MOZ_ANDROID_TAB_QUEUE && AppConstants.NIGHTLY_BUILD) && pref.getSummary().equals(getString(R.string.pref_category_customize_summary))) { The first condition here is incorrect - I need to remove the negation. -> if(AppConstants.MOZ_ANDROID_TAB_QUEUE && AppConstants.NIGHTLY_BUILD && pref.getSummary()...
Comment on attachment 8563466 [details] [diff] [review] Add item in Settings to manage tab queue Review of attachment 8563466 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, just one comment. You should also add this to the testSettingsMenuItems test, and make sure you it only runs on nightly+tab_queue - this should be pretty straightforward since there should be other prefs behind flags. (I'm not finding the patches where this pref is actually used. Point me to them? Or is it not done yet?) ::: mobile/android/base/preferences/GeckoPreferences.java @@ +661,4 @@ > } else if (pref instanceof PanelsPreferenceCategory) { > mPanelsPreferenceCategory = (PanelsPreferenceCategory) pref; > } > + if(!(AppConstants.MOZ_ANDROID_TAB_QUEUE && AppConstants.NIGHTLY_BUILD) && pref.getSummary().equals(getString(R.string.pref_category_customize_summary))) { Can you add an android:key to the category_customize PreferenceScreen xml element, and check for that instead of string matching? ::: mobile/android/base/resources/xml-v11/preferences_customize_tablet.xml @@ +38,5 @@ > android:persistent="true" /> > > + <CheckBoxPreference android:key="android.not_a_preference.tab_queue" > + android:title="@string/pref_tab_queue_title" > + android:summary="@string/pref_tab_queue_summary" /> Is there a android:defaultValue?
Attachment #8563466 - Flags: review?(liuche) → review+
> -> if(AppConstants.MOZ_ANDROID_TAB_QUEUE && AppConstants.NIGHTLY_BUILD && > pref.getSummary()... Consider making MOZ_ANDROID_TAB_QUEUE depend on NIGHTLY_BUILD in Bug 1132185, unless you want to have functionality in Nightly while the feature ships elsewhere. This will simplify your conditionals, and minimize the amount of code you need to change when you let this feature ride the trains. E.g., # Enable the tab queue in Nightly only. This will go away in Bug 1132507. MOZ_ANDROID_TAB_QUEUE=$NIGHTLY_BUILD then if (AppConstants.MOZ_ANDROID_TAB_QUEUE && pref.getSummary()) {
(In reply to Richard Newman [:rnewman] from comment #4) > > -> if(AppConstants.MOZ_ANDROID_TAB_QUEUE && AppConstants.NIGHTLY_BUILD && > > pref.getSummary()... > > Consider making MOZ_ANDROID_TAB_QUEUE depend on NIGHTLY_BUILD in Bug > 1132185, unless you want to have functionality in Nightly while the feature > ships elsewhere. This will simplify your conditionals, and minimize the > amount of code you need to change when you let this feature ride the trains. > > E.g., > > # Enable the tab queue in Nightly only. This will go away in Bug 1132507. > MOZ_ANDROID_TAB_QUEUE=$NIGHTLY_BUILD > We removed this flag in the submitted patch for bug 1132185 (as keeping it in would enable it across the board - we currently just want to be able to enable from mozconfig), but have added bug 1133524 to track the tab queue dependency on the nightly flag.
Just asking for review again to make sure that I got the test stuff correct - it was pretty simple but having not done it before I want to make sure I've not missed anything. Also I've corrected the tab queue pref name as on further inspection using the 'android.not_a_preference' prefix implies that the preference is gecko based, which this is not. The preferences currently aren't used anywhere as that's a separate bug (yet to file). Final copy is in flux - tracking that in bug 1133755
Attachment #8563466 - Attachment is obsolete: true
Attachment #8565448 - Flags: review?(liuche)
Comment on attachment 8565448 [details] [diff] [review] Add item in Settings to manage tab queue Review of attachment 8565448 [details] [diff] [review]: ----------------------------------------------------------------- So this is NOT a gecko pref, but rather, an Android pref, correct? See comment about Gecko/Android preferences, the naming is a bit confusing... r+ with changing the preference keys back. ::: mobile/android/base/preferences/GeckoPreferences.java @@ +662,4 @@ > } else if (pref instanceof PanelsPreferenceCategory) { > mPanelsPreferenceCategory = (PanelsPreferenceCategory) pref; > } > + if((AppConstants.MOZ_ANDROID_TAB_QUEUE && AppConstants.NIGHTLY_BUILD) && (pref.hasKey() && PREFS_CUSTOMIZE_SCREEN.equals(pref.getKey()))) { I think pref.getKey() is already a variable in this scope, so you don't need to make all these calls to get the key. ::: mobile/android/base/resources/xml-v11/preferences.xml @@ +18,4 @@ > > <PreferenceScreen android:title="@string/pref_category_customize" > android:summary="@string/pref_category_customize_summary" > + android:key="preferences.customize_screen" Actually...android:key being prefixed by android.not_a_preference means it is NOT a Gecko pref, so you should switch the naming of these keys back. http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/GeckoPreferences.java#765 ...these really should be named android.not_gecko_pref, but I think at the time of writing, all the prefs were Gecko, so things were either a pref or not, and the case where we use Android-only-not-Gecko SharedPreferences wasn't considered. Hence the confusing naming. ::: mobile/android/base/resources/xml-v11/preferences_customize.xml @@ +30,4 @@ > android:entryValues="@array/pref_restore_values" > android:persistent="true" /> > > + <CheckBoxPreference android:key="app.feature.tab_queue" Ditto android.not_a_preference naming. ::: mobile/android/base/resources/xml-v11/preferences_customize_tablet.xml @@ +37,4 @@ > android:entryValues="@array/pref_restore_values" > android:persistent="true" /> > > + <CheckBoxPreference android:key="app.feature.tab_queue" Ditto android.not_a_preference naming. ::: mobile/android/base/strings.xml.in @@ +124,4 @@ > <string name="pref_category_advanced">&pref_category_advanced;</string> > <string name="pref_category_customize">&pref_category_customize;</string> > <string name="pref_category_customize_summary">&pref_category_customize_summary;</string> > + <string name="pref_category_customize_alt_summary">&pref_category_customize_alt_summary;</string> Nit: maybe keep the newline separator between the sections? ::: mobile/android/base/tests/testSettingsMenuItems.java @@ +181,5 @@ > } > > + // Tab Queue > + if (AppConstants.NIGHTLY_BUILD && AppConstants.MOZ_ANDROID_TAB_QUEUE) { > + String[] tabQueue = { StringHelper.TAB_QUEUE_LABEL, "Prevent tabs from opening immediately, but open all queued tabs the next time " + StringHelper.BRAND_NAME + " loads." }; I guess technically we should add the alternative summary text, but we don't actually check for it anyways right now, so that's probably fine - though it could be good to have the check to make sure we're not shipping something in the wrong release. Up to you.
Attachment #8565448 - Flags: review?(liuche) → review+
removed the section of test which was failing https://hg.mozilla.org/integration/fx-team/rev/9d5ae6ec32cb
Blocks: 1138409
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Blocks: 1138865
>> +<!ENTITY pref_tab_queue_summary "Prevent tabs from opening immediately, but open all queued tabs the next time &brandShortName; loads."> Sorry for being finicky and mentioning this after noticing during l10n (again), but please avoid using end stops in summaries or tooltips. This is now an exeption in this file where it probably should not - the only summary needing one led to bug 1147465.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: