811307 - [AccessFu] Add mochitest for enabling

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

Breaking the ice on AccessFu (jsat) mochitests.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 681040 [details] [diff] [review]
patch

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

::: accessible/tests/mochitest/jsat/test_alive.html
@@ +58,5 @@
> +
> +  <a target="_blank"
> +     href="https://bugzilla.mozilla.org/show_bug.cgi?id=??????"
> +     title="Add prelimenary jsat (AccessFu) support">
> +    Mozilla Bug ??????

I need to update this of course.

Comment 3

[Marco Zehe (:MarcoZ)]

Comment on attachment 681040 [details] [diff] [review]
patch

Please change the title of the test_alive.html file. ;)

Other than that, this looks correct to me. Even this monstrous call in utils.js. ;)

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Marco Zehe (:MarcoZ) from comment #3)
> Comment on attachment 681040 [details] [diff] [review]
> patch
> 
> Please change the title of the test_alive.html file. ;)

Good catch. Fixed locally.

Comment 5

[Eitan Isaacson (Leave thru 12/23) [:eeejay]]

Comment on attachment 681040 [details] [diff] [review]
patch

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

This is good. Pending on a new place to put the top level window getter.

::: accessible/src/jsat/AccessFu.jsm
@@ +99,5 @@
>      this.chromeWin.addEventListener('TabOpen', this);
>      this.chromeWin.addEventListener('TabSelect', this);
> +
> +    if (this.readyCallback)
> +      this.readyCallback();

Nice. This is low-profile, and doesn't introduce more API complexity.

::: accessible/tests/mochitest/jsat/test_alive.html
@@ +36,5 @@
> +
> +      AccessFu.readyCallback = function readyCallback() {
> +        gAccessFuStarted = true;
> +      }
> +     

Trailing whitespace. Do we care about that?

::: accessible/tests/mochitest/utils.js
@@ +1,4 @@
> +/**
> + * Return main window (crosses chrome boundary)
> + */
> + function getMainWindow(aWindow)

The name should probably indicate top-level and chrome, maybe getTopChromeWindow?

Seems excessive to start a new utils file for this. What about dropping it in common.js, or have a jsat_common.js since we will probably need this in all jsat tests.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Eitan Isaacson [:eeejay] from comment #5)
> Comment on attachment 681040 [details] [diff] [review]
> patch
> 
> Review of attachment 681040 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is good. Pending on a new place to put the top level window getter.
> 
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +99,5 @@
> >      this.chromeWin.addEventListener('TabOpen', this);
> >      this.chromeWin.addEventListener('TabSelect', this);
> > +
> > +    if (this.readyCallback)
> > +      this.readyCallback();
> 
> Nice. This is low-profile, and doesn't introduce more API complexity.

Thanks.

> 
> ::: accessible/tests/mochitest/jsat/test_alive.html
> @@ +36,5 @@
> > +
> > +      AccessFu.readyCallback = function readyCallback() {
> > +        gAccessFuStarted = true;
> > +      }
> > +     
> 
> Trailing whitespace. Do we care about that?
> 
> ::: accessible/tests/mochitest/utils.js
> @@ +1,4 @@
> > +/**
> > + * Return main window (crosses chrome boundary)
> > + */
> > + function getMainWindow(aWindow)
> 
> The name should probably indicate top-level and chrome, maybe
> getTopChromeWindow?

Sure I can make that change. I had been biased with the variable name "mainWindow" in the related wiki:
https://developer.mozilla.org/en/docs/Working_with_windows_in_chrome_code

> 
> Seems excessive to start a new utils file for this. What about dropping it
> in common.js, or have a jsat_common.js since we will probably need this in
> all jsat tests.

I originally had it in common.js... I'll move it back there.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch2

Comment 8

[Eitan Isaacson (Leave thru 12/23) [:eeejay]]

Comment on attachment 681130 [details] [diff] [review]
patch2

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

Perfect.

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Oof forgot a final qrefresh. I went with your naming suggestion so even more perfect!

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94e3a41d532e

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Hmmm, I might need to turn AccessFu off at the end of the test otherwise it will be actively running for the remainder of our test presumably.

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5324de8c398

Interestingly this causes perma-oranges on some otherwise intermittent oranges (same thing happened on try). I'll investigate before relanding.

