Closed
Bug 903478
Opened 11 years ago
Closed 11 years ago
[fig] Update testHistoryTab (now called testMostRecentPage) for new about:home
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 26
People
(Reporter: lucasr, Assigned: lucasr)
References
Details
Attachments
(3 files)
6.08 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
2.23 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
10.49 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #788978 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #788979 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #788980 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 4•11 years ago
|
||
FYI: this is a straight port of the original testHistoryTab.
Comment 5•11 years ago
|
||
Comment on attachment 788978 [details] [diff] [review] (1/3) Tag all ListViews in the new about:home UI Review of attachment 788978 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/HomePager.java @@ +41,5 @@ > + static final String LIST_TAG_BOOKMARKS = "bookmarks"; > + static final String LIST_TAG_READING_LIST = "reading_list"; > + static final String LIST_TAG_MOST_VISITED = "most_visited"; > + static final String LIST_TAG_MOST_RECENT = "most_recent"; > + static final String LIST_TAG_LAST_TABS = "last_tabs"; Add a comment documenting what these are used for. Hopefully that will help us remember to add new tags for any new fragments we create in the future.
Attachment #788978 -
Flags: review?(margaret.leibovic) → review+
Comment 6•11 years ago
|
||
Comment on attachment 788979 [details] [diff] [review] (2/3) Add findListViewWithTag() to BaseTest Review of attachment 788979 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/tests/BaseTest.java.in @@ +282,5 @@ > } > mAsserter.is(urlBarText, url, "Browser toolbar URL stayed the same"); > } > > + public final ListView findListViewWithTag(String tag) { Nit: add some documentation on top of this method. @@ +295,5 @@ > + } > + } > + > + return null; > + } This does seem generic enough to belong in BaseTest, but if it's only going to be used for about:home views, I wonder if it would be better in AboutHomeTest.
Attachment #788979 -
Flags: review?(margaret.leibovic) → review+
Comment 7•11 years ago
|
||
Comment on attachment 788980 [details] [diff] [review] (3/3) Add testMostRecentPage for new about:home Review of attachment 788980 [details] [diff] [review]: ----------------------------------------------------------------- I know you're just porting over testHistoryTab, but it's still kinda painful to see some of this code being added back. Please be sure to file a follow-up on tidying this up, and perhaps add some comments in places where you know we want something to change. ::: mobile/android/base/tests/testMostRecentPage.java.in @@ +174,5 @@ > + } > + } > + > + private void testClick(final ListView listview, String url) { > + // waitForText(url); Can this just be removed? @@ +222,5 @@ > + uri = uri.buildUpon().appendQueryParameter("profile", "default") > + .appendQueryParameter("sync", "true").build(); > + resolver.delete(uri, "url = ?", new String[] { > + "http://mochi.test:8888/tests/robocop/robocop_blank_01.html" > + }); We should udpate this use the new deleteBookmark method in AboutHomeTest. It's okay to do that in a follow-up.
Attachment #788980 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 8•11 years ago
|
||
(In reply to :Margaret Leibovic from comment #5) > Comment on attachment 788978 [details] [diff] [review] > (1/3) Tag all ListViews in the new about:home UI > > Review of attachment 788978 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/home/HomePager.java > @@ +41,5 @@ > > + static final String LIST_TAG_BOOKMARKS = "bookmarks"; > > + static final String LIST_TAG_READING_LIST = "reading_list"; > > + static final String LIST_TAG_MOST_VISITED = "most_visited"; > > + static final String LIST_TAG_MOST_RECENT = "most_recent"; > > + static final String LIST_TAG_LAST_TABS = "last_tabs"; > > Add a comment documenting what these are used for. Hopefully that will help > us remember to add new tags for any new fragments we create in the future. Done.
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to :Margaret Leibovic from comment #6) > Comment on attachment 788979 [details] [diff] [review] > (2/3) Add findListViewWithTag() to BaseTest > > Review of attachment 788979 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/tests/BaseTest.java.in > @@ +282,5 @@ > > } > > mAsserter.is(urlBarText, url, "Browser toolbar URL stayed the same"); > > } > > > > + public final ListView findListViewWithTag(String tag) { > > Nit: add some documentation on top of this method. Done. > @@ +295,5 @@ > > + } > > + } > > + > > + return null; > > + } > > This does seem generic enough to belong in BaseTest, but if it's only going > to be used for about:home views, I wonder if it would be better in > AboutHomeTest. Fair enough. Moved to AboutHomeTest.
Assignee | ||
Comment 10•11 years ago
|
||
Pushed the first two patches in order to land the fix for bug 903448: http://hg.mozilla.org/projects/fig/rev/45ed1f18184f http://hg.mozilla.org/projects/fig/rev/fbbc87078c3c I'll push the last patch once the test goes green on try.
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/45ed1f18184f https://hg.mozilla.org/mozilla-central/rev/fbbc87078c3c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•