960359 - Home banner will never show up if user disables "Top Sites" panel

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[:Margaret Leibovic]

Right now the home banner is hard-coded into the top sites layout. So, if a user disables the top sites panel, they'll never see it :(

Maybe we should rework this so that the home banner always appears as part of the default panel.

This isn't an issue at the moment, but will be once bug 942875 lands.

Comment 1

[Deb Richardson [:dria] (plz NEEDINFO)]

I think we need to rework things so the home banner always appears as part of the default panel. We're going to be relying on the promotional banner for stuff, both internally and with partners.

Comment 2

[Josh Dover [:jdover]]

Attachment: Refactor HomeBanner out of TopSitesPanel, display on default page

Refactored the HomeBanner out of TopSitesPanel and made it an implementer of the Decor interface to be placed within the HomePager. Also generalized the Decor interface a bit and made the HomePager able to accept multiple Decors to pass event along to.

Comment 3

[Josh Dover [:jdover]]

Oh, and this patch should fix bug 961523 as well.

Comment 4

[:Margaret Leibovic]

Comment on attachment 8365266 [details] [diff] [review]
Refactor HomeBanner out of TopSitesPanel, display on default page

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

Cool, this is definitely on the right track. However, testing with this patch applied, I notice some flickering of the banner as I swipe between different panels, and sometimes the banner wouldn't appear when I swiped back to the default panel, so we need to figure out what's going wrong there. I also want to think if there's a cleaner approach we could take here, since re-purposing the Decor interface feels a bit forced.

::: mobile/android/base/home/HomeBanner.java
@@ +41,5 @@
> +    final TextView mTextView;
> +    final ImageView mIconView;
> +    final ImageButton mCloseButton;
> +
> +    private float mListTouchY = -1;

You should rename this variable, since there isn't necessarily a list behind the banner.

