It would be nice to have more Tilt notifications

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

12 Branch
Firefox 12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tilt])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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"
(Assignee)

Updated

5 years ago
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Whiteboard: [tilt]
Or INITIALIZING, INITIALIZED, DESTROYING, DESTROYED, etc.
(Assignee)

Comment 2

5 years ago
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).
(Assignee)

Comment 3

5 years ago
Created attachment 589479 [details] [diff] [review]
v1

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

Comment 4

5 years ago
Created attachment 589635 [details] [diff] [review]
v2

More better tests.
(Assignee)

Updated

5 years ago
Attachment #589635 - Flags: review?(rcampbell)
(Assignee)

Comment 5

5 years ago
Try for v2: https://tbpl.mozilla.org/?tree=Try&rev=642af0bf9d4dv
(Assignee)

Comment 6

5 years ago
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.
Attachment #589479 - Attachment is obsolete: true
Attachment #589635 - Attachment is obsolete: true
Attachment #589635 - Flags: review?(rcampbell)
Attachment #589900 - Flags: review?(rcampbell)
(Assignee)

Updated

5 years ago
Depends on: 715163
(Assignee)

Updated

5 years ago
Blocks: 719049
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.
(Assignee)

Comment 8

5 years ago
(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!
(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.
tracking-firefox11: --- → ?
Attachment #589900 - Flags: review?(rcampbell) → review+
https://hg.mozilla.org/integration/fx-team/rev/2090ef611402
tracking-firefox11: ? → ---
Whiteboard: [tilt] → [tilt][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2090ef611402
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [tilt][fixed-in-fx-team] → [tilt]
Target Milestone: --- → Firefox 12
You need to log in before you can comment on or make changes to this bug.