897481 - [fig] Update testShareLink.java.in - re-implement [Test share popup in Top Sites]

Status: RESOLVED FIXED

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

Description

[Mark Capella [:capella]]

Bug 896576 removed getAllPagesList from BaseTest disabling this test.

This needs to be re-implemented when the new Testing |utility class to get things from about:home| concept is finalized.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Update testShareLink for new about:home

Comment 2

[:Margaret Leibovic]

Comment on attachment 789575 [details] [diff] [review]
Update testShareLink for new about:home

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

This looks good, but I would prefer if you consider renaming some of the variables for clarity before landing.

::: mobile/android/base/tests/testShareLink.java.in
@@ +74,2 @@
>  
> +        ListView blist = findListViewWithTag("bookmarks");

I wonder if we should define these tags as constants somewhere.

@@ +80,3 @@
>  
> +        // Scroll down a bit so that the bookmarks list has more
> +        // items on screen. 

whitespace

@@ +81,5 @@
> +        // Scroll down a bit so that the bookmarks list has more
> +        // items on screen. 
> +        mActions.drag(width, width, mDriver.getGeckoHeight() - 10, mDriver.getGeckoHeight() / 2);
> +
> +        View bookmark = blist.getChildAt(1);

You should keep a comment about why we're getting the child at 1.

@@ +89,5 @@
> +        // Test the share popup in the Most Visited tab
> +        openAboutHomeTab(AboutHomeTabs.MOST_VISITED);
> +
> +        ListView tslist = findListViewWithTag("most_visited");
> +        mAsserter.is(waitForListToLoad(tslist), true, "list is properly loaded");

I think mvlist would be better a better name, since that matches the tag (I asssume tslist stands for top sites list). Or maybe we should even name it mostVisistedList to be even clearer, since that's not really that long.

@@ +91,5 @@
> +
> +        ListView tslist = findListViewWithTag("most_visited");
> +        mAsserter.is(waitForListToLoad(tslist), true, "list is properly loaded");
> +
> +        View allpages = tslist.getChildAt(1);

allpages? Isn't this just the top item in the most visited list? I think visitedItem or something like that would be a better name.

@@ +98,5 @@
> +
> +        // Test the share popup in the Most Recent tab
> +        openAboutHomeTab(AboutHomeTabs.MOST_RECENT);
> +
> +        ListView hlist = findListViewWithTag("most_recent");

Similar feelings about the variable name here.

Comment 3

[:Margaret Leibovic]

Comment on attachment 789575 [details] [diff] [review]
Update testShareLink for new about:home

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

::: mobile/android/base/tests/testShareLink.java.in
@@ +77,3 @@
>  
> +        int width = mDriver.getGeckoWidth() / 2;
> +        int y = mDriver.getGeckoHeight() / 2;

I just noticed you're not using this y variable. Get rid of it?

Also, width seems like a bad variable name, since this is actually half the width.

Actually, maybe what would be best is to just do:

int width = mDriver.getGeckoWidth();
int height = mDriver.getGeckoHeight();
mActions.drag(width/2, width/2, height - 10, height/2);

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #2)
> Comment on attachment 789575 [details] [diff] [review]
> Update testShareLink for new about:home
> 
> Review of attachment 789575 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, but I would prefer if you consider renaming some of the
> variables for clarity before landing.
> 
> ::: mobile/android/base/tests/testShareLink.java.in
> @@ +74,2 @@
> >  
> > +        ListView blist = findListViewWithTag("bookmarks");
> 
> I wonder if we should define these tags as constants somewhere.

Let's focus on organizing these things on the new API.

> @@ +80,3 @@
> >  
> > +        // Scroll down a bit so that the bookmarks list has more
> > +        // items on screen. 
> 
> whitespace

Fixed.

> @@ +81,5 @@
> > +        // Scroll down a bit so that the bookmarks list has more
> > +        // items on screen. 
> > +        mActions.drag(width, width, mDriver.getGeckoHeight() - 10, mDriver.getGeckoHeight() / 2);
> > +
> > +        View bookmark = blist.getChildAt(1);
> 
> You should keep a comment about why we're getting the child at 1.

Done.
 
> @@ +89,5 @@
> > +        // Test the share popup in the Most Visited tab
> > +        openAboutHomeTab(AboutHomeTabs.MOST_VISITED);
> > +
> > +        ListView tslist = findListViewWithTag("most_visited");
> > +        mAsserter.is(waitForListToLoad(tslist), true, "list is properly loaded");
> 
> I think mvlist would be better a better name, since that matches the tag (I
> asssume tslist stands for top sites list). Or maybe we should even name it
> mostVisistedList to be even clearer, since that's not really that long.

Done.

> @@ +91,5 @@
> > +
> > +        ListView tslist = findListViewWithTag("most_visited");
> > +        mAsserter.is(waitForListToLoad(tslist), true, "list is properly loaded");
> > +
> > +        View allpages = tslist.getChildAt(1);
> 
> allpages? Isn't this just the top item in the most visited list? I think
> visitedItem or something like that would be a better name.

Renamed, done.

> @@ +98,5 @@
> > +
> > +        // Test the share popup in the Most Recent tab
> > +        openAboutHomeTab(AboutHomeTabs.MOST_RECENT);
> > +
> > +        ListView hlist = findListViewWithTag("most_recent");
> 
> Similar feelings about the variable name here.

Done.

Comment 6

[Lucas Rocha (:lucasr)]

All green: https://tbpl.mozilla.org/?tree=Try&rev=9b63d4c33db2
Pushed: https://hg.mozilla.org/integration/fx-team/rev/5744bc7c930e

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5744bc7c930e