Closed Bug 1157637 Opened 6 years ago Closed 6 years ago

Create ActionBar Handler and Gecko SelectionCarets tests

Categories

(Firefox for Android Graveyard :: Text Selection, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: capella, Assigned: capella)

Details

Attachments

(1 file, 4 obsolete files)

Work on bug 988143 pointed out an issue with the core/Gecko SelectionCarets code, which we corrected in bug 1156037.

This provides a good case to start work on the new test infra we'll need before final cut-over from the original Android carets.

Posting and cc:ing for now... I'll r? to margaret later ... we still have approvals outstanding on bug 988143 before we get to hers. After that we'll tag her with this one also ;-)
Attached patch bug1157637.diff (obsolete) — Splinter Review
Assignee: nobody → markcapella
Attachment #8596517 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8596885 - Flags: review?(margaret.leibovic)
Comment on attachment 8596885 [details] [diff] [review]
bug1157637.diff

Pulling review om this ... It needs some refinements to get past the TRY server.
Attachment #8596885 - Flags: review?(margaret.leibovic)
Attached patch bug1157637.diff (obsolete) — Splinter Review
This is tighter than the first one. Also see:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=983be19d8072
(all oranges unrelated to this test).
Attachment #8596885 - Attachment is obsolete: true
Attachment #8598519 - Flags: review?(margaret.leibovic)
Comment on attachment 8598519 [details] [diff] [review]
bug1157637.diff

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

I think we should take this opportunity to make this into a proper JavaScript test. Let me know if you need help doing that. I know this is more work right now, but I think it's the right investment before writing even more testcases for this new code.

In the long term, I think we should find a path forward to deprecate this roboextender stuff and remove it from the tree.
Nice converted version ... works well at home, no push to try yet.
Attachment #8598519 - Attachment is obsolete: true
Attachment #8598519 - Flags: review?(margaret.leibovic)
Attachment #8601004 - Flags: review?(margaret.leibovic)
Push to try, trigger buncha times looking for random oranges ... found none :-)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecbbc0937cfc
Comment on attachment 8601004 [details] [diff] [review]
bug1157637_testSelectionCarets.diff

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

Nice! Thanks for taking the time to update this.

I have some comments to address, but this is definitely going in the right direction.

::: mobile/android/tests/browser/robocop/testSelectionCarets.java
@@ +42,5 @@
> +        super.setUp();
> +
> +        // Ensure Gecko Selection and Touch carets are enabled.
> +        PrefsHelper.setPref(SELECTION_CARETS_PREF, true);
> +        PrefsHelper.setPref(TOUCH_CARET_PREF, true);

Since this is a JavaScript test, you can just do this directly in the JS part. We have some other test that do similar things:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/testTrackingProtection.js#119

@@ +70,5 @@
> +                final MotionEvent motionEvent =
> +                    MotionEvent.obtain(meTime, meTime, MotionEvent.ACTION_DOWN, meX, meY, 0);
> +
> +                final GeckoEvent geckoEvent = GeckoEvent.createLongPressEvent(motionEvent);
> +                GeckoAppShell.sendEventToGecko(geckoEvent);

Hm, I wonder if there's any way to synthesize a long press straight from JS... if there isn't a straightforward way to do that, I'm fine with leaving this logic in Java.

@@ +95,5 @@
> +                    }
> +                    final GeckoEvent event =
> +                        GeckoEvent.createBroadcastEvent(TAB_CHANGE_EVENT, args.toString());
> +                    GeckoAppShell.sendEventToGecko(event);
> +                    break;

Instead of doing this message passing business, could you just use a "TabSelect" event listener in JS?

::: mobile/android/tests/browser/robocop/testSelectionCarets.js
@@ +9,5 @@
> +Cu.import("resource://gre/modules/Task.jsm");
> +Cu.import("resource://gre/modules/Messaging.jsm");
> +Cu.import('resource://gre/modules/Geometry.jsm');
> +
> +const WIN = Services.wm.getMostRecentWindow("navigator:browser");

Nit: Name this `gChromeWin` to be more consistent with other places in the tree where we declare a similar local variable.

@@ +13,5 @@
> +const WIN = Services.wm.getMostRecentWindow("navigator:browser");
> +const TEST_URL = "http://mochi.test:8888/tests/robocop/testSelectionCarets.html";
> +
> +// Hold a ref to the ActionBar for testing state.
> +const ACTION_BAR = WIN.ActionBarHandler;

Nit: Call this variable `ActionBarHandler` (similar to how we declare `BrowserApp` local variables in other tests).

