Closed Bug 884075 Opened 6 years ago Closed 6 years ago

JS API for super toasts

Categories

(Firefox for Android :: General, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 25

People

(Reporter: wesj, Assigned: wesj)

References

Details

Attachments

(6 files, 11 obsolete files)

5.55 KB, patch
wesj
: review+
Details | Diff | Splinter Review
6.48 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
6.88 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
4.66 KB, patch
Details | Diff | Splinter Review
14.74 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
1.06 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
I started this in bug 872388, but the latest patches have turned into some refactoring too. Moving it here to have a clean slate.
Attached patch Patch (obsolete) — Splinter Review
When I originally took super toast, I started out with Roman Nurik's stuff from here:

http://code.google.com/p/romannurik-code/source/browse/misc/undobar/

It took a listener in the constructor for the undo bar. Turns out, I think that was kinda stupid for us. Here I'm just gonna pass a listener when you show a toast. That helps fix a problem bnicholson had in the other patch where mToast was owned by GeckoApp (because Toast:Show messages wind up there), but needed its listener registered in BrowserApp (for the bookmarks stuff).
Attachment #763827 - Flags: review?(margaret.leibovic)
Attached patch Patch 2/5 (obsolete) — Splinter Review
As part of this, addons need to be able to pass an icon to the dialog. This moves some code used for adding icons to menu items to BitmapUtils to make that easier.
Attachment #763828 - Flags: review?(margaret.leibovic)
This makes us use drawables in super toasts instead of just taking a resource id.
Attachment #763830 - Flags: review?(margaret.leibovic)
Attached patch Patch 4/5 (obsolete) — Splinter Review
This adds a reason to why we're hiding a super toast. This is needed as a way to tell the js api when we're hiding a toast, but the user didn't click on it. 

Stolen from a talk by some Valve guys, who use a reason to descrbie why their zombie AI does whatever its doing in Left4Dead.
Attachment #763832 - Flags: review?(margaret.leibovic)
Attached patch Patch 5/5 - jsAPI (obsolete) — Splinter Review
Finally, this adds the actual API.
Attachment #763833 - Flags: review?(margaret.leibovic)
Comment on attachment 763827 [details] [diff] [review]
Patch

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

::: mobile/android/base/widget/ButtonToast.java
@@ +51,5 @@
> +    public enum ReasonHidden {
> +        CLICKED,
> +        TIMEOUT,
> +        STARTUP
> +    }

I don't see you using this anywhere in this patch... but I do see it used in the later patches. It would be nice to move this to the patch where it's introduced, but NBD :)

