We are rearranging the tabs. The UI tests have to updated accordingly.

This patch fixes the obvious problems in AboutHomeTest. Try push here: https://tbpl.mozilla.org/?tree=Try&rev=acbd45a72332 This patch does some lazy stuff with swiping to the correct AboutHome screen, by overshooting and then clicking to the right screen - it wouldn't be too bad to count the indexes and swipe exactly the correct number of times, so I can do that if it's preferred.

Comment on attachment 806646 [details] [diff] [review] Part 1: fix AboutHomeTest Review of attachment 806646 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/tests/AboutHomeTest.java.in @@ +158,3 @@ > /** > + * This method can be used to open the different tabs of about:home. > + * With four panes, we want to swipe to the correct half of the view pages. This is clever :) Maybe add a little more detail to the comment here about how we do this so that we can click on the correct tab title. @@ +170,3 @@ > switch (tab) { > + case TOP_SITES : { > + // Swipe right left twice (page index 1). Is there a typo here? I don't understand this comment. @@ +170,5 @@ > switch (tab) { > + case TOP_SITES : { > + // Swipe right left twice (page index 1). > + mActions.drag(0, halfWidth, halfHeight, halfHeight); > + mActions.drag(0, halfWidth, halfHeight, halfHeight); I wonder if it would be worthwhile to factor these out into helper methods for moving one page to the right or one page to the left. Or actually, since it looks like we always do them in pairs we could have methods to move all the way to the left or all the way to the right.

This patch has better swiping behavior, and is more general. There's a little ugliness because AboutHomeTabs doesn't exactly match aboutHomeTabs (maybe this should be aboutHomePages).

Comment on attachment 806810 [details] [diff] [review] [alternative] Part 1: better swiping Not quite finished yet, I found out what was causing the oranges - tablet mode.

Comment on attachment 806810 [details] [diff] [review] [alternative] Part 1: better swiping Review of attachment 806810 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/tests/AboutHomeTest.java.in @@ +24,5 @@ > * To use any of these methods in your test make sure it extends AboutHomeTest instead of BaseTest > */ > abstract class AboutHomeTest extends BaseTest { > + protected enum AboutHomeTabs {MOST_RECENT, TABS_FROM_LAST_TIME, TOP_SITES, BOOKMARKS, READING_LIST}; > + protected final String HISTORY_TAB = "HISTORY"; // AboutHomeTabs does not contain a HISTORY item. I'm confused here - why can't you just add HISTORY to AboutHomeTabs?

This patch updates the existing for the new AboutHome. There's a hack in here that I haven't found the reason for, where (inexplicably) swipe events are getting rejected [1], unless we initiate an about:home swipe event before the problematic mDriver.getGeckoTop call. I'll do some more investigation, but the try push is here: https://tbpl.mozilla.org/?tree=Try&rev=5f5329683477 [1] 09-19 15:45:22.827: W/InputDispatcher(391): Permission denied: injecting event from pid 8028 uid 10058 to window Window{41fee300 u0 com.android.launcher/com.android.launcher2.Launcher} owned by uid 10029 09-19 15:45:22.827: W/InputManager(391): Input event injection from pid 8028 permission denied.

Adds testing for new Top Sites.

Comment on attachment 807382 [details] [diff] [review] Part 1: Update tests for tablet, new AboutHome Review of attachment 807382 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good to me. I think that we should just try to land something that works to fix the oranges now, and we can work back on m-c to continue to make the tests better. ::: mobile/android/base/tests/AboutHomeTest.java.in @@ +224,5 @@ > + } > + } else { > + mSolo.clickOnText(tab.toString()); > + } > + return; Let's factor this out into a different method, perhaps clickAboutHomeTab(Tab tab);

Comment on attachment 807384 [details] [diff] [review] Part 2: Add testing for new Top Sites layout Review of attachment 807384 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/tests/testShareLink.java.in @@ +81,5 @@ > + // Prepopulate top sites with more than 4 history items to overflow tiles. > + inputAndLoadUrl(getAbsoluteUrl("/robocop/robocop_blank_01.html")); > + inputAndLoadUrl(getAbsoluteUrl("/robocop/robocop_blank_02.html")); > + inputAndLoadUrl(getAbsoluteUrl("/robocop/robocop_blank_03.html")); > + inputAndLoadUrl(getAbsoluteUrl("/robocop/robocop_boxes.html")); It would be nice if we could just stuff these directly into the DB, rather than loading them like this. Why not just use DataBaseHelper.addOrUpdateMobileBookmark: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/DatabaseHelper.java.in#56 I believe the query will return unvisited bookmarks, although they won't be listed as high as visited ones.

Responded to comments, I think this is an all-green try build: https://tbpl.mozilla.org/?tree=Try&rev=9a819eed62d3

