Last Comment Bug 718973 - It would be nice to have more Tilt notifications
: It would be nice to have more Tilt notifications
Status: RESOLVED FIXED
[tilt]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: Firefox 12
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
Depends on: 715163
Blocks: 719049
  Show dependency treegraph
 
Reported: 2012-01-18 03:29 PST by Victor Porof [:vporof][:vp]
Modified: 2012-01-28 06:23 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (40.02 KB, patch)
2012-01-18 06:02 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2 (44.00 KB, patch)
2012-01-18 13:51 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v3 (45.62 KB, patch)
2012-01-19 10:25 PST, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Splinter Review

Description Victor Porof [:vporof][:vp] 2012-01-18 03:29:09 PST
With all the added animations and transitions and async initialization, it would be to everyone's benefit to have a couple more notifications. Tests will be easier to write too!

In the end, I see the notifications looking like this:

// Fires when Tilt starts the initialization.
INITIALIZED: "tilt-initialized",

// Fires immediately after initialization is complete.
// (when the canvas overlay is visible and the 3D mesh is completely created)
AFTER_INITIALIZATION_FINISHED: "tilt-after-initialization-finished",

// Fires immediately before the destruction is finished.
// (just before the canvas overlay is removed from its parent node)
BEFORE_DESTRUCTION_FINISHED: "tilt-before-destruction-finished",

// Fires when Tilt is completely destroyed.
DESTROYED: "tilt-destroyed",

// Fires when Tilt is shown (after a tab-switch).
SHOWN: "tilt-shown",

// Fires when Tilt is hidden (after a tab-switch).
HIDDEN: "tilt-hidden"
Comment 1 Panos Astithas [:past] 2012-01-18 03:41:10 PST
Or INITIALIZING, INITIALIZED, DESTROYING, DESTROYED, etc.
Comment 2 Victor Porof [:vporof][:vp] 2012-01-18 05:12:48 PST
Good idea. So:

// Fires when Tilt starts the initialization.
INITIALIZING: "tilt-initializing",

// Fires immediately after initialization is complete.
// (when the canvas overlay is visible and the 3D mesh is completely created)
INITIALIZED: "tilt-initialized",

// Fires immediately before the destruction is started.
DESTROYING: "tilt-destroying",

// Fires immediately before the destruction is finished.
// (just before the canvas overlay is removed from its parent node)
BEFORE_DESTROYED: "tilt-before-destroyed",

// Fires when Tilt is completely destroyed.
DESTROYED: "tilt-destroyed",

// Fires when Tilt is shown (after a tab-switch).
SHOWN: "tilt-shown",

// Fires when Tilt is hidden (after a tab-switch).
HIDDEN: "tilt-hidden",

// Fires once Tilt highlights an element in the page.
HIGHLIGHTING: "tilt-highlighting",

// Fires once Tilt stops highlighting any element.
UNHIGHLIGHTING: "tilt-unhighlighting",

// Fires when a node is remove from the 3D mesh.
NODE_REMOVED: "tilt-node-removed"


I think this covers just about everything that could be useful (especially for anyone who would like to create an extension on top of Tilt).
Comment 3 Victor Porof [:vporof][:vp] 2012-01-18 06:02:34 PST
Created attachment 589479 [details] [diff] [review]
v1

Let's see if this survives try:
https://tbpl.mozilla.org/?tree=Try&rev=53192342fceb
Comment 4 Victor Porof [:vporof][:vp] 2012-01-18 13:51:21 PST
Created attachment 589635 [details] [diff] [review]
v2

More better tests.
Comment 5 Victor Porof [:vporof][:vp] 2012-01-18 14:58:50 PST
Try for v2: https://tbpl.mozilla.org/?tree=Try&rev=642af0bf9d4dv
Comment 6 Victor Porof [:vporof][:vp] 2012-01-19 10:25:35 PST
Created attachment 589900 [details] [diff] [review]
v3

Cleaned up and added a function to handle keyframe notifications to avoid making the loop function too bloated.
Comment 7 Rob Campbell [:rc] (:robcee) 2012-01-24 12:55:15 PST
Comment on attachment 589900 [details] [diff] [review]
v3

 
-  // Fires when Tilt is destroyed.
+  // Fires immediately before the destruction is started.
+  DESTROYING: "tilt-destroying",
+
+  // Fires immediately before the destruction is finished.
+  // (just before the canvas overlay is removed from its parent node)
+  BEFORE_DESTROYED: "tilt-before-destroyed",
+

is there a need for this level of granularity here or is this notification speculative? I'd think we could do away with BEFORE_DESTROYED without upsetting anyone.

+      let content = presenter.contentWindow;
+      let pageXOffset = content.pageXOffset * presenter.transforms.zoom;
+      let pageYOffset = content.pageYOffset * presenter.transforms.zoom;
+

