Closed Bug 974723 Opened 6 years ago Closed 6 years ago

Send shown event when a banner message is displayed

Categories

(Firefox for Android :: Awesomescreen, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed
fennec 29+ ---

People

(Reporter: jdover, Assigned: jdover)

References

Details

Attachments

(2 files, 4 obsolete files)

Our JS code assumes that a banner message is shown when it has been requested, which is not necessarily true. Java should send a message back to Home.jsm when the banner has actually been shown to the user.
Blocks: home-banner
Attachment #8379206 - Flags: review?(margaret.leibovic)
Comment on attachment 8379206 [details] [diff] [review]
Send shown event when banner message is displayed

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

::: mobile/android/base/home/HomeBanner.java
@@ +191,5 @@
>          final PropertyAnimator animator = new PropertyAnimator(100);
>          animator.attach(this, Property.TRANSLATION_Y, 0);
>          animator.start();
> +
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("HomeBanner:Shown", (String) getTag()));

I don't think this is the right place to do this, since this will get called as the user scrolls up and down. A better place would be where we set the text and then call animateUp.

::: mobile/android/modules/Home.jsm
@@ +73,3 @@
>  
> +  let _handleShown = function(id) {
> +    dump("Calling onshown for " + id);

Remove this dump statement before landing.

@@ +126,5 @@
>        // If this is the first message we're adding, add
>        // observers to listen for requests from the Java UI.
>        if (Object.keys(_messages).length == 1) {
>          Services.obs.addObserver(this, "HomeBanner:Get", false);
> +        Services.obs.addObserver(this, "HomeBanner:Shown", false);

Make sure to remove the observer down below as well.
Attachment #8379206 - Flags: review?(margaret.leibovic) → review-
Not the end of the world if this doesn't make 29, but snippets metrics won't be exactly what we want if that's the case (just have to make sure people looking at dashboards understand that).
tracking-fennec: --- → 29+
Attached patch patch (obsolete) — Splinter Review
Based on your patch in bug 977155
Attachment #8379206 - Attachment is obsolete: true
Attachment #8383245 - Flags: review?(margaret.leibovic)
Status: NEW → ASSIGNED
Comment on attachment 8383245 [details] [diff] [review]
patch

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

This is looking good, but I think there's a better solution.

::: mobile/android/base/home/HomeBanner.java
@@ +145,5 @@
>                      // Show the banner if it is currently enabled.
>                      if (mEnabled) {
>                          setVisibility(VISIBLE);
>                          animateUp();
> +                        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("HomeBanner:Shown", (String) getTag()));

We should also add a call down below, now that we actually set the banner to visible in two places. Although we would need to do something to make sure we don't send this message every time the banner is re-enabled.

Maybe instead of always setting the visibility to VISIBLE in setEnabled, we should have a check to see if it's already visible, and we should only set the visibility if it's not already visible. Then we can add something in setVisibility to send this message when the visibility is set to VISIBLE.

What do you think of that?

::: mobile/android/modules/Home.jsm
@@ +73,3 @@
>  
> +  let _handleShown = function(id) {
> +    dump("Calling onshown for " + id);

Make sure you get rid of this dump statement when you post a patch for checkin.
Attachment #8383245 - Flags: review?(margaret.leibovic) → feedback+
Attached patch patch (obsolete) — Splinter Review
(In reply to :Margaret Leibovic from comment #5)
> Comment on attachment 8383245 [details] [diff] [review]
> patch
> 
> Review of attachment 8383245 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking good, but I think there's a better solution.
> 
> ::: mobile/android/base/home/HomeBanner.java
> @@ +145,5 @@
> >                      // Show the banner if it is currently enabled.
> >                      if (mEnabled) {
> >                          setVisibility(VISIBLE);
> >                          animateUp();
> > +                        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("HomeBanner:Shown", (String) getTag()));
> 
> We should also add a call down below, now that we actually set the banner to
> visible in two places. Although we would need to do something to make sure
> we don't send this message every time the banner is re-enabled.
> 
> Maybe instead of always setting the visibility to VISIBLE in setEnabled, we
> should have a check to see if it's already visible, and we should only set
> the visibility if it's not already visible. Then we can add something in
> setVisibility to send this message when the visibility is set to VISIBLE.
> 
> What do you think of that?

I made a performShow() method that does that visibility checking and only calls 'shown' if the banner hasn't been shown yet. I think this should prevent any duplicate calls.
Attachment #8383245 - Attachment is obsolete: true
Attachment #8383358 - Flags: review?(margaret.leibovic)
Comment on attachment 8383358 [details] [diff] [review]
patch

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

Nice.

::: mobile/android/base/home/HomeBanner.java
@@ +183,5 @@
>              animateDown();
>          }
>      }
>  
> +    private void performShow() {

I think you can just call this show().
Attachment #8383358 - Flags: review?(margaret.leibovic) → review+
Attached patch patch (obsolete) — Splinter Review
Attachment #8383358 - Attachment is obsolete: true
Attachment #8383947 - Flags: review+
Keywords: checkin-needed
Ee'll need to update this to rebase on top of the new patch in bug 977155. Luckily, I think that will actually make the patch simpler, and we can get rid of the performShow() method.

It won't be a perfect solution, but I think we can just send this message when we set the banner to visible, and not worry about the case where we set it to visible when the user isn't on the default panel.
Keywords: checkin-needed
Attached patch patchSplinter Review
Attachment #8383947 - Attachment is obsolete: true
Attachment #8384849 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/9a0daca21f30
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9a0daca21f30
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Comment on attachment 8384849 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 960359
User impact if declined: we won't be able to collect metrics for how often snippets are seen
Testing completed (on m-c, etc.): landed on m-c 3/3
Risk to taking this patch (and alternatives if risky): low-risk, adds back existing functionality that was removed in patch for bug 960359
String or IDL/UUID changes made by this patch: none
Attachment #8384849 - Flags: approval-mozilla-aurora?
Attachment #8384849 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs some love for Aurora.
Flags: needinfo?(jdover)
Attached patch patch auroraSplinter Review
Attachment #8387868 - Flags: review+
Flags: needinfo?(jdover)
You need to log in before you can comment on or make changes to this bug.