756287 - [AccessFu] Introduce PresenterContext to encapsulate and cache tree operations

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

Right now the main AccessFu object does the new ancestry calculation and provides it to all presenters so that each presenter does not have to do it for itself. Other operations might soon be necessary like getting a flat list of descendants. This won't scale in AccessFu, and it shouldn't need to be there in the first place.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Introduce PresenterContext

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 624928 [details] [diff] [review]
Introduce PresenterContext

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

::: accessible/src/jsat/Presenters.jsm
@@ +147,3 @@
>        this._hide();
>        return;
>      }

what is consumer of this behavior?

@@ +333,5 @@
> +
> +/**
> + * Presenter context
> + */
> +

not sure if you need a new line here

btw, "presenter context" comment for PresenterContext object doesn't make things clearer really

@@ +334,5 @@
> +/**
> + * Presenter context
> + */
> +
> +function PresenterContext(aAccessible, aOldAccessible) {

it makes sense to comment arguments

@@ +348,5 @@
> +  get oldAccessible() {
> +    return this._oldAccessible;
> +  },
> +
> +  get newAncestry() {

it'd be nice to keep them documented

Comment 3

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

Comment on attachment 624928 [details] [diff] [review]
Introduce PresenterContext

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

r=me, the world should not implode yet please address all reviewer comments :)

'get' is an operator I don't recall seeing used before. I assume you are using it for sugar? (to avoid underscores when referring to data members)

::: accessible/src/jsat/Presenters.jsm
@@ +344,5 @@
> +  get accessible() {
> +    return this._accessible;
> +  },
> +
> +  get oldAccessible() {

Where is this called?

@@ +348,5 @@
> +  get oldAccessible() {
> +    return this._oldAccessible;
> +  },
> +
> +  get newAncestry() {

I'd like a comment about what newAncestry does. I now understand what it is for but only after looking at the code and thinking about it for 15+ minutes. (I appreciate what this does in terms of TTS Ux - nice)

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to David Bolter [:davidb] from comment #3)
> Comment on attachment 624928 [details] [diff] [review]
> Introduce PresenterContext
> 
> Review of attachment 624928 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, the world should not implode yet please address all reviewer comments
> :)
> 
> 'get' is an operator I don't recall seeing used before. I assume you are
> using it for sugar? (to avoid underscores when referring to data members)
> 

Not sure about sugar, but just trying more strict encapsulation. Since there are no "set" operators the attributes are read-only, setting them with obf.foo = "bar" is a no-op.

This guarantees the cached info is valid since the accessible and old accessible are only set at construction.

> ::: accessible/src/jsat/Presenters.jsm
> @@ +344,5 @@
> > +  get accessible() {
> > +    return this._accessible;
> > +  },
> > +
> > +  get oldAccessible() {
> 
> Where is this called?
> 

It isn't, yet. But it seems like it could be useful in the future.

> @@ +348,5 @@
> > +  get oldAccessible() {
> > +    return this._oldAccessible;
> > +  },
> > +
> > +  get newAncestry() {
> 
> I'd like a comment about what newAncestry does. I now understand what it is
> for but only after looking at the code and thinking about it for 15+
> minutes. (I appreciate what this does in terms of TTS Ux - nice)

Aye. This doesn't introduce new functionality, I just moved it from AccessFu where it was probably more obscure. I'll put in a few words.

Comment 5

[Eitan Isaacson [:eeejay]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ae944ea53b59

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ae944ea53b59

Comment 7

[alexander :surkov (:asurkov)]

Eitan, you should address (fix or answer) comments before patch landing (not all concerns from my comment #2 are addressed).