718973 - It would be nice to have more Tilt notifications

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Victor Porof [:vporof][:vp]]

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 (he/him) [:past] (please ni?)]

Or INITIALIZING, INITIALIZED, DESTROYING, DESTROYED, etc.

Comment 2

[Victor Porof [:vporof][:vp]]

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]]

Attachment: v1

Let's see if this survives try:
https://tbpl.mozilla.org/?tree=Try&rev=53192342fceb

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: v2

More better tests.

Comment 5

[Victor Porof [:vporof][:vp]]

Try for v2: https://tbpl.mozilla.org/?tree=Try&rev=642af0bf9d4dv

Comment 6

[Victor Porof [:vporof][:vp]]

Attachment: 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)]

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]]

(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)]

(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)]

https://hg.mozilla.org/integration/fx-team/rev/2090ef611402

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/2090ef611402