1130368 - Add toast feedback after a user clicks a link

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_oib_toast1.png

Once the user decides to give this a go, every link clicked could have a feedback toast offering confirmation, and a way to switch to Firefox.

Need to determine copy

Comment 1

[Chenxia Liu [:liuche]]

What's the context for this bug, share (overlay)? I don't know if there's a metabug.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Context: Open in Background

Comment 3

[Martyn Haigh (:mhaigh)]

Attachment: Add toast feedback after a user clicks a link

This patch relies on the build flags from bug 1132185 (not currently landed).

This patch removes the View intent filter from the main BrowserApp class and moves it to a newly created TanQueue Activity.  This activity works out if we should load fennec normally or start a service which shows an overlay over the launching application.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8563517 [details] [diff] [review]
Add toast feedback after a user clicks a link

Review of attachment 8563517 [details] [diff] [review]:
-----------------------------------------------------------------

Why use a Service here rather than an Activity? If we're the foreground Activity, we can draw over the other application if we don't take up the full-screen (think share overlay [1]). That way we don't need to take the SYSTEM_ALERT_WINDOW. Services are typically reserved for background work (i.e. non-drawing).

I've been doing some refactoring work to make an AbstractShareDialog super class in bug 1122302. If the animations are similar here (i.e. slide in from the bottom), we could refactor that further to "AbstractSlideInDialog", or similar (which might reduce the amount of work you'd have to do here).

Because I'll be gone until Wednesday, I mentioned to rnewman if he's too busy to review my stuff, he may want to punt to you so you can check that out. If you agree that the refactoring is a good idea, then you can import locally and do some refactoring off that (or land the non-user facing parts - not sure which those are though!).

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/overlays/ui/ShareDialog.java

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +177,5 @@
>  <!ENTITY pref_donottrack_title "Do not track">
>  <!ENTITY pref_donottrack_summary "&brandShortName; will tell sites that you do not want to be tracked">
>  
> +<!ENTITY tab_queue_toast_message "Open later">
> +<!ENTITY tab_queue_toast_action "Open now">

Non-final? Having a dialog with "Open later | Open now (button)" seems confusing.

Perhaps use Anthony's text content?

::: mobile/android/base/resources/values/styles.xml
@@ +895,5 @@
> +    <!-- Completely hide the activity - this is used for the tab queue -->
> +    <style name="TabQueueActivity">
> +        <item name="android:windowIsTranslucent">true</item>
> +        <item name="android:windowBackground">@android:color/transparent</item>
> +        <item name="android:windowContentOverlay">@null</item>

I'm unfamiliar with these - seems like some may be redundant, but if this is what you need, I'd believe it. Psht. Android. :P

