Add item in Settings to manage tab queue

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: antlam, Assigned: mhaigh)

Tracking

unspecified
Firefox 39
x86
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Summary: Add item in Settings to manage open-in-background → Add item in Settings to manage tab queue
(Assignee)

Updated

4 years ago
Assignee: nobody → mhaigh
(Assignee)

Updated

4 years ago
Depends on: 1132185
(Assignee)

Comment 1

4 years ago
Created attachment 8563466 [details] [diff] [review]
Add item in Settings to manage tab queue

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)
(Assignee)

Comment 2

4 years ago
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()) {
(Assignee)

Comment 5

4 years ago
(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.
(Assignee)

Comment 6

4 years ago
Created attachment 8565448 [details] [diff] [review]
Add item in Settings to manage tab queue

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+
(Assignee)

Comment 10

4 years ago
removed the section of test which was failing

https://hg.mozilla.org/integration/fx-team/rev/9d5ae6ec32cb
(Assignee)

Updated

4 years ago
Blocks: 1138409
https://hg.mozilla.org/mozilla-central/rev/9d5ae6ec32cb
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
(Assignee)

Updated

4 years ago
Blocks: 1138865

Comment 12

4 years ago
>> +<!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.
You need to log in before you can comment on or make changes to this bug.