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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: 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.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

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 2

[Geoff Brown [:gbrown]]

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.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

removed Log, added comment and added the race change
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb9c17efd1e4

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

backout wrong patch
https://hg.mozilla.org/integration/mozilla-inbound/rev/4132e64e267a

land right patch
https://hg.mozilla.org/integration/mozilla-inbound/rev/818fb2809aeb

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/818fb2809aeb