Closed Bug 965537 Opened 9 years ago Closed 9 years ago

[AccessFu] Refactor content-script.js

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

Attachments

(1 file, 2 obsolete files)

It is becoming a big collection of random functions.
Assignee: nobody → eitan
Attached patch Introduce content controller (obsolete) — Splinter Review
Attachment #8380853 - Flags: review?(yzenevich)
Attachment #8380853 - Attachment is obsolete: true
Attachment #8380853 - Flags: review?(yzenevich)
Attachment #8383034 - Flags: review?(yzenevich)
Attachment #8383034 - Attachment is obsolete: true
Attachment #8383034 - Flags: review?(yzenevich)
Attachment #8383152 - Flags: review?(yzenevich)
Comment on attachment 8383152 [details] [diff] [review]
[AccessFu] Introduce ContentControl.

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

Looks very cool. r=me with questions/nits.

::: accessible/src/jsat/AccessFu.jsm
@@ +298,5 @@
>          break;
>        case 'Accessibility:Focus':
>          this._focused = JSON.parse(aData);
>          if (this._focused) {
> +	  this.autoMove({ forcePresent: true, noOpIfOnScreen: true, delay: 0 });

Delay 0 should be a default.
nit: indentation.

@@ +342,5 @@
>          if (this._focused) {
>            // We delay this for half a second so the awesomebar could close,
>            // and we could use the current coordinates for the content item.
>            // XXX TODO figure out how to avoid magic wait here.
> +	  this.autoMove({

Nit: indentation.

::: accessible/src/jsat/ContentControl.jsm
@@ +58,5 @@
> +  get vc() {
> +    return Utils.getVirtualCursor(this.document);
> +  },
> +
> +  addListenersFor: function ContentControl_addListenersFor(aMessageManager) {

I would rename to addCursorListener since it's just 1 message name we listen to and have 1 listener.

@@ +95,5 @@
> +
> +  handleMove: function ContentControl_handleMove(aMessage) {
> +    let origin = aMessage.json.origin;
> +    let action = aMessage.json.action;
> +    let rule = aMessage.json.rule;

let rule = TraversalRules[aMessage.json.rule];

@@ +98,5 @@
> +    let action = aMessage.json.action;
> +    let rule = aMessage.json.rule;
> +    let vc = this.vc;
> +
> +    if (origin != 'child' && this.sendToChild(aMessage, vc)) {

Lost a helpful comment here.

@@ +104,5 @@
> +    }
> +
> +    let moved = vc[action](TraversalRules[rule]);
> +
> +    if (moved) {

I think it the block below should look like(from what i understand):
if (moved) {
  if (origin === 'child') {
    Utils.getMessageManager(aMessage.target).sendAsyncMessage(
      'AccessFu:ClearCursor');
  } else {
    if (action === 'moveNext') {
      action = 'moveFirst';
    } else if (action === 'movePrevious') {
      action = 'moveLast';
    }
    this.sendToChild(aMessage, vc, {
      action: action
    });
  }
} else if (!this._childMessageSenders.has(aMessage.target)) {
  this.sendToParent(aMessage);
}

@@ +113,5 @@
> +        action = 'moveLast';
> +      }
> +
> +      if (origin === 'child') {
> +        Utils.getMessageManager(aMessage.target).

nit: bad line breakage.

@@ +128,5 @@
> +    }
> +  },
> +
> +  handleMoveToPoint: function ContentControl_handleMoveToPoint(aMessage) {
> +    let[x, y] = [aMessage.json.x, aMessage.json.y];

nit: space after let.

@@ +136,5 @@
> +
> +    let dpr = win.devicePixelRatio;
> +    this.vc.moveToPoint(rule, x * dpr, y * dpr, true);
> +
> +    let delta = Utils.isContentProcess ? {

no need to assign delta if not content process.
let delta;
if (Utils.isContentProcess) {
  delta = {
    x: x - win.mozInnerScreenX,
    y: y - win.mozInnerScreenY
  };
}

@@ +169,5 @@
> +
> +    return null;
> +  },
> +
> +  sendToChild: function ContentControl_sendToChild(aMessage,

I think order of args is a little confusing: perhaps [vc, aMessage, aReplacer] this way it looks clearer that aReplacer extends aMessage.json (sort of :) ).

@@ +232,5 @@
> +        return;
> +      }
> +
> +      if (aOptions.moveToFocused) {
> +        let focusedAcc =

acc = Utils.AccRetrieval.getAccessibleFor(this.document.activeElement) || acc;

@@ +264,5 @@
> +        forcePresentFunc();
> +      }
> +    };
> +
> +    if (aOptions.delay !== 0) {

If we have a default 0 we won't need 2 checks for delay.

::: accessible/src/jsat/EventManager.jsm
@@ +97,5 @@
>        switch (aEvent.type) {
>        case 'wheel':
>        {
>          let attempts = 0;
> +	let delta = aEvent.deltaX || aEvent.deltaY;

nit: indentations here and below.

@@ +102,5 @@
> +	this.contentScope.contentControl.autoMove(
> +	  null,
> +	  { moveMethod: delta > 0 ? 'moveNext' : 'movePrevious',
> +	    onScreenOnly: true,
> +	    noOpIfOnScreen: true });

noOpIfOnScreen: true should be a default.

@@ +184,5 @@
>        }
>        case Events.SCROLLING_START:
>        {
> +        this.contentScope.contentControl.autoMove(aEvent.accessible,
> +						  { delay: 0 });

Delay 0 should be a default.
nit: indentation.

@@ +273,5 @@
>          // Put vc where the focus is at
>          let acc = aEvent.accessible;
>          let doc = aEvent.accessibleDocument;
>          if (acc.role != Roles.DOCUMENT && doc.role != Roles.CHROME_WINDOW) {
> +	  this.contentScope.contentControl.autoMove(acc, { delay: 0 });

delay: 0 should be a default.
Attachment #8383152 - Flags: review?(yzenevich) → review+
All good feedback, and fixed. Some push back below..

(In reply to Yura Zenevich [:yzen] from comment #4)
> ::: accessible/src/jsat/ContentControl.jsm
> @@ +58,5 @@
> > +  get vc() {
> > +    return Utils.getVirtualCursor(this.document);
> > +  },
> > +
> > +  addListenersFor: function ContentControl_addListenersFor(aMessageManager) {
> 
> I would rename to addCursorListener since it's just 1 message name we listen
> to and have 1 listener.
> 

I simply removed that method.

> @@ +95,5 @@
> > +
> > +  handleMove: function ContentControl_handleMove(aMessage) {
> > +    let origin = aMessage.json.origin;
> > +    let action = aMessage.json.action;
> > +    let rule = aMessage.json.rule;
> 
> let rule = TraversalRules[aMessage.json.rule];
> 

Remove that variable.

> @@ +98,5 @@
> > +    let action = aMessage.json.action;
> > +    let rule = aMessage.json.rule;
> > +    let vc = this.vc;
> > +
> > +    if (origin != 'child' && this.sendToChild(aMessage, vc)) {
> 
> Lost a helpful comment here.
> 
> @@ +104,5 @@
> > +    }
> > +
> > +    let moved = vc[action](TraversalRules[rule]);
> > +
> > +    if (moved) {
> 
> I think it the block below should look like(from what i understand):
> if (moved) {
>   if (origin === 'child') {
>     Utils.getMessageManager(aMessage.target).sendAsyncMessage(
>       'AccessFu:ClearCursor');
>   } else {
>     if (action === 'moveNext') {
>       action = 'moveFirst';
>     } else if (action === 'movePrevious') {
>       action = 'moveLast';
>     }
>     this.sendToChild(aMessage, vc, {
>       action: action
>     });
>   }
> } else if (!this._childMessageSenders.has(aMessage.target)) {
>   this.sendToParent(aMessage);
> }
> 

Yup! Much better. Added more comments too.

> @@ +136,5 @@
> > +
> > +    let dpr = win.devicePixelRatio;
> > +    this.vc.moveToPoint(rule, x * dpr, y * dpr, true);
> > +
> > +    let delta = Utils.isContentProcess ? {
> 
> no need to assign delta if not content process.
> let delta;
> if (Utils.isContentProcess) {
>   delta = {
>     x: x - win.mozInnerScreenX,
>     y: y - win.mozInnerScreenY
>   };
> }
> 

That is functionally the same, so I am not touching this. If that is OK. My subjective opinion is that assigning variables in conditional scopes while having to declare the variable outside the scope is ugly. Also, uninitialized variables are bad.

> @@ +169,5 @@
> > +
> > +    return null;
> > +  },
> > +
> > +  sendToChild: function ContentControl_sendToChild(aMessage,
> 
> I think order of args is a little confusing: perhaps [vc, aMessage,
> aReplacer] this way it looks clearer that aReplacer extends aMessage.json
> (sort of :) ).
> 

Done!

> @@ +264,5 @@
> > +        forcePresentFunc();
> > +      }
> > +    };
> > +
> > +    if (aOptions.delay !== 0) {
> 
> If we have a default 0 we won't need 2 checks for delay.
> 

I still prefer to. Because of the weird things that could happen with this._autoMove. If the delay is 0, it should do the move immediately.

> @@ +102,5 @@
> > +	this.contentScope.contentControl.autoMove(
> > +	  null,
> > +	  { moveMethod: delta > 0 ? 'moveNext' : 'movePrevious',
> > +	    onScreenOnly: true,
> > +	    noOpIfOnScreen: true });
> 
> noOpIfOnScreen: true should be a default.
> 

I don't think so.. we usually don't want that behavior.
https://hg.mozilla.org/mozilla-central/rev/38659c5a5c70
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.