Robotium: Add simple test for 'Open Link' context menu

RESOLVED FIXED in Firefox 13

Status

()

Firefox for Android
General
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

unspecified
Firefox 13
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 596424 [details] [diff] [review]
patch

This patch adds a simple test and testpage for checking the context menu support on a web page. Long tapping on a link should make the context menu appear. Opening link in a new tab should cause a new tab to be opened with the new page.

Passes on honeycomb and gingerbread. Passed on Try.
Attachment #596424 - Flags: review?(gbrown)
Comment on attachment 596424 [details] [diff] [review]
patch

>+        mAsserter.ok(mSolo.waitForText("Open"), "looking for context menu action", "found 'Open Link'");
>+        mSolo.clickOnText("Open");
>+
>+        // Wait for the new tab and page to load
>+        mActions.expectGeckoEvent("Tab:Added").blockForEvent();
>+        mActions.expectGeckoEvent("DOMContentLoaded").blockForEvent();
>+

Drive-by comment: this part seems racy; the events could fire between clickOnText() and the listener registration in expectGeckoEvent(). It would be better to call expectGeckoEvent before doing the click, and then blockForEvent() after. expectGeckoEvent returns an EventExpecter for exactly this reason... although perhaps that needs to be better documented :)
Comment on attachment 596424 [details] [diff] [review]
patch

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

r+ with race addressed.

::: mobile/android/base/tests/testWebContentContextMenu.java.in
@@ +16,5 @@
> +        loadUrl(url);
> +
> +        DisplayMetrics dm = new DisplayMetrics();
> +        getActivity().getWindowManager().getDefaultDisplay().getMetrics(dm);
> +        Log.i("TBN", "******** density= " + dm.density);

We are trying to keep Log.i() statements out of the tests. Either remove this, or use mAsserter.dumpLog (goes to logcat + conditionally to log file).

@@ +18,5 @@
> +        DisplayMetrics dm = new DisplayMetrics();
> +        getActivity().getWindowManager().getDefaultDisplay().getMetrics(dm);
> +        Log.i("TBN", "******** density= " + dm.density);
> +
> +        float top = mDriver.getGeckoTop() + 30 * dm.density;

It took me a minute to realize why you were using 30 here -- consider adding a one line comment.

@@ +27,5 @@
> +        mSolo.clickOnText("Open");
> +
> +        // Wait for the new tab and page to load
> +        mActions.expectGeckoEvent("Tab:Added").blockForEvent();
> +        mActions.expectGeckoEvent("DOMContentLoaded").blockForEvent();

I agree with :kats' racy comment.
Attachment #596424 - Flags: review?(gbrown) → review+
removed Log, added comment and added the race change
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb9c17efd1e4
https://hg.mozilla.org/mozilla-central/rev/818fb2809aeb
Assignee: nobody → mark.finkle
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
You need to log in before you can comment on or make changes to this bug.