Closed Bug 1477002 Opened 2 years ago Closed 2 years ago

AccessibilityTest times out with USE_MULTIPROCESS=false

Categories

(GeckoView :: General, enhancement, P2)

enhancement

Tracking

(firefox-esr52 wontfix, firefox-esr60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: eeejay, Assigned: eeejay)

Details

Attachments

(2 files, 2 obsolete files)

If the tests are run with e10s off, they time out after the first test passes.

If the tests are run individually, they all pass. We should somehow figure out how to run this suite both with e10s on and off on a regular basis.
I think I figured this out! patch soon.
Assignee: nobody → eitan
This patch does several things:
 1. When "domwindowopened" is dispatched it often doesn't have a
document yet, so we need to wait for it to load before determining if we
should attach it.
 2. Instead of managing individual message managers use a broadcaster
and load delayed scripts. This makes new window additions more robust.
 3. A content script now doesn't need a ready/start message but
initializes in-line. This added more complexity which we don't need. All
the info that we passed to it in AccessFu:Start can be gotten in other
ways (also, Services.appinfo.ID now works in child processes, so no need
for that).
 4. Tweaked the tests to support inline frame script initilization.
 5. Removed the scroll callback from content-script.js that was not used
anymore.
Attachment #8993804 - Flags: review?(yzenevich)
Doesn't seem to apply any more.
Flags: needinfo?(eitan)
This patch does several things:
 1. When "domwindowopened" is dispatched it often doesn't have a
document yet, so we need to wait for it to load before determining if we
should attach it.
 2. Instead of managing individual message managers use a broadcaster
and load delayed scripts. This makes new window additions more robust.
 3. A content script now doesn't need a ready/start message but
initializes in-line. This added more complexity which we don't need. All
the info that we passed to it in AccessFu:Start can be gotten in other
ways (also, Services.appinfo.ID now works in child processes, so no need
for that).
 4. Tweaked the tests to support inline frame script initilization.
 5. Removed the scroll callback from content-script.js that was not used
anymore.
Attachment #8994639 - Flags: review?(yzenevich)
Attachment #8993804 - Attachment is obsolete: true
Attachment #8993804 - Flags: review?(yzenevich)
Did you try the one I just attached?
Flags: needinfo?(eitan)
P2 because GeckoView uses e10s by default so this doesn't block Focus+GeckoView.
Comment on attachment 8994639 [details] [diff] [review]
Use message broadcaster to manage jsat content scripts. r?yzen

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

This is great, thanks!

::: accessible/jsat/AccessFu.jsm
@@ +28,5 @@
>    SET_SELECTION: "GeckoView:AccessibilitySetSelection",
>    CLIPBOARD: "GeckoView:AccessibilityClipboard",
>  };
>  
> +const ACCESSFU_MESSAGE = {

This should go into Constants.jsm

@@ +33,5 @@
> +  PRESENT: "AccessFu:Present",
> +  DOSCROLL: "AccessFu:DoScroll",
> +};
> +
> +const FRAME_SCRIPT = "chrome://global/content/accessibility/content-script.js";

Perhaps this too since we do use this in jsatcommon

::: accessible/jsat/Utils.jsm
@@ +399,5 @@
>      let message = (typeof(args[0]) === "function" ? args[0]() : args).join(" ");
>      message = "[" + Utils.ScriptName + "] " + this._LEVEL_NAMES[aLogLevel + 1] +
>        " " + message + "\n";
>      dump(message);
> +    // Note: used for testing purposes. If |this.useConsoleService| is true, also log

nit: this comment should probably move to where useConsoleService is declared (L391).

::: accessible/jsat/content-script.js
@@ +16,2 @@
>    "resource://gre/modules/accessibility/ContentControl.jsm");
>  ChromeUtils.defineModuleGetter(this, "Roles",

Roles is no longer used as well as Presentation.

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/AccessibilityTest.kt
@@ +357,5 @@
>  
>      @Test fun testMoveByLine() {
>          var nodeId = AccessibilityNodeProvider.HOST_VIEW_ID
>          sessionRule.session.loadTestPath(LOREM_IPSUM_HTML_PATH)
> +        waitForInitialFocus();

There are now more tests fun's could you update those too?
Attachment #8994639 - Flags: review?(yzenevich) → review+
This patch does several things:
 1. When "domwindowopened" is dispatched it often doesn't have a
document yet, so we need to wait for it to load before determining if we
should attach it.
 2. Instead of managing individual message managers use a broadcaster
and load delayed scripts. This makes new window additions more robust.
 3. A content script now doesn't need a ready/start message but
initializes in-line. This added more complexity which we don't need. All
the info that we passed to it in AccessFu:Start can be gotten in other
ways (also, Services.appinfo.ID now works in child processes, so no need
for that).
 4. Tweaked the tests to support inline frame script initilization.
 5. Removed the scroll callback from content-script.js that was not used
anymore.
Attachment #8994639 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/beb25a105a98
Use message broadcaster to manage jsat content scripts. r=yzen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/beb25a105a98
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 63 → mozilla63
You need to log in before you can comment on or make changes to this bug.