Comment on attachment 807770 [details] [diff] [review] Part 1: Update AboutHomeTest v2 Review of attachment 807770 [details] [diff] [review]: ----------------------------------------------------------------- I have some questions, but if this is green, let's go ahead and land it. ::: mobile/android/base/tests/AboutHomeTest.java.in @@ +205,5 @@ > focusUrlBar(); > ViewPager pager = (ViewPager)mSolo.getView(ViewPager.class, 0); > + > + // Handle tablets by just clicking the visible tab title. > + if (mDevice.type.equals("tablet")) { I think you may have misunderstood my last review comment, I meant factoring out this whole tablet case into a different method, not just the code to click on the top-level about:home tab. But this isn't a big deal, we can clean this up later if we want. @@ +207,5 @@ > + > + // Handle tablets by just clicking the visible tab title. > + if (mDevice.type.equals("tablet")) { > + // Just click for tablets, since all the titles are visible. > + if (AboutHomeTabs.HISTORY == tab || AboutHomeTabs.MOST_RECENT == tab || AboutHomeTabs.TABS_FROM_LAST_TIME == tab) { Shouldn't the HISTORY case just do clickAboutHomeTab(tab) like the else case here? @@ +261,4 @@ > waitForAboutHomeTab(aboutHomeTabs.indexOf(tab.toString())); > break; > } > + case TABS_FROM_LAST_TIME: { Nit: For readability, put the TABS_FROM_LSAT_TIME case next to the MOST_RECENT case, since they have similar logic because they're both in the HISTORY page.

Comment on attachment 807771 [details] [diff] [review] Part 2: Add testing for new Top Sites layout Review of attachment 807771 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/tests/testShareLink.java.in @@ +89,5 @@ > + inputAndLoadUrl(getAbsoluteUrl("/robocop/robocop_blank_05.html")); > + inputAndLoadUrl(getAbsoluteUrl("/robocop/robocop_boxes.html")); > + inputAndLoadUrl(getAbsoluteUrl("/robocop/robocop_search.html")); > + inputAndLoadUrl(getAbsoluteUrl("/robocop/robocop_text_page.html")); > + } I still don't really like that this will take a while to run, but I agree it's good to do black-box style testing, and this is the way to get that list to fill up. If we ever write another test that requires the top sites list to be populated, I think we should consider putting this in its own test, so that we never end up needing to do this in multiple tests.

Addressed comments, carrying over r+

(wrong patch)

https://hg.mozilla.org/mozilla-central/rev/20d55d6b15ec https://hg.mozilla.org/mozilla-central/rev/ccea777b4002

Comment on attachment 807845 [details] [diff] [review] Part 1: Update AboutHomeTest v3 [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 862793 (the new about:home UI) User impact if declined: This is part of a series of (high priority) UI changes that we'd like to get in for Firefox 26. Here's a summary: * History and Top Sites are separate pages again * Thumbnails in the Top Sites page (instead of Bookmarks page) * When entering Home, Top Sites is always the default page Testing completed (on m-c, etc.): This patch has been in our Nightly builds for almost a week now. No obvious regressions found. UI tests have been updated accordingly in this patch. Risk to taking this patch (and alternatives if risky): There is some level of risk involved here as this is not a trivial change. However, we've agreed that these changes are important enough to be worth the risk. We'd like to avoid shipping a rather big UI change in Firefox 26 to then change it all over again in the following release as this would be a too disruptive experience for our users. Given the risk involved and the fact that we're adding/changing strings, it's important to have these patches uplifted as soon as possible. String or IDL/UUID changes made by this patch: n/a This is part of new-new-abouthome (see bug 917394)

Comment on attachment 807771 [details] [diff] [review] Part 2: Add testing for new Top Sites layout [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 862793 (the new about:home UI) User impact if declined: This is part of a series of (high priority) UI changes that we'd like to get in for Firefox 26. Here's a summary: * History and Top Sites are separate pages again * Thumbnails in the Top Sites page (instead of Bookmarks page) * When entering Home, Top Sites is always the default page Testing completed (on m-c, etc.): This patch has been in our Nightly builds for almost a week now. No obvious regressions found. UI tests have been updated accordingly in this patch. Risk to taking this patch (and alternatives if risky): There is some level of risk involved here as this is not a trivial change. However, we've agreed that these changes are important enough to be worth the risk. We'd like to avoid shipping a rather big UI change in Firefox 26 to then change it all over again in the following release as this would be a too disruptive experience for our users. Given the risk involved and the fact that we're adding/changing strings, it's important to have these patches uplifted as soon as possible. String or IDL/UUID changes made by this patch: n/a This is part of new-new-abouthome (see bug 917394)

Add dependencies on the new content that is tested by these patches.

https://hg.mozilla.org/releases/mozilla-aurora/rev/607a8726ced1 https://hg.mozilla.org/releases/mozilla-aurora/rev/0a14b7023a95