::: mobile/android/base/tabqueue/TabQueue.java
@@ +15,5 @@
> +import android.text.TextUtils;
> +import android.util.Log;
> +
> +public class TabQueue extends Locales.LocaleAwareActivity {
> +    private static final String LOGTAG = "TabQueue";

nit: "Gecko" + TabQueue.class.getSimpleName();

Gecko by convention, and getting the name dynamically is better for refactoring.

@@ +25,5 @@
> +        Intent intent = getIntent();
> +
> +        // For the moment lets exit early and start fennec as normal if we're not in nightly with
> +        // the tab queue build flag.
> +        if (!(AppConstants.MOZ_ANDROID_TAB_QUEUE && AppConstants.NIGHTLY_BUILD)) {

Maybe `AppConstants.MOZ_ANDROID_TAB_QUEUE = <whatever it is now> && AppConstants.NIGHTLY_BUILD;`. Keeps things a bit more consistent.

@@ +56,5 @@
> +    /**
> +     * Start fennec with the supplied intent.
> +     */
> +    private void loadNormally(Intent intent) {
> +        intent.setClass(getApplicationContext(), BrowserApp.class);

Why application context?

@@ +65,5 @@
> +    /**
> +     * Abort as we were started with no URL.
> +     */
> +    private void abortDueToNoURL() {
> +        Log.d(LOGTAG, "Unable to process tab queue insertion. No URL found!");

We should notify the user here that the queue failed.

::: mobile/android/base/tabqueue/TabQueueHelper.java
@@ +5,5 @@
> +
> +package org.mozilla.gecko.tabqueue;
> +
> +public class TabQueueHelper {
> +    public static final long TOAST_TIMEOUT = 3000;

Why not make this a constant in TabQueueService?

::: mobile/android/base/tabqueue/TabQueueService.java
@@ +20,5 @@
> +import android.view.WindowManager;
> +import android.widget.Button;
> +import android.widget.TextView;
> +
> +// On launch of an external url this service displays a view overtop of the currently running activity with an action

nit: javadoc.

@@ +31,5 @@
> +    private TextView mMessageView;
> +    private Button openNowButton;
> +    private final Handler mHideHandler = new Handler();
> +    private WindowManager.LayoutParams mParams;
> +    private HideRunnable mHideRunnable;

nit: no Hungarian notation. :P

@@ +47,5 @@
> +
> +        windowManager = (WindowManager) getSystemService(WINDOW_SERVICE);
> +
> +        LayoutInflater layoutInflater = (LayoutInflater) getSystemService(LAYOUT_INFLATER_SERVICE);
> +        layout = layoutInflater.inflate(R.layout.button_toast, null);

Seems there is an easier way to do this: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=b323b586b5e6#3439

@@ +50,5 @@
> +        LayoutInflater layoutInflater = (LayoutInflater) getSystemService(LAYOUT_INFLATER_SERVICE);
> +        layout = layoutInflater.inflate(R.layout.button_toast, null);
> +
> +        mMessageView = (TextView) layout.findViewById(R.id.toast_message);
> +        mMessageView.setText(getResources().getText(R.string.tab_queue_toast_message));

micro-nit: Local var for Resources instance so we don't need to call the function twice

Comment 5

[Martyn Haigh (:mhaigh)]

> Why use a Service here rather than an Activity? If we're the foreground
> Activity, we can draw over the other application if we don't take up the
> full-screen (think share overlay [1]). That way we don't need to take the
> SYSTEM_ALERT_WINDOW. Services are typically reserved for background work
> (i.e. non-drawing).

This is the main difference between this and the share overlay is that this will allow the user to ignore the toast and carry on using the application in the background.

I tried a fair few approaches on this before opting to go down the current route.  The issue with using an activity to display the toast is that although we can set the background of the acitvity to invisible so the user can see the application behind it, that application is now in a background state which means that it doesn't respond to touch events and there's no way that I can route touch events from our activity to the application behind.  This method is the same as FB use to do their chatheads (http://stackoverflow.com/questions/15975988/what-apis-in-android-is-facebook-using-to-create-chat-heads).


> I've been doing some refactoring work to make an AbstractShareDialog super
> class in bug 1122302. If the animations are similar here (i.e. slide in from
> the bottom), we could refactor that further to "AbstractSlideInDialog", or
> similar (which might reduce the amount of work you'd have to do here).

This may become applicable once we get the Tab Queue prompt work in, but I'm not sure it applies here.

> 
> ::: mobile/android/base/resources/values/styles.xml
> @@ +895,5 @@
> > +    <!-- Completely hide the activity - this is used for the tab queue -->
> > +    <style name="TabQueueActivity">
> > +        <item name="android:windowIsTranslucent">true</item>
> > +        <item name="android:windowBackground">@android:color/transparent</item>
> > +        <item name="android:windowContentOverlay">@null</item>
> 
> I'm unfamiliar with these - seems like some may be redundant, but if this is
> what you need, I'd believe it. Psht. Android. :P

I'll go back and check all of these.
 
> 
> 
> @@ +25,5 @@
> > +        Intent intent = getIntent();
> > +
> > +        // For the moment lets exit early and start fennec as normal if we're not in nightly with
> > +        // the tab queue build flag.
> > +        if (!(AppConstants.MOZ_ANDROID_TAB_QUEUE && AppConstants.NIGHTLY_BUILD)) {
> 
> Maybe `AppConstants.MOZ_ANDROID_TAB_QUEUE = <whatever it is now> &&
> AppConstants.NIGHTLY_BUILD;`. Keeps things a bit more consistent.

I accept that this isn't the most understandable code - I have bug 1133524 to add a dependancy on the nightly build flag to the tab queue flag.  I'm not 100% sure what the fix you mention does that the code doesn't already do - maybe I'm missing something?

> 
> @@ +56,5 @@
> > +    /**
> > +     * Start fennec with the supplied intent.
> > +     */
> > +    private void loadNormally(Intent intent) {
> > +        intent.setClass(getApplicationContext(), BrowserApp.class);
> 
> Why application context?

I try to rememver to use an Activity's Context within that Activity, and the Application Context when passing a context beyond the scope of an Activity to avoid memory leaks.  This may be overkill as this comes from the dark days of Android - would like to hear any reasons to not do this! 

> 
> @@ +65,5 @@
> > +    /**
> > +     * Abort as we were started with no URL.
> > +     */
> > +    private void abortDueToNoURL() {
> > +        Log.d(LOGTAG, "Unable to process tab queue insertion. No URL found!");
> 
> We should notify the user here that the queue failed.

In testing I was getting a few ghost invocations of this service, hence no user facing UI currently.  I imagine getting here via a user action is the edge case and certainly a lot less annoying than getting random toasts pop up telling you that there was no url for something you didn't do!

> 
> ::: mobile/android/base/tabqueue/TabQueueHelper.java
> @@ +5,5 @@
> > +
> > +package org.mozilla.gecko.tabqueue;
> > +
> > +public class TabQueueHelper {
> > +    public static final long TOAST_TIMEOUT = 3000;
> 
> Why not make this a constant in TabQueueService?
> 

This file eventually will become a helper class for the entire tab queue work - this is just the beginning.

> 
> @@ +31,5 @@
> > +    private TextView mMessageView;
> > +    private Button openNowButton;
> > +    private final Handler mHideHandler = new Handler();
> > +    private WindowManager.LayoutParams mParams;
> > +    private HideRunnable mHideRunnable;
> 
> nit: no Hungarian notation. :P

;P

Comment 6

[Martyn Haigh (:mhaigh)]

Also - will hold off final copy atm.  bug 1133755 opened which will get all copy in place - lets move forwards with the knowledge that we need to go back and replace copy before we ship.

Comment 7

[Martyn Haigh (:mhaigh)]

I've added bug 1134148 which will track what we should do in the event that we get a View intent received without a URL.  Any discussions on that front would be welcome but I'm going to not include the reporting of missing url data to the user from this patch.

Comment 8

[Martyn Haigh (:mhaigh)]

Attachment: Add toast feedback after a user clicks a link

I cleaned up the styles, turns out there was an android defined style just for this occassion!

Also tried out the ButtonToast class, but functionality of that is just different enough that it felt whatever benefits were had by using a pre defined class were nullified by having to keep track of both visibility state and view-attached-to-window state over just the latter by itself.  

Other nits addressed :)

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Martyn Haigh (:mhaigh) from comment #5)
> > Maybe `AppConstants.MOZ_ANDROID_TAB_QUEUE = <whatever it is now> &&
> > AppConstants.NIGHTLY_BUILD;`. Keeps things a bit more consistent.
> 
> I accept that this isn't the most understandable code - I have bug 1133524
> to add a dependancy on the nightly build flag to the tab queue flag.  I'm
> not 100% sure what the fix you mention does that the code doesn't already do
> - maybe I'm missing something?

Sorry if I was unclear - I was saying that you could initialize the AppConstants value with the current value you have and NIGHTLY_BUILD, rather than adding a NIGHTLY_BUILD check here. I don't know where you'd use MOZ_ANDROID_TAB_QUEUE but not care that it's a Nightly build, so if that's the case, it'd be better to include it in the constant in the first place.

> > Why application context?
> 
> I try to rememver to use an Activity's Context within that Activity, and the
> Application Context when passing a context beyond the scope of an Activity
> to avoid memory leaks.  This may be overkill as this comes from the dark
> days of Android - would like to hear any reasons to not do this!

Not that I know of - I was asking for my own edification! :)

> > We should notify the user here that the queue failed.
> 
> In testing I was getting a few ghost invocations of this service,

Why is that? It seems like we'd rather solve this than squelching output because of it.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8565914 [details] [diff] [review]
Add toast feedback after a user clicks a link

Review of attachment 8565914 [details] [diff] [review]:
-----------------------------------------------------------------

Looking pretty good - just a lot of nits on my end because I'm picky! :)

---

I don't think the service is ever stopped. From the docs [1]:

There are two reasons that a service can be run by the system. If someone calls Context.startService() then the system will retrieve the service (creating it and calling its onCreate() method if needed) and then call its onStartCommand(Intent, int, int) method with the arguments supplied by the client. The service will at this point continue running until Context.stopService() or stopSelf() is called.

We should stop it at some point.

[1]: https://developer.android.com/reference/android/app/Service.html#ServiceLifecycle

::: mobile/android/base/AndroidManifest.xml.in
@@ +171,5 @@
>                  <category android:name="android.intent.category.DEFAULT" />
>              </intent-filter>
> +#ifndef MOZ_ANDROID_TAB_QUEUE
> +            <!-- The main reason for the Tab Queue build flag is to not mess with the VIEW intent filter
> +                 before the rest of the plumbing is in place -->

You may want to add a comment saying that the entry point from external applications (i.e. an action VIEW intent) is TabQueueService since most developers may look here first.

@@ +251,5 @@
> +        <!-- The main reason for the Tab Queue build flag is to not mess with the VIEW intent filter
> +             before the rest of the plumbing is in place -->
> +
> +        <service android:name="org.mozilla.gecko.tabqueue.TabQueueService"
> +                 android:label="Tab Queue Service">

Should be @string.

::: mobile/android/base/resources/values/styles.xml
@@ +892,4 @@
>          <item name="android:paddingRight">8dp</item>
>      </style>
>  
> +    <!-- Completely hide the activity - this is used for the tab queue -->

nit: comment is redundant to style name (i.e. we already know this is for the tab queue)

::: mobile/android/base/tabqueue/TabQueue.java
@@ +15,5 @@
> +import android.text.TextUtils;
> +import android.util.Log;
> +import android.widget.Toast;
> +
> +public class TabQueue extends Locales.LocaleAwareActivity {

nit: class comment.

@@ +39,5 @@
> +            return;
> +        }
> +
> +        final String pageUrl = new WebURLFinder(dataString).bestWebURL();
> +        if (TextUtils.isEmpty(pageUrl)) {

I noticed this code is the same as the ShareDialog - perhaps we should factor it out into a helper methods? (followup?)

e.g. https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/overlays/ui/ShareDialog.java?rev=78bb737b85ea#131

@@ +66,5 @@
> +    /**
> +     * Abort as we were started with no URL.
> +     */
> +    private void abortDueToNoURL() {
> +        // Lets decide what to do here in bug 1134148

nit: // TODO: ...

@@ +67,5 @@
> +     * Abort as we were started with no URL.
> +     */
> +    private void abortDueToNoURL() {
> +        // Lets decide what to do here in bug 1134148
> +        Log.d(LOGTAG, "Unable to process tab queue insertion. No URL found!");

nit: Could be helpful to include the argument String here that led to this failing.

::: mobile/android/base/tabqueue/TabQueueHelper.java
@@ +5,5 @@
> +
> +package org.mozilla.gecko.tabqueue;
> +
> +public class TabQueueHelper {
> +    public static final long TOAST_TIMEOUT = 3000;

nit: Will this be used anywhere else? If not, I'd move it to TabQueueService.

If not moved, it should probably be protected, or package-private.

::: mobile/android/base/tabqueue/TabQueueService.java
@@ +26,5 @@
> +
> +/**
> + * On launch this service displays a view over the currently running activity with an action
> + * to open the url in fennec immediately.  If the user takes no action or the service receives another intent, the
> + * url is added to a file which is then read in fennec on next launch.  The view is inserted from this service, in

nit: The view -> A view.

@@ +28,5 @@
> + * On launch this service displays a view over the currently running activity with an action
> + * to open the url in fennec immediately.  If the user takes no action or the service receives another intent, the
> + * url is added to a file which is then read in fennec on next launch.  The view is inserted from this service, in
> + * conjunction with the SYSTEM_ALERT_WINDOW permission, to display the view on top of the application in the background
> + * whilst still allowing the user to interact with the background application.

Mention what the purpose of this view is.

Maybe also link to the chat heads article (or SO post) with some motivations for not making this an Activity, as I questioned.

@@ +36,5 @@
> +
> +    private WindowManager windowManager;
> +    private View layout;
> +    private Button openNowButton;
> +    private final Handler handler = new Handler();

This Handler is created for the calling thread [1]:

When you create a new Handler, it is bound to the thread / message queue of the thread that is creating it.

I think we should explicitly define a thread so we don't end up doing work on the UI thread, or something. I think you can use the Looper constructor. Name the handler appropriately.

[1]: https://developer.android.com/reference/android/os/Handler.html

@@ +54,5 @@
> +
> +        windowManager = (WindowManager) getSystemService(WINDOW_SERVICE);
> +
> +        LayoutInflater layoutInflater = (LayoutInflater) getSystemService(LAYOUT_INFLATER_SERVICE);
> +        layout = layoutInflater.inflate(R.layout.button_toast, null);

nit: -> toastLayout

@@ +62,5 @@
> +        TextView messageView = (TextView) layout.findViewById(R.id.toast_message);
> +        messageView.setText(resources.getText(R.string.tab_queue_toast_message));
> +
> +        openNowButton = (Button) layout.findViewById(R.id.toast_button);
> +        openNowButton.setEnabled(true);

Are buttons not enabled by default? That's curious!

@@ +65,5 @@
> +        openNowButton = (Button) layout.findViewById(R.id.toast_button);
> +        openNowButton.setEnabled(true);
> +        openNowButton.setText(resources.getText(R.string.tab_queue_toast_action));
> +
> +        layoutParams = new WindowManager.LayoutParams(

nit: -> toastLayoutParams

@@ +69,5 @@
> +        layoutParams = new WindowManager.LayoutParams(
> +                WindowManager.LayoutParams.WRAP_CONTENT,
> +                WindowManager.LayoutParams.WRAP_CONTENT,
> +                WindowManager.LayoutParams.TYPE_PHONE,
> +                WindowManager.LayoutParams.FLAG_NOT_FOCUSABLE,

If this is not focusable, how will this affect accessibility? Perhaps this is just something we should think about later though.

@@ +74,5 @@
> +                PixelFormat.TRANSLUCENT);
> +
> +        layoutParams.gravity = Gravity.BOTTOM | Gravity.CENTER_HORIZONTAL;
> +        layoutParams.x = 0;
> +        layoutParams.y = 0;

Why is it necessary to set x & y explicitly? Should this be handled by gravity?

@@ +81,5 @@
> +    private abstract class HideRunnable implements Runnable {
> +        // If true then remove the toast from the view hierarchy when run.
> +        private boolean mShouldHide = true;
> +
> +        public void shouldHide(boolean hide) {

nit: setShouldHide, by convention.

@@ +92,5 @@
> +            }
> +            execute();
> +        }
> +
> +        public abstract void execute();

nit: -> onRun or onPostRun or onMaybeRun similar

@@ +99,5 @@
> +    @Override
> +    public int onStartCommand(final Intent intent, int flags, int startId) {
> +
> +        if (hideRunnable != null) {
> +            // If there's already a runnable then run it but keep the view attached to the window.

nit: Make this more general, e.g. "If we're already displaying a toast, keep displaying it but store the previous link's url. The open button will refer to..." etc.

@@ +131,5 @@
> +        });
> +
> +        handler.postDelayed(hideRunnable, TabQueueHelper.TOAST_TIMEOUT);
> +
> +        return super.onStartCommand(intent, flags, startId);

We should return one of the START_* flags here, right?

What happens if our service is killed mid-way? This should probably be START_REDELIVER_INTENT but I leave further research to you.

@@ +134,5 @@
> +
> +        return super.onStartCommand(intent, flags, startId);
> +    }
> +
> +    private void hide() {

nit: -> hideToast (maybe also update the runnable name)

@@ +138,5 @@
> +    private void hide() {
> +        windowManager.removeView(layout);
> +    }
> +
> +    private void addUrlToList(Intent intentParam) {

nit: What list? I'd prefer if this referenced the background nature of it, e.g. addURLToBackgroundTabList

@@ +140,5 @@
> +    }
> +
> +    private void addUrlToList(Intent intentParam) {
> +        if (intentParam == null) {
> +            // This should never happen, but lets return silently instead of crash if it does!

Let's log it.

Comment 11

[Martyn Haigh (:mhaigh)]

Attachment: Add toast feedback after a user clicks a link

Thanks for the last feedback - very thorough! I've addressed nits and reworked a lot of the service - don't suppose you'd take another peek at it, would ya?

ta

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8571403 [details] [diff] [review]
Add toast feedback after a user clicks a link

Review of attachment 8571403 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/tabqueue/TabQueue.java
@@ +20,5 @@
> + * the tab queue functionality is enabled and forwards the intent to the TabQueueService to process if it is.
> + *
> + * If the tab queue functionality is not enabled then it forwards the intent to BrowserApp to handle as normal.
> + */
> +public class TabQueue extends Locales.LocaleAwareActivity {

Given the class description, I'd prefer a name like, "TabQueueIntentForwarder" or *Dispatcher. Maybe *Handler, but less so because it offloads the work to other classes.

::: mobile/android/base/tabqueue/TabQueueService.java
@@ +30,5 @@
> + * to open the url in fennec immediately.  If the user takes no action or the service receives another intent, the
> + * url is added to a file which is then read in fennec on next launch, in order to allow the user to quickly queue
> + * urls to open without having to open Fennec each time.  A View is inserted from this service, in
> + * conjunction with the SYSTEM_ALERT_WINDOW permission, to display the view on top of the application in the background
> + * whilst still allowing the user to interact with the background application.

nit: application as opposed to a background Activity which is non-interactable.

Or similar.

@@ +36,5 @@
> + * General approach taken is similar to the FB chat heads functionality:
> + *   http://stackoverflow.com/questions/15975988/what-apis-in-android-is-facebook-using-to-create-chat-heads
> + */
> +public class TabQueueService extends Service {
> +     private static final String LOGTAG = "Gecko" + TabQueueService.class.getSimpleName();

nit: indentation.

@@ +59,5 @@
> +        super.onCreate();
> +
> +        HandlerThread thread = new HandlerThread("TabQueueHandlerThread");
> +        thread.start();
> +        tabQueueHandler = new Handler(thread.getLooper());

Why a new thread?

Sorry if I'm being unclear - I don't know which thread a service runs on and I'm unsure where we'd want to run these runnables: the service's thread, the UI thread (are they the same?), or a new thread.

[1]: https://stackoverflow.com/questions/6369287/accessing-ui-thread-handler-from-a-service

@@ +90,5 @@
> +     * when run, unless explicitly instructed not to.
> +     */
> +    private abstract class StopServiceRunnable implements Runnable {
> +
> +        private boolean shouldStopService = true;

volatile? If we're starting a second thread, this can be accessed from different threads, right?

@@ +92,5 @@
> +    private abstract class StopServiceRunnable implements Runnable {
> +
> +        private boolean shouldStopService = true;
> +
> +        public void setShouldNotStopService() {

nit: Maybe this should take a boolean? Seems to fit convention more, and is more flexible, but I understand where it can be confusing.

@@ +115,5 @@
> +            // If we're already displaying a toast, keep displaying it but store the previous link's url.
> +            // The open button will refer to the most recently opened link.
> +            tabQueueHandler.removeCallbacks(stopServiceRunnable);
> +            stopServiceRunnable.setShouldNotStopService();
> +            stopServiceRunnable.run();

I assume this will run fast enoug that we don't need to run it on another thread?

Perhaps instead (I weakly prefer this):

  stopServiceRunnable.run(false);

and...

void run() {
  run(true);
}

void run(boolean shouldStopService) {
  ...
}

@@ +117,5 @@
> +            tabQueueHandler.removeCallbacks(stopServiceRunnable);
> +            stopServiceRunnable.setShouldNotStopService();
> +            stopServiceRunnable.run();
> +        } else {
> +            windowManager.addView(toastLayout, toastLayoutParams);

Does this have to happen from the UI thread?

If not, maybe it should.

@@ +146,5 @@
> +        });
> +
> +        tabQueueHandler.postDelayed(stopServiceRunnable, TOAST_TIMEOUT);
> +
> +        return START_REDELIVER_INTENT;

from the docs:

This Intent will remain scheduled for redelivery until the service calls stopSelf(int) with the start ID provided to onStartCommand(Intent, int, int).

https://developer.android.com/reference/android/app/Service.html#START_REDELIVER_INTENT

@@ +153,5 @@
> +    /**
> +     * Removes the View from the view hierarchy and stops the service.
> +     */
> +    private void destroy() {
> +        windowManager.removeView(toastLayout);

Does this have to happen from the UI thread?

Comment 13

[Martyn Haigh (:mhaigh)]

Attachment: Add toast feedback after a user clicks a link

(In reply to Michael Comella (:mcomella) from comment #12)
> Comment on attachment 8571403 [details] [diff] [review]

> 
> Given the class description, I'd prefer a name like,
> "TabQueueIntentForwarder" or *Dispatcher. Maybe *Handler, but less so
> because it offloads the work to other classes.

Yeah - dispatcher seems good

> 
> ::: mobile/android/base/tabqueue/TabQueueService.java
> @@ +30,5 @@
> > + * to open the url in fennec immediately.  If the user takes no action or the service receives another intent, the
> > + * url is added to a file which is then read in fennec on next launch, in order to allow the user to quickly queue
> > + * urls to open without having to open Fennec each time.  A View is inserted from this service, in
> > + * conjunction with the SYSTEM_ALERT_WINDOW permission, to display the view on top of the application in the background
> > + * whilst still allowing the user to interact with the background application.
> 
> nit: application as opposed to a background Activity which is
> non-interactable.
> 
> Or similar.

Aaahhh..sorry - which bit are you referring to?


> 
> @@ +59,5 @@
> > +        super.onCreate();
> > +
> > +        HandlerThread thread = new HandlerThread("TabQueueHandlerThread");
> > +        thread.start();
> > +        tabQueueHandler = new Handler(thread.getLooper());
> 
> Why a new thread?
> 
> Sorry if I'm being unclear - I don't know which thread a service runs on and
> I'm unsure where we'd want to run these runnables: the service's thread, the
> UI thread (are they the same?), or a new thread.

An Android Service runs on the UI thread (!) [http://developer.android.com/guide/components/services.html]  This is the reason that we can access the UI from the main service methods and the reason we create a new thread to offload potentially expensive operations (like file IO) on to.


> 
> [1]:
> https://stackoverflow.com/questions/6369287/accessing-ui-thread-handler-from-
> a-service
> 
> @@ +90,5 @@
> > +     * when run, unless explicitly instructed not to.
> > +     */
> > +    private abstract class StopServiceRunnable implements Runnable {
> > +
> > +        private boolean shouldStopService = true;
> 
> volatile? If we're starting a second thread, this can be accessed from
> different threads, right?

Good catch - yeah - this and the stopServiceRunnable are accessed from a different thread.

> 
> Perhaps instead (I weakly prefer this):
> 
>   stopServiceRunnable.run(false);
> 
> and...
> 
> void run() {
>   run(true);
> }
> 
> void run(boolean shouldStopService) {
>   ...
> }
> 

Yeah - that seems cleaner - ta.

> 
> @@ +146,5 @@
> > +        });
> > +
> > +        tabQueueHandler.postDelayed(stopServiceRunnable, TOAST_TIMEOUT);
> > +
> > +        return START_REDELIVER_INTENT;
> 
> from the docs:
> 
> This Intent will remain scheduled for redelivery until the service calls
> stopSelf(int) with the start ID provided to onStartCommand(Intent, int, int).
> 
> https://developer.android.com/reference/android/app/Service.
> html#START_REDELIVER_INTENT
> 

So - after a bit of investigation I've kept START_REDELIVER_INTENT and instead of selfStop, I'm now using stopSelfResult [https://developer.android.com/reference/android/app/Service.html#stopSelfResult%28int%29] which will clear the Intent with the provided start ID from the redelivery queue and stop the service altogether if there are no more intents left to process (I'm paraphrasing - there's some more detail in there but this is essentially what happens).  I've not been able to test the service being killed and restarted, but can confirm that the stopSelfResult calls are being processed in the correct order and the service onDestroy method is being called at the correct time.

Comment 14

[Martyn Haigh (:mhaigh)]

Attachment: Add toast feedback after a user clicks a link

Have added code to cleanup the thread in the service onDestroy method.

Comment 15

[Richard Newman [:rnewman]]

I left a comment in one of the other bugs that you should probably have an IntentService rather than managing a thread yourself.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Martyn Haigh (:mhaigh) from comment #13)
> > ::: mobile/android/base/tabqueue/TabQueueService.java
> > @@ +30,5 @@
> > > + * to open the url in fennec immediately.  If the user takes no action or the service receives another intent, the
> > > + * url is added to a file which is then read in fennec on next launch, in order to allow the user to quickly queue
> > > + * urls to open without having to open Fennec each time.  A View is inserted from this service, in
> > > + * conjunction with the SYSTEM_ALERT_WINDOW permission, to display the view on top of the application in the background
> > > + * whilst still allowing the user to interact with the background application.
> > 
> > nit: application as opposed to a background Activity which is
> > non-interactable.
> > 
> > Or similar.
> 
> Aaahhh..sorry - which bit are you referring to?

The end bit - essentially, I'd like you to explain your motivations for using a Service rather than an Activity.

(In reply to Richard Newman [:rnewman] from comment #15)
> I left a comment in one of the other bugs that you should probably have an
> IntentService rather than managing a thread yourself.

Seems reasonable to me too, though I wonder what happens if we try to post delayed runnables and return from onHandleIntent - will the Service get killed after returning or after the delayed runnables run? Does it matter (i.e. the Runnables will still run if the Service is dead). Also, is IntentService restarted if it's killed before the work is finished?

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8576042 [details] [diff] [review]
Add toast feedback after a user clicks a link

Review of attachment 8576042 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay - I was trying to hammer out some 38 stuff.

Ignore my previous reservations on IntentService - I was thinking we'd replace this whole Service with an IntentService, but thinking about it shallowly I think it makes more sense to replace just the handler with an IntentService. However, I'm not sure how difficult or expensive it is to communicate between Services (i.e. to tell the calling Service what to do).

In any case, I'm confused. Investigate ^. But you have my r+ if you decide to go this route - just explain why.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +180,5 @@
>  <!ENTITY pref_donottrack_title "Do not track">
>  <!ENTITY pref_donottrack_summary "&brandShortName; will tell sites that you do not want to be tracked">
>  
> +<!ENTITY tab_queue_toast_message "Open later">
> +<!ENTITY tab_queue_toast_action "Open now">

I *think* this gets translated when it gets bumped to Aurora, so let's try to land the copy in the same version as this.

::: mobile/android/base/tabqueue/TabQueueService.java
@@ +169,5 @@
> +        }
> +        final ContextUtils.SafeIntent intent = new ContextUtils.SafeIntent(intentParam);
> +        final String intentData = intent.getDataString();
> +
> +        // TODO Add url to tab queue here.

nit: If we're leaving this unimplemented, add the bug # it'll be implemented in.

Comment 18

[Richard Newman [:rnewman]]

(In reply to Michael Comella (:mcomella) from comment #16)

> Seems reasonable to me too, though I wonder what happens if we try to post
> delayed runnables and return from onHandleIntent - will the Service get
> killed after returning or after the delayed runnables run?

That depends how you queue up the runnables, mostly. Martyn explained that there are really two sets of things happening here — handling/queueing the open intents, and then fetching them. Linear intent handling is easily handled via an IntentService, parallel fetching not so well suited.

> Does it matter
> (i.e. the Runnables will still run if the Service is dead). Also, is
> IntentService restarted if it's killed before the work is finished?

If you specify START_REDELIVER_INTENT, it will be.

http://developer.android.com/reference/android/app/Service.html#START_REDELIVER_INTENT

Comment 19

[Martyn Haigh (:mhaigh)]

> The end bit - essentially, I'd like you to explain your motivations for
> using a Service rather than an Activity.

So I just rewrote it using an activity instead of a service and it works pretty well, apart from one problem: the native app resolver dialog stays on screen for the duration of the toast.  If fennec is already chosen as the default then this isn't an issue.

This is a shame as with the view being in the Activity, we can get rid of the Service and replace with an IntentService which makes the code a lot cleaner.

> > I left a comment in one of the other bugs that you should probably have an
> > IntentService rather than managing a thread yourself.
> 
> Seems reasonable to me too, though I wonder what happens if we try to post
> delayed runnables and return from onHandleIntent - will the Service get
> killed after returning or after the delayed runnables run? Does it matter
> (i.e. the Runnables will still run if the Service is dead). Also, is
> IntentService restarted if it's killed before the work is finished?

The problem with an IntentService is that each call to the service results in a completely new instance being created - so each toast is a new view which sticks around for 3 seconds or so, we can't do the intelligent view reuse.

Comment 20

[Martyn Haigh (:mhaigh)]

https://hg.mozilla.org/integration/fx-team/rev/5e3818a01dd8

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Martyn Haigh (:mhaigh) from comment #19)
> > The end bit - essentially, I'd like you to explain your motivations for
> > using a Service rather than an Activity.
> 
> So I just rewrote it using an activity instead of a service and it works
> pretty well, apart from one problem: the native app resolver dialog stays on
> screen for the duration of the toast.  If fennec is already chosen as the
> default then this isn't an issue.

How did you display the toast? I found out the ShareDialog uses Toast.show, and it seems like we can add any arbitrary content to the toast (but I haven't done any research). The background app seems to be interactable while the Toast fades away.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/overlays/ui/OverlayToastHelper.java?rev=48e03a88b6a8#70

Comment 23

[Gian-Carlo Pascutto [:gcp]]

This requires the additional permission "allow Firefox to draw over other apps". I'm seeing some confusion over why Firefox requires that permissions, and the Android docs aren't encouraging:

"Very few applications should use this permission; these windows are intended for system-level interaction with the user."

Comment 24

[Martyn Haigh (:mhaigh)]

mcomella - I'm tracking your comment in bug 1155291.  I'll investigate if we can display the toast with the existing functionality without the permission bump using the method you describe.

gcp - the new permission bump is needed so that we can show an interactive view over whatever the user is currently doing, whilst still allowing the user to interact with the background application.  Without this permission we can show our view but it essentially becomes a modal dialogue in functionality as it prevents the user from interacting with the app in the background.

Comment 25

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #23)
> This requires the additional permission "allow Firefox to draw over other
> apps". I'm seeing some confusion over why Firefox requires that permissions,
> and the Android docs aren't encouraging:
> 
> "Very few applications should use this permission; these windows are
> intended for system-level interaction with the user."

Yeah, I saw this too. Thanks for bringing it up GCP!

Though the guidelines are clear about where Google stands on this, I think that the exception can and should be made in this case. 

For us, this keeps most consistent with existing Firefox UI and we chose to create a feature that would require breaking these "conventions". Since, the value of this features lies in this "cross-application" experience, I think we'll need to be breaking some guidelines (that currently don't give us much options for our idea) but we should definitely be aware of these things.

Comment 26

[Teodora Vermesan (:TeoVermesan)]

After a user clicks a link outside Firefox will display "Tab queued in Nightly | Open now" toast notification. If you wait a few seconds will display "Nightly | 1 tab waiting" notification in the android notification bar or if you choose immediately "Open now", will open the link in Firefox
Verified as fixed using:
Device: Alcatel One Touch (Android 4.1.2)
Build: Firefox for Android 40.0a1 (2015-04-28)