Closed Bug 811307 Opened 7 years ago Closed 7 years ago

[AccessFu] Add mochitest for enabling

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: davidb, Assigned: yzen)

Details

Attachments

(2 files, 13 obsolete files)

14.32 KB, patch
eeejay
: review+
Details | Diff | Splinter Review
8.53 KB, patch
MarcoZ
: review+
Details | Diff | Splinter Review
Breaking the ice on AccessFu (jsat) mochitests.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dbolter
Attachment #681040 - Flags: review?(eitan)
Attachment #681040 - Flags: feedback?(marco.zehe)
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 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. ;)
Attachment #681040 - Flags: feedback?(marco.zehe) → feedback+
(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 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.
(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.
Attached patch patch2 (obsolete) — Splinter Review
Attachment #681040 - Attachment is obsolete: true
Attachment #681040 - Flags: review?(eitan)
Attachment #681130 - Flags: review?(eitan)
Comment on attachment 681130 [details] [diff] [review]
patch2

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

Perfect.
Attachment #681130 - Flags: review?(eitan) → review+
Oof forgot a final qrefresh. I went with your naming suggestion so even more perfect!
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.
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 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.
Attachment #681130 - Flags: review+ → review-
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/eisaacson@mozilla.com-16bcd9599226
From that try run, all a11y mochitests failed with bug 781120.
I wonder why... ?
The debug runs also showed a leak. Are we sure this try run included the patch from bug 850005?
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.
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/mzehe@mozilla.com-2a90f51410a2
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?
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?
Attachment #726588 - Attachment is obsolete: true
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/mzehe@mozilla.com-feeeff98d470
This patch works, it starts and stops AccessFu and logs both instances.
Attachment #726706 - Flags: review?(eitan)
Attachment #726615 - Attachment is obsolete: true
(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.
(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 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().
Attachment #726706 - Flags: review?(eitan) → review-
(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.
(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
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. ;-)
Steal away! Eitan has a good idea of how this can be nice. Please chat with him first.
(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.
Assignee: dbolter → marco.zehe
Marco any luck?
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.
Attachment #726706 - Attachment is obsolete: true
(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 :)
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. :)
Assignee: marco.zehe → yura.zenevich
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.
Flags: needinfo?(marco.zehe)
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.
Flags: needinfo?(marco.zehe) → needinfo?(eitan)
Attached patch Slightly updated test by Marco. (obsolete) — Splinter Review
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.
Attachment #736122 - Flags: feedback?(marco.zehe)
Attachment #736122 - Flags: feedback?(eitan)
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.
Attachment #736123 - Flags: feedback?(marco.zehe)
Attachment #736123 - Flags: feedback?(eitan)
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 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?
(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!
(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.
Attached patch Latest test. (obsolete) — Splinter Review
Attachment #736122 - Attachment is obsolete: true
Attachment #736122 - Flags: feedback?(marco.zehe)
Attachment #736122 - Flags: feedback?(eitan)
Attachment #736260 - Flags: feedback?(marco.zehe)
Attachment #736260 - Flags: feedback?(eitan)
Attachment #736123 - Attachment is obsolete: true
Attachment #736123 - Flags: feedback?(marco.zehe)
Attachment #736123 - Flags: feedback?(eitan)
Attachment #736262 - Flags: feedback?(marco.zehe)
Attachment #736262 - Flags: feedback?(eitan)
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.
Attachment #736260 - Flags: feedback?(marco.zehe) → feedback+
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.
Attachment #736262 - Flags: feedback?(marco.zehe) → feedback+
Comment on attachment 736260 [details] [diff] [review]
Latest test.

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

This looks good.
Attachment #736260 - Flags: feedback?(eitan) → feedback+
Flags: needinfo?(eitan)
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?
Attachment #736262 - Flags: feedback?(eitan) → feedback+
Attached patch Patch with comments addressed. (obsolete) — Splinter Review
Attachment #736262 - Attachment is obsolete: true
Attachment #738539 - Flags: review?(eitan)
Attached patch Slightly improved tests. (obsolete) — Splinter Review
Attachment #736260 - Attachment is obsolete: true
Attachment #738543 - Flags: review?(marco.zehe)
Attachment #734659 - Attachment is obsolete: true
Attachment #738543 - Flags: review?(marco.zehe) → review+
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.
Attachment #738539 - Flags: review?(eitan) → review+
(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. :)
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.
(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
Fixed the issue related to AccessFu:start message. Will push to try and try the build on device.
Attachment #738539 - Attachment is obsolete: true
Attachment #739428 - Flags: feedback?(eitan)
Attached patch Tests v4 (obsolete) — Splinter Review
Attachment #738543 - Attachment is obsolete: true
Attachment #739429 - Flags: feedback?(marco.zehe)
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 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.
Attachment #739429 - Flags: feedback?(marco.zehe) → review+
Comment on attachment 739428 [details] [diff] [review]
Enabling testing for AccessFu v4

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

r=me
Attachment #739428 - Flags: feedback?(eitan) → review+
Attached patch Tests v5Splinter Review
Attachment #739429 - Attachment is obsolete: true
Attachment #739641 - Flags: review?(marco.zehe)
Attachment #739641 - Flags: feedback?(eitan)
Build from try is available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/yura.zenevich@gmail.com-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 on attachment 739641 [details] [diff] [review]
Tests v5

Nice! r=me.
Attachment #739641 - Flags: review?(marco.zehe) → review+
Attachment #681130 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/6915e00b6d62
https://hg.mozilla.org/mozilla-central/rev/8f39ea436f1d
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.