905262 - [fig] Create a JS add-on API for adding content to the promotional banner

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

We'll need bug 862801 to land before starting work on this, but here's the project wiki page:
https://wiki.mozilla.org/Mobile/Projects/About:home_-_Add-ons_can_add_content_to_the_%22promotional_banner%22_tile

Comment 1

[:Margaret Leibovic]

Attachment: WIP

This is based on top of the patch for bug 862801, and very closely resembles the patch for bug 848242.

A few things:

* This API just lets you set text (based mainly on the fact that the patch for bug 862801 just adds a TextView), but we should expand this to let you add an image as well (if we do this with compound drawables this can still be a TextView).

* We'll need to hide the banner until we get the "HomeSnippet:Data" event, since it won't have content before then. Maybe we can animate it up from the bottom to make this delay seem more intentional.

* I decided to make JS in charge of deciding which message to show in the banner, but we still need to update this to decide how to rotate through the messages.

* Naming bikeshed opportunity: at first I wanted to call this HomeSnippets and create individual Snippet objects, but it bothered me that HomeSnippet.java was a singular snippet. I actually feel that that should probably be renamed to something more semantic, like HomeBanner, so maybe I can go back to calling my object HomeSnippets.

Comment 2

[Sriram Ramasubramanian [:sriram]]