@@ +91,5 @@
> +            GeckoAppShell.getEventDispatcher().registerEventListener("HomeBanner:Data", this);
> +            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("HomeBanner:Get", null));
> +        } else {
> +            setVisibility(View.GONE);
> +            GeckoAppShell.getEventDispatcher().unregisterEventListener("HomeBanner:Data", this);

Why are you unregistering this listener here? This logic looks like it could be prone to race bugs, since this method would be called on the UI thread, but handleMessage is called on the gecko thread, so it's possible we could still receive a HomeBanner:Data message before we get the chance to unregister the listener.

It's probably more robust to add the listener when the view is created and remove it when the view is destroyed, but add a check where we update the UI after receiving a message to make sure we really should be showing the banner.

@@ +106,5 @@
>              // Store the current message id to pass back to JS in the view's OnClickListener.
>              setTag(message.getString("id"));
> +            setText(message.getString("text"));
> +            setIcon(message.optString("iconURI"));
> +            setVisibility(View.VISIBLE);

You need to call setVisibility on the UI thread, but handleMessage runs on the gecko thread.

::: mobile/android/base/home/HomePager.java
@@ +90,5 @@
>          public void onPageScrolled(int position, float positionOffset, int positionOffsetPixels);
> +        public void setVisibility(int visibility);
> +    }
> +
> +    interface TitleClickTarget {

Instead of re-purposing Decor and adding this separate TitleClickTarget interface, I think it might be simpler to just have two different interfaces for the two different types of "decoration" things we're adding (TabMenuStrip and HomeBanner). As it is in this patch, HomeBanner doesn't even make use of all of the methods in Decor.

@@ +139,2 @@
>  
> +        if (child instanceof TitleClickTarget) {

I know you're just continuing an existing pattern here, but I don't like how we're doing all these instanceof checks to figure out what to do with the child view.

@@ +148,2 @@
>  
> +        if (child instanceof HomePagerTabStrip || child instanceof TabMenuStrip) {

Why do we also need this check for TabMenuStrip now?

::: mobile/android/base/resources/layout-large-v11/home_pager.xml
@@ +7,5 @@
>       layout based on screen size -->
> +<FrameLayout xmlns:android="http://schemas.android.com/apk/res/android"
> +             android:layout_width="fill_parent"
> +             android:layout_height="fill_parent"
> +             android:orientation="vertical">

It's confusing that home_pager.xml doesn't have the HomePager view at its root now.

It would probably make more sense to just put the home banner in side home_pager_container, instead of creating another FrameLayout:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/gecko_app.xml#30

::: mobile/android/base/resources/layout/home_top_sites_panel.xml
@@ +7,5 @@
> +        android:id="@+id/list"
> +        style="@style/Widget.TopSitesListView"
> +        android:layout_width="fill_parent"
> +        android:layout_height="fill_parent"
> +        android:orientation="vertical" />

I don't think this orientation property does anything here, since HomeListView is a ListView.

Comment 5

[Josh Dover [:jdover]]

Attachment: Refactor HomeBanner out of TopSitesPanel, display on default page

(In reply to :Margaret Leibovic from comment #4)
> Cool, this is definitely on the right track. However, testing with this
> patch applied, I notice some flickering of the banner as I swipe between
> different panels, and sometimes the banner wouldn't appear when I swiped
> back to the default panel, so we need to figure out what's going wrong
> there. I also want to think if there's a cleaner approach we could take
> here, since re-purposing the Decor interface feels a bit forced.

Fixed the flickering and banner not showing up when you swipe back as well as
removed the usage of the Decor interface for controlling of the banner.

> 
> ::: mobile/android/base/home/HomePager.java
> @@ +90,5 @@
> >          public void onPageScrolled(int position, float positionOffset, int positionOffsetPixels);
> > +        public void setVisibility(int visibility);
> > +    }
> > +
> > +    interface TitleClickTarget {
> 
> Instead of re-purposing Decor and adding this separate TitleClickTarget
> interface, I think it might be simpler to just have two different interfaces
> for the two different types of "decoration" things we're adding
> (TabMenuStrip and HomeBanner). As it is in this patch, HomeBanner doesn't
> even make use of all of the methods in Decor.

Reverted many of the changes to the decor interface and instead setup the toolbar and 
banner as part of the show() method.

> @@ +139,2 @@
> >  
> > +        if (child instanceof TitleClickTarget) {
> 
> I know you're just continuing an existing pattern here, but I don't like how
> we're doing all these instanceof checks to figure out what to do with the
> child view.

Changes mentioned above removed these checks.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8366392 [details] [diff] [review]
Refactor HomeBanner out of TopSitesPanel, display on default page

I'm a bit confused by this patch... is this a patch on top of the old patch? If so, you should just fold them together and post the one patch that applies on m-c.

Comment 7

[Josh Dover [:jdover]]

Attachment: Refactor HomeBanner out of TopSitesPanel, display on default page

Ah, meant to make a patch of the combined commits. Here it is, sorry for confusion.

Comment 8

[:Margaret Leibovic]

Comment on attachment 8366723 [details] [diff] [review]
Refactor HomeBanner out of TopSitesPanel, display on default page

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

Nice! I like this a lot more. I'd just like to see an updated version that addresses these comments before giving an r+.

Also, I'd like lucasr to take a look at the animation code, since I'm less familiar with any potential animation gotchas.

::: mobile/android/base/home/HomeBanner.java
@@ +44,5 @@
> +
> +    private float mTouchY = -1;
> +    private boolean mSnapBannerToTop;
> +    private boolean mDismissed = false;
> +    private boolean mScrollingPages = false;

It would be good to add some documentation about what these are all for.

@@ +84,2 @@
>  
> +    public boolean onTouch(View view, MotionEvent event) {

It's a bit confusing that this isn't actually a method implementing the OnTouchListener interface, and also that this isn't actually getting called when the banner is touched, but rather when the home pager is touched. Since you're not using the view parameter here, you could omit that, and then rename this method to something more descriptive of what it's actually doing, like `handleTouchEvent` or `maybeAnimateForScroll`.

@@ +85,5 @@
> +    public boolean onTouch(View view, MotionEvent event) {
> +        if (!mDismissed && !mScrollingPages) {
> +            animateForScroll(event);
> +        }
> +        return false;

You don't need to return anything for this method, since you're not actually using the return value.

@@ +91,2 @@
>  
> +    public void defaultPageSelected(boolean defaultPage) {

I don't like boolean parameters, since it makes it more difficult to follow what's going on when reading code where they're used. Perhaps we should have two methods, defaultPageSelected() and nonDefaultPageSelected()?

@@ +95,5 @@
> +        } else {
> +            ThreadUtils.postToUiThread(new Runnable() {
> +                @Override
> +                public void run() {
> +                    animateDown();

You don't need to do this if the banner isn't already showing, right?

@@ +103,3 @@
>      }
>  
> +    public void onPageScrolled(int position, float positionOffset, int positionOffsetPixels) {

Once again, it's confusing that this has the same signature as a method on a different interface, but it's not actually implementing this interface. Since you're only using this method to set mScrollingPages, I think it could be better to replace this with a setScrollingPages method, and have HomePager call that with true/false.

::: mobile/android/base/home/HomePager.java
@@ -84,5 @@
>          public void onAddPagerView(String title);
>          public void removeAllPagerViews();
>          public void onPageSelected(int position);
>          public void onPageScrolled(int position, float positionOffset, int positionOffsetPixels);
> -        public void setOnTitleClickListener(OnTitleClickListener onTitleClickListener);

No need to change the name of this param.

@@ +207,5 @@
>                              1.0f);
>          }
>      }
>  
> +    private void setupTabStrip() {

Add some documentation to talk about what this method is doing.

Comment 9

[:Margaret Leibovic]

Comment on attachment 8366723 [details] [diff] [review]
Refactor HomeBanner out of TopSitesPanel, display on default page

I'd just like some additional eyes on this :)

Comment 10

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #8)

> @@ +91,2 @@
> >  
> > +    public void defaultPageSelected(boolean defaultPage) {
> 
> I don't like boolean parameters, since it makes it more difficult to follow
> what's going on when reading code where they're used. Perhaps we should have
> two methods, defaultPageSelected() and nonDefaultPageSelected()?

Actually, the banner doesn't need to know about default pages or not, it just needs to know if it should show or hide, so I think it would be better to just have methods that handle that (and they can be smart and bail early if the banner is already hidden).

Comment 11

[Josh Dover [:jdover]]

Attachment: Refactor HomeBanner out of TopSitesPanel, display on default page

Fixed all of your comments and added an optimization to not perform animations if the banner is already in the state that would have been performed by the animation.

Comment 12

[Josh Dover [:jdover]]

Attachment: Refactor HomeBanner out of TopSitesPanel, display on default page

Accidently included some other changes in the last patch, review this one. I'll get better at this one day ;)

Comment 13

[:Margaret Leibovic]

Comment on attachment 8366905 [details] [diff] [review]
Refactor HomeBanner out of TopSitesPanel, display on default page

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

I'm still not entirely happy with this patch, but we're getting there! Sorry for the review churn, hopefully we can land this soon :)

::: mobile/android/base/home/HomeBanner.java
@@ +43,5 @@
> +    final ImageButton mCloseButton;
> +
> +    // Used for tracking scroll length
> +    private float mTouchY = -1;
> +    // Used to detect for upwards scroll to push banner all the way up

Nit: Blank line above comment lines.

@@ +48,5 @@
> +    private boolean mSnapBannerToTop;
> +    // Used so that we don't move the banner when scrolling between pages
> +    private boolean mScrollingPages = false;
> +
> +    private boolean mDismissed = false;

Add a comment for this one, too, indicating that this is true if the user explicitly dismissed the banner.

@@ +80,2 @@
>          GeckoAppShell.getEventDispatcher().registerEventListener("HomeBanner:Data", this);
> +        animateDown();

Why do we need to animate down here? If it's just to hide the banner, can we just make sure the banner is hidden by default when the view is created? Also, if it's just to hide the banner, we probably want to hide it immediately, not animate it.

@@ +88,2 @@
>  
> +    public void handleHomeTouch(MotionEvent event) {

Add some documentation about when this is called.

@@ +88,4 @@
>  
> +    public void handleHomeTouch(MotionEvent event) {
> +        if (!mDismissed && !mScrollingPages) {
> +            animateForScroll(event);

Since this method body is so small, I think you could just get rid of the animateForScroll helper and put it's contents directly in here. To save on indentation, you can just return early |if (mDismissed || mScrollingPages)|.

@@ +102,5 @@
> +        // if showing?
> +        animateDown();
> +    }
> +
> +    public void setScrollingPages(boolean scollingPages) {

Typo: s/scollingPages/scrollingPages.

@@ +119,5 @@
> +                    setIcon(message.optString("iconURI"));
> +                    animateUp();
> +                } catch (JSONException e) {
> +                    Log.e(LOGTAG, "Exception handling " + event + " message", e);
> +                    return;

No need for this return statement.

@@ +128,2 @@
>  
> +    private void setupCloseButton() {

Since this method is only used once, I'd actually prefer to get rid of it and just keep its body in the HomeBanner constructor.

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

Not your fault, but shouldn't this also be called on the UI thread?

Also, looking at BitmapUtils, I think we'll call onBitmapFound from the same thread that we called getDrawable from (or from the UI thread itself), so I actually don't think you need this postToUiThread call down below, since setIcon is now called from the UI thread:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/BitmapUtils.java#44

::: mobile/android/base/home/HomePager.java
@@ -84,5 @@
>          public void onAddPagerView(String title);
>          public void removeAllPagerViews();
>          public void onPageSelected(int position);
>          public void onPageScrolled(int position, float positionOffset, int positionOffsetPixels);
> -        public void setOnTitleClickListener(OnTitleClickListener onTitleClickListener);

You forgot to address my last comment, you shouldn't to change this :)

@@ -123,5 @@
>          setFocusableInTouchMode(true);
>      }
>  
> -    @Override
> -    public void addView(View child, int index, ViewGroup.LayoutParams params) {

Why did you move this initialization code out of addView and into helper methods in the constructor? I'm wary of moving where we set up the tab strip, since that's out of the scope of this bug.

@@ +235,5 @@
> +    }
> +
> +    private void setupBanner() {
> +        mHomeBanner = (HomeBanner) ((ViewGroup) getParent()).findViewById(R.id.home_banner);
> +        setOnPageChangeListener(new HomePagerOnPageChangeListener());

This second line deals with more than just the banner. I think you can get rid of this helper method and just set mHomeBanner and the page change listener directly where you currently call this method.

@@ +277,5 @@
>              mDecor.onPageSelected(item);
>          }
> +        if (mHomeBanner != null) {
> +            if (item == mDefaultPanelIndex) {
> +                mHomeBanner.showBanner();

Nice, I feel like this is much clearer.

@@ +298,5 @@
> +    @Override
> +    public boolean dispatchTouchEvent(MotionEvent event) {
> +        // Get touches to pages, pass to banner, and forward to pages.
> +        if (mHomeBanner != null) {
> +            mHomeBanner.handleHomeTouch(event);

Nice.

::: mobile/android/base/resources/layout/gecko_app.xml
@@ +4,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
>                  xmlns:gecko="http://schemas.android.com/apk/res-auto"
> +                android:id="@+id/gecko_app"

Why did you add this?

::: mobile/android/base/resources/layout/home_pager.xml
@@ -4,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <!-- This file is used to include the home pager in gecko app
>       layout based on screen size -->
> -

Oops, this looks like an extraneous change.

Comment 14

[Josh Dover [:jdover]]

Attachment: Refactor HomeBanner out of TopSitesPanel, display on default page

Updated per comments.

(In reply to :Margaret Leibovic from comment #13)
> @@ +160,5 @@
> >              @Override
> >              public void onBitmapFound(final Drawable d) {
> >                  // Bail if getDrawable doesn't find anything.
> >                  if (d == null) {
> > +                    mIconView.setVisibility(View.GONE);
> 
> Not your fault, but shouldn't this also be called on the UI thread?

setText, setIcon are both called from the UI thread in handleMessage().

> > -        public void setOnTitleClickListener(OnTitleClickListener onTitleClickListener);
> 
> You forgot to address my last comment, you shouldn't to change this :)

Uh oh!

> 
> @@ -123,5 @@
> >          setFocusableInTouchMode(true);
> >      }
> >  
> > -    @Override
> > -    public void addView(View child, int index, ViewGroup.LayoutParams params) {
> 
> Why did you move this initialization code out of addView and into helper
> methods in the constructor? I'm wary of moving where we set up the tab
> strip, since that's out of the scope of this bug.

Good point.

> 
> @@ +235,5 @@
> > +    }
> > +
> > +    private void setupBanner() {
> > +        mHomeBanner = (HomeBanner) ((ViewGroup) getParent()).findViewById(R.id.home_banner);
> > +        setOnPageChangeListener(new HomePagerOnPageChangeListener());
> 
> This second line deals with more than just the banner. I think you can get
> rid of this helper method and just set mHomeBanner and the page change
> listener directly where you currently call this method.

Totally forgot that, thanks.

> 
> ::: mobile/android/base/resources/layout/gecko_app.xml
> @@ +4,5 @@
> >     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> >  
> >  <RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
> >                  xmlns:gecko="http://schemas.android.com/apk/res-auto"
> > +                android:id="@+id/gecko_app"
> 
> Why did you add this?

Accidental change that got through from my patch on keyboard stuff.

Comment 15

[:Margaret Leibovic]

Comment on attachment 8367605 [details] [diff] [review]
Refactor HomeBanner out of TopSitesPanel, display on default page

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

Thanks for bearing with me, this looks great!

As a follow-up, can you file a bug to hide the banner if all the panels are disabled? We always want to show the banner on the default panel, but to avoid being obnoxious, I think we should just hide it if there are no active panels.

::: mobile/android/base/home/HomeBanner.java
@@ +47,5 @@
> +    private boolean mSnapBannerToTop;
> +    // Used so that we don't move the banner when scrolling between pages
> +    private boolean mScrollingPages = false;
> +    // User has dismissed the banner using the close button
> +    private boolean mDismissed = false;

Oops, my nit was misinterpreted. I meant I *like* when there are empty lines above the comments :)

Comment 16

[Josh Dover [:jdover]]

Attachment: Refactor HomeBanner out of TopSitesPanel, display on default page

Fixed comments.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/793ffe6ed42e

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/793ffe6ed42e

Comment 19

[:Margaret Leibovic]

Backed out for causing bug 966307:
https://hg.mozilla.org/integration/fx-team/rev/5fb066c6d76c

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Merge of backout (still 29):
https://hg.mozilla.org/mozilla-central/rev/5fb066c6d76c

Comment 21

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

Just wanted to note that the History panel has two buttons at the bottom that may be hidden by a floating Home Banner. Margaret mentioned this would likely be fixed with bug 967085,  if it lands.

Comment 22

[Josh Dover [:jdover]]

Attachment: Refactor HomeBanner to be visible on any default panel

This patch removes non-vital refactoring from HomeBanner as well as implicitly tracking when the banner has been hidden by the HomePager to ensure that the banner is not shown at an inappropriate time.

Comment 23

[:Margaret Leibovic]

Comment on attachment 8374465 [details] [diff] [review]
Refactor HomeBanner to be visible on any default panel

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

I haven't had time to look through this yet, but let's have lucasr take a look as well.

I also started on improving test coverage over in bug 935264. Feel free to take a look at that patch and offer any more ideas!

Comment 24

[:Margaret Leibovic]

I just built with this patch to test it, and the first thing I saw was a banner appear on top of a regular webpage :/

What have you been doing to test? I just made an add-on that added some home banners, that's probably the easiest way to test.

Comment 25

[:Margaret Leibovic]

Comment on attachment 8374465 [details] [diff] [review]
Refactor HomeBanner to be visible on any default panel

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

::: mobile/android/base/home/HomeBanner.java
@@ +174,5 @@
> +     *      1) The user has dismissed the banner
> +     *      2) The HomePager has hidden the banner
> +     */
> +    private boolean isShowAllowed() {
> +        return !mDismissed && !mHidden;

I'm suspicious of needing to check flags like this. I think keeping track of state like this is what's causing these visibility tracking bugs. Is there some other way for us to just really directly tie the visibility of this banner view to that of the HomePager?

I think we should try to avoid conflating the issue of the user dismissing the banner for now. We can address that in bug 961523.

Comment 26

[:Margaret Leibovic]

Thinking about this a bit more, I think we should go back to including the home banner as part of the home pager layout, since that seems like the most reliable way to keep their visibility in sync. Also, conceptually, the banner *should* be a child of the home pager.

Taking a step back, what were the real issues with that previous approach? The Decor interface? I think we should try approaching this with a fresh attitude to see what we can do to make this work.

Comment 27

[:Margaret Leibovic]

Comment on attachment 8374465 [details] [diff] [review]
Refactor HomeBanner to be visible on any default panel

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

r-, since this still has visibility problems.

::: mobile/android/base/resources/layout/home_pager.xml
@@ +13,5 @@
>                                    android:layout_height="fill_parent"
>                                    android:background="@android:color/white"
>                                    android:visibility="gone">
>  
> +    <org.mozilla.gecko.home.HomePagerTabStrip android:id="@+id/phone_menu_strip"

You don't need these HomePagerTabStrip ids anymore.

Comment 28

[Lucas Rocha (:lucasr)]

Comment on attachment 8374465 [details] [diff] [review]
Refactor HomeBanner to be visible on any default panel

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

I'd like to understand what are the issues/limitations with the Decor-based approach. If we end up having to place the banner outside of HomePager, I'd prefer to do it via a custom implementation of the home_pager_container layout instead.

Comment 29

[:Margaret Leibovic]

Attachment: isDecor proof of concept

Here's a proof of concept that makes the HomeBanner a child of the HomePager view.

This makes the banner show up on top of every panel, and it doesn't handle scrolling, but it's a start in the right direction.

I think what screwed us up the first time we tried going with an approach like this is that we have a Decor interface in HomePager that should really be named something more specific to the tab strip. I don't think we should scope creep this bug to deal with that, but that would be a good follow-up.

For this bug, I think we should make whatever changes we need to make specifically for the home banner, and not try to share code with the tab strip. If it turns out there are ways we can refactor things, we can do that in a follow-up.

What do you think of that plan?

Comment 30

[Josh Dover [:jdover]]

Sounds fantastic. I think I have a bit of trying to fix too many things at once. I'll work with this and get all the details in. Thanks for your patience :)

Comment 31

[Josh Dover [:jdover]]

Attachment: 01 - Remove HomeBanner from TopSitesPanel

Comment 32

[Josh Dover [:jdover]]

Attachment: Control HomeBanner visibility from home_pager_container

Comment 33

[Josh Dover [:jdover]]

Attachment: Display HomeBanner on default page

Comment 34

[Josh Dover [:jdover]]

Attachment: Animate banner when scrolling in a home panel

This patch sequence breaks up the different concerns of the home panel so it easier to understand the complexity of showing and hiding the banner. This should NEVER be able to display the banner of webpage content as it is hidden with the HomePager in BrowserApp.

Unfortunately, isDecor does not work with showing a view on top of the ViewPager, thus we must put the banner inside of the FrameLayout container that is already present.

Comment 35

[:Margaret Leibovic]

Comment on attachment 8375888 [details] [diff] [review]
01 - Remove HomeBanner from TopSitesPanel

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

Nice small patch! Upgrading this to an r+, since this will need to happen no matter what :)

Comment 36

[:Margaret Leibovic]

Comment on attachment 8375890 [details] [diff] [review]
Control HomeBanner visibility from home_pager_container

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

::: mobile/android/base/BrowserApp.java
@@ +1781,3 @@
>          if (mHomePager != null) {
>              mHomePager.hide();
>          }

If we're setting the visibility of the container, I don't think we need to hide the HomePager itself anymore. However, this hide() method does more than just set the visibility, so we still need to call it, but maybe we should remove the setVisibility call in there and rename it to something like unload().

Similarly, we could rename HomePager.show() to something like HomePager.load().

I wonder how lucasr feels about this, so I'm flagging him for feedback as well.

However, I feel like hiding/showing the container is a much better approach to managing visibility than hiding/showing the banner.

::: mobile/android/base/resources/layout/gecko_app.xml
@@ +43,5 @@
> +                                                   android:layout_gravity="bottom"
> +                                                   android:gravity="center_vertical"
> +                                                   android:visibility="gone"
> +                                                   android:clickable="true"
> +                                                   android:focusable="true"/>

Note to self (and others): bug 968640 is already filed about making this into a ViewStub, so we shouldn't worry about doing that fix here.

Comment 37

[:Margaret Leibovic]

Comment on attachment 8375891 [details] [diff] [review]
Display HomeBanner on default page

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

We need to address the race condition introduced by the asynchrous gecko message passing. I feel like the best approach might be to keep the "HomeBanner:Get" message in the constructor, set the banner data in handleMessage, then decide whether or not we should actually show the banner when we're done with all that.

I know I've said I really don't like flags about whether or not something should be visible, but maybe we do actually need that here :/ Maybe instead of calling show/hide directly, onPageSelected should just enable/disable the banner as appropriate, and then we could just show the banner if it's enabled. Although with that approach we'd still need more logic to decide if we have data and can actually show the banner when it gets enabled.

Sorry, I'm mostly just thinking aloud here... but we definitely do need to change the flow around here.

::: mobile/android/base/home/HomeBanner.java
@@ +44,5 @@
> +    // Used to detect for upwards scroll to push banner all the way up
> +    private boolean mSnapBannerToTop;
> +
> +    // User has dismissed the banner using the close button
> +    private boolean mDismissed = false;

I think I mentioned this in a previous comment, let's not worry about keeping track of the dismissed state just yet. We can do that in bug 961523.

@@ +70,5 @@
>  
>          closeButton.setOnClickListener(new View.OnClickListener() {
>              @Override
>              public void onClick(View view) {
> +                animateDown();

Let's just leave this alone for right now. We should get some UX input, but I kinda feel like clicking the close button should just make the banner immediately go away.

::: mobile/android/base/home/HomePager.java
@@ +352,5 @@
> +            }
> +
> +            if (mHomeBanner != null) {
> +                if (position == mDefaultPageIndex) {
> +                    mHomeBanner.show();

This is going to fetch a new message every time the page changes. I don't think that's what we want... I think we should try to decouple the showing/hiding from the message fetching, but I'm not sure the best way to approach that off the top of my head.

Also, we don't want this kind of asynchronous behavior in show(), since this will introduce a race condition where the banner could show up on the wrong panel if the user swipes between panels quickly.

Comment 38

[:Margaret Leibovic]

Comment on attachment 8375894 [details] [diff] [review]
Animate banner when scrolling in a home panel

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

This looks like a pretty straightforward port of the old logic.

Comment 39

[Josh Dover [:jdover]]

(In reply to :Margaret Leibovic from comment #37)
> Comment on attachment 8375891 [details] [diff] [review]
> Display HomeBanner on default page
> 
> Review of attachment 8375891 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We need to address the race condition introduced by the asynchrous gecko
> message passing. I feel like the best approach might be to keep the
> "HomeBanner:Get" message in the constructor, set the banner data in
> handleMessage, then decide whether or not we should actually show the banner
> when we're done with all that.
> 
> I know I've said I really don't like flags about whether or not something
> should be visible, but maybe we do actually need that here :/ Maybe instead
> of calling show/hide directly, onPageSelected should just enable/disable the
> banner as appropriate, and then we could just show the banner if it's
> enabled. Although with that approach we'd still need more logic to decide if
> we have data and can actually show the banner when it gets enabled.
> 
> Sorry, I'm mostly just thinking aloud here... but we definitely do need to
> change the flow around here.
> 

I think we can make this API easier to understand by letting the HomePager enable/disable the banner, and allow the banner to control the showing/hiding:

In HomeBanner we have a public method that HomePager can call instead of show/hide. This would set a mEnabled flag internally:
> public void setEnabled(boolean enabled);

HomeBanner would then only check that flag in animateUp() to make sure it is OK to show the banner now. I agree that flags can be messy, but I think if we nail the naming and comments, we can make it clear.

Thoughts?

Comment 40

[Lucas Rocha (:lucasr)]

Comment on attachment 8375890 [details] [diff] [review]
Control HomeBanner visibility from home_pager_container

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

Yeah, Margaret's suggestion makes sense:
- Rename HomePager's show()/hide() to load()/unload()
- Remove setVisibility() calls from HomePager's load()/unload() methods

Just slightly mixed about the show()->load() renaming as we actually animate HomePager in show(). Not a big deal I guess.

Comment 41

[Josh Dover [:jdover]]

Attachment: Control HomeBanner visibility from home_pager_container

This changes show/hide to load/unload and removes the setVisibility() calls in HomePager

Comment 42

[Josh Dover [:jdover]]

Attachment: Display HomeBanner on default page

This seems to be the simplist, fool-proof way of handling the async message interaction with the hiding and showing of the banner:

1) Message is requested the in constructor of HomeBanner.
2) HomePager uses setEnabled() on HomeBanner which will set an internal flag, and if the message has been received, show the banner.
3) When the message is received, if the banner is enabled (checking the internal flag), show the banner.

Comment 43

[Josh Dover [:jdover]]

Attachment: Animate banner when scrolling in a home panel

Updated to reflect naming changes

Comment 44

[:Margaret Leibovic]

Comment on attachment 8377732 [details] [diff] [review]
Control HomeBanner visibility from home_pager_container

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

This looks good to me, but I haven't applied it to test. You should try running testHomeBanner locally, and also make a try push before we land this.

::: mobile/android/base/BrowserApp.java
@@ +1710,5 @@
>              mHomePager = (HomePager) homePagerStub.inflate();
>          }
>  
> +        if (mHomePagerContainer != null) {
> +            mHomePagerContainer.setVisibility(View.VISIBLE);

mHomePagerContainer is set in onCreate, so it shouldn't be null in here. Also, if it did happen to be null here, we'd avoid crashing, but the home pager wouldn't end up being shown properly, which definitely isn't good. So, I'd say we can just get rid of this null check.

@@ +1775,5 @@
>          // Display the previously hidden web content (which prevented screen reader access).
>          mLayerView.setVisibility(View.VISIBLE);
>  
> +        if (mHomePagerContainer != null) {
> +            mHomePagerContainer.setVisibility(View.GONE);

Same comment applies here.

::: mobile/android/base/home/HomePager.java
@@ +195,4 @@
>      }
>  
>      /**
>       * Loads and initializes the pager.

This existing comment helps me feel good about this rename :)

@@ +245,5 @@
>          }
>      }
>  
>      /**
>       * Hides the pager and removes all child fragments.

You should update this comment as well.

Comment 45

[:Margaret Leibovic]

Comment on attachment 8377735 [details] [diff] [review]
Animate banner when scrolling in a home panel

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

::: mobile/android/base/home/HomePager.java
@@ +278,5 @@
>  
> +    @Override
> +    public boolean dispatchTouchEvent(MotionEvent event) {
> +        if (mHomeBanner != null) {
> +            mHomeBanner.handleHomeTouch(event);

Looks like there's a patch missing in here (or some part of this patch is missing), since I don't see mHomeBanner being declared anywhere. Clearing review until this gets sorted out.

Comment 46

[Josh Dover [:jdover]]

Attachment: 02 - Control HomeBanner visibility from home_pager_container

Fixed nits

Comment 47

[Josh Dover [:jdover]]

Attachment: Display HomeBanner on default page

Comment 48

[Josh Dover [:jdover]]

Attachment: Animate banner when scrolling in a home panel

Comment 49

[:Margaret Leibovic]

Comment on attachment 8377898 [details] [diff] [review]
Display HomeBanner on default page

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

This is looking good. I just want to see one more version that checks if mEnabled changed before trying to animate the banner.

::: mobile/android/base/home/HomeBanner.java
@@ +41,5 @@
> +    // Used for tracking scroll length
> +    private float mTouchY = -1;
> +
> +    // Used to detect for upwards scroll to push banner all the way up
> +    private boolean mSnapBannerToTop;

These field should have been defined in the next patch (since they're unused here), but I'll forgive you :)

@@ -56,5 @@
>  
>          closeButton.setOnClickListener(new View.OnClickListener() {
>              @Override
>              public void onClick(View view) {
> -                HomeBanner.this.setVisibility(View.GONE);

No need to change this.

@@ +149,5 @@
>          });
>      }
> +
> +    public void setEnabled(boolean enabled) {
> +        Log.d("Boo", "Enabled: " + enabled);

Remove this logging before landing.

@@ +150,5 @@
>      }
> +
> +    public void setEnabled(boolean enabled) {
> +        Log.d("Boo", "Enabled: " + enabled);
> +        mEnabled = enabled;

I think we should add a check here to see if the enabled state actually changed, and only try doing animateUp/animateDown in that case. We're calling setEnabled every time the page changes, but if you swipe between two non-default panels, we shouldn't need to do any work.

@@ +154,5 @@
> +        mEnabled = enabled;
> +        if (enabled) {
> +            animateUp();
> +        } else {
> +            animateDown();

Do we actually want to animate down where swiping between pages? You should post a build for ibarlow to test and see how he feels about this.

I haven't made a build to test myself, but I imagine it probably does feel better than just making the banner hide immediately.

@@ +161,5 @@
> +
> +    private void animateUp() {
> +        // Check to make sure that message has been received and the banner has been enabled.
> +        // Necessary to avoid race conditions between show() and handleMessage() calls.
> +        if (!mEnabled || TextUtils.isEmpty(((TextView) findViewById(R.id.text)).getText())) {

Nit: Pull this cast out into a local variable so it's easier to read this.

@@ +168,5 @@
> +
> +        setVisibility(VISIBLE);
> +
> +        // No need to animate if already translated.
> +        if (ViewHelper.getTranslationY(this) == 0) {

What happens the first time the banner is shown? Does it just appear, not animate? Should we be setting an initial translation on the banner?

@@ +179,5 @@
> +    }
> +
> +    private void animateDown() {
> +        // No need to animate if already translated.
> +        if (ViewHelper.getTranslationY(this) == getHeight()) {

Should we also have a check here to see if the banner isn't visible?

Comment 50

[:Margaret Leibovic]

Comment on attachment 8377899 [details] [diff] [review]
Animate banner when scrolling in a home panel

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

r+ with comments addressed.

::: mobile/android/base/home/HomeBanner.java
@@ +203,5 @@
>          animator.start();
>      }
> +
> +    public void handleHomeTouch(MotionEvent event) {
> +        if (!mEnabled || mScrollingPages) {

Instead of checking mEnabled, I think we should check if the banner is actually visible. No need to do touch event handling if there's no banner to move up and down.

Although, I suppose when we set mEnabled to false, we animate down, so there is a period of time where mEnabled is false and the banner is still visible.

So, check both :)

@@ +215,5 @@
> +            }
> +
> +            case MotionEvent.ACTION_MOVE: {
> +                // There is a chance that we won't receive ACTION_DOWN, if the touch event
> +                // actually started on the Grid instead of the List. Treat this as first event.

This comment is out of date. I believe this is the case all of the time now, so you should update this to explain why we keep track of mTouchY ourselves.

Comment 51

[Josh Dover [:jdover]]

Attachment: Display HomeBanner on default page

(In reply to :Margaret Leibovic from comment #49)
> Comment on attachment 8377898 [details] [diff] [review]
> Display HomeBanner on default page
> 
> Review of attachment 8377898 [details] [diff] [review]:
> -----------------------------------------------------------------
> Do we actually want to animate down where swiping between pages? You should
> post a build for ibarlow to test and see how he feels about this.
> 
> I haven't made a build to test myself, but I imagine it probably does feel
> better than just making the banner hide immediately.

I think it looks much better animating down between pages, rather than disappearing. Test build: https://drive.google.com/file/d/0BwqqgfwYmRg4dndwYzZkRDhGNkk/edit?usp=sharing

> @@ +168,5 @@
> > +
> > +        setVisibility(VISIBLE);
> > +
> > +        // No need to animate if already translated.
> > +        if (ViewHelper.getTranslationY(this) == 0) {
> 
> What happens the first time the banner is shown? Does it just appear, not
> animate? Should we be setting an initial translation on the banner?

Added an initial translation to make banner animate on first appearance.

Comment 52

[Josh Dover [:jdover]]

Attachment: Animate banner when scrolling in a home panel

Just want to double-check your r+ after these comments.

(In reply to :Margaret Leibovic from comment #50)
> Comment on attachment 8377899 [details] [diff] [review]
> Animate banner when scrolling in a home panel
> 
> Review of attachment 8377899 [details] [diff] [review]:
> -----------------------------------------------------------------
> Instead of checking mEnabled, I think we should check if the banner is
> actually visible. No need to do touch event handling if there's no banner to
> move up and down.
> 
> Although, I suppose when we set mEnabled to false, we animate down, so there
> is a period of time where mEnabled is false and the banner is still visible.
> 
> So, check both :)

Actually I don't think you can check the visibility here (with the way it works currently at least) because the visibility will be set to GONE if it gets animated down, even though the banner has not been disabled.

> @@ +215,5 @@
> > +            }
> > +
> > +            case MotionEvent.ACTION_MOVE: {
> > +                // There is a chance that we won't receive ACTION_DOWN, if the touch event
> > +                // actually started on the Grid instead of the List. Treat this as first event.
> 
> This comment is out of date. I believe this is the case all of the time now,
> so you should update this to explain why we keep track of mTouchY ourselves.

Thanks for bringing this line to my attention! Actually since the banner intercepts ALL touches to the HomePager, it will always receive the ACTION_DOWN event, so this block can actually be removed.

Comment 53

[:Margaret Leibovic]

(In reply to Josh Dover from comment #52)
> Created attachment 8377968 [details] [diff] [review]
> Animate banner when scrolling in a home panel
> 
> Just want to double-check your r+ after these comments.
> 
> (In reply to :Margaret Leibovic from comment #50)
> > Comment on attachment 8377899 [details] [diff] [review]
> > Animate banner when scrolling in a home panel
> > 
> > Review of attachment 8377899 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > Instead of checking mEnabled, I think we should check if the banner is
> > actually visible. No need to do touch event handling if there's no banner to
> > move up and down.
> > 
> > Although, I suppose when we set mEnabled to false, we animate down, so there
> > is a period of time where mEnabled is false and the banner is still visible.
> > 
> > So, check both :)
> 
> Actually I don't think you can check the visibility here (with the way it
> works currently at least) because the visibility will be set to GONE if it
> gets animated down, even though the banner has not been disabled.

Ah, fair point. While reviewing these patches, I had actually wondered if we should bother setting the visibility to GONE after animating down, since that wasn't a part of the old code.

Comment 54

[:Margaret Leibovic]

(In reply to Josh Dover from comment #51)
> Created attachment 8377965 [details] [diff] [review]
> Display HomeBanner on default page
> 
> (In reply to :Margaret Leibovic from comment #49)
> > Comment on attachment 8377898 [details] [diff] [review]
> > Display HomeBanner on default page
> > 
> > Review of attachment 8377898 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > Do we actually want to animate down where swiping between pages? You should
> > post a build for ibarlow to test and see how he feels about this.
> > 
> > I haven't made a build to test myself, but I imagine it probably does feel
> > better than just making the banner hide immediately.
> 
> I think it looks much better animating down between pages, rather than
> disappearing. Test build:
> https://drive.google.com/file/d/0BwqqgfwYmRg4dndwYzZkRDhGNkk/edit?usp=sharing

Testing with my demo add-on [1] and this build, I noticed a few problems:
* After scrolling down the default panel, then switching panels back and forth, the banner re-appears on top of the partially scrolled list.
* If you make an upward scroll gesture when the default panel list is too short to scroll, the banner scrolls off the page and stays off (perhaps this is an issue we should sort out in bug 961501).
* I saw a case where the banner didn't appear on the default panel when I opened about:home, but I can't reproduce it :(

Josh, do you have try server access? You should push these patches to try before we land them.

[1] http://people.mozilla.org/~mleibovic/homedemo.xpi

Comment 55

[Josh Dover [:jdover]]

Attachment: Animate banner when scrolling in a home panel

> Testing with my demo add-on [1] and this build, I noticed a few problems:
> * After scrolling down the default panel, then switching panels back and
> forth, the banner re-appears on top of the partially scrolled list.
> * If you make an upward scroll gesture when the default panel list is too
> short to scroll, the banner scrolls off the page and stays off (perhaps this
> is an issue we should sort out in bug 961501).
> * I saw a case where the banner didn't appear on the default panel when I
> opened about:home, but I can't reproduce it :(
> 
> Josh, do you have try server access? You should push these patches to try
> before we land them.

- Issue 1) is fixed by tracking mUserSwipedDown
- Issues 2) and probably 3) were fixed by removing the setVisibility(GONE) from animateDown()

Comment 56

[Josh Dover [:jdover]]

Attachment: Update HomeBanner test for new implementation

Yay for tests! Try build: https://tbpl.mozilla.org/?tree=Try&rev=a773b6116c79

Comment 57

[:Margaret Leibovic]

(In reply to Josh Dover from comment #56)
> Created attachment 8378732 [details] [diff] [review]
> Update HomeBanner test for new implementation
> 
> Yay for tests! Try build: https://tbpl.mozilla.org/?tree=Try&rev=a773b6116c79

This is not looking good :(

Comment 58

[Josh Dover [:jdover]]

Attachment: 04 - Update HomeBanner test for new implementation

Fixed issue with home pager visibility checks. New try build running: https://tbpl.mozilla.org/?tree=Try&rev=43fc2bc9951b

Comment 59

[:Margaret Leibovic]

(In reply to Josh Dover from comment #58)
> Created attachment 8379162 [details] [diff] [review]
> Update HomeBanner test for new implementation
> 
> Fixed issue with home pager visibility checks. New try build running:
> https://tbpl.mozilla.org/?tree=Try&rev=43fc2bc9951b

Looks like you still have some orange :(

At least it's actually in testHomeBanner this time. It looks like that translation check isn't working for us here.

Comment 60

[:Margaret Leibovic]

Also looks like we're running into a problem with testBookmarksPanel, specifically here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/AboutHomeTest.java#231

I'm sorry this is such a pain :(

Comment 61

[Josh Dover [:jdover]]

Attachment: 03 - Display HomeBanner on default page and animate

Comment 62

[:Margaret Leibovic]

Comment on attachment 8379294 [details] [diff] [review]
03 - Display HomeBanner on default page and animate

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

Thanks for folding those patches, this is easier to review now. This is looking good, but I'd just like to see a polished up version that addresses these comments.

Looking into the tests a bit, I think the tests need updating, not this patch, but let's sort that out before we reach that conclusion :)

::: mobile/android/base/home/HomeBanner.java
@@ +180,5 @@
> +        if (!mEnabled || TextUtils.isEmpty(textView.getText()) || mUserSwipedDown) {
> +            return;
> +        }
> +
> +        setVisibility(VISIBLE);

I would like to see us remove any visibility logic from animateUp/animateDown. We can put this setVisbility(VISIBLE) call up in handleMessage before we call animateUp.

Because we're setting an initial translation on the banner, we shouldn't need to worry about the case where we end up in handleMessage after the user has swiped to a non-default page. The visibility will change, but the banner won't appear because we'll bail before animating up.

I wondered if we even need to be setting visibility at all anymore, but then I realized that we'll still want to set the visibility to GONE when the user hits the "x" button. So basicailly, visibility should correspond to whether or not the banner is "active", in the sense that it's in a state where we want to show it to the user, and "enabled" corresponds to whether or not the user in on the default panel.

@@ +194,5 @@
> +    }
> +
> +    private void animateDown() {
> +        // No need to animate if already translated or gone.
> +        if (ViewHelper.getTranslationY(this) == getHeight() || getVisibility() == GONE) {

To keep the notion of visibility out of the animateUp/animdateDown methods, we can get rid of this visibility check here, and perhaps move it up above to setEnabled, where we can avoid trying to animate at all if the banner isn't visible. Also, in handleHomeTouch we already bail early if the visibility is GONE, so that case is already handled.

::: mobile/android/base/resources/layout/gecko_app.xml
@@ -41,5 @@
>                                                     android:layout_height="@dimen/home_banner_height"
>                                                     android:background="@drawable/home_banner"
>                                                     android:layout_gravity="bottom"
>                                                     android:gravity="center_vertical"
> -                                                   android:visibility="gone"

Let's keep this.

Comment 63

[:Margaret Leibovic]

Comment on attachment 8379162 [details] [diff] [review]
04 - Update HomeBanner test for new implementation

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

This is also looking good, but we need to tweak more things to make the tests work.

Comment 64

[Josh Dover [:jdover]]

Attachment: 02 - Control HomeBanner visibility from home_pager_container

Copied change to tablet specific layout xml

Comment 65

[Josh Dover [:jdover]]

Attachment: 03 - Display HomeBanner on default page and animate

Comment 66

[Josh Dover [:jdover]]

Attachment: 04 - Update HomeBanner test for new implementation

Fixed up tests utilities in AboutHomeComponent for detecting visibility to reflect changes to how visibility is controlled in patch (2).

Comment 67

[Josh Dover [:jdover]]

Attachment: 03 - Display HomeBanner on default page and animate

Awesome this should be ready now :) 

The last try build succeeded and this one should too: https://tbpl.mozilla.org/?tree=Try&rev=9afb94e6262f

Comment 68

[:Margaret Leibovic]

Comment on attachment 8379417 [details] [diff] [review]
03 - Display HomeBanner on default page and animate

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

\o/

Comment 69

[:Margaret Leibovic]

Comment on attachment 8379370 [details] [diff] [review]
04 - Update HomeBanner test for new implementation

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

Nice.

::: mobile/android/base/tests/testHomeBanner.java
@@ +6,5 @@
>  import org.mozilla.gecko.R;
>  import org.mozilla.gecko.tests.helpers.*;
>  
>  import android.view.View;
> +import android.util.Log;

Get rid of this import for landing.

@@ -56,5 @@
>  
> -        // AboutHomeComponent calls mSolo.getView, which will fail an assertion if the
> -        // view is not present, so we need to use findViewById in this case.
> -        final View banner = getActivity().findViewById(R.id.home_banner);
> -        assertTrue("The HomeBanner is not visible", banner == null || banner.getVisibility() != View.VISIBLE);

Nice that we can get rid of this now!

Comment 70

[Josh Dover [:jdover]]

Attachment: 04 - Update HomeBanner test for new implementation

Removed Log import

Comment 71

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/a4baa3782ae2
https://hg.mozilla.org/integration/fx-team/rev/e6dbf0549360
https://hg.mozilla.org/integration/fx-team/rev/a6905ad185fa
https://hg.mozilla.org/integration/fx-team/rev/56d5e91fe13e

Comment 72

[:Margaret Leibovic]

I'll request approval to uplift to Aurora after it bakes on Nightly a bit.

Comment 73

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

https://hg.mozilla.org/mozilla-central/rev/a4baa3782ae2
https://hg.mozilla.org/mozilla-central/rev/e6dbf0549360
https://hg.mozilla.org/mozilla-central/rev/a6905ad185fa
https://hg.mozilla.org/mozilla-central/rev/56d5e91fe13e

Comment 74

[:Margaret Leibovic]

Josh, can you make a folded patch for us to uplift to aurora?

Comment 75

[Josh Dover [:jdover]]

Attachment: Folded patch

Here you go!

Comment 76

[:Margaret Leibovic]

Comment on attachment 8383221 [details] [diff] [review]
Folded patch

Thanks Josh!

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 942875, combined with the fact that we want to enable snippets and the sync promo banner on 29
User impact if declined: home banner will not appear if "top sites" panel is disabled
Testing completed (on m-c, etc.): landed on m-c 2/21
Risk to taking this patch (and alternatives if risky): the first time we landed a patch for this bug, it caused all sorts of problems with the banner appearing where it shouldn't, but with the new approach we took, there are fewer follow-up issues, and we've been working on addressing them
String or IDL/UUID changes made by this patch: none

Comment 77

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ca89d8111a36

Comment 78

[Teodora Vermesan (:TeoVermesan)]

Home banner will appear even if the user disables "Top Sites" panel, so:
Verified fixed on:
Builds: Firefox for Android 29.0a2 (2014-03-11) and  Firefox for Android 30.0a1 (2014-03-11)
Device: LG Nexus 4
OS: Android 4.4

Comment 79

[:Margaret Leibovic]

Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.