Comment 13

[Eitan Isaacson (Leave thru 12/23) [:eeejay]]

Comment on attachment 681130 [details] [diff] [review]
patch2

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

Re-reviewing in light of the orange. This is likely a memory leak. Bug #850005 should resolve the deep AccessFu leak issues. There is one issue here below.

::: accessible/src/jsat/AccessFu.jsm
@@ +99,5 @@
>      this.chromeWin.addEventListener('TabOpen', this);
>      this.chromeWin.addEventListener('TabSelect', this);
> +
> +    if (this.readyCallback)
> +      this.readyCallback();

AccessFu.readyCallback should be deleted after it is called. since it has an indirect reference to a window object.

Comment 14

[Eitan Isaacson (Leave thru 12/23) [:eeejay]]

https://tbpl.mozilla.org/?tree=Try&rev=16bcd9599226

Comment 15

[Mozilla RelEng Bot]

Try run for 16bcd9599226 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=16bcd9599226
Results (out of 387 total builds):
    exception: 6
    success: 330
    warnings: 46
    failure: 4
    other: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-16bcd9599226

Comment 16

[Marco Zehe (:MarcoZ)]

From that try run, all a11y mochitests failed with bug 781120.

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

I wonder why... ?

Comment 18

[Marco Zehe (:MarcoZ)]

The debug runs also showed a leak. Are we sure this try run included the patch from bug 850005?

Comment 19

[Eitan Isaacson (Leave thru 12/23) [:eeejay]]

Ok. Lets keep the r- on that patch. There are two things that are missing:

1. A teardown. AccessFu needs to be disabled. If not, it keeps capturing keystrokes and other things.
2. Calling attach() before setting the pref would be a lot more interesting, since we would be testing that AccessFu gets enabled when the pref changes.

Bonus points: test for the case when the activate pref is 2 (auto). That means adding a special test mode, and listening for the Android "Accessibility:Settings" notification.

Comment 20

[Mozilla RelEng Bot]

Try run for 2a90f51410a2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2a90f51410a2
Results (out of 19 total builds):
    exception: 18
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2a90f51410a2

Comment 21

[Marco Zehe (:MarcoZ)]

Attachment: [AccessFu] Add mochitest for enabling. Tear-down bits by MarcoZ.

This is a stab at a tear-down. However, the try run I kicked off gives me the following error:
04:07:30 INFO - 5002 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/jsat/test_alive.html | uncaught exception - Error: Only one top-level window could used with AccessFu at resource://gre/modules/accessibility/Utils.jsm:26
What's the best way to tear AccessFu down?

Comment 22

[Marco Zehe (:MarcoZ)]

Attachment: [AccessFu] Add mochitest for enabling. Tear-down bits by MarcoZ.

I tried taking a stab at the tear-down, but I encountered this error on the try run:

04:07:30 INFO - 5002 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/jsat/test_alive.html | uncaught exception - Error: Only one top-level window could used with AccessFu at resource://gre/modules/accessibility/Utils.jsm:26

Looks like it doesn't take kindly to just switching the pref from 1 to 0 between calls and hoping it'll just work. :)
Eitan, any hints on what to do to best tear it down after the test?

Comment 23

[Mozilla RelEng Bot]

Try run for feeeff98d470 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=feeeff98d470
Results (out of 381 total builds):
    exception: 1
    success: 340
    warnings: 34
    failure: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-feeeff98d470

Comment 24

[Marco Zehe (:MarcoZ)]

Attachment: [AccessFu] Add mochitest for enabling. Tear-down bits by MarcoZ.

This patch works, it starts and stops AccessFu and logs both instances.

Comment 25

[Eitan Isaacson (Leave thru 12/23) [:eeejay]]