@@ +57,2 @@
>      // State objects
>      private static class Toast {

This isn't in m-c. Which patches to you have applied before this that haven't landed yet? I'm having a tough time following what exactly landed already in bug 872388.
Attachment #763827 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 763828 [details] [diff] [review]
Patch 2/5

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

I have a few questions on this one...

::: mobile/android/base/BrowserApp.java
@@ +1381,5 @@
> +            BitmapUtils.getDrawable(this, info.icon, new BitmapUtils.BitmapLoader() {
> +                @Override
> +                public void onBitmapFound(Drawable d) {
> +                    if (d == null) {
> +                        item.setIcon(R.drawable.ic_menu_addons_filler);

Maybe it could be a good follow-up to rename this drawable to something more generic. Same idea as those menu arrow resources :)

::: mobile/android/base/gfx/BitmapUtils.java
@@ +34,5 @@
>  
>      private BitmapUtils() {}
>  
> +    public interface BitmapLoader {
> +      public void onBitmapFound(Drawable d);

Nit: 4-space indentation.

@@ +38,5 @@
> +      public void onBitmapFound(Drawable d);
> +    }
> +
> +    public static void getDrawable(final Context context, final String data, final BitmapLoader loader) {
> +        if (TextUtils.isEmpty(data)) {

Does .startsWith throw on empty strings? If not, you can get rid of this if statement and just let the default case handle this down below.

@@ +45,5 @@
> +        }
> +
> +        if (data.startsWith("data")) {
> +            BitmapDrawable d = new BitmapDrawable(getBitmapFromDataURI(data));
> +            loader.onBitmapFound(d);

How about just returning instead of using else statements? I feel like I usually prefer that, and it would be more consistent with the return up above.

@@ +48,5 @@
> +            BitmapDrawable d = new BitmapDrawable(getBitmapFromDataURI(data));
> +            loader.onBitmapFound(d);
> +
> +        } else if (data.startsWith("jar:") || data.startsWith("file://")) {
> +            (new AsyncTask<Void, Void, Drawable>() {

I can never remember our policy about when to use AsyncTask vs other flavors of AsyncTask. Have you verified this is the right one to use here?

@@ +53,5 @@
> +                @Override
> +                public Drawable doInBackground(Void... params) {
> +                    try {
> +                        if (data.startsWith("jar:jar")) {
> +                            return GeckoJarReader.getBitmapDrawable(context.getResources(), data);

This isn't in the original code you're moving over. Why the change?
Comment on attachment 763830 [details] [diff] [review]
Patch 3/5 - Use drawables in toasts

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

::: mobile/android/base/widget/ButtonToast.java
@@ +103,5 @@
>      }
>  
> +    public void show(final boolean immediate, final CharSequence message,
> +                     final CharSequence buttonMessage, final int buttonDrawableId,
> +                     final ToastListener listener) {

I don't love these subtly different method signatures. How do overloaded methods work if the one different parameter is passed in as null? I would prefer if we just the show signature to take a drawable (what you made into showInternal), then let the callers be responsible for getting the drawable to pass in.
Apologies these are all based on top of the patch in bug 880454.
Comment on attachment 763832 [details] [diff] [review]
Patch 4/5

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

Neat, this looks like a cool API. Just one question about whether or not we need a null check.

::: mobile/android/base/widget/ButtonToast.java
@@ +166,5 @@
>              mHideHandler.postDelayed(mHideRunnable, TOAST_DURATION);
>              return;
>          }
>  
> +        if (mCurrentToast != null && mCurrentToast.listener != null) {

I haven't looked too closely at the patch in bug 880454, but why would mCurrentToast ever be null here? I don't think we should add null checks in cases where we would never expect something to be null, to avoid hiding real bugs.
Attachment #763832 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 763833 [details] [diff] [review]
Patch 5/5 - jsAPI

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

::: mobile/android/base/GeckoApp.java
@@ +545,5 @@
>                  final String duration = message.getString("duration");
> +                final String buttonText = message.optString("button");
> +                final String buttonIcon = message.optString("buttonIcon");
> +                final String buttonId = message.optString("buttonId");
> +                handleShowToast(msg, duration, buttonText, buttonIcon, buttonId);

Does this even need to be in GeckoApp? Do we expect generic gecko things to use the NativeWindow API? It feels like that API was developed to let devs interact with browser chrome, so it seems like all that should be tied directly to BrowserApp.

Although, I do see it's used in SelectionHandler.js, which is a content-based thing, so maybe it does need to be here. It's hard to tease apart what's browser chrome from what's just a web view!

@@ +811,5 @@
>          ThreadUtils.postToUiThread(new Runnable() {
>              @Override
>              public void run() {
> +                if (!TextUtils.isEmpty(buttonId) &&
> +                   (!TextUtils.isEmpty(buttonText) || !TextUtils.isEmpty(buttonIcon))) {

I think it might be clearer to just handle this logic directly in handleMessage, then make a separate showButtonToast method for that case (and I think it would be nice to rename this current method showToast, since that's really what it's doing, not "handling" something).

@@ +837,5 @@
>                  Toast toast;
>                  if (duration.equals("long"))
>                      toast = Toast.makeText(sAppContext, message, Toast.LENGTH_LONG);
>                  else
>                      toast = Toast.makeText(sAppContext, message, Toast.LENGTH_SHORT);

While you're in here, you could add braces on these lines to match our Java style :)

::: mobile/android/chrome/content/browser.js
@@ +201,5 @@
>                                    "resource://gre/modules/Geometry.jsm");
>  
>  function resolveGeckoURI(aURI) {
> +  if (!aURI)
> +    return aURI;

This will just return null or undefined or "" or whatever falsey value you passed in, is that really what we want? I think we should be more explicitly returning null (or "") if this is going in a message to Java.

@@ +1550,5 @@
>    },
>  
>    toast: {
> +    _callbacks: {},
> +    show: function(aMessage, aDuration, aOptions) {

Nice use of an options object.
Attached patch Patch 2/5 (obsolete) — Splinter Review
(In reply to :Margaret Leibovic from comment #7)
> Maybe it could be a good follow-up to rename this drawable to something more
> generic. Same idea as those menu arrow resources :)

Heh. Will do.

> Does .startsWith throw on empty strings? If not, you can get rid of this if
> statement and just let the default case handle this down below.

This also catches null strings. I think its probably good to handle them if we can.

> How about just returning instead of using else statements? I feel like I

done.

> I can never remember our policy about when to use AsyncTask vs other flavors
> of AsyncTask. Have you verified this is the right one to use here?

I flipped this to UIAsyncTask and to use our background thread. I think that's right. If you're requesting a Bitmap, it seems reasonable that you want to use it in the UI.

> This isn't in the original code you're moving over. Why the change?

The original code pretended to handle jar:jars's, but actually doesn't. This fixes it. Just one jar: url's should be supported by the Android url class though.
Attachment #763828 - Attachment is obsolete: true
Attachment #763828 - Flags: review?(margaret.leibovic)
Comment on attachment 763830 [details] [diff] [review]
Patch 3/5 - Use drawables in toasts

I should have marked this as an r- when I left a comment before. I'll wait for an answer to my questions and maybe a new patch.
Attachment #763830 - Flags: review?(margaret.leibovic) → review-
Attachment #763833 - Flags: review?(margaret.leibovic) → review-
Assignee: nobody → wjohnston
Blocks: 887402
No longer blocks: 887402
Attached patch Patch 1/5Splinter Review
Attachment #763827 - Attachment is obsolete: true
Attachment #769815 - Flags: review+
Attached patch Patch 2/5 (obsolete) — Splinter Review
Attachment #765659 - Attachment is obsolete: true
Attachment #769816 - Flags: review?(margaret.leibovic)
Attached patch Patch 3/5 (obsolete) — Splinter Review
Attachment #763830 - Attachment is obsolete: true
Attachment #769817 - Flags: review?(margaret.leibovic)
Attached patch Patch 5/5 (obsolete) — Splinter Review
Attachment #763833 - Attachment is obsolete: true
Attachment #769873 - Flags: review?(margaret.leibovic)
Attached patch Patch 4/5 (obsolete) — Splinter Review
I'll not reset r+ since you hate that.
Attachment #763832 - Attachment is obsolete: true
Comment on attachment 769816 [details] [diff] [review]
Patch 2/5

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

This mostly looks good, but I want to see a new patch where we don't call loader.onBitmapFound(null) when we shouldn't.

::: mobile/android/base/gfx/BitmapUtils.java
@@ +37,5 @@
> +    public interface BitmapLoader {
> +        public void onBitmapFound(Drawable d);
> +    }
> +
> +    public static void getDrawable(final Context context, final String data, final BitmapLoader loader) {

Nit: Let's take this opportunity to add some documentation about what this does :)

@@ +78,5 @@
> +                }
> +            }).execute();
> +        }
> +
> +        loader.onBitmapFound(null);

This is going to fire even if you kick off the UiAsyncTask up above. Maybe a better way to do this would be to add a case up above to check if data doesn't start with "jar:" or "file://", and call loader.onBitmapFound(null) there then return early. Then this UiAsyncTask business could just be the default at the end of the method.
Attachment #769816 - Flags: review?(margaret.leibovic) → review-
Comment on attachment 769817 [details] [diff] [review]
Patch 3/5

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

I like this a lot more than the last version, I'm just confused about some of the changes that ended up in here.

::: mobile/android/base/gfx/BitmapUtils.java
@@ +9,2 @@
>  import org.mozilla.gecko.util.ThreadUtils;
> +import org.mozilla.gecko.util.UiAsyncTask;

Did you mean to add these imports in the last patch?

::: mobile/android/base/widget/ButtonToast.java
@@ +60,3 @@
>              message = aMessage;
>              buttonMessage = aButtonMessage;
> +            buttonDrawable = aIcon;

OCD nit: Name this parameter aDrawable, to be consistent with buttonDrawable.

@@ +97,2 @@
>          show(t, immediate);
> +        return t;

I'm confused, why do you need to return the Toast here now?
Attachment #769817 - Flags: review?(margaret.leibovic) → review-
Comment on attachment 769873 [details] [diff] [review]
Patch 5/5

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

::: mobile/android/base/GeckoApp.java
@@ +542,5 @@
>      public void handleMessage(String event, JSONObject message) {
>          try {
>              if (event.equals("Toast:Show")) {
>                  final String msg = message.getString("message");
> +                final JSONObject button = message.optJSONObject("buttonId");

I think you meant "button" here, not "buttonId".

@@ +831,5 @@
> +                         final String buttonIcon, final String buttonId) {
> +        ThreadUtils.postToUiThread(new Runnable() {
> +            @Override
> +            public void run() {
> +                BitmapUtils.getDrawable(GeckoApp.this, buttonIcon, new BitmapUtils.BitmapLoader() {

BitmapUtils.getDrawable kicks off a UiAsyncTask in some cases, so the call to postToUiThread seems superfluous in that case. Maybe we can change BitmapUtils.getDrawable to just ensure onBitmapFound is always called on the UI thread? I'm not sure what's best here.

@@ +836,5 @@
> +                    public void onBitmapFound(Drawable d) {
> +                        mToast.show(false,
> +                            message,
> +                            buttonText,
> +                            d,

Nit: Can we just put these all on the same line?

@@ +842,5 @@
> +                                @Override
> +                                public void onButtonClicked() {
> +                                    GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Toast:Click", buttonId));
> +                                }
> +    

Whitespace.

@@ +854,5 @@
> +                        );
> +                    }
> +                });
> +            }
> +        });

Ugh, async callbacks look so gross in Java.

::: mobile/android/chrome/content/browser.js
@@ +1582,5 @@
> +
> +      if (aOptions.button) {
> +        msg.button = aOptions.button;
> +        if (aOptions.button.icon)
> +          msg.button.icon = resolveGeckoURI(msg.button.icon);

Nit: Add braces or a blank line after this to make it more readable. Also, add a comment about why we need to call resolveGeckoURI.

@@ +1585,5 @@
> +        if (aOptions.button.icon)
> +          msg.button.icon = resolveGeckoURI(msg.button.icon);
> +        msg.button.id = uuidgen.generateUUID().toString();
> +
> +        this._callbacks[msg.buttonId] = aOptions.button.callback;

this._callbacks[msg.button.id] right?

In general, this code feels a little unreadable. Maybe it would be worthwhile to add some local variables and construct msg.button from them.
Attachment #769873 - Flags: review?(margaret.leibovic) → review-
Attached patch Patch 2/5Splinter Review
This creates the BitmapUtils.getDrawable(url, listener) stuff, but forces the listener to always be called on the UIThread. Also adds some docs.
Attachment #769816 - Attachment is obsolete: true
Attachment #770510 - Flags: review?(margaret.leibovic)
Attached patch Patch 3/5Splinter Review
Use the new BitmapUtils stuff in ButtonToast. This also adds a little padding to make these look better.
Attachment #769817 - Attachment is obsolete: true
Attachment #770513 - Flags: review?(margaret.leibovic)
Attached patch Patch 4/5Splinter Review
Attachment #769883 - Attachment is obsolete: true
Attached patch Patch 5/5Splinter Review
I think this is a little more readable, while still being safe. What do you think?
Attachment #769873 - Attachment is obsolete: true
Attachment #770518 - Flags: review?(margaret.leibovic)
Attachment #770510 - Attachment is patch: true
Comment on attachment 770510 [details] [diff] [review]
Patch 2/5

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

Nice.
Attachment #770510 - Flags: review?(margaret.leibovic) → review+
Attachment #770513 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 770518 [details] [diff] [review]
Patch 5/5

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

Nice, I like this more. I think it's good to be explicit like this as much as possible, especially when passing around JS objects, so we know what's really going on in them.

::: mobile/android/base/resources/values/dimens.xml
@@ +75,5 @@
>      <dimen name="validation_message_height">50dp</dimen>
>      <dimen name="validation_message_margin_top">6dp</dimen>
>      <dimen name="forward_default_offset">-13dip</dimen>
>      <dimen name="addressbar_offset_left">32dp</dimen>
> +    <dimen name="toast_button_padding">8dp</dimen>

This seems to have appeared in both patches... but this will probably sort itself out when you go to land these :)
Attachment #770518 - Flags: review?(margaret.leibovic) → review+
Backed out for Android 2.2 robocop-2 orange:
{
27 INFO TEST-UNEXPECTED-FAIL | testMasterPassword | Exception caught - junit.framework.AssertionFailedError: 26 INFO TEST-UNEXPECTED-FAIL | testMasterPassword | Waiting for Incorrect password notification - The Incorrect password notification appears
}
https://tbpl.mozilla.org/php/getParsedLog.php?id=25320377&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/308a474b0ef9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/69d9290e58ee
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f4a2d2218a29
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a5cd81c55a7a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3b73e496a5c8
Attached patch Patch (obsolete) — Splinter Review
This is what I'm seeing as the error. Will push to try though (once its open).
Attachment #776480 - Flags: review?(margaret.leibovic)
Comment on attachment 776480 [details] [diff] [review]
Patch

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

::: mobile/android/chrome/content/browser.js
@@ +1583,5 @@
>          message: aMessage,
>          duration: aDuration
>        };
>  
>        if (aOptions.button) {

I feel like we would normally fix this issue by just doing |if (aOptions && aOptions.button)| here. Your solution here works if aOptions is undefined, but it won't work if a developer explicitly passes null, so it would be safer to do the aOptions check in the if statement, especially since this is an API we're encouraging add-on authors to use. "Be liberal in what you accept" or whatever that UNIX programming book says ;)
Attachment #776480 - Flags: review?(margaret.leibovic) → review-
Attached patch PatchSplinter Review
Still waiting for try on the other patch, but I pushed this one as well:

https://tbpl.mozilla.org/?tree=Try&rev=d812f15ae589
Attachment #776480 - Attachment is obsolete: true
Attachment #776784 - Flags: review?(margaret.leibovic)
Comment on attachment 776784 [details] [diff] [review]
Patch

Thanks.
Attachment #776784 - Flags: review?(margaret.leibovic) → review+
Depends on: 899577
You need to log in before you can comment on or make changes to this bug.