Closed Bug 960359 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified
fennec 29+ ---

People

(Reporter: Margaret, Assigned: jdover)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 30 obsolete files)

40.08 KB, patch
Details | Diff | Splinter Review
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.
Flags: needinfo?(deb)
Keywords: productwanted
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.
Flags: needinfo?(deb)
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → 29+
Assignee: margaret.leibovic → jdover
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.
Attachment #8365266 - Flags: review?(margaret.leibovic)
Oh, and this patch should fix bug 961523 as well.
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.
Attachment #8365266 - Flags: review?(margaret.leibovic) → feedback+
Keywords: productwanted
(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.
Attachment #8366392 - Flags: review?(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.
Attachment #8365266 - Attachment is obsolete: true
Ah, meant to make a patch of the combined commits. Here it is, sorry for confusion.
Attachment #8366392 - Attachment is obsolete: true
Attachment #8366392 - Flags: review?(margaret.leibovic)
Attachment #8366723 - Flags: review?(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.
Attachment #8366723 - Flags: review?(margaret.leibovic) → feedback+
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 :)
Attachment #8366723 - Flags: feedback?(lucasr.at.mozilla)
(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).
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.
Attachment #8366723 - Attachment is obsolete: true
Attachment #8366723 - Flags: feedback?(lucasr.at.mozilla)
Attachment #8366900 - Flags: review?(margaret.leibovic)
Accidently included some other changes in the last patch, review this one. I'll get better at this one day ;)
Attachment #8366900 - Attachment is obsolete: true
Attachment #8366900 - Flags: review?(margaret.leibovic)
Attachment #8366905 - Flags: review?(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.
Attachment #8366905 - Flags: review?(margaret.leibovic) → review-
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.
Attachment #8366905 - Attachment is obsolete: true
Attachment #8367605 - Flags: review?(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 :)
Attachment #8367605 - Flags: review?(margaret.leibovic) → review+
Fixed comments.
Attachment #8367605 - Attachment is obsolete: true
Attachment #8367734 - Flags: review+
Keywords: checkin-needed
Status: NEW → ASSIGNED
Blocks: 961523
https://hg.mozilla.org/mozilla-central/rev/793ffe6ed42e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Depends on: 966307
Blocks: 920791
Backed out for causing bug 966307:
https://hg.mozilla.org/integration/fx-team/rev/5fb066c6d76c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 967085
Target Milestone: Firefox 29 → ---
Depends on: 967016
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.
Status: REOPENED → ASSIGNED
Depends on: 935264
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.
Attachment #8367734 - Attachment is obsolete: true
Attachment #8374465 - Flags: review?(margaret.leibovic)
Blocks: home-banner
No longer blocks: 920791, 961523
No longer depends on: 967085
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!
Attachment #8374465 - Flags: feedback?(lucasr.at.mozilla)
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 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.
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 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.
Attachment #8374465 - Flags: review?(margaret.leibovic) → review-
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.
Attachment #8374465 - Flags: feedback?(lucasr.at.mozilla)
Attached patch isDecor proof of concept (obsolete) — Splinter Review
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?
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 :)
Attachment #8374465 - Attachment is obsolete: true
Attachment #8375888 - Flags: feedback?(margaret.leibovic)
Attachment #8375890 - Flags: feedback?(margaret.leibovic)
Attachment #8375891 - Flags: feedback?(margaret.leibovic)
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.
Attachment #8375894 - Flags: feedback?(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 :)
Attachment #8375888 - Flags: feedback?(margaret.leibovic) → review+
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.
Attachment #8375890 - Flags: feedback?(margaret.leibovic)
Attachment #8375890 - Flags: feedback?(lucasr.at.mozilla)
Attachment #8375890 - Flags: feedback+
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.
Attachment #8375891 - Flags: feedback?(margaret.leibovic) → feedback-
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.
Attachment #8375894 - Flags: feedback?(margaret.leibovic) → feedback+
(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 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.
Attachment #8375890 - Flags: feedback?(lucasr.at.mozilla) → feedback+
This changes show/hide to load/unload and removes the setVisibility() calls in HomePager
Attachment #8375890 - Attachment is obsolete: true
Attachment #8377732 - Flags: review?(margaret.leibovic)
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.
Attachment #8375891 - Attachment is obsolete: true
Attachment #8377734 - Flags: review?(margaret.leibovic)
Updated to reflect naming changes
Attachment #8377734 - Attachment is obsolete: true
Attachment #8377734 - Flags: review?(margaret.leibovic)
Attachment #8377735 - Flags: review?(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.
Attachment #8377732 - Flags: review?(margaret.leibovic) → review+
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.
Attachment #8377735 - Flags: review?(margaret.leibovic)
Fixed nits
Attachment #8377732 - Attachment is obsolete: true
Attachment #8377897 - Flags: review+
Attachment #8377898 - Flags: review?(margaret.leibovic)
Attachment #8375894 - Attachment is obsolete: true
Attachment #8377735 - Attachment is obsolete: true
Attachment #8377899 - Flags: review?(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?
Attachment #8377898 - Flags: review?(margaret.leibovic) → feedback+
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.
Attachment #8377899 - Flags: review?(margaret.leibovic) → review+
(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.
Attachment #8377898 - Attachment is obsolete: true
Attachment #8377965 - Flags: review?(margaret.leibovic)
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.
Attachment #8377899 - Attachment is obsolete: true
Attachment #8377968 - Flags: review?(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.
(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
Blocks: 974723
> 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()
Attachment #8377968 - Attachment is obsolete: true
Attachment #8377968 - Flags: review?(margaret.leibovic)
Attachment #8378722 - Flags: review?(margaret.leibovic)
Yay for tests! Try build: https://tbpl.mozilla.org/?tree=Try&rev=a773b6116c79
Attachment #8378732 - Flags: review?(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 :(
Priority: -- → P1
Fixed issue with home pager visibility checks. New try build running: https://tbpl.mozilla.org/?tree=Try&rev=43fc2bc9951b
Attachment #8378732 - Attachment is obsolete: true
Attachment #8378732 - Flags: review?(margaret.leibovic)
Attachment #8379162 - Flags: review?(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.
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 :(
Attachment #8375888 - Attachment description: Remove HomeBanner from TopSitesPanel → 01 - Remove HomeBanner from TopSitesPanel
Attachment #8377897 - Attachment description: Control HomeBanner visibility from home_pager_container → 02 - Control HomeBanner visibility from home_pager_container
Attachment #8375769 - Attachment is obsolete: true
Attachment #8377965 - Attachment is obsolete: true
Attachment #8378722 - Attachment is obsolete: true
Attachment #8377965 - Flags: review?(margaret.leibovic)
Attachment #8378722 - Flags: review?(margaret.leibovic)
Attachment #8379294 - Flags: review?(margaret.leibovic)
Attachment #8379162 - Attachment description: Update HomeBanner test for new implementation → 04 - Update HomeBanner test for new implementation
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.
Attachment #8379294 - Flags: review?(margaret.leibovic) → feedback+
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.
Attachment #8379162 - Flags: review?(margaret.leibovic) → feedback+
Copied change to tablet specific layout xml
Attachment #8377897 - Attachment is obsolete: true
Attachment #8379367 - Flags: review+
Attachment #8379294 - Attachment is obsolete: true
Attachment #8379369 - Flags: review?(margaret.leibovic)
Fixed up tests utilities in AboutHomeComponent for detecting visibility to reflect changes to how visibility is controlled in patch (2).
Attachment #8379162 - Attachment is obsolete: true
Attachment #8379370 - Flags: review?(margaret.leibovic)
Awesome this should be ready now :) 

The last try build succeeded and this one should too: https://tbpl.mozilla.org/?tree=Try&rev=9afb94e6262f
Attachment #8379369 - Attachment is obsolete: true
Attachment #8379369 - Flags: review?(margaret.leibovic)
Attachment #8379417 - Flags: review?(margaret.leibovic)
Blocks: 975239
Comment on attachment 8379417 [details] [diff] [review]
03 - Display HomeBanner on default page and animate

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

\o/
Attachment #8379417 - Flags: review?(margaret.leibovic) → review+
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!
Attachment #8379370 - Flags: review?(margaret.leibovic) → review+
Removed Log import
Attachment #8379370 - Attachment is obsolete: true
Attachment #8379845 - Flags: review+
Keywords: checkin-needed
I'll request approval to uplift to Aurora after it bakes on Nightly a bit.
Flags: needinfo?(margaret.leibovic)
Depends on: 976175
No longer depends on: 976175
Josh, can you make a folded patch for us to uplift to aurora?
Flags: needinfo?(margaret.leibovic)
Attached patch Folded patchSplinter Review
Here you go!
Attachment #8375888 - Attachment is obsolete: true
Attachment #8379367 - Attachment is obsolete: true
Attachment #8379417 - Attachment is obsolete: true
Attachment #8379845 - Attachment is obsolete: true
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
Attachment #8383221 - Flags: approval-mozilla-aurora?
Attachment #8383221 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Status: RESOLVED → VERIFIED
Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.
Blocks: 1014025
You need to log in before you can comment on or make changes to this bug.