Closed Bug 791400 Opened 7 years ago Closed 7 years ago

[AccessFu] Refactor accessfu to allow OOP content

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: eeejay, Assigned: eeejay)

Details

Attachments

(1 file, 1 obsolete file)

This is going to be big...
Attachment #664305 - Attachment description: Tada! → Separate content from chrome.
Attachment #664305 - Flags: feedback?(dbolter)
Sorry for the delay. Going to block off time for this soon.
Comment on attachment 664305 [details] [diff] [review]
Separate content from chrome.

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

Cool. Great that you got this going. Re; review, I've been jumping forward and backward as I learn what you are doing here (and thanks for IRCing). I think I'll submit this first round of review which distractions abound.

::: accessible/src/jsat/AccessFu.jsm
@@ +11,5 @@
>  
>  var EXPORTED_SYMBOLS = ['AccessFu'];
>  
>  Cu.import('resource://gre/modules/Services.jsm');
> +Cu.import('resource://gre/modules/tts.jsm');

Don't forget this dependency ;)

@@ +148,5 @@
> +                          {method: 'start', buildApp: Utils.MozBuildApp});
> +      break;
> +      case 'AccessFu:Present':
> +      try {
> +        for each (var presenter in aMessage.json) {

nit: use 'let'?

@@ +223,5 @@
> +  },
> +
> +  Speech: function Speech(aDetails, aBrowser) {
> +    function doSpeech() {
> +      for each (var action in aDetails.actions)

nit: use 'let'?

@@ +287,5 @@
> +  Android: function Android(aDetails, aBrowser) {
> +    if (!this._bridge)
> +      this._bridge = Cc['@mozilla.org/android/bridge;1'].getService(Ci.nsIAndroidBridge);
> +
> +    for each (var androidEvent in aDetails) {

nit: use 'let'?

@@ +475,5 @@
> +  },
> +
> +  keyMap: {
> +    a: ['moveNext', 'Anchor'],
> +    A: ['movePrevious', 'Anchor'],

I like this next/prev wording better than forward/backward. Aside: this structure would line up nicely if it was 'movePrev'.

::: accessible/src/jsat/EventManager.jsm
@@ +50,5 @@
> +    );
> +  },
> +
> +  stop: function stop() {
> +    Services.obs.removeObserver(this, 'accessible-event');

Can you pop the presenters here too?

@@ +58,5 @@
> +  handleEvent: function handleEvent(aEvent) {
> +    try {
> +      switch (aEvent.type) {
> +      case 'DOMActivate':
> +        {

nit: This case block indenting seems inconsistent with the other switch cases in AccessFu.

@@ +61,5 @@
> +      case 'DOMActivate':
> +        {
> +          let activatedAcc =
> +            Utils.AccRetrieval.getAccessibleFor(aEvent.originalTarget);
> +          let[state, extState] = Utils.getStates(activatedAcc);

nit: add a space after let?

@@ +86,5 @@
> +            }
> +          );
> +          break;
> +        }
> +

nit: extra line.

@@ +96,5 @@
> +
> +  observe: function observe(aSubject, aTopic, aData) {
> +    switch (aTopic) {
> +      case 'accessible-event':
> +        var event;

You have a single case switch here, and it doesn't have {}'s in the case block. Maybe make it an if? (Or are you trying to keep symmetry with other observe: impl?)

@@ +205,5 @@
> +      case Ci.nsIAccessibleEvent.EVENT_TEXT_INSERTED:
> +      case Ci.nsIAccessibleEvent.EVENT_TEXT_REMOVED:
> +      {
> +        if (aEvent.isFromUserInput) {
> +          // XXX support live regions as well.

File a bug? (Good first bug?)

@@ +244,5 @@
> +      }
> +    }
> +  },
> +
> +  present: function present(aPresenterFunc) {

Strategy pattern, an old friend.

@@ +249,5 @@
> +    try {
> +      this.sendMsgFunc(
> +        "AccessFu:Present",
> +        [aPresenterFunc(p) for each (p in this.presenters)].
> +          filter(function(d) {return !!d;}));

Funky jazztastic.

@@ +266,5 @@
> +
> +    if ((aStateFlags & loadingState) == loadingState)
> +      tabstate = 'loading';
> +
> +    if ((aStateFlags & loadedState) == loadedState && !aWebProgress.isLoadingDocument)

Can you make one of these if's an else if?

@@ +297,5 @@
> +
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener,
> +                                         Ci.nsISupportsWeakReference,
> +                                         Ci.nsISupports,
> +                                         Ci.nsIObserver])

I'm not really sure what this does or how this works. Do you really need to explicitly generate nsISupports here?

::: accessible/src/jsat/Presenters.jsm
@@ +26,5 @@
>  function Presenter() {}
>  
>  Presenter.prototype = {
>    /**
> +   * The type of presenter. Used for matching it with the appropriate ouput adapter.

Please avoid saying adapter here :) It threw me during review since I was thinking formal adapter pattern. How about:
* The type of presenter, used for matching to appropriate output method.

::: accessible/src/jsat/content-script.js
@@ +249,5 @@
> +    // XXX: Ideally this would be an a11y event. Bug #742280.
> +    removeEventListener('DOMActivate', EventManager, true);
> +  });
> +
> +sendAsyncMessage('AccessFu:Ready');

It is good this is fired only once since we don't guard against multiple calls right? When is content-script.js processed?
(In reply to David Bolter [:davidb] from comment #3)

> think I'll submit this first round of review which distractions abound.

'while' distractions abound.
(In reply to David Bolter [:davidb] from comment #3)
> Comment on attachment 664305 [details] [diff] [review]
> Separate content from chrome.
> 
> Review of attachment 664305 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Cool. Great that you got this going. Re; review, I've been jumping forward
> and backward as I learn what you are doing here (and thanks for IRCing). I
> think I'll submit this first round of review which distractions abound.
> 
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +11,5 @@
> >  
> >  var EXPORTED_SYMBOLS = ['AccessFu'];
> >  
> >  Cu.import('resource://gre/modules/Services.jsm');
> > +Cu.import('resource://gre/modules/tts.jsm');
> 
> Don't forget this dependency ;)

I'll put that patch in a separate bug, and just have it log speech commands for now.

> 
> @@ +148,5 @@
> > +                          {method: 'start', buildApp: Utils.MozBuildApp});
> > +      break;
> > +      case 'AccessFu:Present':
> > +      try {
> > +        for each (var presenter in aMessage.json) {
> 
> nit: use 'let'?
> 
> @@ +223,5 @@
> > +  },
> > +
> > +  Speech: function Speech(aDetails, aBrowser) {
> > +    function doSpeech() {
> > +      for each (var action in aDetails.actions)
> 
> nit: use 'let'?
> 
> @@ +287,5 @@
> > +  Android: function Android(aDetails, aBrowser) {
> > +    if (!this._bridge)
> > +      this._bridge = Cc['@mozilla.org/android/bridge;1'].getService(Ci.nsIAndroidBridge);
> > +
> > +    for each (var androidEvent in aDetails) {
> 
> nit: use 'let'?
> 

All non-global 'var's have been replaced by 'let's.

> @@ +475,5 @@
> > +  },
> > +
> > +  keyMap: {
> > +    a: ['moveNext', 'Anchor'],
> > +    A: ['movePrevious', 'Anchor'],
> 
> I like this next/prev wording better than forward/backward. Aside: this
> structure would line up nicely if it was 'movePrev'.
> 

The wording above is consistent with the pivot API, I think it is less confusing.

> ::: accessible/src/jsat/EventManager.jsm
> @@ +50,5 @@
> > +    );
> > +  },
> > +
> > +  stop: function stop() {
> > +    Services.obs.removeObserver(this, 'accessible-event');
> 
> Can you pop the presenters here too?
> 

Yup, good catch.

> @@ +58,5 @@
> > +  handleEvent: function handleEvent(aEvent) {
> > +    try {
> > +      switch (aEvent.type) {
> > +      case 'DOMActivate':
> > +        {
> 
> nit: This case block indenting seems inconsistent with the other switch
> cases in AccessFu.
> 

Yeah, switch block indentations are inconsistent, sorry. I fixed this, but I am generally unhappy about it. Whitespace!

> @@ +61,5 @@
> > +      case 'DOMActivate':
> > +        {
> > +          let activatedAcc =
> > +            Utils.AccRetrieval.getAccessibleFor(aEvent.originalTarget);
> > +          let[state, extState] = Utils.getStates(activatedAcc);
> 
> nit: add a space after let?
> 

Yep, gjslint somehow did that.

> @@ +86,5 @@
> > +            }
> > +          );
> > +          break;
> > +        }
> > +
> 
> nit: extra line.

Done.

> 
> @@ +96,5 @@
> > +
> > +  observe: function observe(aSubject, aTopic, aData) {
> > +    switch (aTopic) {
> > +      case 'accessible-event':
> > +        var event;
> 
> You have a single case switch here, and it doesn't have {}'s in the case
> block. Maybe make it an if? (Or are you trying to keep symmetry with other
> observe: impl?)
> 

planning for the future when we observe for other things besides accessible events, and staying with switch blocks on event callbacks.

> @@ +205,5 @@
> > +      case Ci.nsIAccessibleEvent.EVENT_TEXT_INSERTED:
> > +      case Ci.nsIAccessibleEvent.EVENT_TEXT_REMOVED:
> > +      {
> > +        if (aEvent.isFromUserInput) {
> > +          // XXX support live regions as well.
> 
> File a bug? (Good first bug?)
> 

Bug 795957.

> @@ +244,5 @@
> > +      }
> > +    }
> > +  },
> > +
> > +  present: function present(aPresenterFunc) {
> 
> Strategy pattern, an old friend.
> 

Is that what it is called? I'm doing that to deal with js modules modularized namespaces.

> @@ +249,5 @@
> > +    try {
> > +      this.sendMsgFunc(
> > +        "AccessFu:Present",
> > +        [aPresenterFunc(p) for each (p in this.presenters)].
> > +          filter(function(d) {return !!d;}));
> 
> Funky jazztastic.
> 

They are porting python to js!

> @@ +266,5 @@
> > +
> > +    if ((aStateFlags & loadingState) == loadingState)
> > +      tabstate = 'loading';
> > +
> > +    if ((aStateFlags & loadedState) == loadedState && !aWebProgress.isLoadingDocument)
> 
> Can you make one of these if's an else if?
> 

Yup.

> @@ +297,5 @@
> > +
> > +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener,
> > +                                         Ci.nsISupportsWeakReference,
> > +                                         Ci.nsISupports,
> > +                                         Ci.nsIObserver])
> 
> I'm not really sure what this does or how this works. Do you really need to
> explicitly generate nsISupports here?
> 

It makes sure to return this object if any of those interfaces are queried. I know it is needed for web progress, so I added every other interface it is supposed to support too.

> ::: accessible/src/jsat/Presenters.jsm
> @@ +26,5 @@
> >  function Presenter() {}
> >  
> >  Presenter.prototype = {
> >    /**
> > +   * The type of presenter. Used for matching it with the appropriate ouput adapter.
> 
> Please avoid saying adapter here :) It threw me during review since I was
> thinking formal adapter pattern. How about:
> * The type of presenter, used for matching to appropriate output method.
> 

Done.

> ::: accessible/src/jsat/content-script.js
> @@ +249,5 @@
> > +    // XXX: Ideally this would be an a11y event. Bug #742280.
> > +    removeEventListener('DOMActivate', EventManager, true);
> > +  });
> > +
> > +sendAsyncMessage('AccessFu:Ready');
> 
> It is good this is fired only once since we don't guard against multiple
> calls right? When is content-script.js processed?

The :Ready is received by the chrome script, which sends a :Start and the event manager is started. The event manager is guarded against multiple calls to start().
Here is a revized version after review #1.
Attachment #664305 - Attachment is obsolete: true
Attachment #664305 - Flags: feedback?(dbolter)
Attachment #666600 - Flags: review?(dbolter)
Attachment #666600 - Flags: review?(dbolter) → review+
https://hg.mozilla.org/mozilla-central/rev/ec776b922b23

Should this have tests?
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(In reply to Ryan VanderMeulen from comment #8)
> https://hg.mozilla.org/mozilla-central/rev/ec776b922b23
> 
> Should this have tests?

We don't currently have test coverage for AccessFu, but it is a priority :)
You need to log in before you can comment on or make changes to this bug.