Closed Bug 972047 Opened 10 years ago Closed 10 years ago

[AccessFu] Add tests for OOP message handling and general integration

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla30

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

Attachments

(1 file, 2 obsolete files)

Introduce AccessFu content integration mochitest.
13.44 KB, patch
yzen
: review+
Details | Diff | Splinter Review 
The IPC message passing in AccessFu is one of the more fragile parts. The process isolation also gives us an opportunity to exercise the content scripts and get great test coverage.
I'll push this to try and post here in a minute.
Attachment #8375191 - Flags: review?(yzenevich)
Depends on: 972630, 972083
OK. Looks like try agrees with this one..
https://tbpl.mozilla.org/?tree=Try&rev=f1a8f3538bea
Attachment #8375191 - Attachment is obsolete: true
Attachment #8375191 - Flags: review?(yzenevich)
Attachment #8377875 - Flags: review?(yzenevich)
Comment on attachment 8377875 [details] [diff] [review]
Introduce AccessFu content integration mochitest.

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

The test passes, but I get this error:
0:16.23 System JS : ERROR chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/jsatcommon.js:239 - TypeError: this.currentPair[0] is null

Looks really cool, I left some comments/questions (some of them are open ended like using simpletest's nextTest machinery instead of pump).

::: accessible/src/jsat/content-script.js
@@ +400,5 @@
>        eventManager = new EventManager(this);
>      }
>      eventManager.start();
> +
> +    sendAsyncMessage('AccessFu:ContentStarted', {});

We don't need to pass empty an object argument. Let's just send a message.

::: accessible/tests/mochitest/jsat/doc_content_integration.html
@@ +5,5 @@
> +  <meta charset="utf-8" />
> +  <script>
> +    var frameContents = '<html>' +
> +      '<head><title>such app</title></head>' +
> +      '<body>' + 

nit: whitespace

::: accessible/tests/mochitest/jsat/jsatcommon.js
@@ +167,5 @@
> +    this.finishedCallback = aFinishedCallback;
> +    var self = this;
> +
> +    // Get top content message manager, and set it up.
> +    this.topMessageManager = Utils.getMessageManager(currentBrowser());

Lets put topMessageManager into this.mms. This way we won't need special code when stopping.

