Closed Bug 899183 Opened 6 years ago Closed 6 years ago

[fig] Re-implement robocop testBookmarklets.java.in

Categories

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

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 26
Tracking Status
fennec 26+ ---

People

(Reporter: capella, Assigned: lucasr)

References

Details

Attachments

(1 file)

Bug 896576 removed the getBookmarksList() logic from BaseTest.java.in and disabled this test (which depends on it).

This needs to be re-implemented when the new Testing |utility class to get things from about:home| concept is finalized.
Attachment #789598 - Flags: review?(margaret.leibovic)
Assignee: nobody → lucasr.at.mozilla
Comment on attachment 789598 [details] [diff] [review]
Update testBookmarklets for new about:home

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

r+ with comments addressed.

::: mobile/android/base/tests/testBookmarklets.java.in
@@ +39,5 @@
>          // add the bookmarklet to the database. there's currently no way to
>          // add this using the UI, so we go through the content provider.
>          addOrUpdateMobileBookmark(title, js);
>  
> +        // Test the share popup in the Bookmarks page

This comment looks wrong.

@@ +49,5 @@
> +        int width = mDriver.getGeckoWidth() / 2;
> +        int y = mDriver.getGeckoHeight() / 2;
> +
> +        // Scroll down so that the bookmarks list has more items on screen.
> +        mActions.drag(width, width, mDriver.getGeckoHeight() - 10, mDriver.getGeckoHeight() / 2);

Reviewing this patch actually prompted me to revisit and add a new comment to the patch in bug 897481. Same comment applies here.

And since this is copy/pasted, maybe it should be pulled into a method on AboutHomeTest.

@@ +56,1 @@
>          Boolean found = false;

Any reason that this is a Boolean instead of a boolean?

@@ +56,4 @@
>          Boolean found = false;
> +        for (int i = bookmarks.getHeaderViewsCount(); i < bookmarks.getAdapter().getCount(); i++) {
> +            Cursor c = (Cursor)bookmarks.getItemAtPosition(i);
> +            String turl = c.getString(c.getColumnIndexOrThrow("url"));

Can we come up with a better variable name than turl?
Attachment #789598 - Flags: review?(margaret.leibovic) → review+
tracking-fennec: --- → ?
Priority: -- → P1
Hardware: ARM → All
tracking-fennec: ? → 26+
(In reply to :Margaret Leibovic from comment #2)
> Comment on attachment 789598 [details] [diff] [review]
> Update testBookmarklets for new about:home
> 
> Review of attachment 789598 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments addressed.
> 
> ::: mobile/android/base/tests/testBookmarklets.java.in
> @@ +39,5 @@
> >          // add the bookmarklet to the database. there's currently no way to
> >          // add this using the UI, so we go through the content provider.
> >          addOrUpdateMobileBookmark(title, js);
> >  
> > +        // Test the share popup in the Bookmarks page
> 
> This comment looks wrong.

Fixed.
 
> @@ +49,5 @@
> > +        int width = mDriver.getGeckoWidth() / 2;
> > +        int y = mDriver.getGeckoHeight() / 2;
> > +
> > +        // Scroll down so that the bookmarks list has more items on screen.
> > +        mActions.drag(width, width, mDriver.getGeckoHeight() - 10, mDriver.getGeckoHeight() / 2);
> 
> Reviewing this patch actually prompted me to revisit and add a new comment
> to the patch in bug 897481. Same comment applies here.
> 
> And since this is copy/pasted, maybe it should be pulled into a method on
> AboutHomeTest.

Fixed.

> @@ +56,1 @@
> >          Boolean found = false;
> 
> Any reason that this is a Boolean instead of a boolean?

Nope, fixed.
 
> @@ +56,4 @@
> >          Boolean found = false;
> > +        for (int i = bookmarks.getHeaderViewsCount(); i < bookmarks.getAdapter().getCount(); i++) {
> > +            Cursor c = (Cursor)bookmarks.getItemAtPosition(i);
> > +            String turl = c.getString(c.getColumnIndexOrThrow("url"));
> 
> Can we come up with a better variable name than turl?

aUrl? Done.
https://hg.mozilla.org/mozilla-central/rev/7848c8ff7608
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.