(In reply to Marco Zehe (:MarcoZ) from comment #24)
> Created attachment 726706 [details] [diff] [review]
> [AccessFu] Add mochitest for enabling. Tear-down bits by MarcoZ.
> 
> This patch works, it starts and stops AccessFu and logs both instances.

Is this the same patch you put in the previous try?
https://tbpl.mozilla.org/?tree=Try&rev=feeeff98d470

If so, I still see a lot of orange.

Comment 26

[Eitan Isaacson (Leave thru 12/23) [:eeejay]]

(In reply to Marco Zehe (:MarcoZ) from comment #22)
> Created attachment 726615 [details] [diff] [review]
> [AccessFu] Add mochitest for enabling. Tear-down bits by MarcoZ.
> 
> I tried taking a stab at the tear-down, but I encountered this error on the
> try run:
> 
> 04:07:30 INFO - 5002 ERROR TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/a11y/accessible/jsat/test_alive.html | uncaught
> exception - Error: Only one top-level window could used with AccessFu at
> resource://gre/modules/accessibility/Utils.jsm:26
> 
> Looks like it doesn't take kindly to just switching the pref from 1 to 0
> between calls and hoping it'll just work. :)

This is because it is done asynchronously. You set the pref and then the observer is fired later. So you need to chain everything in the callbacks. In other words, in the ready call, you continue the test, and in the done callback you explicitly finish the test.

> Eitan, any hints on what to do to best tear it down after the test?

As a failsafe, I would also do that in document unload.

Comment 27

[Eitan Isaacson (Leave thru 12/23) [:eeejay]]

Comment on attachment 726706 [details] [diff] [review]
[AccessFu] Add mochitest for enabling. Tear-down bits by MarcoZ.

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

Like I said above, using the pref should be possible. It is asynchronous, so the test needs to be chained in the various callbacks.

::: accessible/src/jsat/AccessFu.jsm
@@ +62,5 @@
> +   * Exposes the ability to shut down AccessFu from the outside.
> +   */
> +  detach: function detach() {
> +    this._disable();
> +  },

We should not enable/disable directly, but only via pref.

A detach method might be needed in the future for actually disassociating the window, but it would need to do more than just disabling, it would need to remove observers and event handlers that are set up in attach().

Comment 28

[Eitan Isaacson (Leave thru 12/23) [:eeejay]]

(In reply to Eitan Isaacson [:eeejay] from comment #27)
> 
> We should not enable/disable directly, but only via pref.
> 

To clarify, doing it directly is a bad idea because you would be putting AccessFu into a compromised state where the pref says one thing, but AccessFu is in a different state.

Comment 29

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Eitan Isaacson [:eeejay] from comment #28)
> (In reply to Eitan Isaacson [:eeejay] from comment #27)
> > 
> > We should not enable/disable directly, but only via pref.
> > 
> 
> To clarify, doing it directly is a bad idea because you would be putting
> AccessFu into a compromised state where the pref says one thing, but
> AccessFu is in a different state.

OK

Comment 30

[Marco Zehe (:MarcoZ)]

Eitan, the patch I put here was not the one I did the try run with, I did that and then managed to run the tests locally, too, and fix it up that way. I will take your comments into account and put up a new patch tomorrow. That is, if David allows me to continue stealing the bug. ;-)

Comment 31

[David Bolter [:davidb] (NeedInfo me for attention)]

Steal away! Eitan has a good idea of how this can be nice. Please chat with him first.

Comment 32

[Eitan Isaacson (Leave thru 12/23) [:eeejay]]

(In reply to Marco Zehe (:MarcoZ) from comment #30)
> Eitan, the patch I put here was not the one I did the try run with, I did
> that and then managed to run the tests locally, too, and fix it up that way.
> I will take your comments into account and put up a new patch tomorrow. That
> is, if David allows me to continue stealing the bug. ;-)

I think the goal of this patch should be having a common bomb-proof setup/teardown for this and future accessfu tests. The way a test should be run is like this:
AccessFuTest.addTest(testMe);
AccessFuTest.run();

Where the function testMe should be a specific test.

AccessfuTest.run() should:
1. do AccessFu.attach()
2. Set a ready callback (see below)
3. set the activate pref to 1

The ready callback should:
1. iterate over added tests and run them safely (in try/catch).
2. set the done callback (see below).
3. set the activate pref to 0

the done callback should:
1. detach AccessFu with a new detach method that would unregister all the observers and reverse everything attach() does.
2. call SimpleTest.finish()

AccessFuTest should live in a new common js file in the jsat test folder so all future AccessFu tests could use it.

Sorry if this is too specific. Please take creative control and have fun with it. As long as it answers the goal above.

Comment 33

[David Bolter [:davidb] (NeedInfo me for attention)]

Marco any luck?

Comment 34

[Marco Zehe (:MarcoZ)]

Attachment: [AccessFu] Add mochitest for enabling. Tear-down bits by MarcoZ.

Dumping this here for safe-keeping. It currently gives an error message at tear-down that aWindow.shell isn't defined. It also starts the AccessFu event manager after everything has already shut down again, at least according to the log I get. Nightly hangs around with or without the larger AccessFu.detach than in the previous patch, and if I only have the _disable() call in there, eventManager keeps interfering with future tests and does all kinds of stupid stuff. I currently don't find where this thing is getting started and how to properly tear it down. Comments appreciated, otherwise I'll also continue banging on this tomorrow.

Comment 35

[Yura Zenevich [:yzen]]

(In reply to Marco Zehe (:MarcoZ) from comment #34)
> Created attachment 734659 [details] [diff] [review]
> [AccessFu] Add mochitest for enabling. Tear-down bits by MarcoZ.
> 
> Dumping this here for safe-keeping. It currently gives an error message at
> tear-down that aWindow.shell isn't defined. It also starts the AccessFu
> event manager after everything has already shut down again, at least
> according to the log I get. Nightly hangs around with or without the larger
> AccessFu.detach than in the previous patch, and if I only have the
> _disable() call in there, eventManager keeps interfering with future tests
> and does all kinds of stupid stuff. I currently don't find where this thing
> is getting started and how to properly tear it down. Comments appreciated,
> otherwise I'll also continue banging on this tomorrow.

Hi Marco,

I've taken a look at the patch and here are my observations:

* aWindow.shell is only present in b2g and I believe this is why you are seeing that shell is undefined error.
* When you are removing a listener to 'ContentStart' event in detach, you need to provide the original listener object as well.
* Another thing I noticed, but not sure if it is necessary, is that if you want to truly detach AccessFu I think you would need to clear the reference to aWindow object created by Utils.init.
* Also, I tried checking if AccessFu._enabled is true to avoid disabling twice.

After the above changes I was able to run the test successfully.
Let me know if it works for you :)

Comment 36

[Marco Zehe (:MarcoZ)]

Yura, would you like to take this bug and apply your changes to the patch? I'd be happy to give feedback or review on it. You seem to have a much better grasp on these intricacies than I do. :)

Comment 37

[Yura Zenevich [:yzen]]

Hi Marco,

I got to a point where I have a nice looking detach that works properly however I ran into a problem:

AccessFu has a method _loadFrameScript that is called in _enable for every message manager. It is also called on 'remote-browser-frame-shown' and 'TabOpen' events.
The method adds some message listeners that could be removed on disable however it also calls loadFrameScript for 'content-script.js'. This means that we are loading 'content-script.js' every time we enable.. As far as I understand, this process is irreversible and thus should only happen once. Would you know if that's the case? Perhaps you have any suggestions as to how to go around this issue?

Thanks.

Comment 38

[Marco Zehe (:MarcoZ)]

Phew, I'm afraid this is a little beyond my knowledge of the intricacies of the out-of-process workings. Deferring to Eitan who is on PTO this week, so we may not get an answer until next week.

But it definitely has to do with the way we have to deal with separate content processes in Firefox OS and possibly Firefox for Android. On Windows, Linux and Mac, OOP is currently not supported and has been postponed for an unknown time.

Comment 39

[Yura Zenevich [:yzen]]

Attachment: Slightly updated test by Marco.

This now passes. However, I think a more reliable test would be listening for some definitive event such as "AccessFu:Start" or some other one like that.

Comment 40

[Yura Zenevich [:yzen]]

Attachment: Changes to AccessFu to enable testing.

These are all the changes I had to make to make sure that AccessFu gets disabled and detached cleanly.

I also feel like done and ready and done callbacks should, perhaps, be handled through some sort of an event mechanism.

Comment 41

[Marco Zehe (:MarcoZ)]

Comment on attachment 736122 [details] [diff] [review]
Slightly updated test by Marco.

>+    var aWindow = getMainChromeWindow(window);
>+
>+    AccessFu.attach(aWindow);

Why this change? You don't appear to use the aWindow variable anywhere else.

Also, aWindow should be named slightly differently, since by convention, aWindow would indicate a parameter passed, not a local variable. I'd call it localWin or chromeWin or something similar.

Comment 42

[Marco Zehe (:MarcoZ)]

Comment on attachment 736123 [details] [diff] [review]
Changes to AccessFu to enable testing.

>+  _handleMessageManager: function _handleMessageManager(aMessageManager) {
>+    if (this._enabled) {
>+      this._addMessageListeners(mm);
>+    }
>+    this._loadFrameScript(mm);
>   },

You meant to write "aMessageManager" instead of "mm", right? Does zour patch still work with this change?

Comment 43

[Yura Zenevich [:yzen]]

(In reply to Marco Zehe (:MarcoZ) from comment #41)
> Comment on attachment 736122 [details] [diff] [review]
> Slightly updated test by Marco.
> 
> >+    var aWindow = getMainChromeWindow(window);
> >+
> >+    AccessFu.attach(aWindow);
> 
> Why this change? You don't appear to use the aWindow variable anywhere else.
> 
> Also, aWindow should be named slightly differently, since by convention,
> aWindow would indicate a parameter passed, not a local variable. I'd call it
> localWin or chromeWin or something similar.

Ah, good point, when I was created it getMainChromeWindow was used twice and then I removed the second case. Thanks!

Comment 44

[Yura Zenevich [:yzen]]

(In reply to Marco Zehe (:MarcoZ) from comment #42)
> Comment on attachment 736123 [details] [diff] [review]
> Changes to AccessFu to enable testing.
> 
> >+  _handleMessageManager: function _handleMessageManager(aMessageManager) {
> >+    if (this._enabled) {
> >+      this._addMessageListeners(mm);
> >+    }
> >+    this._loadFrameScript(mm);
> >   },
> 
> You meant to write "aMessageManager" instead of "mm", right? Does zour patch
> still work with this change?

Thanks for catching that, yes it still worked event with this typo :) I'll update the patches.

Comment 45

[Yura Zenevich [:yzen]]

Attachment: Latest test.

Comment 46

[Yura Zenevich [:yzen]]

Attachment: Latest changes to AccessFu to enable testing.

Comment 47

[Marco Zehe (:MarcoZ)]

Comment on attachment 736260 [details] [diff] [review]
Latest test.

This looks good to me. Would be interesting to have a try-server run to see how these changes fare on different platforms in production conditions.

Comment 48

[Marco Zehe (:MarcoZ)]

Comment on attachment 736262 [details] [diff] [review]
Latest changes to AccessFu to enable testing.

These changes look good to me as well, I also tested a local build with these patches and found that everything still works as expected so far.

Comment 49

[Eitan Isaacson (Leave thru 12/23) [:eeejay]]

Comment on attachment 736260 [details] [diff] [review]
Latest test.

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

This looks good.

Comment 50

[Eitan Isaacson (Leave thru 12/23) [:eeejay]]

Comment on attachment 736262 [details] [diff] [review]
Latest changes to AccessFu to enable testing.

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

This looks pretty good. I'd like to see it tested on Android by disabling and enabling TalkBack and playing with the pref.

::: accessible/src/jsat/AccessFu.jsm
@@ +42,5 @@
> +      if (Utils.MozBuildApp === 'b2g') {
> +        this._contentStartListener = function _contentStartListener() {
> +          aWindow.shell.contentBrowser.contentWindow.addEventListener(
> +            'mozContentEvent', this, false, true);
> +        };

I think you need to bind the function to "this" or else "this" that is in the function is not AccessFu.

@@ +44,5 @@
> +          aWindow.shell.contentBrowser.contentWindow.addEventListener(
> +            'mozContentEvent', this, false, true);
> +        };
> +        aWindow.addEventListener('ContentStart', this._contentStartListener,
> +          false);

Why not just make AccessFu the listener object instead of this._contentStartListener?

Comment 51

[Yura Zenevich [:yzen]]

Attachment: Patch with comments addressed.

Comment 52

[Yura Zenevich [:yzen]]

Attachment: Slightly improved tests.

Comment 53

[Eitan Isaacson (Leave thru 12/23) [:eeejay]]

Comment on attachment 738539 [details] [diff] [review]
Patch with comments addressed.

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

This looks good. A few thing need to happen before this lands:
1. A successful try run (for this and the other patch).
2. Testing on Android for the following cases without restarting firefox:
 a. activate pref set to 0: accessfu disabled both with system a11y on or off.
 b. activate pref set to 1: accessfu enabled both with system a11y on or off.
 c. activate pref set to 2: accessfu enabled/disabled depending on system pref.

Comment 54

[Marco Zehe (:MarcoZ)]

(In reply to Eitan Isaacson [:eeejay] from comment #53)
> This looks good. A few thing need to happen before this lands:
> 1. A successful try run (for this and the other patch).

I just started that. Results will be posted to this bug once completed, but if you'd like to watch, the link is:
https://tbpl.mozilla.org/?tree=Try&rev=466f4d6dcadd

> 2. Testing on Android for the following cases without restarting firefox:
>  a. activate pref set to 0: accessfu disabled both with system a11y on or
> off.
>  b. activate pref set to 1: accessfu enabled both with system a11y on or off.
>  c. activate pref set to 2: accessfu enabled/disabled depending on system
> pref.

I definitely need this to be done by a sighted person, since parts of this involve the accessibility leaving a blind guy in the dark on purpose. :)

Comment 55

[Yura Zenevich [:yzen]]

After doing some investigation why AccessFu would not resume on an already opened page on Android, after being disabled and then activated again, I found that the EventManager would not start after AccessFu gets enabled. Here's the log:

...
I/Gecko   (17143): [AccessFu] INFO disable
I/Gecko   (17143): [AccessFu] INFO TouchAdapter.stop
I/Gecko   (17143): [AccessFu] INFO EventManager.stop mobile/android
I/Gecko   (17143): [AccessFu] INFO enable
I/Gecko   (17143): [AccessFu] INFO TouchAdapter.start
...

I think it's possible to write a test for it, so I'll try to do that and fix the issue.

Comment 56

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Yura Zenevich [:yzen] from comment #55)
> I think it's possible to write a test for it, so I'll try to do that and fix
> the issue.

<3

Comment 57

[Yura Zenevich [:yzen]]

Attachment: Enabling testing for AccessFu v4

Fixed the issue related to AccessFu:start message. Will push to try and try the build on device.

Comment 58

[Yura Zenevich [:yzen]]

Attachment: Tests v4

Comment 59

[Marco Zehe (:MarcoZ)]

Comment on attachment 739429 [details] [diff] [review]
Tests v4

> +      // and finish the test run.
> +      SimpleTest.finish();
> +    };

Why no return; here any more?

> +  waitForExplicitFinish: function AccessFuTest_waitForExplicitFinish() {
> +    this._waitForExplicitFinish = true;
> +  },

Where is it guaranteed that this is set to "false" initially, and not have some random value in the case the test file doesn't call waitForExplicitFinish()?

The tests look good otherwise, but I'd like these two above points answered, please.

Comment 60

[Marco Zehe (:MarcoZ)]

Comment on attachment 739429 [details] [diff] [review]
Tests v4

We cleared the questions over IRC. Changing the feedback to a formal review to save a step. :) r=me.

Comment 61

[Eitan Isaacson (Leave thru 12/23) [:eeejay]]

Comment on attachment 739428 [details] [diff] [review]
Enabling testing for AccessFu v4

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

r=me

Comment 62

[Yura Zenevich [:yzen]]

Attachment: Tests v5

Comment 63

[Yura Zenevich [:yzen]]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=6c4837273537

Comment 64

[Yura Zenevich [:yzen]]

Build from try is available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6c4837273537/try-android-armv6/fennec-23.0a1.en-US.android-arm-armv6.apk

I tested it by setting accessfu.activate to 0, 1 and 2 and turning talk back on and off, it seems to behave correctly.

Comment 65

[Marco Zehe (:MarcoZ)]

Comment on attachment 739641 [details] [diff] [review]
Tests v5

Nice! r=me.

Comment 66

[Eitan Isaacson (Leave thru 12/23) [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6915e00b6d62
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f39ea436f1d

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6915e00b6d62
https://hg.mozilla.org/mozilla-central/rev/8f39ea436f1d