why are those here? I think I've seen this change slightly in just about every patch!

in TiltVisualizer.jsm
+/*global Components, Services, ChromeWorker */

augh!

and this patch is getting complicated. Saving this much. I need to look at the changes you've made to initialization more closely.
Comment 8 Victor Porof [:vporof][:vp] 2012-01-24 23:05:00 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #7)
> Comment on attachment 589900 [details] [diff] [review]
> v3
> 
>  
> -  // Fires when Tilt is destroyed.
> +  // Fires immediately before the destruction is started.
> +  DESTROYING: "tilt-destroying",
> +
> +  // Fires immediately before the destruction is finished.
> +  // (just before the canvas overlay is removed from its parent node)
> +  BEFORE_DESTROYED: "tilt-before-destroyed",
> +
> 
> is there a need for this level of granularity here or is this notification
> speculative? I'd think we could do away with BEFORE_DESTROYED without
> upsetting anyone.
> 

Removing BEFORE_DESTROYED would simplify the patch by only 2 lines :) I also see plenty use cases for this (which would translate to "after the outro animation finished". Suppose someone wanted to create an addon ading fade transition from tilt to highlighter after the outro animation is finished. The BEFORE_DESTROYED is mandatory here.

> +      let content = presenter.contentWindow;
> +      let pageXOffset = content.pageXOffset * presenter.transforms.zoom;
> +      let pageYOffset = content.pageYOffset * presenter.transforms.zoom;
> +
> 
> why are those here? I think I've seen this change slightly in just about
> every patch!
> 

This hasn't really changed. These lines are moved from 162 to 181. Also, TiltUtils.getDocumentZoom() is identical to presenter.transforms.zoom here.

> in TiltVisualizer.jsm
> +/*global Components, Services, ChromeWorker */
> 
> augh!
> 

Please ignore all the annoying global comments. I burn them in bug 719732.

> and this patch is getting complicated. Saving this much. I need to look at
> the changes you've made to initialization more closely.

I honestly think we need these notifications. Along with helping other addon ninjas, writing tests was already getting highly messy!
Comment 9 Rob Campbell [:rc] (:robcee) 2012-01-25 06:36:24 PST
(In reply to Victor Porof from comment #8)
> (In reply to Rob Campbell [:rc] (robcee) from comment #7)
> > Comment on attachment 589900 [details] [diff] [review]
> > v3
> > 
> >  
> > -  // Fires when Tilt is destroyed.
> > +  // Fires immediately before the destruction is started.
> > +  DESTROYING: "tilt-destroying",
> > +
> > +  // Fires immediately before the destruction is finished.
> > +  // (just before the canvas overlay is removed from its parent node)
> > +  BEFORE_DESTROYED: "tilt-before-destroyed",
> > +
> > 
> > is there a need for this level of granularity here or is this notification
> > speculative? I'd think we could do away with BEFORE_DESTROYED without
> > upsetting anyone.
> 
> Removing BEFORE_DESTROYED would simplify the patch by only 2 lines :) I also
> see plenty use cases for this (which would translate to "after the outro
> animation finished". Suppose someone wanted to create an addon ading fade
> transition from tilt to highlighter after the outro animation is finished.
> The BEFORE_DESTROYED is mandatory here.

Maybe the name is what's bugging me then. Should be OUTRO_FINISHED if that's the real state we're trying to capture.

And I find your example use case to be highly theoretical. :)

> > +      let content = presenter.contentWindow;
> > +      let pageXOffset = content.pageXOffset * presenter.transforms.zoom;
> > +      let pageYOffset = content.pageYOffset * presenter.transforms.zoom;
> > +
> > 
> > why are those here? I think I've seen this change slightly in just about
> > every patch!
> 
> This hasn't really changed. These lines are moved from 162 to 181. Also,
> TiltUtils.getDocumentZoom() is identical to presenter.transforms.zoom here.

mm. k.

> > in TiltVisualizer.jsm
> > +/*global Components, Services, ChromeWorker */
> > 
> > augh!
> > 
> 
> Please ignore all the annoying global comments. I burn them in bug 719732.

I'm trying!

> > and this patch is getting complicated. Saving this much. I need to look at
> > the changes you've made to initialization more closely.
> 
> I honestly think we need these notifications. Along with helping other addon
> ninjas, writing tests was already getting highly messy!

I believe that. Finer grained notifications can be helpful for testing, but feel like a pretty big hammer.

I'll continue the review, but selling this for aurora approval feels tricky.
Comment 10 Rob Campbell [:rc] (:robcee) 2012-01-27 12:50:22 PST
https://hg.mozilla.org/integration/fx-team/rev/2090ef611402
Comment 11 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-01-28 06:23:50 PST
https://hg.mozilla.org/mozilla-central/rev/2090ef611402

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