Closed
Bug 1148431
Opened 9 years ago
Closed 9 years ago
Create UI to inform users of Tab Queue and allow them to turn on or ignore
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox40 verified)
VERIFIED
FIXED
Firefox 40
Tracking | Status | |
---|---|---|
firefox40 | --- | verified |
People
(Reporter: mhaigh, Assigned: mhaigh)
References
Details
Attachments
(1 file, 4 obsolete files)
38.09 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
Create a UI to inform users of Tab Queue and allow them to either opt in, opt out or ignore. In order to not irritate the user, we should not show the prompt until we've had three external urls opened with fennec, at which point we should show the UI. The user can: - opt in, which will set the tab queue preference to true; - opt out which will not set the tab queue preference but set a preference to never show the tab queue prompt again; - ignore by pressing back or touching outside of the buttons. If the user ignores the prompt, the prompt will hide and then wait for another 3 external urls to be launched with the VIEW action and then show again, it will show a maximum of three times, after which it will not show again.
We have to be careful on how we phrase the message - if the prompt says, "Do you want to open this tab in background?", I might say no because this just happens to be the only tab I'll ever want to open in the foreground. We should be sure to indicate that this switches a preference and is a general state, not a one time open-in-background.
Assignee | ||
Comment 2•9 years ago
|
||
Start off by refactoring some existing assets which we're going to use later. In this case we're going to use the button drawable from the first run code, so lets rename those assets to be a bit more generic and update the references to them.
Attachment #8586721 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 3•9 years ago
|
||
Here we add the new prompt layout file, including strings, dimens, assets, colors and the layout file itself. One thing to note here is the addition of this code in the dimens file: + <!-- http://blog.danlew.net/2015/01/06/handling-android-resources-with-non-standard-formats/ --> + <item name="match_parent" type="dimen">-1</item> + <item name="wrap_content" type="dimen">-2</item> This is included as on a phone we want the layout to be full width, but in a tablet we want it to have a set width. This is difficult to do as we can't set a dimen of match_parent or wrap_content as it throws errors. By defining those two items, we can now include @dimen/match_parent anywhere we would normally define dp values without issue. More details here: http://blog.danlew.net/2015/01/06/handling-android-resources-with-non-standard-formats/ Also to note is that I include xmlns:tools="http://schemas.android.com/tools" and tools:text="..." in the layout file. These are there because our strings.xml structure means that IntelliJ can't work out what strings should be in there - I've left in because it doesn't hurt and may be of use to the next person who has to refactor this file. Happy to remove if needs be.
Attachment #8586724 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 4•9 years ago
|
||
Here we add the actual java file for the prompt. I've gone down the route of having the prompt as an Activity, rather than following the First Run route of being a View, as we wanted the entire UI covered by the partially translucent background (the first run overlay does not cover the awesomebar ui componenets). We also refactor the ShareOverlayActivity style name - apparently there were no uses of this in the project, so perhaps it was leftover code?
Attachment #8586728 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 5•9 years ago
|
||
Here we wire the prompt in to BrowserApp. As BrowserApp has the launch flag of SingleTask we have to monitor for VIEW action intents in two places (onCreate and onNewIntent).
Attachment #8586743 -
Flags: review?(michael.l.comella)
Comment on attachment 8586721 [details] [diff] [review] Part 1 - Refactor existing assets Review of attachment 8586721 [details] [diff] [review]: ----------------------------------------------------------------- r+ w/ nits. nit: I'd prefer if these names spoke to both the colors and the round corners (unless we assume all buttons will be round - check with antlam? Or maybe just assume and we can refactor later). Something like -> button_background_action_orange_round.xml I would prefer to prepend with a common prefix so I didn't start with the color (e.g. button_background_toolbar_grey_round will show up near ^).
Attachment #8586721 -
Flags: review?(michael.l.comella) → review+
Comment on attachment 8586724 [details] [diff] [review] Part 2 - Add prompt layout Review of attachment 8586724 [details] [diff] [review]: ----------------------------------------------------------------- Are these new assets trimaged? Looks reasonable. I'd like to take a look at tab_queue_prompt.xml once again either 1) before it lands (flag me for review again) or, if you're in a rush to land this, 2) after it lands (file a bug and assign me to look into it). ::: mobile/android/base/resources/drawable/tab_queue_dismiss_button_foreground.xml @@ +2,5 @@ > +<!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > +<selector xmlns:android="http://schemas.android.com/apk/res/android"> > + <item android:color="@color/tab_queue_dismiss_button_foreground" android:state_enabled="true" /> nits for readability: I prefer when these are multi-lined (e.g. [1]). I also like the state we're matching against to be first in the tag (again, see [1]). `state_enabled="true"` is also the default state so you can put it at the bottom and remove the state_enabled attr entirely (so it is just item & android:color). (don't see [1] :P) [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable/fxaccount_button_background.xml#5 ::: mobile/android/base/resources/layout/tab_queue_prompt.xml @@ +1,4 @@ > +<?xml version="1.0" encoding="utf-8"?><!-- This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > +<RelativeLayout RelativeLayouts are expensive - perhaps use a FrameLayout here? Did you model this after overlay_share_dialog? It should be much cleaner now, with the exception of bug 1148244. @@ +10,5 @@ > + android:layout_gravity="bottom|center" > + android:clipChildren="false" > + android:clipToPadding="false"> > + > + <RelativeLayout I think all of the views are "layout_below=..." - can you use a LinearLayout here? It's generally more performant because the measurement algorithms are much simpler. @@ +17,5 @@ > + android:background="@android:color/white"> > + > + <TextView > + android:id="@+id/title" > + android:layout_width="@dimen/tab_queue_content_width" Probably easier if this is match_parent here and below, then you can remove tab_queue_container_width from dimens and reduce clutter. @@ +91,5 @@ > + style="@style/Widget.BaseButton" > + android:layout_width="@dimen/tab_queue_button_width" > + android:layout_height="match_parent" > + android:layout_gravity="center" > + android:background="@color/android:white" This needs a pressed color too, right? ::: mobile/android/base/resources/values/colors.xml @@ +70,5 @@ > <color name="firstrun_tabstrip">#1193CB</color> > <color name="firstrun_pager_background">#16A3DF</color> > > + <!-- Tab Queue --> > + <color name="tab_queue_dismiss_button_foreground">#16A3DF</color> Are these colors from antlam? Ask him if they should be added to the palette (followup? block fennec-colors-v1). ::: mobile/android/base/resources/values/dimens.xml @@ +6,5 @@ > <resources> > > + <!-- http://blog.danlew.net/2015/01/06/handling-android-resources-with-non-standard-formats/ --> > + <item name="match_parent" type="dimen">-1</item> > + <item name="wrap_content" type="dimen">-2</item> I'd prefer this at the very bottom, ala my patch. But maybe I'll just land first. ;) @@ +69,5 @@ > <dimen name="firstrun_content_width">300dp</dimen> > <dimen name="firstrun_min_height">180dp</dimen> > > + <dimen name="tab_queue_content_width">260dp</dimen> > + <dimen name="tab_queue_min_height">180dp</dimen> Unused? @@ +73,5 @@ > + <dimen name="tab_queue_min_height">180dp</dimen> > + <dimen name="tab_queue_button_width">148dp</dimen> > + <dimen name="tab_queue_container_width">@dimen/match_parent</dimen> > + > + nit: extra newline
Attachment #8586724 -
Flags: review?(michael.l.comella) → review+
Comment on attachment 8586728 [details] [diff] [review] Part 3 - Add prompt file Review of attachment 8586728 [details] [diff] [review]: ----------------------------------------------------------------- Good - a bit unsure about animating in code (note: I know very little about Android animations so I'd like an explanation). ::: mobile/android/base/AndroidManifest.xml.in @@ +255,5 @@ > > <service android:name="org.mozilla.gecko.tabqueue.TabQueueService" /> > > + <activity android:name="org.mozilla.gecko.tabqueue.TabQueuePrompt" > + android:label="@MOZ_APP_DISPLAYNAME@" Though I think this should only be relevant on L (because of bug 1137928), not sure we'd want to use "Firefox" - check with Antlam? @@ +256,5 @@ > <service android:name="org.mozilla.gecko.tabqueue.TabQueueService" /> > > + <activity android:name="org.mozilla.gecko.tabqueue.TabQueuePrompt" > + android:label="@MOZ_APP_DISPLAYNAME@" > + android:launchMode="singleTask" I'd think we'd want this to work like the ShareDialog and not spawn a new task - i.e. singleTop. Why did you choose singleTask? Remember that the behavior is different pre-Lollipop. ::: mobile/android/base/resources/values/styles.xml @@ -823,5 @@ > <item name="android:paddingTop">0dp</item> > </style> > > - <!-- Make the share overlay activity appear like an overlay. --> > - <style name="ShareOverlayActivity" parent="Gecko"> This got moved to themes.xml - don't forget to rebase. ::: mobile/android/base/tabqueue/TabQueueHelper.java @@ +37,5 @@ > > + // result codes for returning from the prompt > + public static final int TAB_QUEUE_YES = 201; > + public static final int TAB_QUEUE_NO = 202; > + public static final int TAB_QUEUE_LATER = 203; I'd prefer if these were enums - you can set custom values to return in enums. ::: mobile/android/base/tabqueue/TabQueuePrompt.java @@ +21,5 @@ > +import com.nineoldandroids.animation.ObjectAnimator; > +import com.nineoldandroids.view.ViewHelper; > + > +public class TabQueuePrompt extends Locales.LocaleAwareActivity { > + public static final String LOGTAG = TabQueuePrompt.class.getSimpleName(); nit: "Gecko" + TabQueuePrompt... I think rnewman yelled at me once for not including Gecko. @@ +35,5 @@ > + @Override > + protected void onCreate(Bundle savedInstanceState) { > + super.onCreate(savedInstanceState); > + > + GeckoAppShell.ensureCrashHandling(); Does this work? @@ +64,5 @@ > + buttonContainer = findViewById(R.id.button_container); > + enabledConfirmation = findViewById(R.id.enabled_confirmation); > + > + ViewHelper.setTranslationY(containerView, 500); > + ViewHelper.setAlpha(containerView, 0); Why did you choose to do this in code? Also, where does 500 come from? @@ +67,5 @@ > + ViewHelper.setTranslationY(containerView, 500); > + ViewHelper.setAlpha(containerView, 0); > + > + final Animator translateAnimator = ObjectAnimator.ofFloat(containerView, "translationY", 0); > + translateAnimator.setDuration(400); Constants for time, here and below.
Attachment #8586728 -
Flags: review?(michael.l.comella) → review+
Attachment #8586743 -
Attachment is patch: true
Attachment #8586743 -
Attachment mime type: application/mbox → text/plain
(In reply to Martyn Haigh (:mhaigh) from comment #4) > We also refactor the ShareOverlayActivity style name - apparently there were > no uses of this in the project, so perhaps it was leftover code? In the AndroidManifest - you changed it already, don't worry.
Comment on attachment 8586743 [details] [diff] [review] Part 4 - Wire in prompt to gecko life cycle Review of attachment 8586743 [details] [diff] [review]: ----------------------------------------------------------------- By the way, if I'm not mistaken, your new Activity can get onNewIntent calls if the user, on Android L (ugh, bug 1137928), uses the recent app switcher to switch out and activates the Activity again from another app (i.e. shares another link to Firefox & and thus tab queue). Test and make sure your Activity can handle this.
Comment on attachment 8586743 [details] [diff] [review] Part 4 - Wire in prompt to gecko life cycle Review of attachment 8586743 [details] [diff] [review]: ----------------------------------------------------------------- lgtm w/ nits. ::: mobile/android/base/BrowserApp.java @@ +168,4 @@ > > // Request ID for startActivityForResult. > private static final int ACTIVITY_REQUEST_PREFERENCES = 1001; > + private static final int ACTIVITY_REQUEST_TAB_QUEUE = 2001; Where do these request IDs come from? Do we have to worry about collisions somewhere else in the product? If so, maybe file a followup to to have a RequestCodeHelper or something that delegates request codes - perhaps it can just be an enum (their values are incremented by default)? @@ +2431,4 @@ > } > }); > break; > + case ACTIVITY_REQUEST_TAB_QUEUE: nit: newlines above and below the cases! @@ +2440,5 @@ > + > + // By making this one more than EXTERNAL_LAUNCHES_BEFORE_SHOWING_PROMPT we ensure the prompt > + // will never show again without having to keep track of an extra pref. > + editor.putInt(TabQueueHelper.PREF_TAB_QUEUE_LAUNCHES, > + TabQueueHelper.EXTERNAL_LAUNCHES_BEFORE_SHOWING_PROMPT + 1); Rather than exposing the internals in this other class, perhaps add a method to TabQueueHelper like neverShowPrompt() which does ^. @@ +2455,5 @@ > + } else if (resultCode == TabQueueHelper.TAB_QUEUE_LATER) { > + // The user clicks outside of one of the button areas, so let's reset the pref for counting VIEW > + // action launches and increment the number of times they've seen the prompt. > + editor.remove(TabQueueHelper.PREF_TAB_QUEUE_LAUNCHES); > + nit: ws @@ +2459,5 @@ > + > + int timesPromptShown = prefs.getInt(TabQueueHelper.PREF_TAB_QUEUE_TIMES_PROMPT_SHOWN, 0) + 1; > + editor.putInt(TabQueueHelper.PREF_TAB_QUEUE_TIMES_PROMPT_SHOWN, timesPromptShown); > + } > + editor.apply(); nit: newline above. @@ +3500,4 @@ > } > } > > + private void shouldShowTabQueuePrompt(final Intent intent) { nit: -> showTabQueuePromptIfApplicable or similar @@ +3510,5 @@ > + && Intent.ACTION_VIEW.equals(intent.getAction()) > + && TabQueueHelper.shouldShowTabQueuePrompt(BrowserApp.this)) { > + Intent intent = new Intent(BrowserApp.this, TabQueuePrompt.class); > + intent.setData(intent.getData()); > + startActivityForResult(intent, ACTIVITY_REQUEST_TAB_QUEUE); Why do we do this asynchronously? In theory, the user can end up interacting with the browser only to have the prompt pop up awkwardly some time later - it'd be great if it just popped up to start. Perhaps followup if you're trying to get this landed? ::: mobile/android/base/tabqueue/TabQueueHelper.java @@ +40,5 @@ > // result codes for returning from the prompt > public static final int TAB_QUEUE_YES = 201; > public static final int TAB_QUEUE_NO = 202; > public static final int TAB_QUEUE_LATER = 203; > + public static final int MAX_TIMES_TO_SHOW_PROMPT = 3; nit; TAB_QUEUE_YES/NO/LATER are result codes whereas these are values - I'd prefer if there was a newline between them.
Attachment #8586743 -
Flags: review?(michael.l.comella) → review+
Assignee | ||
Comment 12•9 years ago
|
||
> Are these new assets trimaged? They are now! > Looks reasonable. I'd like to take a look at tab_queue_prompt.xml once again > either 1) before it lands (flag me for review again) or, if you're in a rush > to land this, 2) after it lands (file a bug and assign me to look into it). Sure - I've added a follow up - bug 1152244 > @@ +17,5 @@ > > + android:background="@android:color/white"> > > + > > + <TextView > > + android:id="@+id/title" > > + android:layout_width="@dimen/tab_queue_content_width" > > Probably easier if this is match_parent here and below, then you can remove > tab_queue_container_width from dimens and reduce clutter. The reason for this extra dimen is that we don't want the text filling the width of the box, so either we define the content width or the padding on each side - either way we have to define something and this is the easier way. > @@ +91,5 @@ > > + style="@style/Widget.BaseButton" > > + android:layout_width="@dimen/tab_queue_button_width" > > + android:layout_height="match_parent" > > + android:layout_gravity="center" > > + android:background="@color/android:white" > > This needs a pressed color too, right? Nope - no pressed background colour as this is a textview, not a button. > ::: mobile/android/base/resources/values/colors.xml > @@ +70,5 @@ > > <color name="firstrun_tabstrip">#1193CB</color> > > <color name="firstrun_pager_background">#16A3DF</color> > > > > + <!-- Tab Queue --> > > + <color name="tab_queue_dismiss_button_foreground">#16A3DF</color> > > Are these colors from antlam? Ask him if they should be added to the palette > (followup? block fennec-colors-v1). antlam: should these colours be added to the palette?
Flags: needinfo?(alam)
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #8) > Comment on attachment 8586728 [details] [diff] [review] > Part 3 - Add prompt file > > Review of attachment 8586728 [details] [diff] [review]: > ----------------------------------------------------------------- > > Good - a bit unsure about animating in code (note: I know very little about > Android animations so I'd like an explanation). There's no real difference to defining in code or XML. I think we want to go down the XML route in the end if there's more than one instance which is semantically the same. I've filed bug 1152250 to refactor the java defined animations for tabqueue and first run. > > ::: mobile/android/base/AndroidManifest.xml.in > @@ +255,5 @@ > > > > <service android:name="org.mozilla.gecko.tabqueue.TabQueueService" /> > > > > + <activity android:name="org.mozilla.gecko.tabqueue.TabQueuePrompt" > > + android:label="@MOZ_APP_DISPLAYNAME@" > > Though I think this should only be relevant on L (because of bug 1137928), > not sure we'd want to use "Firefox" - check with Antlam? Actually it doesn't make any difference if we remove the label (tested on a 5.1 device), so for the moment lets remove it. > > @@ +256,5 @@ > > <service android:name="org.mozilla.gecko.tabqueue.TabQueueService" /> > > > > + <activity android:name="org.mozilla.gecko.tabqueue.TabQueuePrompt" > > + android:label="@MOZ_APP_DISPLAYNAME@" > > + android:launchMode="singleTask" > > I'd think we'd want this to work like the ShareDialog and not spawn a new > task - i.e. singleTop. Why did you choose singleTask? Good spot -> singleTop > ::: mobile/android/base/tabqueue/TabQueueHelper.java > @@ +37,5 @@ > > > > + // result codes for returning from the prompt > > + public static final int TAB_QUEUE_YES = 201; > > + public static final int TAB_QUEUE_NO = 202; > > + public static final int TAB_QUEUE_LATER = 203; > > I'd prefer if these were enums - you can set custom values to return in > enums. As discussed over IM - I think that we would certainly get better encapsulation if we use enums but we do that at the sacrifice of readability. I've filed bug 1152256 to track this. > @@ +64,5 @@ > > + buttonContainer = findViewById(R.id.button_container); > > + enabledConfirmation = findViewById(R.id.enabled_confirmation); > > + > > + ViewHelper.setTranslationY(containerView, 500); > > + ViewHelper.setAlpha(containerView, 0); > > Why did you choose to do this in code? > > Also, where does 500 come from? > > @@ +67,5 @@ > > + ViewHelper.setTranslationY(containerView, 500); > > + ViewHelper.setAlpha(containerView, 0); > > + > > + final Animator translateAnimator = ObjectAnimator.ofFloat(containerView, "translationY", 0); > > + translateAnimator.setDuration(400); > > Constants for time, here and below. Lets review all the animation work in bug 1152250. This code was taken almost verbatim from the firstrun code, so we can clean and improve both at once.
Assignee | ||
Comment 14•9 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #10) > Comment on attachment 8586743 [details] [diff] [review] > Part 4 - Wire in prompt to gecko life cycle > > Review of attachment 8586743 [details] [diff] [review]: > ----------------------------------------------------------------- > > By the way, if I'm not mistaken, your new Activity can get onNewIntent calls > if the user, on Android L (ugh, bug 1137928), uses the recent app switcher > to switch out and activates the Activity again from another app (i.e. shares > another link to Firefox & and thus tab queue). Test and make sure your > Activity can handle this. So actually it seems that the prompt activity doesn't get onNewIntent calls, and the dispatcher activity is so short lived that 1) I don't think this is going to be a problem and 2) I can't recreate nor test it! So there was an issue with the flow you suggest (uses the recent app switcher > to switch out and activates the Activity again from another app), but not the one that you're suggesting. If the user followed your steps, with the code as submitted, they would see the prompt appearing each time a new VIEW action intent was received. I've moved some of the logic around to prevent this from happening.
Assignee | ||
Comment 15•9 years ago
|
||
> ::: mobile/android/base/BrowserApp.java > @@ +168,4 @@ > > > > // Request ID for startActivityForResult. > > private static final int ACTIVITY_REQUEST_PREFERENCES = 1001; > > + private static final int ACTIVITY_REQUEST_TAB_QUEUE = 2001; > > Where do these request IDs come from? Do we have to worry about collisions > somewhere else in the product? The scope of these IDs is this file (plus sub and super classes), hence I placed the ID below the existing ID so that it's easy to find. There's no need to worry about collisions as currently there are only two calls to startActivityForResult from this file. > > @@ +2440,5 @@ > > + > > + // By making this one more than EXTERNAL_LAUNCHES_BEFORE_SHOWING_PROMPT we ensure the prompt > > + // will never show again without having to keep track of an extra pref. > > + editor.putInt(TabQueueHelper.PREF_TAB_QUEUE_LAUNCHES, > > + TabQueueHelper.EXTERNAL_LAUNCHES_BEFORE_SHOWING_PROMPT + 1); > > Rather than exposing the internals in this other class, perhaps add a method > to TabQueueHelper like neverShowPrompt() which does ^. Good call - much nicer. > @@ +3510,5 @@ > > + && Intent.ACTION_VIEW.equals(intent.getAction()) > > + && TabQueueHelper.shouldShowTabQueuePrompt(BrowserApp.this)) { > > + Intent intent = new Intent(BrowserApp.this, TabQueuePrompt.class); > > + intent.setData(intent.getData()); > > + startActivityForResult(intent, ACTIVITY_REQUEST_TAB_QUEUE); > > Why do we do this asynchronously? In theory, the user can end up interacting > with the browser only to have the prompt pop up awkwardly some time later - > it'd be great if it just popped up to start. > > Perhaps followup if you're trying to get this landed? We do this asynchronously as we're reading & writing prefs - I'm pretty sure we don't want to do this on the UI thread.
Assignee | ||
Comment 16•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3405e61216e
Assignee | ||
Comment 17•9 years ago
|
||
Attachment #8586721 -
Attachment is obsolete: true
Attachment #8586724 -
Attachment is obsolete: true
Attachment #8586728 -
Attachment is obsolete: true
Attachment #8586743 -
Attachment is obsolete: true
Attachment #8589664 -
Flags: review?(michael.l.comella)
Comment 18•9 years ago
|
||
(In reply to Martyn Haigh (:mhaigh) from comment #12) > > ::: mobile/android/base/resources/values/colors.xml > > @@ +70,5 @@ > > > <color name="firstrun_tabstrip">#1193CB</color> > > > <color name="firstrun_pager_background">#16A3DF</color> > > > > > > + <!-- Tab Queue --> > > > + <color name="tab_queue_dismiss_button_foreground">#16A3DF</color> > > > > Are these colors from antlam? Ask him if they should be added to the palette > > (followup? block fennec-colors-v1). > > antlam: should these colours be added to the palette? Hm, let's sort out these colors. The only blue in this in-product feature should be our "link blue", which is in the "dismiss" button. The call-to-action button should be the "action orange" and then the "action orange pressed". The tip is also "action orange" as well.
Flags: needinfo?(alam) → needinfo?(michael.l.comella)
Flags: needinfo?(michael.l.comella) → needinfo?(mhaigh)
(In reply to Martyn Haigh (:mhaigh) from comment #15) > > @@ +3510,5 @@ > > > + && Intent.ACTION_VIEW.equals(intent.getAction()) > > > + && TabQueueHelper.shouldShowTabQueuePrompt(BrowserApp.this)) { > > > + Intent intent = new Intent(BrowserApp.this, TabQueuePrompt.class); > > > + intent.setData(intent.getData()); > > > + startActivityForResult(intent, ACTIVITY_REQUEST_TAB_QUEUE); > > > > Why do we do this asynchronously? In theory, the user can end up interacting > > with the browser only to have the prompt pop up awkwardly some time later - > > it'd be great if it just popped up to start. > > We do this asynchronously as we're reading & writing prefs - I'm pretty sure > we don't want to do this on the UI thread. Good call. However, I looked up SharedPreferences use and, if my source is correct, it seems the slowest part is only in the initial loading (and using the non-async write method) - perhaps using the UI thread is fine. Source: http://stackoverflow.com/a/4371883
Filed bug 1152438 to keep around SharedPreferences references so we don't have extra file stats.
Comment on attachment 8589664 [details] [diff] [review] Create UI to inform users of Tab Queue and allow them to turn on or ignore Review of attachment 8589664 [details] [diff] [review]: ----------------------------------------------------------------- I only did a quick skim considering I've reviewed most of this already but lgtm. ::: mobile/android/base/BrowserApp.java @@ +3479,4 @@ > } > } > > + private void shouldShowTabQueuePrompt(final Intent intent) { This should be showTabQueuePromptIfApplicable ::: mobile/android/base/tabqueue/TabQueueHelper.java @@ +46,5 @@ > + > + /** > + * Check if we should show the tab queue prompt. > + */ > + public static boolean showTabQueuePromptIfApplicable(Context context) { Add what we return to the javadoc. This doesn't actually do the showing so I'd call it "shouldShowTabQueuePrompt" @@ +196,5 @@ > + public static void processTabQueuePromptResponse(int resultCode, Context context) { > + final SharedPreferences prefs = GeckoSharedPrefs.forApp(context); > + final SharedPreferences.Editor editor = prefs.edit(); > + > + if (resultCode == TAB_QUEUE_YES) { switch statement? That way it's intuitive to have to handle unexpected values.
Attachment #8589664 -
Flags: review?(michael.l.comella) → review+
Assignee | ||
Comment 22•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/fc26d177eb06
Comment 23•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fc26d177eb06
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment 24•9 years ago
|
||
<!ENTITY tab_queue_prompt_tip_text "you can change this later in Settings"> Is this string expected to start lowercase?
Comment 25•9 years ago
|
||
(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #24) > <!ENTITY tab_queue_prompt_tip_text "you can change this later in Settings"> > > Is this string expected to start lowercase? I think so, based on bug 1112195. Look at the mock-up : https://bug1112195.bugzilla.mozilla.org/attachment.cgi?id=8562818 That said, my OCD wants an uppercase :)
Comment 26•9 years ago
|
||
Thanks, couldn't find UX specs in this bug.
> That said, my OCD wants an uppercase :)
++ Luckily, I'll live with the uppercase Italian version of it :-)
Comment 27•9 years ago
|
||
The prompt appears when you have three external links opened with Nightly and you try to open the fourth. There are 3 options: - "Enable" it by tapping the "Enable button": this will display a green check symbol and the preference is enabled in settings - "Not now": will not set the preference to true and the prompt won't show again - "Ignore "it by tapping the back button or outside the prompt. The prompt will hide and will appear after another 3 external links are opened again in Nightly. If you ignore it once again, it will show another time, after another 3 links are opened. If you also ignore it now, it will not show again.(It appears only three times) So, verified as fixed using: Device: Alcatel One Touch (Android 4.1.2) Build: Firefox for Android 40.0a1 (2015-04-28)
Status: RESOLVED → VERIFIED
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(mhaigh)
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•