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
•