@@ +25,5 @@
> +const TAB_CHANGE_EVENT = "testSelectionCarets:TabChange";
> +const TAB_STOP_EVENT = "STOP";
> +
> +// Ref to the loaded test document.
> +let doc;

To avoid scope confusion, I would just make this a local variable inside the testSelectionCarets generator, then pass it as a parameter to helper functions that need it.

@@ +37,5 @@
> +
> +function is(lhs, rhs, text) {
> +  do_report_result(lhs === rhs, "[ " + lhs + " === " + rhs + " ] " + text,
> +    Components.stack.caller, false);
> +}

Instead of declaring your own helpers here, you should use ones that are defined in robocop_head.js. You can look at other JavaScriptTest implementations to see how we do assertions.

(I also filed bug 1158925 to move even more shared functions into that file to avoid continual copy/paste between tests.)

@@ +133,5 @@
> +function do_promiseLongPressResult(midPoint) {
> +  return new Promise(resolve => {
> +    let observer = (subject, topic, data) => {
> +      if (topic === STATUS_UPDATE_EVENT) {
> +        let text = ACTION_BAR._getSelectedText();

Since ACTION_BAR is only used in here, you could also just make ActionBarHandler a local variable in here, the same way you did with BrowserApp down below.
Attachment #8601004 - Flags: review?(margaret.leibovic) → feedback+
re: Hm, I wonder if there's any way to synthesize a long press straight from JS...

   I need to synthesize this from Java to parallel the current process. We create longpress in APZ code [1] and pass it across the bridge as an AndroidGeckoEvent through AppShell to the android widget code [2], where it eventually hits SelectionCarets. I don't see a way to do this going from JS via XPCOM straight to Gecko, if that's the path you were considering.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/JavaPanZoomController.java?rev=4d66d5d58232#1358
[2] http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp?rev=9c22b105b0e3#835


re: Instead of doing this message passing business, could you just use a "TabSelect" event listener in JS?

   Our Javascript tests start out loading about:home, then the test script into tab 0 via a tab load with a url like [1]. 'TabSelect' events trigger at the beginning of the load cycle. I want to avoid loading my actual testpage until that tab is finished loading, to allow Java to finalize its gfx/layer/viewport/zoomConstraints information for the first one, before moving to the other.

   tl;dr is that I was receiving random-oranges in initial testing that I tracked to apparent Java concurrency issues in that area, screwing up the zoom scaling factor and preventing the longpress from triggering text selection in the proper place. Serializing the tab load order by keying off the final tab "STOP" event controls for this.

[1] http://mochi.test:8888/tests/robocop/robocop_javascript.html?slug=1430992702161&path=testSelectionCarets.js


re: To avoid scope confusion, I would just make this a local variable inside the testSelectionCarets generator, then pass it as a parameter.

   I changed this as requested, but plead guilty with an explanation. I had done it that way originally, as it seemed to me a little awkward for |doc| to be passed from the main test through 8 method calls to getFirstCharPressPoint() where it's not actually used, to the final consumer selectElementFirstChar().


re: Instead of declaring your own helpers here, you should use ones that are defined in robocop_head.js.

   Currently there are no similar functions I see in robocop_head.js that allows you to pass a text message associated with the assert. I find more value in something like these helpers, and actually cloned it from places like [1].

   I had noticed that bug you filed to clean up stuff like this and figured this would also get pulled into that effort. If it helps, I can this bugs' helpers there, and add a note to the bug pointing out the new cases (below [1]) to be included in the back-fill.

[1] http://mxr.mozilla.org/mozilla-central/search?string=lhs%2C+rhs%2C+text&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Attachment #8601004 - Attachment is obsolete: true
Attachment #8602685 - Flags: review?(margaret.leibovic)
Comment on attachment 8602685 [details] [diff] [review]
bug1157637_testSelectionCarets.diff

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

If it's green on try, good to go!

::: mobile/android/tests/browser/robocop/testSelectionCarets.js
@@ +164,5 @@
> +  yield do_promiseTabChangeEvent(BrowserApp.selectedTab.id, TAB_STOP_EVENT);
> +
> +  // Ensure Gecko Selection and Touch carets are enabled.
> +  Services.prefs.setBoolPref(SELECTION_CARETS_PREF, true);
> +  Services.prefs.setBoolPref(TOUCH_CARET_PREF, true);

We should also clear these prefs in the cleanup function below.
Attachment #8602685 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/101feffdaed8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.