@@ +170,5 @@
> +    // Get top content message manager, and set it up.
> +    this.topMessageManager = Utils.getMessageManager(currentBrowser());
> +    this.setupMessageManager(this.topMessageManager, function () {
> +      // Get child message managers.
> +      var frames = currentTabDocument().querySelectorAll('iframe');

The whole block below can probably be changed to:

for (var i = 0; i < frames.length; ++i) {
  var mm = Utils.getMessageManager(frames[i]);
  self.mms.push(mm);
  ++self.toSetup;
  self.setupMessageManager(mm, function () {
    if (--self.toSetup === 0) {
      self.pump();
    }
  });
}

@@ +178,5 @@
> +        if (mm) {
> +          self.mms.push(mm);
> +        }
> +      }
> +      

nit: whitespace

@@ +182,5 @@
> +      
> +      var toSetup = self.mms.length;
> +      for (var i = 0; i < self.mms.length; i++) {
> +        self.setupMessageManager(self.mms[i], function () {
> +          if (--toSetup == 0) {

nit: use === to compare with 0.

@@ +193,5 @@
> +  },
> +
> +  setupMessageManager:  function (aMessageManager, aCallback) {
> +    function contentScript() {
> +      addMessageListener('AccessFuTest:Focus', function (aMessage) {

Both listeners are almost identical, I would factor that out.

@@ +208,5 @@
> +        }
> +      });
> +    }
> +
> +    if (aMessageManager) {

I probably want to return here if !aMessageManager (and do the rest only if ok).

@@ +211,5 @@
> +
> +    if (aMessageManager) {
> +      aMessageManager.addMessageListener('AccessFu:Present', this);
> +    }
> +    

nit: whitespace

@@ +216,5 @@
> +    aMessageManager.addMessageListener('AccessFu:Ready', function (aMessage) {
> +      aMessageManager.addMessageListener(
> +        'AccessFu:ContentStarted',
> +        function (aMessage) {
> +          if (aCallback) {

Do we ever not pass aCallback? if not, lets just have:
aMessageManager.addMessageListener('AccessFu:ContentStarted', aCallback);

@@ +225,5 @@
> +    });
> +    aMessageManager.loadFrameScript(
> +      'chrome://global/content/accessibility/content-script.js', false);
> +    aMessageManager.loadFrameScript(
> +      'data:,(' + contentScript.toString() + ')();', false);

Let's make |contentScript| a separate file and load it like the actual one.

@@ +228,5 @@
> +    aMessageManager.loadFrameScript(
> +      'data:,(' + contentScript.toString() + ')();', false);
> +  },
> +
> +  pump: function() {

Ideally it would be nice to reuse what mochitest provides (nextTest etc stuff).

@@ +239,5 @@
> +        this.topMessageManager.sendAsyncMessage(this.currentPair[0].name,
> +                                                this.currentPair[0].json);
> +      }
> +    } else if (this.finishedCallback) {
> +      this.topMessageManager.sendAsyncMessage('AccessFu:Stop', {});

nit: no need for {} argument.

@@ +240,5 @@
> +                                                this.currentPair[0].json);
> +      }
> +    } else if (this.finishedCallback) {
> +      this.topMessageManager.sendAsyncMessage('AccessFu:Stop', {});
> +      for (var i = 0; i < this.mms.length; i++) {

nit: for (var mm of this.mms) or even fancier:
[mm.sendAsyncMessage('AccessFu:Stop') for (mm of this.mms)];

@@ +241,5 @@
> +      }
> +    } else if (this.finishedCallback) {
> +      this.topMessageManager.sendAsyncMessage('AccessFu:Stop', {});
> +      for (var i = 0; i < this.mms.length; i++) {
> +	this.mms[i].sendAsyncMessage('AccessFu:Stop', {});

nit: indentation, no need for {} argument.

@@ +248,5 @@
> +    }
> +  },
> +
> +  receiveMessage: function(aMessage) {
> +    if (!this.currentPair)

nit: {}

@@ +253,5 @@
> +      return;
> +
> +    var expected = this.currentPair[1];
> +
> +    if (expected) {

nit: if (expected && expected.speak)

@@ +269,5 @@
> +    this.pump();
> +  },
> +
> +  extractUtterance: function(aData) {
> +    for (var i = 0; i < aData.length; i++) {

nit: for (var output of aData)

@@ +272,5 @@
> +  extractUtterance: function(aData) {
> +    for (var i = 0; i < aData.length; i++) {
> +      var output = aData[i];
> +      if (output && output.type === 'Speech') {
> +        for (var ii = 0; ii < output.details.actions.length; ii++) {

for (var action of output.details.actions)

::: accessible/tests/mochitest/jsat/test_content_integration.html
@@ +58,5 @@
> +
> +        var clearCursor = { name: 'AccessFu:ClearCursor',
> +                            json: { origin: 'top' }
> +                          };
> +                                                              

nit: whitespace

@@ +116,5 @@
> +
> +            // Move cursor with focus in outside document
> +            [simpleMoveNext,
> +            { speak: 'Phone status bar Traversal Rule test document' }],
> +            [function (mm) {

This function is repeated twice.

@@ +126,5 @@
> +            // Blur button and reset cursor
> +            [function (mm) {
> +              mm.sendAsyncMessage(
> +                'AccessFuTest:Blur', { selector: 'button' });
> +            }, null], 

nit: whitespace

@@ +142,5 @@
> +
> +            // XXX: Set focus on iframe itself.
> +            // XXX: Set focus on element in iframe when cursor is outside of it.
> +            // XXX: Set focus on element in iframe when cursor is in iframe.
> +]);

nit: indentation

@@ +144,5 @@
> +            // XXX: Set focus on element in iframe when cursor is outside of it.
> +            // XXX: Set focus on element in iframe when cursor is in iframe.
> +]);
> +        
> +        

nit: whitespace

@@ +145,5 @@
> +            // XXX: Set focus on element in iframe when cursor is in iframe.
> +]);
> +        
> +        
> +        contentTest.start(function () {

Since we already handle functions in the queue, this is better of not being a special case, you can just do it as the last item in the queue.

@@ +147,5 @@
> +        
> +        
> +        contentTest.start(function () {
> +          closeBrowserWindow();
> +          SimpleTest.finish();

This probably is more convenient being the last default thing in the queue.

@@ +158,5 @@
> +
> +    SimpleTest.waitForExplicitFinish();
> +    addLoadEvent(
> +      function () {
> +        openBrowserWindow(

This should probably be the first item in the queue.

@@ +160,5 @@
> +    addLoadEvent(
> +      function () {
> +        openBrowserWindow(
> +          function () {
> +            SpecialPowers.pushPrefEnv({

Do we need to reset the preferences as well (or is are they just set for that window)? Also I would probably do this internally in AccessFuContentTest.

@@ +165,5 @@
> +              "set": [
> +                // TODO: remove this as part of bug 820712
> +                ["network.disable.ipc.security", true],
> +                
> +                

nit: whitespace

@@ +170,5 @@
> +                ["dom.ipc.browser_frames.oop_by_default", true],
> +                ["dom.mozBrowserFramesEnabled", true],
> +                ["browser.pagethumbnails.capturing_disabled", true]
> +              ]
> +            }, doTest) }, 

nit: whitespace
Attachment #8377875 - Flags: review?(yzenevich)
(In reply to Yura Zenevich [:yzen] from comment #4)
> Comment on attachment 8377875 [details] [diff] [review]
> Introduce AccessFu content integration mochitest.
> 
> Review of attachment 8377875 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The test passes, but I get this error:
> 0:16.23 System JS : ERROR
> chrome://mochitests/content/a11y/accessible/tests/mochitest/jsat/jsatcommon.
> js:239 - TypeError: this.currentPair[0] is null
> 
> Looks really cool, I left some comments/questions (some of them are open
> ended like using simpletest's nextTest machinery instead of pump).
> 
> ::: accessible/src/jsat/content-script.js
> @@ +400,5 @@
> >        eventManager = new EventManager(this);
> >      }
> >      eventManager.start();
> > +
> > +    sendAsyncMessage('AccessFu:ContentStarted', {});
> 
> We don't need to pass empty an object argument. Let's just send a message.


Done.

> 
> ::: accessible/tests/mochitest/jsat/doc_content_integration.html
> @@ +5,5 @@
> > +  <meta charset="utf-8" />
> > +  <script>
> > +    var frameContents = '<html>' +
> > +      '<head><title>such app</title></head>' +
> > +      '<body>' + 
> 
> nit: whitespace
> 

Done.

> ::: accessible/tests/mochitest/jsat/jsatcommon.js
> @@ +167,5 @@
> > +    this.finishedCallback = aFinishedCallback;
> > +    var self = this;
> > +
> > +    // Get top content message manager, and set it up.
> > +    this.topMessageManager = Utils.getMessageManager(currentBrowser());
> 
> Lets put topMessageManager into this.mms. This way we won't need special
> code when stopping.
> 

OK!

> @@ +170,5 @@
> > +    // Get top content message manager, and set it up.
> > +    this.topMessageManager = Utils.getMessageManager(currentBrowser());
> > +    this.setupMessageManager(this.topMessageManager, function () {
> > +      // Get child message managers.
> > +      var frames = currentTabDocument().querySelectorAll('iframe');
> 
> The whole block below can probably be changed to:
> 
> for (var i = 0; i < frames.length; ++i) {
>   var mm = Utils.getMessageManager(frames[i]);
>   self.mms.push(mm);
>   ++self.toSetup;
>   self.setupMessageManager(mm, function () {
>     if (--self.toSetup === 0) {
>       self.pump();
>     }
>   });
> }
> 

OK!

> @@ +178,5 @@
> > +        if (mm) {
> > +          self.mms.push(mm);
> > +        }
> > +      }
> > +      
> 
> nit: whitespace
> 

Done.

> @@ +182,5 @@
> > +      
> > +      var toSetup = self.mms.length;
> > +      for (var i = 0; i < self.mms.length; i++) {
> > +        self.setupMessageManager(self.mms[i], function () {
> > +          if (--toSetup == 0) {
> 
> nit: use === to compare with 0.
> 

Done.

> @@ +193,5 @@
> > +  },
> > +
> > +  setupMessageManager:  function (aMessageManager, aCallback) {
> > +    function contentScript() {
> > +      addMessageListener('AccessFuTest:Focus', function (aMessage) {
> 
> Both listeners are almost identical, I would factor that out.
> 

OK!

> @@ +208,5 @@
> > +        }
> > +      });
> > +    }
> > +
> > +    if (aMessageManager) {
> 
> I probably want to return here if !aMessageManager (and do the rest only if
> ok).
> 

Yeah, not sure why that conditional is there. We should assume aMessageManaget is not null.

> @@ +211,5 @@
> > +
> > +    if (aMessageManager) {
> > +      aMessageManager.addMessageListener('AccessFu:Present', this);
> > +    }
> > +    
> 
> nit: whitespace

Done.

> 
> @@ +216,5 @@
> > +    aMessageManager.addMessageListener('AccessFu:Ready', function (aMessage) {
> > +      aMessageManager.addMessageListener(
> > +        'AccessFu:ContentStarted',
> > +        function (aMessage) {
> > +          if (aCallback) {
> 
> Do we ever not pass aCallback? if not, lets just have:
> aMessageManager.addMessageListener('AccessFu:ContentStarted', aCallback);
> 

OK!

> @@ +225,5 @@
> > +    });
> > +    aMessageManager.loadFrameScript(
> > +      'chrome://global/content/accessibility/content-script.js', false);
> > +    aMessageManager.loadFrameScript(
> > +      'data:,(' + contentScript.toString() + ')();', false);
> 
> Let's make |contentScript| a separate file and load it like the actual one.
> 

Nah!

> @@ +228,5 @@
> > +    aMessageManager.loadFrameScript(
> > +      'data:,(' + contentScript.toString() + ')();', false);
> > +  },
> > +
> > +  pump: function() {
> 
> Ideally it would be nice to reuse what mochitest provides (nextTest etc
> stuff).
> 

As we discussed, couldn't find that.

> @@ +239,5 @@
> > +        this.topMessageManager.sendAsyncMessage(this.currentPair[0].name,
> > +                                                this.currentPair[0].json);
> > +      }
> > +    } else if (this.finishedCallback) {
> > +      this.topMessageManager.sendAsyncMessage('AccessFu:Stop', {});
> 
> nit: no need for {} argument.

Done.

> 
> @@ +240,5 @@
> > +                                                this.currentPair[0].json);
> > +      }
> > +    } else if (this.finishedCallback) {
> > +      this.topMessageManager.sendAsyncMessage('AccessFu:Stop', {});
> > +      for (var i = 0; i < this.mms.length; i++) {
> 
> nit: for (var mm of this.mms) or even fancier:
> [mm.sendAsyncMessage('AccessFu:Stop') for (mm of this.mms)];
> 

I like the former, since the latter makes emacs yell at me for not catching the value of the expression.

> @@ +241,5 @@
> > +      }
> > +    } else if (this.finishedCallback) {
> > +      this.topMessageManager.sendAsyncMessage('AccessFu:Stop', {});
> > +      for (var i = 0; i < this.mms.length; i++) {
> > +	this.mms[i].sendAsyncMessage('AccessFu:Stop', {});
> 
> nit: indentation, no need for {} argument.

Done.

> 
> @@ +248,5 @@
> > +    }
> > +  },
> > +
> > +  receiveMessage: function(aMessage) {
> > +    if (!this.currentPair)
> 
> nit: {}

Done.

> 
> @@ +253,5 @@
> > +      return;
> > +
> > +    var expected = this.currentPair[1];
> > +
> > +    if (expected) {
> 
> nit: if (expected && expected.speak)

Kind of want to keep those separate because we may have other fields in the expected object to check for in the future.

> 
> @@ +269,5 @@
> > +    this.pump();
> > +  },
> > +
> > +  extractUtterance: function(aData) {
> > +    for (var i = 0; i < aData.length; i++) {
> 
> nit: for (var output of aData)
> 
> @@ +272,5 @@
> > +  extractUtterance: function(aData) {
> > +    for (var i = 0; i < aData.length; i++) {
> > +      var output = aData[i];
> > +      if (output && output.type === 'Speech') {
> > +        for (var ii = 0; ii < output.details.actions.length; ii++) {
> 
> for (var action of output.details.actions)

OK!

> 
> ::: accessible/tests/mochitest/jsat/test_content_integration.html
> @@ +58,5 @@
> > +
> > +        var clearCursor = { name: 'AccessFu:ClearCursor',
> > +                            json: { origin: 'top' }
> > +                          };
> > +                                                              
> 
> nit: whitespace

Done.

> 
> @@ +116,5 @@
> > +
> > +            // Move cursor with focus in outside document
> > +            [simpleMoveNext,
> > +            { speak: 'Phone status bar Traversal Rule test document' }],
> > +            [function (mm) {
> 
> This function is repeated twice.

OK! I made a thing for it..

> 
> @@ +126,5 @@
> > +            // Blur button and reset cursor
> > +            [function (mm) {
> > +              mm.sendAsyncMessage(
> > +                'AccessFuTest:Blur', { selector: 'button' });
> > +            }, null], 
> 
> nit: whitespace

Done.

> 
> @@ +142,5 @@
> > +
> > +            // XXX: Set focus on iframe itself.
> > +            // XXX: Set focus on element in iframe when cursor is outside of it.
> > +            // XXX: Set focus on element in iframe when cursor is in iframe.
> > +]);
> 
> nit: indentation

Done.

> 
> @@ +144,5 @@
> > +            // XXX: Set focus on element in iframe when cursor is outside of it.
> > +            // XXX: Set focus on element in iframe when cursor is in iframe.
> > +]);
> > +        
> > +        
> 
> nit: whitespace
> 

Done.

> @@ +145,5 @@
> > +            // XXX: Set focus on element in iframe when cursor is in iframe.
> > +]);
> > +        
> > +        
> > +        contentTest.start(function () {
> 
> Since we already handle functions in the queue, this is better of not being
> a special case, you can just do it as the last item in the queue.

I kind of like having this a separate thing, it allows us to call it when things go wrong.

> 
> @@ +147,5 @@
> > +        
> > +        
> > +        contentTest.start(function () {
> > +          closeBrowserWindow();
> > +          SimpleTest.finish();
> 
> This probably is more convenient being the last default thing in the queue.
> 

^

> @@ +158,5 @@
> > +
> > +    SimpleTest.waitForExplicitFinish();
> > +    addLoadEvent(
> > +      function () {
> > +        openBrowserWindow(
> 
> This should probably be the first item in the queue.
> 

Same as above, also the queue only runs when the iframe and message managers are loaded. I don't think the standard boilerplate stuff should be in the queue.

> @@ +160,5 @@
> > +    addLoadEvent(
> > +      function () {
> > +        openBrowserWindow(
> > +          function () {
> > +            SpecialPowers.pushPrefEnv({
> 
> Do we need to reset the preferences as well (or is are they just set for
> that window)? Also I would probably do this internally in
> AccessFuContentTest.
> 

This is a special wrapper, the 'push' means that it is set while the test is run, and then unset then it is done. It handles all the cases, such as storing the previous pref and not simply clearing the pref.

> @@ +165,5 @@
> > +              "set": [
> > +                // TODO: remove this as part of bug 820712
> > +                ["network.disable.ipc.security", true],
> > +                
> > +                
> 
> nit: whitespace

Done.

> 
> @@ +170,5 @@
> > +                ["dom.ipc.browser_frames.oop_by_default", true],
> > +                ["dom.mozBrowserFramesEnabled", true],
> > +                ["browser.pagethumbnails.capturing_disabled", true]
> > +              ]
> > +            }, doTest) }, 
> 
> nit: whitespace

Done.

        Attachment #8377875 -
        Attachment is obsolete: true

        Attachment #8380702 -
        Flags: review?(yzenevich)

    
  
Blocks: 976271

    
    

    
  
Comment on attachment 8380702 [details] [diff] [review]
Introduce AccessFu content integration mochitest.

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

r=me with some minor fixes :)

::: accessible/tests/mochitest/jsat/jsatcommon.js
@@ +175,5 @@
> +      var toSetup = 0;
> +      for (var i = 0; i < frames.length; i++ ) {
> +        var mm = Utils.getMessageManager(frames[i]);
> +        if (mm) {
> +	  toSetup++;

nit: mixed spaces and tabs.

@@ +193,5 @@
> +    function contentScript() {
> +      addMessageListener('AccessFuTest:Focus', function (aMessage) {
> +        var elem = content.document.querySelector(aMessage.json.selector);
> +        if (elem) {
> +	  if (aMessage.json.blur) {

nit: mixed spaces and tabs.

@@ +195,5 @@
> +        var elem = content.document.querySelector(aMessage.json.selector);
> +        if (elem) {
> +	  if (aMessage.json.blur) {
> +            elem.blur();
> +	  } else {

nit: mixed spaces and tabs.

@@ +197,5 @@
> +	  if (aMessage.json.blur) {
> +            elem.blur();
> +	  } else {
> +            elem.focus();
> +	  }

nit: mixed spaces and tabs.

@@ +224,5 @@
> +      } else {
> +        this.mms[0].sendAsyncMessage(this.currentPair[0].name,
> +                                     this.currentPair[0].json);
> +      }
> +    } else if (this.finishedCallback) {

else {
  for (var mm of this.mms) {
    mm.sendAsyncMessage('AccessFu:Stop');
  }
  if (this.finishedCallback) {
    this.finishedCallback();
    // Maybe SimpleTest.finish(); here, and just do only 
    // test specific stuff in finishedCallback?
  }
}

@@ +240,5 @@
> +
> +    var expected = this.currentPair[1];
> +
> +    if (expected) {
> +      if (expected.speak != undefined) {

!== when comparing to undefined (or just !expected.speak).

::: accessible/tests/mochitest/jsat/test_content_integration.html
@@ +21,5 @@
> +  <script type="application/javascript" src="jsatcommon.js"></script>
> +
> +  <script type="application/javascript">
> +    function doTest()
> +    {

nit: { on the same line as function

@@ +30,5 @@
> +        var simpleMoveNext = { name: 'AccessFu:MoveCursor',
> +                               json: { action: 'moveNext',
> +                                       rule: 'Simple',
> +                                       inputType: 'gesture',
> +                                       origin: 'top' } };

nit: inconsistent }; indentation here and below.

@@ +60,5 @@
> +                            json: { origin: 'top' }
> +                          };
> +
> +        function focusFunc(aSelector, aBlur) {
> +	  return function (mm) {

nit: indentation

@@ +69,5 @@
> +
> +        var contentTest = new AccessFuContentTest(
> +          [
> +            // Simple traversal forward
> +            [simpleMoveNext,

nit: indentation.

        Attachment #8380702 -
        Flags: review?(yzenevich) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/e0ef930a64cb
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: