Last Comment Bug 756287 - [AccessFu] Introduce PresenterContext to encapsulate and cache tree operations
: [AccessFu] Introduce PresenterContext to encapsulate and cache tree operations
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla15
Assigned To: Eitan Isaacson [:eeejay]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-17 15:36 PDT by Eitan Isaacson [:eeejay]
Modified: 2012-05-19 08:20 PDT (History)
3 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Introduce PresenterContext (11.55 KB, patch)
2012-05-17 15:37 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Review

Description Eitan Isaacson [:eeejay] 2012-05-17 15:36:04 PDT
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] 2012-05-17 15:37:34 PDT
Created attachment 624928 [details] [diff] [review]
Introduce PresenterContext
Comment 2 alexander :surkov 2012-05-17 21:07:50 PDT
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] 2012-05-18 07:53:40 PDT
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] 2012-05-18 11:39:03 PDT
(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] 2012-05-18 11:57:17 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/ae944ea53b59
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-05-18 18:39:14 PDT
https://hg.mozilla.org/mozilla-central/rev/ae944ea53b59
Comment 7 alexander :surkov 2012-05-19 08:20:01 PDT
Eitan, you should address (fix or answer) comments before patch landing (not all concerns from my comment #2 are addressed).

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