(In reply to :Margaret Leibovic from comment #1)
> Created attachment 792522 [details] [diff] [review]
> WIP
> 
> This is based on top of the patch for bug 862801, and very closely resembles
> the patch for bug 848242.
> 
> A few things:
> 
> * This API just lets you set text (based mainly on the fact that the patch
> for bug 862801 just adds a TextView), but we should expand this to let you
> add an image as well (if we do this with compound drawables this can still
> be a TextView).

Currently I'm not using Compound drawables as the size of images returned by the addons might not be same. Hence I'm using an ImageView that centers the image inside it.

> 
> * Naming bikeshed opportunity: at first I wanted to call this HomeSnippets
> and create individual Snippet objects, but it bothered me that
> HomeSnippet.java was a singular snippet. I actually feel that that should
> probably be renamed to something more semantic, like HomeBanner, so maybe I
> can go back to calling my object HomeSnippets.

I'm not really sure on the difference in the names. I am fine with renaming them. Though I don't know if you would have a HomeSnippet(s).java file.

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 792522 [details] [diff] [review]
WIP

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

Seems fine to me.

::: mobile/android/base/home/HomeSnippet.java
@@ +43,5 @@
> +
> +        setOnClickListener(new View.OnClickListener() {
> +            @Override
> +            public void onClick(View v) {
> +                GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("HomeSnippet:Click", null));

I wonder if its worth sending relative coordinates or something here... If we made it possible to use a large image, maybe?

@@ +53,5 @@
> +    public void onAttachedToWindow() {
> +        super.onAttachedToWindow();
> +
> +        GeckoAppShell.getEventDispatcher().registerEventListener("HomeSnippet:Data", this);
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("HomeSnippet:Get", null));

Does this fire often enough to actually update this from time to time?

@@ +107,5 @@
> +            final String text = message.getString("text");
> +            ThreadUtils.postToUiThread(new Runnable() {
> +                @Override
> +                public void run() {
> +                    setText(text);

We really should try to use the HTML Spannable thing here. Also, wrapping so much in a try-catch always makes me cringe.

::: mobile/android/chrome/content/browser.js
@@ +391,5 @@
>  #endif
> +
> +    Cu.import("resource://gre/modules/HomeSnippet.jsm");
> +    HomeSnippet.add({
> +      text: "Margaret is the best!",

Yeah, I think we'll want some sort of icon

::: mobile/android/modules/HomeSnippet.jsm
@@ +33,5 @@
> +  if ("onclick" in options && typeof options.onclick === "function")
> +    this.onclick = options.onclick;
> +}
> +
> +this.HomeSnippet = {

Hmm.. I wonder if we'll want to just create a Home object to wrap all the about:home apis in eventually. i.e.

Home.snippet/banner.add();
Home.panels.add();
Home.widget.add();

@@ +63,5 @@
> +      this._currentMessage = message;
> +
> +      sendMessageToJava({
> +        type: "HomeSnippet:Data",
> +        text: message.text || ""

I don't think we'd want to send this with "";

Comment 4

[:Margaret Leibovic]

This should probably track 26 because bug 862801 doesn't replace the about:home snippet on its own.

Comment 5

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

Comment on attachment 792522 [details] [diff] [review]
WIP

Wes covered all the high points. One thing I failed to realize, but now remember, is that this system will be a rotating banner system. Just adding your banner to the HomeView (or whatever) object does not immediately show the banner. It is added to a pool of potential banners, and the system will show one per startup (or some other period).

Comment 6

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #3)
::: mobile/android/base/home/HomeSnippet.java
> @@ +43,5 @@
> > +
> > +        setOnClickListener(new View.OnClickListener() {
> > +            @Override
> > +            public void onClick(View v) {
> > +                GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("HomeSnippet:Click", null));
> 
> I wonder if its worth sending relative coordinates or something here... If
> we made it possible to use a large image, maybe?

I've thought about this, but I think that can happen in a follow-up if we want it.

> @@ +53,5 @@
> > +    public void onAttachedToWindow() {
> > +        super.onAttachedToWindow();
> > +
> > +        GeckoAppShell.getEventDispatcher().registerEventListener("HomeSnippet:Data", this);
> > +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("HomeSnippet:Get", null));
> 
> Does this fire often enough to actually update this from time to time?

It fires every time we show about:home (including switching tabs). I think this is appropriately often to change the banner message.

> @@ +107,5 @@
> > +            final String text = message.getString("text");
> > +            ThreadUtils.postToUiThread(new Runnable() {
> > +                @Override
> > +                public void run() {
> > +                    setText(text);
> 
> We really should try to use the HTML Spannable thing here.

What kind of markup do you imagine an add-on wanting to put in here? Just links? There's really not that much room, and the user can't zoom this, so it might be hard to hit different targets.

Comment 7

[:Margaret Leibovic]

Attachment: WIP v2

I still need to deal with the banner rotation, but here's another WIP that add icon support and incorporates wesj's Home API idea.

Comment 8

[:Margaret Leibovic]

Oops, I accidentally cleared the tracking flag.

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 799150 [details] [diff] [review]
WIP v2

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

Seems great. Mostly some nit comments.

::: mobile/android/base/GeckoAppShell.java
@@ +2191,5 @@
>      public static void registerEventListener(String event, GeckoEventListener listener) {
>          sEventDispatcher.registerEventListener(event, listener);
>      }
>  
> +    public static EventDispatcher getEventDispatcher() {

Not your bug, but we should really move singleton's like this into their respective class...

::: mobile/android/base/home/HomeBanner.java
@@ +14,3 @@
>  
> +import org.json.JSONException;
> +import org.json.JSONObject;

You wound up not using these, right?

@@ +14,4 @@
>  
> +import org.json.JSONException;
> +import org.json.JSONObject;
> + 

ws

@@ +28,2 @@
>  
> +public class HomeBanner extends LinearLayout 

ws

@@ +81,5 @@
> +
> +    @Override
> +    public void handleMessage(String event, JSONObject message) {
> +        // Don't show a banner if there's no text.
> +        final String text = message.optString("text");

I tend to use message.getString("text"); if its not an optional thing, which will throw. But I kinda hate wrapping things in try-catch blocks. Maybe this is cleaner and catches the case where the caller passes up a real, but empty string.

@@ +86,5 @@
> +        if (TextUtils.isEmpty(text)) {
> +            return;
> +        }
> +
> +        final TextView textView = (TextView) findViewById(R.id.text);

Unless its going to live a different lifecycle than homeBanner, you might just cache this view in the banner. Then again, doing it this way means you don't have to worry about life cycle.

@@ +106,5 @@
> +            @Override
> +            public void onBitmapFound(final Drawable d) {
> +                ThreadUtils.postToUiThread(new Runnable() {
> +                    @Override
> +                    public void run() {

Add a null check for d in here (or outside the runnable). Unlike the example mfinkle sent out yesterday, getDrawable will call the callback with a null result if it doesn't find anything. It takes a callback because some icons require some async file i/o.

::: mobile/android/modules/Home.jsm
@@ +18,5 @@
> +function sendMessageToJava(message) {
> +  return Services.androidBridge.handleGeckoMessage(JSON.stringify(message));
> +}
> +
> +// XXX: Should this go into some shared place?

Yes!

@@ +78,5 @@
> +
> +    sendMessageToJava({
> +      type: "HomeBanner:Data",
> +      text: message.text,
> +      iconURI: message.iconURI

Even though only one of these shows at a time, it still seems like it would be good to pass the id to java and have it pass it back when its clicked, rather than relying on this component to stay in sync with Java. It matches what we do everywhere else (but everywhere else we show multiple addon items at a time...). Also, it just feels more resilient to pass it up. i.e. Have the banner always tell us exactly what was clicked.

@@ +96,5 @@
> +
> +  remove: function(id) {
> +    delete this._messages[id];
> +
> +    if (!Object.keys(this._messages).length) {

Why the ! here instead of == 0? Feels kinda weird to read this way.

Comment 10

[:Margaret Leibovic]

Attachment: WIP v2 (minus my testing hacks)

I made a test add-on to start playing around with this API in a more realistic setting:
https://github.com/leibovic/home-demo

If you have this patch applied you can try it here:
https://github.com/leibovic/home-demo/releases/tag/0.1

Now I'll work on actually addressing the comments to make the patch good :)

Comment 11

[:Margaret Leibovic]

Attachment: patch

I think this addresses all your concerns. It's working well! I made an add-on with multiple banner messages, and it does rotate through them properly every time you re-open the home page.

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 803398 [details] [diff] [review]
patch

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

::: mobile/android/base/home/HomeBanner.java
@@ +61,5 @@
> +
> +        setOnClickListener(new View.OnClickListener() {
> +            @Override
> +            public void onClick(View v) {
> +                GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("HomeBanner:Click", null));

Need to pass the id.

@@ +83,5 @@
> +
> +    @Override
> +    public void handleMessage(String event, JSONObject message) {
> +        try {
> +            // Store the current message id to pass back to JS in the view's OnClickListener.

I don't think you're actually doing this, are you?

@@ +98,5 @@
> +                    setVisibility(View.VISIBLE);
> +                }
> +            });
> +        } catch (JSONException e) {
> +            Log.e(LOGTAG, "Exception handling \"HomeBanner:Data\" message", e);

You might as well use the event parameter here so that this message is more dynamic.

@@ +104,5 @@
> +        }
> +
> +        final String iconURI = message.optString("iconURI");
> +        if (TextUtils.isEmpty(iconURI)) {
> +            return;

Do we need to hide the imageView here so that it doesn't take up space?

@@ +113,5 @@
> +            @Override
> +            public void onBitmapFound(final Drawable d) {
> +                // Bail if getDrawable doesn't find anything.
> +                if (d == null) {
> +                    return;

Same here? Should we hide the view?

::: mobile/android/modules/Home.jsm
@@ +45,5 @@
> +}
> +
> +let HomeBanner = {
> +  // Holds the messages that will rotate through the banner.
> +  _messages: {},

Do we need two lists? Can we get rid of this one?

Comment 13

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #12)
> Comment on attachment 803398 [details] [diff] [review]
> patch
> 
> Review of attachment 803398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomeBanner.java
> @@ +61,5 @@
> > +
> > +        setOnClickListener(new View.OnClickListener() {
> > +            @Override
> > +            public void onClick(View v) {
> > +                GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("HomeBanner:Click", null));
> 
> Need to pass the id.

Oops, good catch. I guess I didn't test out the clicking in my new version to find this didn't work :/


> @@ +104,5 @@
> > +        }
> > +
> > +        final String iconURI = message.optString("iconURI");
> > +        if (TextUtils.isEmpty(iconURI)) {
> > +            return;
> 
> Do we need to hide the imageView here so that it doesn't take up space?

Good question! I had the same one, but I was only thinking about an image from a previous message showing up (that doesn't happen because the lifecycle of these views is so short that they only hang around while one is shown). I think hiding the imageView is a good call for the visual design, so I'll do that (we won't need to worry about un-hiding it, since we only go through here once per view lifetime).

> @@ +113,5 @@
> > +            @Override
> > +            public void onBitmapFound(final Drawable d) {
> > +                // Bail if getDrawable doesn't find anything.
> > +                if (d == null) {
> > +                    return;
> 
> Same here? Should we hide the view?

Yeah, I'll do that.

> ::: mobile/android/modules/Home.jsm
> @@ +45,5 @@
> > +}
> > +
> > +let HomeBanner = {
> > +  // Holds the messages that will rotate through the banner.
> > +  _messages: {},
> 
> Do we need two lists? Can we get rid of this one?

Do you have any good suggestions about how to do that? We need some sort of mapping from id to message to keep track of the messages, and using this object is good for that, but an object does not have the nice list-like properties that help us keep track of which message to show.

Since _messages holds the actual messages, and _queue only holds the ids, we'd need to do something differently to keep the same functionality with one data structure. Unless we can come up with something simple, I don't think it's worth it, since there really should only ever be a handful of messages in rotation here.

Actually... this makes me think of something else. If you have an add-on like "wikipedia fun facts", the add-on author could be nice and take care of adding and removing items a handful a time, but could also alternatively add a ton of messages all at once, which might hurt performance and could also drown out any other banner messages, e.g. a dynamic snippet. I don't know that we can technically prevent this, but maybe we need some best practices advice added to whatever documentation we write for this API. (Add-on reviewers would be able to see if an add-on is doing this or not.)

Comment 14

[:Margaret Leibovic]

Attachment: patch v2

Addressed comments.

I turned the right margin on the ImageView into a left margin on the TextView, so that there's space between the text and the edge of the screen if the image is hidden.

I wonder that if instead of hiding the ImageView, we should have a default icon, similar to menu items. But we can talk to ibarlow and do that in a follow-up bug if that's what we decide we want.

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 804777 [details] [diff] [review]
patch v2

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

::: mobile/android/base/home/HomeBanner.java
@@ +108,5 @@
> +        final ImageView iconView = (ImageView) findViewById(R.id.icon);
> +
> +        if (TextUtils.isEmpty(iconURI)) {
> +            // Hide the image view if we don't have an icon to show.
> +            iconView.setVisibility(View.GONE);

Make sure this shows itself if we're moving from a view without an icon to one with.

@@ +117,5 @@
> +            @Override
> +            public void onBitmapFound(final Drawable d) {
> +                // Bail if getDrawable doesn't find anything.
> +                if (d == null) {
> +                    iconView.setVisibility(View.GONE);

Same here

::: mobile/android/modules/Home.jsm
@@ +79,5 @@
> +  },
> +
> +  _handleClick: function(id) {
> +    let message = this._messages[id];
> +    if (message.onclick)

This will fail if the banner is removed while its showing. Maybe thats ok...

Comment 16

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #15)
> Comment on attachment 804777 [details] [diff] [review]
> patch v2
> 
> Review of attachment 804777 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomeBanner.java
> @@ +108,5 @@
> > +        final ImageView iconView = (ImageView) findViewById(R.id.icon);
> > +
> > +        if (TextUtils.isEmpty(iconURI)) {
> > +            // Hide the image view if we don't have an icon to show.
> > +            iconView.setVisibility(View.GONE);
> 
> Make sure this shows itself if we're moving from a view without an icon to
> one with.

This works because we create a new view every time this banner is shown, so we only ever go through here once.

> ::: mobile/android/modules/Home.jsm
> @@ +79,5 @@
> > +  },
> > +
> > +  _handleClick: function(id) {
> > +    let message = this._messages[id];
> > +    if (message.onclick)
> 
> This will fail if the banner is removed while its showing. Maybe thats ok...

Yeah, I think that's okay. If someone really wants a click on their message to be handled, they shouldn't remove the message.

This points out that once a message is shown, it won't be dynamically un-shown if it's removed. But I also think that's okay.

Comment 17

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/5040ab6bfbc5

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/5040ab6bfbc5

Comment 19

[:Margaret Leibovic]

Comment on attachment 804777 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): about:home re-write (bug 862793)
User impact if declined: we won't be able to show users promotional content on about:home (e.g. the current "set up sync" banner on beta/release)
Testing completed (on m-c, etc.): landed on m-c 9/18
Risk to taking this patch (and alternatives if risky): low-risk, nothing happens unless an add-on is installed that uses this API 
String or IDL/UUID changes made by this patch: none

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 804777 [details] [diff] [review]
patch v2

Margaret, do you have a test addon that QA could use to verify this?  Would be great to ensure there's some testing before this gets to Beta and people do potentially start to code to it.

Comment 21

[:Margaret Leibovic]

(In reply to [e-mail] [:lsblakk] from comment #20)
> Comment on attachment 804777 [details] [diff] [review]
> patch v2
> 
> Margaret, do you have a test addon that QA could use to verify this?  Would
> be great to ensure there's some testing before this gets to Beta and people
> do potentially start to code to it.

In fact I have 2 of them! The first one here is simpler than the second.

https://github.com/leibovic/home-demo/releases/tag/0.1
https://github.com/leibovic/promo-banner/releases/tag/0.1

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Kevin, not sure which of you or Aaron is point for 26 so starting with you, adjust if needed

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8739bac221a5

Comment 24

[:Margaret Leibovic]

Adding dev-doc-needed to remind myself to make an MDN page for this. Will aim to do that this week.

Comment 25

[:Margaret Leibovic]

Documented here:
https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/Home.jsm

Comment 27

[Catalin Suciu [:csuciu]]

Verifying as fixed