689920 - Integrate Tilt with existing Firefox developer tools

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

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

Tilt is to be repackaged and included in Firefox as an integrated developer tool. This means augmenting with the Style Inspector, Style Editor, Highlighter and removing any irrelevant features, as well as making it multi-process capable to fit with the e10s project. A feature page about the project can be found at [0].

Currently, Tilt is waiting for full review on AMO [1] and the source code is on Github [2], but will be added to a hg repository soon.

[0] https://wiki.mozilla.org/DevTools/Features/PageInspectorTilt
[1] https://addons.mozilla.org/en-US/firefox/addon/tilt/
[2] https://github.com/victorporof/Tilt

Comment 1

[Rob Campbell [:rc] (:robcee)]

you don't have to cc me here. (It was too late. I'd already seen everything.)

Comment 2

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

Updates:
1. Tilt has been approved on AMO, version 0.9.1.5
2. Integration is done in branch [0]. Currently Tilt is undergoing heavy refactoring.

[0] http://hg.mozilla.org/users/vporof_mozilla.com/tilt/

Comment 3

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

Attachment: Wip 1

First wip implementation, one FIXME left: package the visualization logic and the controller in the Tilt.jsm module. Currently divided Tilt into 4 modules: TiltGL.jsm (containing thin wrappers around low-level WebGL functions), TiltMath.jsm (containing matrix and vector operations, along with other necessary algebraic functions), TiltUtils.jsm (containing various console, localization, and prefs helpers) and Tilt.jsm (the actual visualizer implementation). This diff does not include tests.

Comment 4

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

Attachment: Wip 2

Working patch, no tests

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 569705 [details] [diff] [review]
Wip 2

I looked at TiltGL.jsm and TiltMath.jsm briefly.

They look good, only marking feedback- to ensure that my nits don't get forgotten.

TiltMath looks good, but:

add: function V3_add(aVec, aVec2, aDest) {
if (!aDest || aVec === aDest) {

Is this an optimization? Are you sure that it helps? I don't know enough about JS optimization.

if (len === 1) {
aDest[0] = x;
aDest[1] = y;
aDest[2] = z;
return aDest;
}

If len is a variable, then len == 1 will almost never be true as that would be a big coincidence in floating point arithmetic, so this looks like a counter-productive optimization.

if (Math.abs(b) < 0.0001) { // ray is parallel to triangle plane

that looks like a magic value. i didn't look enough into this to decide whether a better solution exists. could be fine.

if (a === 0) {

Again, in floating point arithmetic that will practically never happen; so it seems that here too you might want a fuzzy compare like abs(a) < some_value.

again, i didn't look closely enough into this code. just pointing out a question and how the current code answers it in 2 different ways in 2 consecutive lines, without an explanation.

if (len !== 1) {
len = 1 / len;
x *= len;
y *= len;
z *= len;
}

same as above. this if() is a counter productive optimization.

There are more occurences, just search for == which is the first step to do when reviewing floating-point code ;-)


About TiltGL.jsm:

// try to get a shader attribute
if ((io = aContext.getAttribLocation(aProgram, aVariable)) > -1) {
return io;
} else {
// if unavailable, search for a shader uniform
return aContext.getUniformLocation(aProgram, aVariable);
}

I don't like that: this should be split into 2 different functions. getAttribLocation (like, typically, other GL getters) can be slow, so you don't want to call it only to find out that you didn't want to, and there shouldn't exist any use case for trying to get the location of something about which you don't know whether it's an attrib or a uniform.

WebGL usage looks good otherwise.

Comment 6

[Dave Camp (:dcamp)]

Comment on attachment 569705 [details] [diff] [review]
Wip 2

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

Skimmed a couple files, will look more over the next day or two.  Dense, but well-organized.

::: browser/devtools/jar.mn
@@ +9,5 @@
>      content/browser/orion-mozilla.css             (sourceeditor/orion/mozilla.css)
> +    content/browser/tilt.js                       (tilt/chrome/tilt.js)
> +    content/browser/tilt.xul                      (tilt/chrome/tilt.xul)
> +    content/browser/tilt-en.dtd                   (tilt/chrome/tilt-en.dtd)
> +    content/browser/tilt-en.properties            (tilt/chrome/tilt-en.properties)

We've been starting to put stuff in content/browser/devtools:

content/browser/devtools/tilt.js (tilt/chrome/tilt.js)

would be referenced as browser/content/devtools/tilt.js

@@ +10,5 @@
> +    content/browser/tilt.js                       (tilt/chrome/tilt.js)
> +    content/browser/tilt.xul                      (tilt/chrome/tilt.xul)
> +    content/browser/tilt-en.dtd                   (tilt/chrome/tilt-en.dtd)
> +    content/browser/tilt-en.properties            (tilt/chrome/tilt-en.properties)
> +%   overlay chrome://browser/content/browser.xul  chrome://browser/content/tilt.xul

Dunno if we need overlays, we should probably just add browser UI.

::: browser/devtools/tilt/TiltVisualizer.jsm
@@ +227,5 @@
> +      setupTexture();
> +    }
> +
> +    // only redraw if we really have to
> +    if (redraw) {

This is a pretty big if, makes the method a bit tougher to follow when redraw is false.  The other ifs in this method offload their real work to another method, that might be nice here too.

@@ +355,5 @@
> +          coordHeight = coord.height;
> +
> +      // use this node only if it actually has visible dimensions
> +      if (coordWidth > 2 && coordHeight > 2) {
> +

Early return might be nice here.

@@ +416,5 @@
> +                        x,     y,     z,                    /* bottom */  // 8
> +                        x + w, y,     z,                                  // 9
> +                        x + w, y,     z - thickness,                      // 10
> +                        x,     y,     z - thickness);                     // 11
> +

aaaaaaaaaaaaaaaaaaa


Ahem.  What I mean is, "Hopefully someone with a better eye for 3d geometry will take a closer look at this, and other similar things".

@@ +765,5 @@
> + *                             the visualization instance to control
> + * @param {Function} aRequestAnimationFrame
> + *                   function responsible with scheduling loop frames
> + */
> +Tilt.Controller = function T_Controller(aCanvas, aVisualization) {

This isn't too important for now, but I can imagine a world where we'll start to build more HTML/XUL UI layered on top of tilt for handling navigation etc.  I'd imagine wanting to keep that code separate from the nitty-gritty visualization, and we'd want to put this Tilt.Controller out there with that code.

For now though, this is fine.  Moving this to its own file now seems premature.

@@ +889,5 @@
> +  var keyDown = function TC_keyDown(e) {
> +    var code = e.keyCode || e.which;
> +
> +    if (!this._browserBarFocus) {
> +      if (code >= 37 && code <= 40) { // up, down, left or right keys

You can use e.DOM_VK_LEFT and e.DOM_VK_DOWN too.

::: browser/devtools/tilt/chrome/tilt.js
@@ +62,5 @@
> +   * Initializes Tilt.
> +   */
> +  initialize: function TC_initialize() {
> +    // if the visualizer is already open, close it now
> +    // FIXME: tilt needs to be able to work in more than one tab simultaneously

Eek, that's quite a FIXME.  How does this fail right now?  We can file a followup bug to fix this, but should make sure it doesn't fail catastrophically right now.

@@ +91,5 @@
> +   *
> +   * @param {Object} aProperties
> +   *                 an object containing the following properties:
> +   *  @param {Boolean} gc: pass true to do a garbage collection when finished
> +   *  @param {Boolean} timeout: pass true to do the heavy lifting in a timeout

Was briefly confused by the formatting here, maybe remove @param from the property descriptions.

Comment 7

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

(In reply to Benoit Jacob [:bjacob] from comment #5)
> Comment on attachment 569705 [details] [diff] [review] [diff] [details] [review]
> Wip 2
> 
> TiltMath looks good, but:
> 
> add: function V3_add(aVec, aVec2, aDest) {
> if (!aDest || aVec === aDest) {
> 
Most of the time aDest is not specified, and and the purpose of this is to avoid array lookups (which can be slow). For example, in the M4_scale function, I avoid 16 array lookups using this technique, by doing 

      aMat[0 ] *= x;
      aMat[1 ] *= x;
      aMat[2 ] *= x;
      aMat[3 ] *= x;
      aMat[4 ] *= y;
      aMat[5 ] *= y;
      aMat[6 ] *= y;
      aMat[7 ] *= y;
      aMat[8 ] *= z;
      aMat[9 ] *= z;
      aMat[10] *= z;
      aMat[11] *= z;

instead of 

    aDest[0 ] = aMat[0 ] * x;
    aDest[1 ] = aMat[1 ] * x;
    aDest[2 ] = aMat[2 ] * x;
    aDest[3 ] = aMat[3 ] * x;
    aDest[4 ] = aMat[4 ] * y;
    aDest[5 ] = aMat[5 ] * y;
    aDest[6 ] = aMat[6 ] * y;
    aDest[7 ] = aMat[7 ] * y;
    aDest[8 ] = aMat[8 ] * z;
    aDest[9 ] = aMat[9 ] * z;
    aDest[10] = aMat[10] * z;
    aDest[11] = aMat[11] * z;
    aDest[12] = aMat[12];
    aDest[13] = aMat[13];
    aDest[14] = aMat[14];
    aDest[15] = aMat[15];

Doing this a couple of times in a mozRequestAnimationFrame second helps a bit :)

> if (len === 1) {
> aDest[0] = x;
> aDest[1] = y;
> aDest[2] = z;
> return aDest;
> }
> 
> If len is a variable, then len == 1 will almost never be true as that would
> be a big coincidence in floating point arithmetic, so this looks like a
> counter-productive optimization.

Agreed, removed.

> if (Math.abs(b) < 0.0001) { // ray is parallel to triangle plane
> 
> that looks like a magic value. i didn't look enough into this to decide
> whether a better solution exists. could be fine.

If a ray is parallel to a triangle plane, that value would be 0. But because of floating-point arithmetic, it will actually never be 0, which gives wrong results for real-world scenarios.

> if (a === 0) {
> 
> Again, in floating point arithmetic that will practically never happen; so
> it seems that here too you might want a fuzzy compare like abs(a) <
> some_value.

Agreed, replaced.
I was only doing this to avoid division by 0 and get Infinite values.
Same goes with all the other occurrences.

> 
> About TiltGL.jsm:
> 
> // try to get a shader attribute
> if ((io = aContext.getAttribLocation(aProgram, aVariable)) > -1) {
> return io;
> } else {
> // if unavailable, search for a shader uniform
> return aContext.getUniformLocation(aProgram, aVariable);
> }
> 
> I don't like that: this should be split into 2 different functions.
> getAttribLocation (like, typically, other GL getters) can be slow, so you
> don't want to call it only to find out that you didn't want to, and there
> shouldn't exist any use case for trying to get the location of something
> about which you don't know whether it's an attrib or a uniform.
> 
> WebGL usage looks good otherwise.

I tried to avoid manually getAttribLocation and getUniformLocation by parsing the vertex and fragment shader sources and using some regex to get out the uniforms and attributes. This way it was a bit cleaner than first manually caching the locations and then using them. But I agree, this is more of a hack than an accepted technique. Removed.

Comment 8

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

(In reply to Dave Camp (:dcamp) from comment #6)
> Comment on attachment 569705 [details] [diff] [review] [diff] [details] [review]
> Wip 2
> 
> Review of attachment 569705 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Skimmed a couple files, will look more over the next day or two.  Dense, but
> well-organized.
> 
> ::: browser/devtools/tilt/chrome/tilt.js
> @@ +62,5 @@
> > +   * Initializes Tilt.
> > +   */
> > +  initialize: function TC_initialize() {
> > +    // if the visualizer is already open, close it now
> > +    // FIXME: tilt needs to be able to work in more than one tab simultaneously
> 
> Eek, that's quite a FIXME.  How does this fail right now?  We can file a
> followup bug to fix this, but should make sure it doesn't fail
> catastrophically right now.

Just a note: like we discussed, the FIXME is there just for safety reasons. I haven't done thorough testing for this, and it's a self-reminder to do that. It will obviously be dealt with.

Comment 9

[Cedric Vivier [:cedricv]]

(In reply to Victor Porof from comment #7)
> > if (len === 1) {
> > 
> > If len is a variable, then len == 1 will almost never be true as that would
> > be a big coincidence in floating point arithmetic, so this looks like a
> > counter-productive optimization.
> 
> Agreed, removed.
> 
> > if (Math.abs(b) < 0.0001) { // ray is parallel to triangle plane
> > 
> > that looks like a magic value. i didn't look enough into this to decide
> > whether a better solution exists. could be fine.
> 
> If a ray is parallel to a triangle plane, that value would be 0. But because
> of floating-point arithmetic, it will actually never be 0, which gives wrong
> results for real-world scenarios.
> 
> > if (a === 0) {
> > 
> > Again, in floating point arithmetic that will practically never happen; so
> > it seems that here too you might want a fuzzy compare like abs(a) <
> > some_value.
> 
> Agreed, replaced.

For those, instead of using literal 0.0001 like above, you'd probably want to define a const EPSILON so that we don't see this "magic number" in many places.

Comment 10

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

(In reply to Cedric Vivier [cedricv] from comment #9)
> 
> For those, instead of using literal 0.0001 like above, you'd probably want
> to define a const EPSILON so that we don't see this "magic number" in many
> places.

The "magic number" appears only there. But a const seems logical, I'll do that.

Comment 11

[Cedric Vivier [:cedricv]]

Comment on attachment 569705 [details] [diff] [review]
Wip 2

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

::: browser/devtools/tilt/TiltGL.jsm
@@ +1643,5 @@
> +  resizeImageToPowerOfTwo: function TGLTU_resizeImageToPowerOfTwo(aImage, aParameters) {
> +    // make sure the parameters argument is an object
> +    aParameters = aParameters || {};
> +
> +    var isChromePath = (aImage.src || "").indexOf("chrome://"),

With such a name, you'd expect a boolean rather than an offset. So there probably should be != -1 at the end.

@@ +1649,5 @@
> +        isPowerOfTwoHeight = TiltMath.isPowerOfTwo(aImage.height);
> +
> +    // first check if the image is not already power of two
> +    if (aParameters.preserve ||
> +        (isPowerOfTwoWidth && isPowerOfTwoHeight && isChromePath === -1)) {

&& !isChromePath)

@@ +1698,5 @@
> + * @param {Uniform} mvMatrix: the model view matrix
> + * @param {Uniform} projMatrix: the projection matrix
> + * @param {Uniform} color: the color to set the gl_FragColor to
> + */
> +TiltGL.ColorShader = {

Maybe shaders should be loaded from separate files?

::: browser/devtools/tilt/TiltMath.jsm
@@ +107,5 @@
> +   *
> +   * @return {Array} the destination vec3 if specified, first operand otherwise
> +   */
> +  add: function V3_add(aVec, aVec2, aDest) {
> +    if (!aDest || aVec === aDest) {

"let dest = aDest || aVec;" and assign on dest[x], remove the if block?

@@ +134,5 @@
> +   *
> +   * @return {Array} the destination vec3 if specified, first operand otherwise
> +   */
> +  subtract: function V3_subtract(aVec, aVec2, aDest) {
> +    if (!aDest || aVec === aDest) {

Same.

@@ +183,5 @@
> +   *
> +   * @return {Array} the destination vec3 if specified, first operand otherwise
> +   */
> +  scale: function V3_scale(aVec, aVal, aDest) {
> +    if (!aDest || aVec === aDest) {

Same.

@@ +590,5 @@
> +  create: function M3_create(aMat) {
> +    var dest = new Float32Array(9);
> +
> +    if (aMat) {
> +      dest[0] = aMat[0];

This if block could be just mat3.set(aMat, dest);

@@ +600,5 @@
> +      dest[6] = aMat[6];
> +      dest[7] = aMat[7];
> +      dest[8] = aMat[8];
> +    } else {
> +      dest[0] = 1;

This else block could be just "mat3.identity(dest);"

@@ +763,5 @@
> +  create: function M4_create(aMat) {
> +    var dest = new Float32Array(16);
> +
> +    if (aMat) {
> +      dest[0 ] = aMat[0 ];

This if block could be just mat4.set(aMat, dest);

@@ +780,5 @@
> +      dest[13] = aMat[13];
> +      dest[14] = aMat[14];
> +      dest[15] = aMat[15];
> +    } else {
> +      dest[0 ] = 1;

This else block could be just "mat4.identity(dest);"

@@ +2127,5 @@
> +
> +    var halfTheta = Math.acos(cosHalfTheta),
> +        sinHalfTheta = Math.sqrt(1 - cosHalfTheta * cosHalfTheta);
> +
> +    if (Math.abs(sinHalfTheta) < 0.001) {

There is at least this another place where EPSILON should be used, plus wherever a Math.abs(x) < EPSILON comparison would make sense as bjacob pointed out.

@@ +2411,5 @@
> +        rgba = hex.substring(5, hex.length - 1).split(',');
> +        rgba[0] *= 0.00392156863;
> +        rgba[1] *= 0.00392156863;
> +        rgba[2] *= 0.00392156863;
> +        rgba[3] *= 0.00392156863;

In CSS, the alpha component of rgba() is represented in the range 0..1 already, so it probably should be the same here.

::: browser/devtools/tilt/TiltUtils.jsm
@@ +240,5 @@
> +    }
> +    // undesired, you should always pass a defined string for the bundle
> +    if (aName === undefined) {
> +      return "undefined";
> +    }

Shouldn't we just let things fail here (fail fast) instead of displaying nothing?

@@ +310,5 @@
> +    // create the canvas element
> +    var canvas = this.parentNode.ownerDocument.
> +      createElementNS("http://www.w3.org/1999/xhtml", "canvas");
> +
> +    canvas.setAttribute("style", "background: #000;");

Maybe there should be a tilt.css ?

::: browser/devtools/tilt/TiltVisualizer.jsm
@@ +137,5 @@
> +
> +  /**
> +   * Thickness for each mesh stack (element node in the DOM).
> +   */
> +  var thickness = 15;

const THICKNESS_DEFAULT ?

@@ +498,5 @@
> +    meshWireframe = new t.Mesh({
> +      drawMode: t.context.LINES,
> +      vertices: meshStacks.vertices,
> +      indices: new t.IndexBuffer(wireframeIndices),
> +      color: [0, 0, 0, 0.25],

const WIREFRAME_ALPHA_DEFAULT ?

@@ +1054,5 @@
> + * @param {Number} aRadius
> + *                 optional, the radius of the arcball
> + * @param {Array} aInitialTrans
> + *                initial [x, y] translation
> + * @param {Array} aInitialRot

aInitialTrans and aInitialRot should be documented as optional since they are.

@@ +1161,5 @@
> +        deltaKeyRot = this._deltaKeyRot,
> +        deltaKeyTrans = this._deltaKeyTrans;
> +
> +    // the delta should be in the (0..1) interval
> +    aFrameDelta = TiltMath.clamp((aFrameDelta || 30) * 0.01, 0.01, 0.99);

Shouldn't it be clamped 0,1 ?

@@ +1238,5 @@
> +    currentTrans[2] += deltaTrans[2];
> +
> +    // handle additional rotation and translation by the keyboard
> +    if (keyCode[65]) { // w
> +      addKeyRot[0] -= aFrameDelta * 0.2;

0.2 => const ROTATION_STEP ?

@@ +1250,5 @@
> +    if (keyCode[83]) { // d
> +      addKeyRot[1] -= aFrameDelta * 0.2;
> +    }
> +    if (keyCode[37]) { // left
> +      addKeyTrans[0] += aFrameDelta * 50;

50 => const TRANSLATION_STEP ?

@@ +1511,5 @@
> +    gl = TiltGL.create3DContext(canvasgl);
> +    maxSize = gl ? gl.getParameter(gl.MAX_TEXTURE_SIZE) : 0;
> +
> +    // make sure the maximum texture size is valid
> +    if (maxSize < 1) {

Is it ever possible while having a WebGL context? bjacob?

@@ +1556,5 @@
> +  },
> +
> +  /**
> +   * Specifies if Tilt should refresh the visualization mesh.
> +   * Possible values are: 0 (disabled)

consts?

@@ +1577,5 @@
> +/**
> + * Specific color codes for each element in the visualization mesh.
> + */
> +Tilt.Config.Colors = {
> +  "html"     : "#FFF",

Couldn't we declare those in tilt.css and read the rules from the CSS OM (ie. document.stylesheets) ?
This would be more maintainable and open possibilities for theming :)

@@ +1649,5 @@
> +    "void main() {",
> +    "  if (texCoord.x < 0.0) {",
> +    "    gl_FragColor = vec4(color, 1.0);",
> +    "  } else {",
> +    "    gl_FragColor = vec4(texture2D(sampler, texCoord).rgb, 1.0);",

Aren't Tilt's textures always RGBA? Then force opaque here? Couldn't the textures be RGB instead considering the full web page texture is always opaque afaik (via drawWindow).

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

I also have a general concern about Tilt's 3D rotation and handling of mouse input. I think it could be much better if it worked as follows:

* The rotation directions should be independent of where you click and drag the mouse: if I drag vertically I really want a vertical rotation. Currently, it depends, if I drag vertically in the center of the window it does the expected vertical rotation but if I do it in the right of the window it does a rotation around an axis facing the camera. I don't think that such rotations are really useful: I don't see the use case for rotating ("tilting") the view in this way. I think that Tilt only needs 2 axes of rotation, horizontal and vertical.

* If I click on an element and drag the mouse, it probably means that I want to rotate around *that* element, e.g. to see its parents. Tilt is already able to figure what element was clicked, so just compute its center coords and make the rotation axes (horizontal and vertical) pass through it.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Cedric Vivier [cedricv] from comment #11)
> @@ +1649,5 @@
> > +    "void main() {",
> > +    "  if (texCoord.x < 0.0) {",
> > +    "    gl_FragColor = vec4(color, 1.0);",
> > +    "  } else {",
> > +    "    gl_FragColor = vec4(texture2D(sampler, texCoord).rgb, 1.0);",
> 
> Aren't Tilt's textures always RGBA? Then force opaque here? Couldn't the
> textures be RGB instead considering the full web page texture is always
> opaque afaik (via drawWindow).

Also, what is the need for this if() in this fragment shader? It can have a significant performance impact.

Comment 14

[Dave Camp (:dcamp)]

We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.

Comment 15

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

(In reply to Benoit Jacob [:bjacob] from comment #12)
> I also have a general concern about Tilt's 3D rotation and handling of mouse
> input. I think it could be much better if it worked as follows:
> 
> * The rotation directions should be independent of where you click and drag
> the mouse: if I drag vertically I really want a vertical rotation.
> Currently, it depends, if I drag vertically in the center of the window it
> does the expected vertical rotation but if I do it in the right of the
> window it does a rotation around an axis facing the camera. I don't think
> that such rotations are really useful: I don't see the use case for rotating
> ("tilting") the view in this way. I think that Tilt only needs 2 axes of
> rotation, horizontal and vertical.

Tilt's controller is based on an Arcball (Trackball) implementation. What you're describing will fail when:

1. I drag-rotate the visualization on the X axis (vertical drag) for 180 degrees.
2. I drag left. The rotation will be towards the right because of the initial rotation.

This is solved by the use of quaternions and the trackball technique.

> * If I click on an element and drag the mouse, it probably means that I want
> to rotate around *that* element, e.g. to see its parents. Tilt is already
> able to figure what element was clicked, so just compute its center coords
> and make the rotation axes (horizontal and vertical) pass through it.

To do this I can simply move the trackball center of rotation.

Comment 16

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

(In reply to Benoit Jacob [:bjacob] from comment #13)
> (In reply to Cedric Vivier [cedricv] from comment #11)
> > @@ +1649,5 @@
> > > +    "void main() {",
> > > +    "  if (texCoord.x < 0.0) {",
> > > +    "    gl_FragColor = vec4(color, 1.0);",
> > > +    "  } else {",
> > > +    "    gl_FragColor = vec4(texture2D(sampler, texCoord).rgb, 1.0);",
> > 
> > Aren't Tilt's textures always RGBA? Then force opaque here? Couldn't the
> > textures be RGB instead considering the full web page texture is always
> > opaque afaik (via drawWindow).
> 
> Also, what is the need for this if() in this fragment shader? It can have a
> significant performance impact.

It's used to distinguish between drawing the color coded margins on each stack and the textured faces in one pass. A better technique would probably be to always multiply each vertex with a color, regardless if it's on a stack margin or stack face.
The if() remained there as junk from the extension port (where stacks could also be transparent, colored individually, tinted  etc.). I'll remove it and supply colors for each vertex.

Comment 17

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

(In reply to Cedric Vivier [cedricv] from comment #11)
> Comment on attachment 569705 [details] [diff] [review] [diff] [details] [review]
> Wip 2
> 
> Review of attachment 569705 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 

Thanks a lot for the feedback, Cedric! I was saving your time to ask you for a full review once I got the tests finished, but this is really helpful!

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Victor Porof from comment #15)
> Tilt's controller is based on an Arcball (Trackball) implementation. What
> you're describing will fail when:
> 
> 1. I drag-rotate the visualization on the X axis (vertical drag) for 180
> degrees.

Is there any use case for rotating by more than 90 degrees in the context of Tilt?

> 2. I drag left. The rotation will be towards the right because of the
> initial rotation.

Sure, but this becomes a non-issue if we disallow rotating by more than 90 in any direction.

Comment 19

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

(In reply to Benoit Jacob [:bjacob] from comment #18)
> (In reply to Victor Porof from comment #15)
> > Tilt's controller is based on an Arcball (Trackball) implementation. What
> > you're describing will fail when:
> > 
> > 1. I drag-rotate the visualization on the X axis (vertical drag) for 180
> > degrees.
> 
> Is there any use case for rotating by more than 90 degrees in the context of
> Tilt?
> 

Maybe not, maybe yes, you never know how people want to use this stuff :)
I've experimented with a simple XY rotation implementation and it was counter-intuitive for many people who tested (or at least that's what they said). I also believe the trackball is the generally accepted technique for a 3D camera that is suited for rotating (and panning!) a viewed mesh. The XY rotation is way easier to implement, but a trackball is much more ergonomic imho.

Comment 20

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

(In reply to Benoit Jacob [:bjacob] from comment #13)
> (In reply to Cedric Vivier [cedricv] from comment #11)
> > @@ +1649,5 @@
> > > +    "void main() {",
> > > +    "  if (texCoord.x < 0.0) {",
> > > +    "    gl_FragColor = vec4(color, 1.0);",
> > > +    "  } else {",
> > > +    "    gl_FragColor = vec4(texture2D(sampler, texCoord).rgb, 1.0);",
> 
> Also, what is the need for this if() in this fragment shader? It can have a
> significant performance impact.

So, what I'm doing is draw the color coded margins and the textured face of each stack in the mesh using a single draw call (and the mesh is composed of all the stacks).

The way the texCoord buffer looks like is:
... u, v, u, v, u, v, u, v, 0, 0, 0, 0, ..... 0, u, v, u, v, ...
    (coords for face verts) (coords for margins) (next stack)

And the color buffer:
... 1, 1, 1, ..... 1, 1, 1, r, g, b, r, ..... b, 1, 1, 1, 1, ...
    (colors for face verts) (colors for margins) (next stack)

The if() in the shader was used to distinguish between the coords for face verts and the coords for margins. Supposedly I were to simply write:

gl_FragColor = vec4(texture2D(sampler, texCoord).rgb * color, 1.0);

While the stack faces would be colored correctly (because the color is 1, 1, 1 for those vertices), this would not work for the stack margins because the color would be also multiplied with a pixel from the face texture (no matter what the texCoord would be, it will still interfere with the correct margin color).

I see only two ways of removing the if() from the fragment shader:

1. Draw a 1x1 white pixel on the document texture. This way, for the margins vertices, if the texCoord is (0, 0) then color * white would give the right resutls. This may be a bad idea for future because once MOZ_window_region_texture WebGL extension is finished, bug 653656, this won't be the case anymore.

2. Cedric suggested using an array of two samplers, containing the document texture on position 0 and a 1x1 white pixel texture on position 1. This way, we could do sign(length(texcoord)) or texcoord.x+texcoord.y or some other technique to determine the sampler index and get the correct texture color to be multiplied with the stack color.
I've experimented with this, but it seems this will obviously not work because we're in OpenGL ES land and having variable indices for array lookups in a fragment shader is prohibited. Thus,

int i = n; samplers[i] // won't compile, because i is not a fixed index, while
samplers[0] // will work, but it doesn't help our problem.


Am i missing something?

Comment 21

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

Attachment: Wip 3

Addressed feedback from bjacob, dcamp and cedric.
Resolved all issues except the conditional in the visualization fragment shader.

Comment 22

[Cedric Vivier [:cedricv]]

(In reply to Victor Porof from comment #20)
> int i = n; samplers[i] // won't compile, because i is not a fixed index,
> while
> samplers[0] // will work, but it doesn't help our problem.

Good catch. I thought that the no dynamic indexing rule was relaxed for samplers as well as uniforms (tho most implementations do afaik but WebGL only support what is mandated).

How about using texture2dlod then?
Since Tilt is (sensibly enough) not generating mipmaps for content textures, we could use level 1 for the side texture.

Eg:
gl_FragColor = vec4(texture2Dlod(sampler, texCoord, float(texcoord.x > 1.0)).rgb, 1.0);

Comment 23

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

(In reply to Cedric Vivier [cedricv] from comment #22)
> (In reply to Victor Porof from comment #20)
> > int i = n; samplers[i] // won't compile, because i is not a fixed index,
> > while
> > samplers[0] // will work, but it doesn't help our problem.
> 
> Good catch. I thought that the no dynamic indexing rule was relaxed for
> samplers as well as uniforms (tho most implementations do afaik but WebGL
> only support what is mandated).
> 
> How about using texture2dlod then?
> Since Tilt is (sensibly enough) not generating mipmaps for content textures,
> we could use level 1 for the side texture.
> 
> Eg:
> gl_FragColor = vec4(texture2Dlod(sampler, texCoord, float(texcoord.x >
> 1.0)).rgb, 1.0);

Excellent idea!

Comment 24

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

(In reply to Cedric Vivier [cedricv] from comment #22)

One problem though, and I just found this: [0]
"GLSL texture functions that end in "Lod" (eg texture2DLod) are only permitted in the vertex shader." We'll unfortunately need it in the fragment shader.

This is bad for many reasons. For one, doing texture lookups in the vertex shader is awfully implemented in way too many ATI cards (I had to deal with this a lot in the past). Secondly, we'll need interpolated texture coordinates between the vertices to get the correct sampled pixels. The uninterpolated texcoords alone don't have nearly enough detail for a useful texture2DLod.

[0] http://www.khronos.org/webgl/wiki/WebGL_and_OpenGL_Differences#texture2DLod

Comment 25

[Rob Campbell [:rc] (:robcee)]

P2 got nixed during a comment change. Watch your bug flags!

Comment 26

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

Attachment: v1

Working patch. Most of the tests finished, though I'd like to add a few more.

Comment 27

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

Attachment: v1.1

Test additions, typo fixes and other small changes.

Comment 28

[Cedric Vivier [:cedricv]]

Comment on attachment 570967 [details] [diff] [review]
v1.1

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

Cool stuff. r- just because of some files that are not located in the right place afaik.
I've quickly glanced over tests, it seems to me that testing user interaction should be a priority (ie. testing that a mouse click and/or key press does actually changed the scene orientation correctly if at all - eg. you can use EventUtils.synthesizeMouseEvent)

::: browser/devtools/jar.mn
@@ +11,5 @@
> +    content/browser/devtools/tilt.css             (tilt/chrome/tilt.css)
> +    content/browser/devtools/tilt.js              (tilt/chrome/tilt.js)
> +    content/browser/devtools/tilt-en.dtd          (tilt/chrome/tilt-en.dtd)
> +    content/browser/devtools/tilt-en.properties   (tilt/chrome/tilt-en.properties)
> +%   overlay chrome://browser/content/browser.xul  chrome://browser/content/devtools/tilt.xul

It's quite cool to use 'overlay' here :)
However I guess for the browser integration the overlay should be inlined in the various browser/base/content/browser-*.xul files for startup performance reasons.

You probably should check this with robcee or msucan.

::: browser/devtools/tilt/TiltGL.jsm
@@ +55,5 @@
> + *
> + * @param {HTMLCanvasElement} aCanvas
> + *                            the canvas element used for rendering
> + * @param {Function} aFailFunc
> + *                   function called if initialization failed

Optional.

@@ +57,5 @@
> + *                            the canvas element used for rendering
> + * @param {Function} aFailFunc
> + *                   function called if initialization failed
> + * @param {Function} aSuccessFunc
> + *                   function called if initialization worked

Optional.

@@ +147,5 @@
> +  /**
> +   * Clears the color and depth buffers.
> +   */
> +  clear: function TGLR_clear() {
> +    var gl = this.context;

You probably should rather use "let" here and everywhere else you use "var" in Tilt.

@@ +331,5 @@
> +   *
> +   * @param {String} aColor
> +   *                 the string hex/rgb()/rgba() color equivalent
> +   * @param {Number} aMultiplyAlpha
> +   *                 scalar to multiply the alpha element with

Optional.

@@ +361,5 @@
> +   * Sets a default perspective projection, with the near frustum rectangle
> +   * mapped to the canvas width and height bounds.
> +   */
> +  perspective: function TGLR_perspective() {
> +    var fov = 45,

All of our code uses one separate "let" declaration per line. Tilt should probably use the same style for consistency.

@@ +366,5 @@
> +        w = this.width,
> +        h = this.height,
> +        x = w / 2,
> +        y = h / 2,
> +        z = y / Math.tan(TiltMath.radians(45) / 2),

TiltMath.radians(fov) ?

@@ +419,5 @@
> +   *                 the x amount of translation
> +   * @param {Number} y
> +   *                 the y amount of translation
> +   * @param {Number} z
> +   *                 the z amount of translation

Optional.

@@ +503,5 @@
> +  },
> +
> +  /**
> +   * Draws a single triangle.
> +   * Do not abuse this function, it is quite slow, use for debugging only!

And it is unused. Maybe we could remove it?

@@ +602,5 @@
> + *
> + * @param {Object} aRenderer
> + *                 an instance of TiltGL.Renderer used for drawing
> + * @param {Object} aParameters
> + *                 an object containing some of the following properties:

Optional.

@@ +620,5 @@
> +  aParameters = aParameters || {};
> +
> +  // retain each parameter property for easy access
> +  for (var i in aParameters) {
> +    if (aParameters.hasOwnProperty(i) && i !== "draw" && i !== "finalize") {

Why special case those and not eg. drawMode?
Also the typeof function checks below are useless since the property would be already set here anyways.

@@ +757,5 @@
> +   */
> +  finalize: function TGLVB_finalize() {
> +    if (this._ref) {
> +      try {
> +        this._context.deleteBuffer(this._ref);

Did you experience exceptions from deleteBuffer? (spec-wise it should never throw)

@@ +813,5 @@
> +   *
> +   * @param {Array} aElementsArray
> +   *                an array of numbers (unsigned integers)
> +   * @param {Number} aNumItems
> +   *                 how many items to use from the array

Optional.

@@ +840,5 @@
> +   */
> +  finalize: function TGLIB_finalize() {
> +    if (this._ref) {
> +      try {
> +        this._context.deleteBuffer(this._ref);

Did you experience exceptions from deleteBuffer? (spec-wise it should never throw)

@@ +1396,5 @@
> +   */
> +  finalize: function TGLT_finalize() {
> +    if (this._ref !== undefined) {
> +      try {
> +        this._context.deleteTexture(this._ref);

Did you experience exceptions from deleteBuffer? (spec-wise it should never throw)

::: browser/devtools/tilt/TiltMath.jsm
@@ +1792,5 @@
> +
> +    aQuat[0] = -aQuat[0];
> +    aQuat[1] = -aQuat[1];
> +    aQuat[2] = -aQuat[2];
> +    aQuat[3] =  aQuat[3];

We probably don't need this line ;)

@@ +2252,5 @@
> +  clamp: function TM_clamp(aValue, aMin, aMax) {
> +    if (aMin > aMax) {
> +      var save = aMin;
> +      aMin = aMax;
> +      aMax = save;

You could save the temp and enjoy extra coolness with [aMin,aMax] = [aMax,aMin]

::: browser/devtools/tilt/TiltUtils.jsm
@@ +208,5 @@
> +   *                 the initial preference value
> +   *
> +   * @return {Boolean} true if the preference was initialized successfully
> +   */
> +  create: function TUP_create(aPref, aType, aValue) {

Probably can be removed. See comment above about firefox.js

@@ +255,5 @@
> +   *                 the string name in the bundle
> +   *
> +   * @return {String} the equivalent string from the bundle
> +   */
> +  get: function TUSB_get(aName) {

TUSB??

@@ +274,5 @@
> +   *                an array of arguments for the formatted string
> +   *
> +   * @return {String} the equivalent formatted string from the bundle
> +   */
> +  format: function TUSB_format(aName, aArgs) {

TUSB??

::: browser/devtools/tilt/TiltVisualizer.jsm
@@ +291,5 @@
> +    t.clear();
> +    t.perspective();
> +
> +    // apply the preliminary transformations to the model view
> +    t.translate(t.width * 0.5, t.height * 0.5, -STACK_THICKNESS * 30);

Why 30?

@@ +859,5 @@
> +        clickY = e.clientY - e.target.offsetTop;
> +
> +    // a click in Tilt is issued only when the mouse pointer stays in
> +    // relatively the same position
> +    if (Math.abs(this._downX - clickX) < 10 &&

const MOUSE_CLICK_SENSITIVITY/THRESHOLD ?

@@ +1232,5 @@
> +    deltaTrans[2] = (scrollValue - currentTrans[2]) * 0.1;
> +    currentTrans[2] += deltaTrans[2];
> +
> +    // handle additional rotation and translation by the keyboard
> +    if (keyCode[65]) { // w

those key code [numliteral] could use keyCode[KeyEvent.DOM_VK_XXX] instead (and then the comments can be removed).

@@ +1480,5 @@
> +    return this._enabled;
> +  },
> +
> +  set enabled(value) {
> +    TiltUtils.Preferences.set("enabled", "boolean", value);

You should add a default for devtools.tilt.enabled in browser/app/profile/firefox.js

@@ +1496,5 @@
> +    return this._refresh;
> +  },
> +
> +  set refresh(value) {
> +    TiltUtils.Preferences.set("refresh", "integer", value);

Same.

@@ +1514,5 @@
> +   * @param {Function} aCallbackFunc
> +   *                   function called when loading is finished
> +   */
> +  load: function TCS_load(gBrowser, aCallbackFunc) {
> +    // create the preferences if they don't exist yet

When in profile. This could be removed (perhaps the create() function as well).

::: browser/devtools/tilt/chrome/tilt-en.dtd
@@ +1,1 @@
> +<!ENTITY tilt.menu.label     "Tilt">

The localization files should be name tilt.dtd|properties and located in browser/locales/en-US/chrome/browser/devtools

::: browser/devtools/tilt/chrome/tilt.css
@@ +1,1 @@
> +.visualization-html {

This file should be located in browser/themes/(gnomestripe|winstripe|pinstripe|)/browser/devtools/

Comment 29

[Rob Campbell [:rc] (:robcee)]

Good review comments! Thanks for doing that, Cedric.

> +%   overlay chrome://browser/content/browser.xul  chrome://browser/content/devtools/tilt.xul

It's quite cool to use 'overlay' here :)
However I guess for the browser integration the overlay should be inlined in the various browser/base/content/browser-*.xul files for startup performance reasons.

You probably should check this with robcee or msucan.

Yeah, we typically just add directly to browser.xul (and browser-sets.inc, etc) for our devtools features. I think Cedric suggestion of this impacting startup performance is correct. For the sake of 4 top-level entries, the new file probably isn't worth it.

Comment 30

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

Attachment: worker picker

I moved the mesh triangle picker into a Worker. This made the picking about 4x faster, clearly obvious in sites with a huge DOM tree, like Gmail (where the performance increase was from ~3s to <1s).

However, is this something we're allowed to do in devtools (or in general?), or can it be an issue?

Comment 31

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

Attachment: v1.2

Using ChromeWorkers for creating the visualization mesh and node picking.

Comment 32

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

Attachment: v2

Comment 33

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

Attachment: v2.1

Typo fixes, cleanup, let -> var.

Comment 34

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

Attachment: v2.2

Consistent documentation + removed some redundant code

Comment 35

[Cedric Vivier [:cedricv]]

Comment on attachment 573501 [details] [diff] [review]
v2.2

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

Really nice. I didn't find much to comment about, mostly nits ;)

::: browser/base/content/browser-appmenu.inc
@@ +182,4 @@
>                      oncommand="HUDConsoleUI.toggleHUD();"
>                      key="key_webConsole"/>
>            <menuitem id="appmenu_pageInspect"
> +                    type="checkbox"

Nit: Why moving the type attribute? (if not a glitch from splinter)
I kinda like it better like this too BUT from an "atomic commit" perspective it's disturbing ;)

::: browser/devtools/tilt/TiltMath.jsm
@@ +121,5 @@
> +   *                if not specified result is written to the first operand
> +   *
> +   * @return {Array} the destination vec3 if specified, first operand otherwise
> +   */
> +  add: function V3_add(aVec, aVec2, aDest) {

Micro-nit (don't change it if you don't feel it):
From a general API perspective, I think we are all more used to seeing the destination as the first operand and the fact it is indeed going to the first operand when aDest is not specified does not help imho.
It could probably be required and not optional as well, the intent is clearer and would remove conditionals in these (usually [though maybe not in Tilt]) performance-sensitive arithmetic functions (improving throughput and inlining likelihood).

@@ +535,5 @@
> +   *
> +   * @return {Array} the destination mat3 receiving copied values
> +   */
> +  set: function M3_set(aMat, aDest) {
> +    aDest[0] = aMat[0];

aDest.set(aMat);

@@ +692,5 @@
> +   *
> +   * @return {Array} the destination mat4 receiving copied values
> +   */
> +  set: function M4_set(aMat, aDest) {
> +    aDest[0] = aMat[0];

aDest.set(aMat);

@@ +924,5 @@
> +   * @return {Array} the destination mat4 if specified, first operand otherwise
> +   */
> +  toMat3: function M4_toMat3(aMat, aDest) {
> +    if (!aDest) {
> +      aDest = new Float32Array(9);

This does not match documentation, is this intended or the documentation is wrong?

@@ +954,5 @@
> +   * @return {Array} the destination mat4 if specified, first operand otherwise
> +   */
> +  toInverseMat3: function M4_toInverseMat3(aMat, aDest) {
> +    if (!aDest) {
> +      aDest = new Float32Array(9);

Same.

@@ +1634,5 @@
> +   *
> +   * @return {Array} the destination quat4 receiving copied values
> +   */
> +  set: function Q4_set(aQuat, aDest) {
> +    aDest[0] = aQuat[0];

aDest.set(aQuat);

@@ +1998,5 @@
> +   *                an array of elements representing the [x, y, z] axis
> +   * @param {Number} aAngle
> +   *                 the angle of rotation
> +   * @param {Array} aDest
> +   *                optional parameter, the array to write the values to

if not specified, a new Q4 is created.

@@ +2028,5 @@
> +   *                 the pitch angle of rotation
> +   * @param {Number} aRoll
> +   *                 the roll angle of rotation
> +   * @param {Array} aDest
> +   *                optional parameter, the array to write the values to

if not specified, a new Q4 is created.

::: browser/devtools/tilt/TiltUtils.jsm
@@ +153,5 @@
> +      let prefs = Cc["@mozilla.org/preferences-service;1"].
> +        getService(Ci.nsIPrefService).
> +        getBranch(this._branch);
> +
> +      return (aType === "boolean") ? prefs.getBoolPref(aPref) :

switch (aType) ?
Readability + there's no need to evaluate more.

@@ +186,5 @@
> +      let prefs = Cc["@mozilla.org/preferences-service;1"].
> +        getService(Ci.nsIPrefService).
> +        getBranch(this._branch);
> +
> +      return (aType === "boolean") ? prefs.setBoolPref(aPref, aValue) :

switch (aType)

@@ +219,5 @@
> +      let prefs = Cc["@mozilla.org/preferences-service;1"].
> +        getService(Ci.nsIPrefService).
> +        getBranch(this._branch);
> +
> +      return prefs.prefHasUserValue(aPref) ? false :

switch (aType)

::: browser/devtools/tilt/TiltVisualizer.jsm
@@ +486,5 @@
> +  /**
> +   * Sets up event listeners necessary for the presenter.
> +   */
> +  let setupEventListeners = function TVP_setupEventListeners() { /*jshint undef: false */
> +    aContentWindow.addEventListener("resize", onContentWindowResize);

Needs last argument (false).
Though it is optional, omitting it is discouraged in our codebase IIRC (is this recommendation still valid?)

@@ +702,5 @@
> +      TiltUtils.destroyObject(t);
> +      t = null;
> +    }
> +    if (onContentWindowResize) {
> +      aContentWindow.removeEventListener("resize", onContentWindowResize);

", false);". See comment above.

@@ +907,5 @@
> +    this._browserBarFocus = false;
> +  }.bind(this);
> +
> +  // bind commonly used mouse and keyboard events with the controller
> +  aCanvas.addEventListener("mousedown", mouseDown);

Last argument.

@@ +965,5 @@
> +      TiltUtils.destroyObject(update);
> +      update = null;
> +    }
> +    if (mouseDown) {
> +      aCanvas.removeEventListener("mousedown", mouseDown);

Last argument..

::: browser/locales/en-US/chrome/browser/devtools/tilt.properties
@@ +1,1 @@
> +initTilt.error = Could not initialize Tilt, please check the\ntroubleshooting information available at get.webgl.org/troubleshooting.

at http://get.webgl... (so that auto-linking can possibly occur)

::: browser/locales/jar.mn
@@ +19,4 @@
>      locale/browser/devtools/gclicommands.properties   (%chrome/browser/devtools/gclicommands.properties)
>      locale/browser/devtools/webconsole.properties     (%chrome/browser/devtools/webconsole.properties)
>      locale/browser/devtools/inspector.properties      (%chrome/browser/devtools/inspector.properties)
> +    locale/browser/devtools/styleinspector.properties (%chrome/browser/devtools/styleinspector.properties)

Nit: Why moving styleinspector.properties?
(as in first review comment it is disturbing "atomicity" ;P)

Comment 36

[Cedric Vivier [:cedricv]]

One more thing that might improve builtin Tilt :

I think the gfx code does a test at startup to check if the GPU/driver is good enough to enable hardware acceleration (and possibly WebGL).
We might be able to reuse this information in order to disable (with a explanation tooltip) or remove the Tilt menu since we know in advance it won't be able to initialize.

Comment 37

[Benoit Jacob [:bjacob] (mostly away)]

Indeed. The interface to query this information is GfxInfo. Its API is getting redesigned but you can check its current state by looking at the sources of the about:support page, toolkit/content/aboutSupport.js .

Comment 38

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

Since Tilt already has the devtools.tilt.enabled pref, maybe checking the GfxInfo for support should have a devtools.tilt.force-enabled option.

The way I see this is:

1). tilt.enabled = false, then nothing happens and no menu item is shown
2). tilt.enabled = true, then at initialization,
    if GfxInfo says we're ok or tilt.force-enabled = true, continue opening Tilt
3). if the initialization was forced, despite what GfxInfo says, and Tilt cannot start, add a tab to webgl/troubleshooting (like we do now, or a yellow notification on the top with a button taking us to troubleshooting)

Also Tilt is probably going to a highlighter button, like HTML and Style. The behavior can apply the same in that case.

Comment 39

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

(In reply to Benoit Jacob [:bjacob] from comment #37)
> Indeed. The interface to query this information is GfxInfo. Its API is
> getting redesigned but you can check its current state by looking at the
> sources of the about:support page, toolkit/content/aboutSupport.js .

In the aboutSupport.js script, I see that gfxInfo is initialized as:

var gfxInfo = null;
  try {
    // nsIGfxInfo is currently only implemented on Windows
    gfxInfo = Cc["@mozilla.org/gfx/info;1"].getService(Ci.nsIGfxInfo);
  } catch(e) {}

The comment makes me a bit confused. I see that I am able to get webglrenderer (and thus set webglenabled to true), so maybe this is not the case anymore? If this is indeed true, it's probably not a good way to test for WebGL since we can't get consistent results on all OS.

Comment 40

[Benoit Jacob [:bjacob] (mostly away)]

Indeed, nsIGfxInfo is definitely implemented on Windows. That comment is out of date.

You dont want to query webglrenderer as that would mean creating a WebGL context internally, which is slow. Instead, you want to call GetFeatureStatus with FEATURE_WEBGL_OPENGL and FEATURE_WEBGL_ANGLE and see if either works (NO_INFO means not blacklisted).

Comment 41

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

Attachment: v2.3

Using nsIGfxInfo to sniff WebGL capability status.

Comment 42

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

Attachment: v2.4

Addressed cedric's review comments.
I didn't do aDest.set(aSource) for V3/M3/M4/Q4_set functions because the destination may sometimes be an Array, not necessarily a Float32Array.

Comment 43

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

Attachment: v2.5

Initialization speed improvements, especially when handling iframes or document content that isn't rendered.

Comment 44

[Mihai Sucan [:msucan]]

Comment on attachment 575909 [details] [diff] [review]
v2.5

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

Wow, this is going to be awesome.

Too bad on my machine Tilt doesn't work. If I force-enable webgl it barely runs.

General nit: a good number of lines of comments and code do not properly wrap at the 80 chars limit. This is not a very hard limit, but it seems it's not always an isolated case.

Here's my first part of comments. I didn't go through all of the code yet. Will continue tomorrow!

::: browser/app/profile/firefox.js
@@ +1015,5 @@
> +// Enable the Tilt inspector
> +pref("devtools.tilt.enabled", true);
> +
> +// Open the Tilt inspector even if WebGL capabilities are not detected
> +pref("devtools.tilt.force-open", true);

Shouldn't this be false by default?

::: browser/devtools/tilt/Makefile.in
@@ +44,5 @@
> +
> +ifdef ENABLE_TESTS
> +ifneq (mobile,$(MOZ_BUILD_APP))
> +	DIRS += test
> +endif

The ifneq mobile check is not needed. This is not getting built on Fennec anyway.

::: browser/devtools/tilt/Tilt.jsm
@@ +143,5 @@
> +
> +      this.window.QueryInterface(Ci.nsIInterfaceRequestor).
> +        getInterface(Ci.nsIDOMWindowUtils).
> +        garbageCollect();
> +    }, 100);

Just a thought: I am wary of timeouts with arbitrary delays like these. Why is this needed? Also, why is garbage collection needed at the end? This isn't usually what we should do - the browser should handle GC by itself. (not suggesting any changes here, just please explain why you need the timeout and the GC call)

::: browser/devtools/tilt/TiltMath.jsm
@@ +77,5 @@
> +    return dest;
> +  },
> +
> +  /**
> +   * Copies the values of one vec3 to another,

Nit: trailing comma.

@@ +86,5 @@
> +   *                vec3 receiving copied values
> +   *
> +   * @return {Array} the destination vec3 receiving copied values
> +   */
> +  set: function V3_set(aVec, aDest) {

The comment seems wrong. The method returns aVec, the vec3 from where values are copied, it doesn't return aDest (the destination vec3 receiving the copied values).

@@ +221,5 @@
> +    }
> +
> +    let x = aVec[0];
> +    let y = aVec[1];
> +    let z = aVec[2];

Übber-nit: let [x, y, z] = aVec;

Destructuring assignment:
https://developer.mozilla.org/en/New_in_JavaScript_1.7#Destructuring_assignment_(Merge_into_own_page.2Fsection)

(only pointing this out because I see such code in many places throughout Tilt)

@@ +222,5 @@
> +
> +    let x = aVec[0];
> +    let y = aVec[1];
> +    let z = aVec[2];
> +    let len = Math.sqrt(x * x + y * y + z * z);

Why not vec3.length(aVec) ?

@@ +270,5 @@
> +    return aDest;
> +  },
> +
> +  /**
> +   * Caclulates the dot product of two vectors.

s/Caclulates/Calculate/

@@ +284,5 @@
> +    return aVec[0] * aVec2[0] + aVec[1] * aVec2[1] + aVec[2] * aVec2[2];
> +  },
> +
> +  /**
> +   * Caclulates the length of a vec3.

s/Caclulates/Calculate/

@@ +312,5 @@
> +   *                if not specified result is written to the first operand
> +   *
> +   * @return {Array} the destination vec3 if specified, first operand otherwise
> +   */
> +  direction: function V3_direction(aVec, aVec2, aDest) {

It looks to me like direction() can be written as a call to subtract() and one to normalize() - or am I wrong? Why isn't it done as such? Is it for performance concerns?

This also seems to be the case with a good number of methods throughout the file. I can only assume you did this for performance reasons. However, if perf is not a problem in such cases, maybe it would be easier to read and follow the code if it would reuse other parts as much as possible.

@@ +1991,5 @@
> +  },
> +
> +  /**
> +   * Creates a rotation quaternion from axis-angle.
> +   * This function implies that the axis is a normalized vector.

Nit: implies or expects?

@@ +2087,5 @@
> +   *
> +   * @return {Number} the degrees converted to radians
> +   */
> +  radians: function TM_radians(aDegrees) {
> +    return aDegrees * 0.0174532925; // PI / 180

Nit: you could have these magic values calculated once, at the top of the jsm.

(same for other constants like 1/255 or sqrt(255)/255)

::: browser/devtools/tilt/TiltVisualizer.jsm
@@ +74,5 @@
> +let EXPORTED_SYMBOLS = ["TiltVisualizer"];
> +
> +/**
> + * TiltVisualizer constructor.
> + * Initializes the the visualization presenter and controller.

s/the the/the/

@@ +164,5 @@
> +
> +  /**
> +   * Create the renderer, containing useful functions for easy drawing.
> +   */
> +  let t = new TiltGL.Renderer(aCanvas, onError, onLoad, TiltVisualizer.Config);

We usually encourage the use of more descriptive function and variable names.

Reading through the code I got confused and had to recheck what's "t". :)

@@ +361,5 @@
> +   *
> +   * @param {Object} aData
> +   *                 object containing the necessary mesh verts, texcoord etc.
> +   */
> +  let setupMesh = function TVP_setupMesh(aData) {

This method has two big calls to new t.Mesh() each taking quite some heavy-duty objects with functions included. I became confused trying to follow the code.

Would it be possible to simplify the calls to the constructors? Like put the functions you pass to t.Mesh at some upper level in the code and reuse them from where they are defined once (just bind() to whatever object you need).

::: browser/locales/en-US/chrome/browser/devtools/tilt.properties
@@ +1,1 @@
> +initTilt.error = Could not initialize Tilt, please check the\ntroubleshooting information available at http://get.webgl.org/troubleshooting

Please add localization notes for each string explaining where it is expected.

Also, please add a localization note mentioning that the translation of these strings is optional. See the other devtools .properties files - you can copy/paste that note.

Comment 45

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

(In reply to Mihai Sucan [:msucan] from comment #44)
> General nit: a good number of lines of comments and code do not properly
> wrap at the 80 chars limit. This is not a very hard limit, but it seems it's
> not always an isolated case.

Fixed. I generally preferred exceeding 80 by a couple of chars than breaking into new lines because I found it more readable. But it was only a personal style choice :) 

> ::: browser/app/profile/firefox.js
> @@ +1015,5 @@
> > +// Enable the Tilt inspector
> > +pref("devtools.tilt.enabled", true);
> > +
> > +// Open the Tilt inspector even if WebGL capabilities are not detected
> > +pref("devtools.tilt.force-open", true);
> 
> Shouldn't this be false by default?

I thought they're true by default in Nightly and false in Aurora. I can make them false if it's not the case.

> ::: browser/devtools/tilt/Makefile.in
> @@ +44,5 @@
> > +
> > +ifdef ENABLE_TESTS
> > +ifneq (mobile,$(MOZ_BUILD_APP))
> > +	DIRS += test
> > +endif
> 
> The ifneq mobile check is not needed. This is not getting built on Fennec
> anyway.

Removed.

> ::: browser/devtools/tilt/Tilt.jsm
> @@ +143,5 @@
> > +
> > +      this.window.QueryInterface(Ci.nsIInterfaceRequestor).
> > +        getInterface(Ci.nsIDOMWindowUtils).
> > +        garbageCollect();
> > +    }, 100);
> 
> Just a thought: I am wary of timeouts with arbitrary delays like these. Why
> is this needed? Also, why is garbage collection needed at the end? This
> isn't usually what we should do - the browser should handle GC by itself.
> (not suggesting any changes here, just please explain why you need the
> timeout and the GC call)

A call a few lines above this.visualizers[aId].removeOverlay(); immediately removes the canvas overlay which draws the visualization from the selectedBrowser.parentNode.
However, visualizer.cleanup(); makes sure that everything used in Tilt is set to null, by recursively traversing created objects and calling a finalize function. Since Tilt uses quite a lot of small'ish objects (and I can't really think of a way to avoid this because of the WebGL nature), this may take a while. The effect: the canvas overlay isn't immediately removed because all this cleanup needs to be made (it took about 0.5s in worst case, but it may be noticeable on lower hardware).
Thus, I'm setting a small timeout to allow the canvas to be *visually* removed, then continue with the heavy duty cleanup.

This is the same reason I'm forcing a garbageCollect(); there. The memory will eventually be freed, but only after a while (closing/opening a few tabs, waiting a couple of minutes, etc.). Because the visualization needs a canvas rendering of a document, quite large images could be created (~worst case: 8192x8192, which is the WebGL texture limit on most machines and which is HUGE in terms of MB). Opening and closing Tilt repeatedly (and quickly) may let the memory grow considerably fast because it is not freed as fast as it should, giving the illusion of leaks.

> ::: browser/devtools/tilt/TiltMath.jsm
> @@ +77,5 @@
> > +    return dest;
> > +  },
> > +
> > +  /**
> > +   * Copies the values of one vec3 to another,
> 
> Nit: trailing comma.

Fixed.

> @@ +86,5 @@
> > +   *                vec3 receiving copied values
> > +   *
> > +   * @return {Array} the destination vec3 receiving copied values
> > +   */
> > +  set: function V3_set(aVec, aDest) {
> 
> The comment seems wrong. The method returns aVec, the vec3 from where values
> are copied, it doesn't return aDest (the destination vec3 receiving the
> copied values).

Hehe, the comment was actually correct. The returned value was the wrong one. Fixed.

> 
> @@ +222,5 @@
> > +
> > +    let x = aVec[0];
> > +    let y = aVec[1];
> > +    let z = aVec[2];
> > +    let len = Math.sqrt(x * x + y * y + z * z);
> 
> Why not vec3.length(aVec) ?
> 
> @@ +312,5 @@
> > +  direction: function V3_direction(aVec, aVec2, aDest) {
> 
> It looks to me like direction() can be written as a call to subtract() and
> one to normalize() - or am I wrong? Why isn't it done as such? Is it for
> performance concerns?
> 
> This also seems to be the case with a good number of methods throughout the
> file. I can only assume you did this for performance reasons. However, if
> perf is not a problem in such cases, maybe it would be easier to read and
> follow the code if it would reuse other parts as much as possible.
>

The only reason is performance. I prefer writing a couple more lines of code to avoid calling a function which may add overhead. I generally agree with you, reusing stuff is better (obviously), but in the case of very simple and easy math, it's probably better like that (?).

> @@ +270,5 @@
> > +    return aDest;
> > +  },
> > +
> > +  /**
> > +   * Caclulates the dot product of two vectors.
> 
> s/Caclulates/Calculate/
> 
> @@ +284,5 @@
> > +    return aVec[0] * aVec2[0] + aVec[1] * aVec2[1] + aVec[2] * aVec2[2];
> > +  },
> > +
> > +  /**
> > +   * Caclulates the length of a vec3.
> 
> s/Caclulates/Calculate/

Fixed. Also everywhere else in the file.

> @@ +1991,5 @@
> > +  },
> > +
> > +  /**
> > +   * Creates a rotation quaternion from axis-angle.
> > +   * This function implies that the axis is a normalized vector.
> 
> Nit: implies or expects?
> 

Expects. (And implies.) :)

> @@ +2087,5 @@
> > +   *
> > +   * @return {Number} the degrees converted to radians
> > +   */
> > +  radians: function TM_radians(aDegrees) {
> > +    return aDegrees * 0.0174532925; // PI / 180
> 
> Nit: you could have these magic values calculated once, at the top of the
> jsm.
> 
> (same for other constants like 1/255 or sqrt(255)/255)

Fixed.

> ::: browser/devtools/tilt/TiltVisualizer.jsm
> @@ +74,5 @@
> > +let EXPORTED_SYMBOLS = ["TiltVisualizer"];
> > +
> > +/**
> > + * TiltVisualizer constructor.
> > + * Initializes the the visualization presenter and controller.
> 
> s/the the/the/
> 
> @@ +164,5 @@
> > +
> > +  /**
> > +   * Create the renderer, containing useful functions for easy drawing.
> > +   */
> > +  let t = new TiltGL.Renderer(aCanvas, onError, onLoad, TiltVisualizer.Config);
> 
> We usually encourage the use of more descriptive function and variable names.
> 
> Reading through the code I got confused and had to recheck what's "t". :)

Fixed.
I chose a shorter name because it was used extensively throughout the file and it made the code look a bit bloated. See the drawVisualization function for example. But ok, I agree, renamed to "renderer".

> @@ +361,5 @@
> > +   *
> > +   * @param {Object} aData
> > +   *                 object containing the necessary mesh verts, texcoord etc.
> > +   */
> > +  let setupMesh = function TVP_setupMesh(aData) {
> 
> This method has two big calls to new t.Mesh() each taking quite some
> heavy-duty objects with functions included. I became confused trying to
> follow the code.
> 
> Would it be possible to simplify the calls to the constructors? Like put the
> functions you pass to t.Mesh at some upper level in the code and reuse them
> from where they are defined once (just bind() to whatever object you need).

Fixed. Not using mesh anymore, just a regular {}.

> ::: browser/locales/en-US/chrome/browser/devtools/tilt.properties
> @@ +1,1 @@
> > +initTilt.error = Could not initialize Tilt, please check the\ntroubleshooting information available at http://get.webgl.org/troubleshooting
> 
> Please add localization notes for each string explaining where it is
> expected.
> 
> Also, please add a localization note mentioning that the translation of
> these strings is optional. See the other devtools .properties files - you
> can copy/paste that note.

Done.

Comment 46

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

Attachment: v2.6

Addressed msucan's comments. Also a few other minor fixes, typos, etc.

Comment 47

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 575909 [details] [diff] [review]
v2.5

Good stuff!

The only really serious critiques are some long lines (Moz code should be <80 characters for small displays and mxr browsing) and some comment nits.

One setTimeout function in Tilt.jsm should be cleared.

detailed nits below.

browser.xul, browser-sets, browser-appmenu, etc, look good.


nit:
in +++ b/browser/devtools/tilt/Makefile.in

+# Contributor(s):
+#  Victor Porof <[e-mail]> (original author)

indent your V under the 'n'.

+++ b/browser/devtools/tilt/Tilt.jsm

+/*global Components, TiltVisualizer */

I don't think you need this comment.

+ * @param {Window} aWindow
+ *                 the chrome window for which the Tilt instance is created

whither window? ("for which" sounds a little funny, but ...)

+    // use the unique ID of a window object to identify the visualizer instance

this comment line's a little long. Try to keep it under 80 (chars, miles per hour).

in:
+    TiltVisualizer.Config.load(gBrowser, function T_onConfigLoad() {

+        onLoad: function T_onWebGLLoad() {
+          // make sure the visualizer is destroyed on unload
+          gBrowser.selectedBrowser.contentWindow.addEventListener("unload", function T_onContentUnload() {
+            gBrowser.selectedBrowser.contentWindow.removeEventListener("unload", T_onContentUnload);
+            this.destroy(aId);
+          }.bind(this));
+        }.bind(this)

the add/removeEventListener lines are too long. I'd probably format that chunk like:

        gBrowser.selectedBrowser.contentWindow.addEventListener("unload",
          function T_onContentUnload() {
            gBrowser.selectedBrowser.
              contentWindow.
              removeEventListener("unload", T_onContentUnload);
            this.destroy(aId);
          }.bind(this);
        }.bind(this); ...

in destroy:
+    this.window.setTimeout(function T_cleanupTimeout() {
+      visualizer.cleanup();
+
+      this.window.QueryInterface(Ci.nsIInterfaceRequestor).
+        getInterface(Ci.nsIDOMWindowUtils).
+        garbageCollect();
+    }, 100);

You shouldn't use a setTimeout for this. A better way would be to register an event and have your cleanup observer listen for it.


in TiltGL:

comment for useColorShader:

+  /**
+   * Helper function to set active the color shader with required params.

this is worded a little strangely, or I'm misreading. Maybe that should read "Helper function to activate the color shader..." ?

Also, long line in your method declaration. You can gain more space by doing:

  useColorShader:
  function TGLR_useColorShader(aVerticesBuffer, aColor, aMvMatrix, aProjMatrix) {

also, curly braces should be on new lines after function declarations for moz code. One of our more-popular coding style bits!

+  drawIndexedVertices: function TGLR_drawIndexedVertices(aDrawMode, aIndicesBuffer) {

can shorten that line too.

+TiltGL.VertexBuffer = function TGL_VertexBuffer(aContext, aElementsArray, aItemSize, aNumItems) {

long line.

in comment for resizeImageToPowerOfTwo:

+   * Scales an image to power of two width and height.

maybe reword to "Scales an image's width and height to next power of two".

+++ b/browser/devtools/tilt/TiltMath.jsm

+/**
+ * Module containing high performance matrix and vector operations for WebGL.
+ * Inspired by glMatrix, version 0.9.6, (c) 2011 Brandon Jones.
+ */

have you been cleared to use and relicense this code here?

+++ b/browser/devtools/tilt/TiltUtils.jsm

in TiltUtils.Output.log and .error:

+    return true;

why?

TiltUtils.DOM

+  initCanvas: function TUD_initCanvas(aParentNode, aAppendFlag, aWidth, aHeight, aId) {

long line.

getNodeCoordinates' comment:

+   * Returns a node absolute x, y, width and height coordinates, or null
+   * if the passed node is not an ELEMENT_NODE.

maybe "Returns the absolute x, y, width and height coordinates of a node."

+    // compute the iframe position and its offset if the case

eh?

let frameRect is a long line.

(is this why you were asking how performant 10-20 calls to getFrameOffset would be versus 1-2? Calls to getBoundingClientRect can be expensive).

+          parseInt(style.getPropertyValue("padding-top"), 10)

do we really need to specify base 10? Maybe you've been buried in GL for too long. ;)

in traverse:

+        // skip some nodes to avoid too bloated visualization meshes
+        if (!name || aInvisibleElements[name]) {
+          continue;

maybe reword that comment a little, "Skip some nodes to avoid visualization meshes that are too bloated".

+++ b/browser/devtools/tilt/TiltVisualizer.jsm
@@ -0,0 +1,1588 @@

+const STACK_THICKNESS = 15;
+const WIREFRAME_COLOR = [0, 0, 0, 0.25];
+const INITIAL_TRANSITION_DURATION = 100;
+const INITIAL_Z_TRANSLATION = 400;
+
+const MOUSE_CLICK_THRESHOLD = 10;
+const ARCBALL_SENSITIVITY = 0.3;
+const ARCBALL_ROTATION_STEP = 0.2;
+const ARCBALL_TRANSLATION_STEP = 50;
+const ARCBALL_SCROLL_MIN = -3000;
+const ARCBALL_SCROLL_MAX = 500;

wondering if some of these need comments or if any of these will need to be modified at some point. Could user preferences make modifying the responsiveness of the arcball possible? Would that be a good thing?

+++ b/browser/devtools/tilt/TiltWorkerCrafter.js

r+ with nits addressed. Talk to me in IRC.

Comment 48

[Rob Campbell [:rc] (:robcee)]

oh, one other thing,

the Menu item should probably be removed as we're going to surface this as a feature within the highlighter. So, remove the bits in browser-sets, browser-menu and browser-appmenu.

Comment 49

[Rob Campbell [:rc] (:robcee)]

and ... mihai beat me to most of my comments. Good to have double-coverage, I guess. :P

Comment 50

[Mihai Sucan [:msucan]]

Thanks for your updates, Victor! Awesome stuff!


(In reply to Victor Porof from comment #45)
> (In reply to Mihai Sucan [:msucan] from comment #44)
> > General nit: a good number of lines of comments and code do not properly
> > wrap at the 80 chars limit. This is not a very hard limit, but it seems it's
> > not always an isolated case.
> 
> Fixed. I generally preferred exceeding 80 by a couple of chars than breaking
> into new lines because I found it more readable. But it was only a personal
> style choice :) 

If it's only a couple of chars, sure, but what I saw was more than that. ;)


> > ::: browser/app/profile/firefox.js
> > @@ +1015,5 @@
> > > +// Enable the Tilt inspector
> > > +pref("devtools.tilt.enabled", true);
> > > +
> > > +// Open the Tilt inspector even if WebGL capabilities are not detected
> > > +pref("devtools.tilt.force-open", true);
> > 
> > Shouldn't this be false by default?
> 
> I thought they're true by default in Nightly and false in Aurora. I can make
> them false if it's not the case.

I was referring only to devtools.tilt.force-open to be false by default.

> > ::: browser/devtools/tilt/Tilt.jsm
> > @@ +143,5 @@
> > > +
> > > +      this.window.QueryInterface(Ci.nsIInterfaceRequestor).
> > > +        getInterface(Ci.nsIDOMWindowUtils).
> > > +        garbageCollect();
> > > +    }, 100);
> > 
> > Just a thought: I am wary of timeouts with arbitrary delays like these. Why
> > is this needed? Also, why is garbage collection needed at the end? This
> > isn't usually what we should do - the browser should handle GC by itself.
> > (not suggesting any changes here, just please explain why you need the
> > timeout and the GC call)
> 
> A call a few lines above this.visualizers[aId].removeOverlay(); immediately
> removes the canvas overlay which draws the visualization from the
> selectedBrowser.parentNode.
> However, visualizer.cleanup(); makes sure that everything used in Tilt is
> set to null, by recursively traversing created objects and calling a
> finalize function. Since Tilt uses quite a lot of small'ish objects (and I
> can't really think of a way to avoid this because of the WebGL nature), this
> may take a while. The effect: the canvas overlay isn't immediately removed
> because all this cleanup needs to be made (it took about 0.5s in worst case,
> but it may be noticeable on lower hardware).
> Thus, I'm setting a small timeout to allow the canvas to be *visually*
> removed, then continue with the heavy duty cleanup.
> 
> This is the same reason I'm forcing a garbageCollect(); there. The memory
> will eventually be freed, but only after a while (closing/opening a few
> tabs, waiting a couple of minutes, etc.). Because the visualization needs a
> canvas rendering of a document, quite large images could be created (~worst
> case: 8192x8192, which is the WebGL texture limit on most machines and which
> is HUGE in terms of MB). Opening and closing Tilt repeatedly (and quickly)
> may let the memory grow considerably fast because it is not freed as fast as
> it should, giving the illusion of leaks.

Yeah, complex situation here. For one, the timeout is arbitrary - we don't know how much the code will take to execute, so 0.5s is just as good as 0.1s or 1.0s. And memory consumption is still "weird" - I open tilt and after I close it I don't go back to the same amount of memory usage I had before opening tilt. On my machine I still get about 20-30 mb of "left overs" - they don't go away even if I ask for GC, CC and for memory usage minimization (in about:memory). Probably it takes a few minutes for those to clear up?

> > @@ +222,5 @@
> > > +
> > > +    let x = aVec[0];
> > > +    let y = aVec[1];
> > > +    let z = aVec[2];
> > > +    let len = Math.sqrt(x * x + y * y + z * z);
> > 
> > Why not vec3.length(aVec) ?
> > 
> > @@ +312,5 @@
> > > +  direction: function V3_direction(aVec, aVec2, aDest) {
> > 
> > It looks to me like direction() can be written as a call to subtract() and
> > one to normalize() - or am I wrong? Why isn't it done as such? Is it for
> > performance concerns?
> > 
> > This also seems to be the case with a good number of methods throughout the
> > file. I can only assume you did this for performance reasons. However, if
> > perf is not a problem in such cases, maybe it would be easier to read and
> > follow the code if it would reuse other parts as much as possible.
> >
> 
> The only reason is performance. I prefer writing a couple more lines of code
> to avoid calling a function which may add overhead. I generally agree with
> you, reusing stuff is better (obviously), but in the case of very simple and
> easy math, it's probably better like that (?).

Sure, but I see that it kinda "adds up" for a good number of functions. I won't ask for changes here only because of performance concerns.

(albeit, it would be quite interesting to just write the code "as it should be" and let the JIT do its magic. some other day :) )


> > ::: browser/devtools/tilt/TiltVisualizer.jsm
> > @@ +74,5 @@
> > > +let EXPORTED_SYMBOLS = ["TiltVisualizer"];
> > > +
> > > +/**
> > > + * TiltVisualizer constructor.
> > > + * Initializes the the visualization presenter and controller.
> > 
> > s/the the/the/
> > 
> > @@ +164,5 @@
> > > +
> > > +  /**
> > > +   * Create the renderer, containing useful functions for easy drawing.
> > > +   */
> > > +  let t = new TiltGL.Renderer(aCanvas, onError, onLoad, TiltVisualizer.Config);
> > 
> > We usually encourage the use of more descriptive function and variable names.
> > 
> > Reading through the code I got confused and had to recheck what's "t". :)
> 
> Fixed.
> I chose a shorter name because it was used extensively throughout the file
> and it made the code look a bit bloated. See the drawVisualization function
> for example. But ok, I agree, renamed to "renderer".

Thanks! That's less confusing.


> > @@ +361,5 @@
> > > +   *
> > > +   * @param {Object} aData
> > > +   *                 object containing the necessary mesh verts, texcoord etc.
> > > +   */
> > > +  let setupMesh = function TVP_setupMesh(aData) {
> > 
> > This method has two big calls to new t.Mesh() each taking quite some
> > heavy-duty objects with functions included. I became confused trying to
> > follow the code.
> > 
> > Would it be possible to simplify the calls to the constructors? Like put the
> > functions you pass to t.Mesh at some upper level in the code and reuse them
> > from where they are defined once (just bind() to whatever object you need).
> 
> Fixed. Not using mesh anymore, just a regular {}.

Thanks! Will check.

Comment 51

[Mihai Sucan [:msucan]]

Comment on attachment 576729 [details] [diff] [review]
v2.6

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

Thanks for your updates Victor! Cool stuff!

I do have a number of comments below - completed going through TiltVisualizer.jsm. I still have to go through the worker scripts and through the TiltGL.jsm.

Please make sure you fix the keyboard event handling because that is prone to problems. Thank you!

(more comments will come!)

::: browser/devtools/tilt/TiltVisualizer.jsm
@@ +381,5 @@
> +  /**
> +   * Draws a highlighted quad around a currently selected node.
> +   */
> +  let drawHighlight = function TVP_drawHighlight() {
> +    // set the the corresponding state to draw the highlight quad

s/the the/the/

@@ +580,5 @@
> +      return;
> +    }
> +
> +    let worker = new ChromeWorker(
> +      "resource:///modules/devtools/TiltWorkerPicker.js");

What is the lifetime of the ChromeWorker? (I haven't checked the worker code yet).

@@ +848,5 @@
> +    arcball.mouseMove(moveX, moveY);
> +  };
> +
> +  /**
> +   * Called when the the mouse leaves the visualization bounds.

s/the the/the/

@@ +921,5 @@
> +  /**
> +   * Called when the url or search bar are focused.
> +   */
> +  let onBrowserBarFocus = function TVC_onBrowserBarFocus() {
> +    this._browserBarFocus = true;

This was confusing for me when I first saw this.

I believe this code shouldn't need to track if other parts of the UI are focused, because there's always some other field an extension can add.

Maybe you can make the canvas element focusable (add tabindex=1 or something like that)? Then you could change the event listeners to not listen for the whole documentElement, instead you can put the listeners on the canvas element itself.

Thoughts?

@@ +955,5 @@
> +  }
> +  if (searchbar) {
> +    searchbar.addEventListener("focus", onBrowserBarFocus, false);
> +    searchbar.addEventListener("blur", onBrowserBarBlur, false);
> +  }

This is prone to errors.

@@ +1212,5 @@
> +    deltaTrans[2] = (scrollValue - currentTrans[2]) * 0.1;
> +    currentTrans[2] += deltaTrans[2];
> +
> +    // handle additional rotation and translation by the keyboard
> +    if (keyCode[65]) { // w

Please use the Ci.nsIDOMKeyboardEvent.DOM_VK_* constants for the key codes.

@@ +1320,5 @@
> +  };
> +
> +  /**
> +   * Function handling the mouseOver event.
> +   * Call this when the mouse enteres the context bounds.

s/enteres/enters/

@@ +1350,5 @@
> +  };
> +
> +  /**
> +   * Function handling the keyDown event.
> +   * Call this when the a key was pressed.

s/the a/a/

@@ +1361,5 @@
> +  };
> +
> +  /**
> +   * Function handling the keyUp event.
> +   * Call this when the a key was released.

s/the a/a/

@@ +1536,5 @@
> +          let key = rule.selectorText.replace(/^\.visualization-/, "");
> +          let value = rule.style.getPropertyValue("color");
> +
> +          // save the property as a key-value element in the style object
> +          this.Style[key] = TiltMath.hex2rgba(value);

Wow, this is clever and übber-hacky. Do we really want it this way? Why not a JSON?

(not asking for a change here, I am just wondering what was the reasoning... because this has a performance impact on the Tilt initialization with seemingly little benefit?)

@@ +1540,5 @@
> +          this.Style[key] = TiltMath.hex2rgba(value);
> +        }
> +      }
> +
> +      // remove the temporary frame once the the stylesheet settings are loaded

s/the the/the/

@@ +1549,5 @@
> +        aLoadedFunc();
> +      }
> +    }.bind(this), true);
> +
> +    iframe.setAttribute("type", "chrome");

Doesn't type=content work here?

@@ +1552,5 @@
> +
> +    iframe.setAttribute("type", "chrome");
> +    iframe.setAttribute("style", "display: none;");
> +    iframe.setAttribute("src", "data:text/html," +
> +      "<html>" +

Probably you want <!DOCTYPE html> here (to trigger standards mode).

@@ +1555,5 @@
> +    iframe.setAttribute("src", "data:text/html," +
> +      "<html>" +
> +        "<head>" +
> +          "<link rel='stylesheet' type='text/css'" +
> +            "href='chrome://browser/skin/devtools/tilt.css'>" +

Nit: maybe put the href to the CSS at the top of the JSM, in a constant. It would be clearer that the JSM is tied to that specific CSS.

(I had to dig through the code to find this out).

Comment 52

[Mihai Sucan [:msucan]]

Comment on attachment 576729 [details] [diff] [review]
v2.6

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

Final part of my feedback.

General nit: I see some objects (like TiltVisualizer.Presenter, Controller and Arcball ) are built in a programming style that doesn't use .prototype inheritance much. The constructor holds "private" functions and variables and it exposes properties and methods by doing this.foo = bar. This is a style that uses more memory - for example each function is held in memory for each separate instance of the object (if I am not mistaken, please correct me if I am wrong).

You can't change this now, but it should be noted for future reference.

Overall, this is really, really great work - both for users who'll be like "wow" when they'll see Tilt, and for devs who'll want to do further work on Tilt. Thanks a lot for your work and for the patch updates.

More comments below.

::: browser/devtools/tilt/TiltGL.jsm
@@ +53,5 @@
> + */
> +let TiltGL = {};
> +
> +/**
> + * Tilt.Renderer constructor.

Nit: foo constructor in the jsdoc comments seems redundant.

(I see this for many objects.)

@@ +83,5 @@
> +  // check if the context was created successfully
> +  if (this.context) {
> +    // if successful, run a success callback function if available
> +    if ("function" === typeof onLoad) {
> +      onLoad();

Shouldn't this be called at the end of the TiltGL construction? I didn't expect it to be called so early.

(no need to change, just asking)

@@ +162,5 @@
> +
> +  /**
> +   * Clears the color and depth buffers.
> +   */
> +  clear: function TGLR_clear() {

nit: in such cases the Mozilla coding style suggests that a new line is added before the opening curly brace.

@@ +605,5 @@
> + */
> +TiltGL.VertexBuffer =
> +  function TGL_VertexBuffer(aContext, aElementsArray, aItemSize, aNumItems) {
> +
> +  /**

Nit: weird indentation - "function" is at the same level as the body of the function. Suggested indentation:

TiltGL.VertexBuffer = function TGL_VertexBuffer(aContext, aElementsArray,
                                                aItemSize, aNumItems) {

@@ +826,5 @@
> +    this._ref.uniforms = null;
> +
> +    delete this._ref.id;
> +    delete this._ref.attributes;
> +    delete this._ref.uniforms;

You do both this._ref.foo = null and delete this._ref.foo. The former is not needed.

@@ +1161,5 @@
> + * @param {Object} aContext
> + *                 a WebGL context
> + * @param {Object} aProperties
> + *                 optional, an object containing the following properties:
> + *         {Image} source: the source image for the texture

Nit: {Image} type might be confusing. From the comment above I understand it can be an <img> element (nsIDOMHTMLImageElement) or a string (image URL). Or am I confused here?

@@ +1261,5 @@
> +    this._ref.width = null;
> +    this._ref.height = null;
> +    this.onload = null;
> +
> +    delete this._ref.id;

Again = null then delete. Is this needed?

@@ +1286,5 @@
> +   *      {Function} onload: function function called when texture has loaded
> +   */
> +  initTextureAt: function TGLT_initTextureAt(aProperties) {
> +    // load the image from the source in an object
> +    let image = new Image();

Did you test this code path since you moved the code into a JSM?

Is the Image constructor available in the JSM? I doubt, but I might be wrong. Please test and let me know.

@@ +1511,5 @@
> +   *                the image to be scaled
> +   * @param {Object} aProperties
> +   *                 an object containing the following properties:
> +    *        {Image} source: the source image to resize
> +   *       {Boolean} resize: true to resize the image if it has npot dimensions

Broken comment alignment.

@@ +1519,5 @@
> +   *
> +   * @return {Image} the resized image
> +   */
> +  resizeImageToPowerOfTwo:
> +    function TGLTU_resizeImageToPowerOfTwo(aProperties) {

nit: please align f with r.

@@ +1643,5 @@
> + */
> +TiltGL.create3DContext = function TGL_create3DContext(aCanvas, aFlags) {
> +  TiltGL.clearCache();
> +
> +  let names = ["experimental-webgl", "webgl"], context;

Why does this code try the experimental-webgl context first? Given we'll land Tilt inside Firefox and we want this to become a tool that anyone has access to, I would expect it works with a non-experimental webgl implementation - a stable one.

::: browser/devtools/tilt/TiltWorkerCrafter.js
@@ +38,5 @@
> + ***** END LICENSE BLOCK *****/
> +"use strict";
> +
> +/*global self*/
> +self.onmessage = function TWC_onMessage(event) {

A comment about the purpose of this script would also be welcome.

@@ +216,5 @@
> +   * Johannes Baagoe <[e-mail]>, 2010
> +   *
> +   * Seeds a random generator function with a set of passed arguments.
> +   */
> +  seed: function RNG_seed() {

Is this code copied from the link provided? Licensing concerns have been dealt with?

::: browser/devtools/tilt/TiltWorkerPicker.js
@@ +38,5 @@
> + ***** END LICENSE BLOCK *****/
> +"use strict";
> +
> +/*global self*/
> +self.onmessage = function TWP_onMessage(event) {

Perhaps a description in a comment would be welcome, about the purpose of this file.

@@ +118,5 @@
> +    return new Float32Array(3);
> +  }
> +
> +  // performs a vector addition
> +  function add(aVec, aVec2, aDest) {

Given this is a ChromeWorker can't you do Cu.import() the TiltMath.jsm?

That JSM could be reused here, and it doesn't touch the DOM - it should be safe.

@@ +156,5 @@
> +    aDest[2] = x * y2 - y * x2;
> +    return aDest;
> +  }
> +
> +  // caclulates the dot product of two vectors

s/caclulates/calculates/

Comment 53

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

(In reply to Mihai Sucan [:msucan] from comment #52)
> Comment on attachment 576729 [details] [diff] [review] [diff] [details] [review]
> v2.6
> 
> Review of attachment 576729 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Final part of my feedback.
> 
> General nit: I see some objects (like TiltVisualizer.Presenter, Controller
> and Arcball ) are built in a programming style that doesn't use .prototype
> inheritance much. The constructor holds "private" functions and variables
> and it exposes properties and methods by doing this.foo = bar. This is a
> style that uses more memory - for example each function is held in memory
> for each separate instance of the object (if I am not mistaken, please
> correct me if I am wrong).
> 
> You can't change this now, but it should be noted for future reference.
>

You are correct. The reason is that, as a extension, Tilt used only one instance of all these at a time (because it was made to only work on one tab at a time), so the downsides of these 3 closures weren't so painful (plus, it required a lot of this.that which made things look weird). This is not the case anymore, hence the legacy code style :)
I can surely change this now so that it fits the general coding style used throughout Tilt and the devtools modules.

> 
> ::: browser/devtools/tilt/TiltGL.jsm
> @@ +53,5 @@
> > + */
> > +let TiltGL = {};
> > +
> > +/**
> > + * Tilt.Renderer constructor.
> 
> Nit: foo constructor in the jsdoc comments seems redundant.
> (I see this for many objects.)

It should be for all. I made a habit of specifying what function is a constructor in JavaScript (to avoid confusion). It's probably not the case anymore, because people here know what js constructor functions are :)

> @@ +83,5 @@
> > +  // check if the context was created successfully
> > +  if (this.context) {
> > +    // if successful, run a success callback function if available
> > +    if ("function" === typeof onLoad) {
> > +      onLoad();
> 
> Shouldn't this be called at the end of the TiltGL construction? I didn't
> expect it to be called so early.
> 
> (no need to change, just asking)

Hmm, onLoad is just called to tell us that WebGL initialization worked. I should probably rename this callback to something better suited than onLoad.

> @@ +162,5 @@
> > +
> > +  /**
> > +   * Clears the color and depth buffers.
> > +   */
> > +  clear: function TGLR_clear() {
> 
> nit: in such cases the Mozilla coding style suggests that a new line is
> added before the opening curly brace.
>

Agreed.

> @@ +826,5 @@
> > +    this._ref.uniforms = null;
> > +
> > +    delete this._ref.id;
> > +    delete this._ref.attributes;
> > +    delete this._ref.uniforms;
> 
> You do both this._ref.foo = null and delete this._ref.foo. The former is not
> needed.
> 

Delete is an interesting thing in JavaScript. As far as I know, it only removes the way of accessing a property from an object. Implicitly, if not referenced somewhere else, that object will eventually garbage collected. But by setting the value to null first, I'm giving a helping hand to the garbage collector. Is this correct? If it's not needed, I'll remove it.

> @@ +1161,5 @@
> > + * @param {Object} aContext
> > + *                 a WebGL context
> > + * @param {Object} aProperties
> > + *                 optional, an object containing the following properties:
> > + *         {Image} source: the source image for the texture
> 
> Nit: {Image} type might be confusing. From the comment above I understand it
> can be an <img> element (nsIDOMHTMLImageElement) or a string (image URL). Or
> am I confused here?
> 

Yes, it's an image or string. Forgot to update doc :)

> @@ +1286,5 @@
> > +   *      {Function} onload: function function called when texture has loaded
> > +   */
> > +  initTextureAt: function TGLT_initTextureAt(aProperties) {
> > +    // load the image from the source in an object
> > +    let image = new Image();
> 
> Did you test this code path since you moved the code into a JSM?
> 
> Is the Image constructor available in the JSM? I doubt, but I might be
> wrong. Please test and let me know.
> 

I guess this is not needed anymore, for now. One thing I had in mind was using an external image resource as a background (gradient or something), like in the addon, in which case the initTextureAt was necessary. I could remove it now and add it once I'll start working on a UI related followup bug.

> 
> Why does this code try the experimental-webgl context first? Given we'll
> land Tilt inside Firefox and we want this to become a tool that anyone has
> access to, I would expect it works with a non-experimental webgl
> implementation - a stable one.
> 

I may be coming from a different era, but I think only "experimental-webgl" is supported for now. Doing a canvas.getContext("webgl") returns null. I would assume once "webgl" is supported, it will be a synonim to "experimental-webgl". I'm not really sure about this though. Is it a good idea to reverse the order to [webgl, experimental-webgl]? This means that for a certain amount of time (years?), on Tilt initialization, we'll have an extra getContext() called returning null.

> ::: browser/devtools/tilt/TiltWorkerCrafter.js
> @@ +38,5 @@
> > + ***** END LICENSE BLOCK *****/
> > +"use strict";
> > +
> > +/*global self*/
> > +self.onmessage = function TWC_onMessage(event) {
> 
> A comment about the purpose of this script would also be welcome.
> 
> @@ +216,5 @@
> > +   * Johannes Baagoe <[e-mail]>, 2010
> > +   *
> > +   * Seeds a random generator function with a set of passed arguments.
> > +   */
> > +  seed: function RNG_seed() {
> 
> Is this code copied from the link provided? Licensing concerns have been
> dealt with?
> 

As in the case of this and the glMatrix functions, the code isn't copied, but relatively rewritten. Hence the "inspired by" specification, but preserving the original author names. As far as I remember everything was BSD License, but I'll send them an email asking about this.
> 
> @@ +118,5 @@
> > +    return new Float32Array(3);
> > +  }
> > +
> > +  // performs a vector addition
> > +  function add(aVec, aVec2, aDest) {
> 
> Given this is a ChromeWorker can't you do Cu.import() the TiltMath.jsm?
> 
> That JSM could be reused here, and it doesn't touch the DOM - it should be
> safe.
> 

I don't think you can do a Cu.import(), not even in a ChromeWorker.
Am I mistaken?

Comment 54

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

Attachment: screenshot

I know this is not much to look at, but do you have any ideas/suggestions on how this could/should look more like the general Inspector theming? Currently the only themable bit is the fact that each stack representing a node has color coded edges for types (cyan for divs, green for spans, etc.), which is reflected in the highlight quad color. Should we add a background (gradient?) or make other changes?

Comment 55

[Mihai Sucan [:msucan]]

(In reply to Victor Porof from comment #53)
> (In reply to Mihai Sucan [:msucan] from comment #52)
> > Comment on attachment 576729 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > v2.6
> > 
> > Review of attachment 576729 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]:
> > -----------------------------------------------------------------
> > 
> > Final part of my feedback.
> > 
> > General nit: I see some objects (like TiltVisualizer.Presenter, Controller
> > and Arcball ) are built in a programming style that doesn't use .prototype
> > inheritance much. The constructor holds "private" functions and variables
> > and it exposes properties and methods by doing this.foo = bar. This is a
> > style that uses more memory - for example each function is held in memory
> > for each separate instance of the object (if I am not mistaken, please
> > correct me if I am wrong).
> > 
> > You can't change this now, but it should be noted for future reference.
> >
> 
> You are correct. The reason is that, as a extension, Tilt used only one
> instance of all these at a time (because it was made to only work on one tab
> at a time), so the downsides of these 3 closures weren't so painful (plus,
> it required a lot of this.that which made things look weird). This is not
> the case anymore, hence the legacy code style :)
> I can surely change this now so that it fits the general coding style used
> throughout Tilt and the devtools modules.

Thanks! I was thinking that this is a quite invasive/involved change - hence I am not asking for you to do it now, unless you want to.

> > ::: browser/devtools/tilt/TiltGL.jsm
> > @@ +53,5 @@
> > > + */
> > > +let TiltGL = {};
> > > +
> > > +/**
> > > + * Tilt.Renderer constructor.
> > 
> > Nit: foo constructor in the jsdoc comments seems redundant.
> > (I see this for many objects.)
> 
> It should be for all. I made a habit of specifying what function is a
> constructor in JavaScript (to avoid confusion). It's probably not the case
> anymore, because people here know what js constructor functions are :)

You can add a @constructor to mark the function as a constructor.


> > @@ +826,5 @@
> > > +    this._ref.uniforms = null;
> > > +
> > > +    delete this._ref.id;
> > > +    delete this._ref.attributes;
> > > +    delete this._ref.uniforms;
> > 
> > You do both this._ref.foo = null and delete this._ref.foo. The former is not
> > needed.
> > 
> 
> Delete is an interesting thing in JavaScript. As far as I know, it only
> removes the way of accessing a property from an object. Implicitly, if not
> referenced somewhere else, that object will eventually garbage collected.
> But by setting the value to null first, I'm giving a helping hand to the
> garbage collector. Is this correct? If it's not needed, I'll remove it.

Well, as you known given an object A you can have multiple references. Say in foo1.bar = A and foo2.bar = A. Once you remove all references to A the object is garbage collected. The reference is removed either by setting foo1.bar and foo2.bar to null/other value, or by doing delete.

The only difference is that by doing delete you remove the |bar| property from |foo|, while by doing foo.bar = null you only change the value of the property. It's your choice which one you do, but doing foo.bar = null then delete foo.bar is redundant for the purpose of removing references to the objects you want.

Afaik the thing with foo = null and GC is something that only applied to some specific JS engines and versions. It is not a pattern we need to follow here.

> > Why does this code try the experimental-webgl context first? Given we'll
> > land Tilt inside Firefox and we want this to become a tool that anyone has
> > access to, I would expect it works with a non-experimental webgl
> > implementation - a stable one.
> > 
> 
> I may be coming from a different era, but I think only "experimental-webgl"
> is supported for now. Doing a canvas.getContext("webgl") returns null. I
> would assume once "webgl" is supported, it will be a synonim to
> "experimental-webgl". I'm not really sure about this though. Is it a good
> idea to reverse the order to [webgl, experimental-webgl]? This means that
> for a certain amount of time (years?), on Tilt initialization, we'll have an
> extra getContext() called returning null.

Probably it would be safer to just do getContext("experimental-webgl") then, and let it fail when things change. The script expects that the webgl context will be available, but we don't know yet with what kind of changes, so some assumptions in the code will probably be wrong by the time the stable webgl API is done.

I am not sure what the usual policy on these matters...

(I'm from a different era here, having never really played with webgl :) )

> > @@ +216,5 @@
> > > +   * Johannes Baagoe <[e-mail]>, 2010
> > > +   *
> > > +   * Seeds a random generator function with a set of passed arguments.
> > > +   */
> > > +  seed: function RNG_seed() {
> > 
> > Is this code copied from the link provided? Licensing concerns have been
> > dealt with?
> > 
> 
> As in the case of this and the glMatrix functions, the code isn't copied,
> but relatively rewritten. Hence the "inspired by" specification, but
> preserving the original author names. As far as I remember everything was
> BSD License, but I'll send them an email asking about this.

Thanks!


> > @@ +118,5 @@
> > > +    return new Float32Array(3);
> > > +  }
> > > +
> > > +  // performs a vector addition
> > > +  function add(aVec, aVec2, aDest) {
> > 
> > Given this is a ChromeWorker can't you do Cu.import() the TiltMath.jsm?
> > 
> > That JSM could be reused here, and it doesn't touch the DOM - it should be
> > safe.
> 
> I don't think you can do a Cu.import(), not even in a ChromeWorker.
> Am I mistaken?

Just checked the docs. No mention of such capabilities. That's a bummer...

Comment 56

[Dão Gottwald [:dao]]

(In reply to Rob Campbell [:rc] (robcee) from comment #47)
> +        onLoad: function T_onWebGLLoad() {
> +          // make sure the visualizer is destroyed on unload
> +          gBrowser.selectedBrowser.contentWindow.addEventListener("unload",
> function T_onContentUnload() {
> +           
> gBrowser.selectedBrowser.contentWindow.removeEventListener("unload",
> T_onContentUnload);
> +            this.destroy(aId);
> +          }.bind(this));
> +        }.bind(this)
> 
> the add/removeEventListener lines are too long. I'd probably format that
> chunk like:
> 
>         gBrowser.selectedBrowser.contentWindow.addEventListener("unload",
>           function T_onContentUnload() {
>             gBrowser.selectedBrowser.
>               contentWindow.
>               removeEventListener("unload", T_onContentUnload);

This way:

gBrowser.selectedBrowser
        .contentWindow
        .removeEventListener("unload", T_onContentUnload);

>             this.destroy(aId);
>           }.bind(this);
>         }.bind(this); ...

Note that the removeEventListener call is broken. T_onContentUnload and T_onContentUnload(){...}.bind(this) aren't the same.

Comment 57

[Dão Gottwald [:dao]]

Comment on attachment 576729 [details] [diff] [review]
v2.6

>--- a/browser/themes/winstripe/jar.mn
>+++ b/browser/themes/winstripe/jar.mn
>@@ -110,6 +110,7 @@
>         skin/classic/browser/devtools/goto-mdn.png                  (devtools/goto-mdn.png)
>         skin/classic/browser/devtools/csshtmltree.css               (devtools/csshtmltree.css)
>         skin/classic/browser/devtools/gcli.css                      (devtools/gcli.css)
>+        skin/classic/browser/devtools/tilt.css                      (devtools/tilt.css)
>         skin/classic/browser/devtools/toolbarbutton-close.png       (devtools/toolbarbutton-close.png)
>         skin/classic/browser/devtools/breadcrumbs/ltr-end-pressed.png              (devtools/breadcrumbs/ltr-end-pressed.png)
>         skin/classic/browser/devtools/breadcrumbs/ltr-end-selected-pressed.png     (devtools/breadcrumbs/ltr-end-selected-pressed.png)

You missed the aero section.

Comment 58

[Rob Campbell [:rc] (:robcee)]

(In reply to Dão Gottwald [:dao] from comment #56)
> >             this.destroy(aId);
> >           }.bind(this);
> >         }.bind(this); ...
> 
> Note that the removeEventListener call is broken. T_onContentUnload and
> T_onContentUnload(){...}.bind(this) aren't the same.

good catch.

Comment 59

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

(In reply to Rob Campbell [:rc] (robcee) from comment #58)
> (In reply to Dão Gottwald [:dao] from comment #56)
> > >             this.destroy(aId);
> > >           }.bind(this);
> > >         }.bind(this); ...
> > 
> > Note that the removeEventListener call is broken. T_onContentUnload and
> > T_onContentUnload(){...}.bind(this) aren't the same.
> 
> good catch.

Beautiful catch!

Question: Could we possibly use a tilt.js instead of a tilt.css? Initially, all the properties in that css file were represented as a JSON (name: color). Currently, as there's no connection between css and WebGL, that stylesheet is more or less redundant, because we have to create an iframe at initialization, load the css, traverse the object model and parse the properties out, to be translated into stuff usable in WebGL.
This was done in order to avoid hard coding color values in js and allow theming, but it hurts initialization because all the extra iframe creation, loading, parsing. A JSON representation would be much more helpful.
Mihai also thinks that a js file is a much better idea.

Any thoughts?

Comment 60

[Rob Campbell [:rc] (:robcee)]

oh, yes. That does seem like a good idea. Parsing a CSS file manually and applying those styles through JS seems a little over-engineered. Also, not sure there's a huge benefit to making the rendering "stylable" if we get it right. If themeing becomes a thing we want to do, we can always put the JSON blob somewhere visible or allow an override with a tiltUserStyle.js file.

Comment 61

[Mihai Sucan [:msucan]]

Victor: on my system the Tilt tests fail because WebGL is not enabled (blacklisted video card/driver combo). Can you please make the tests not fail when WebGL is not available? Add a warning like "this test was skipped - WebGL is required".

Currently when I run all the tests from browser/devtools I get numerous failures. This should be fixed before Tilt lands.

Thank you!

Comment 62

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

(In reply to Rob Campbell [:rc] (robcee) from comment #60)
> oh, yes. That does seem like a good idea. Parsing a CSS file manually and
> applying those styles through JS seems a little over-engineered. Also, not
> sure there's a huge benefit to making the rendering "stylable" if we get it
> right. If themeing becomes a thing we want to do, we can always put the JSON
> blob somewhere visible or allow an override with a tiltUserStyle.js file.

Does that mean a *style* settings js file should go in browser/themes too?

Comment 63

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

(In reply to Mihai Sucan [:msucan] from comment #61)
> Victor: on my system the Tilt tests fail because WebGL is not enabled
> (blacklisted video card/driver combo). Can you please make the tests not
> fail when WebGL is not available? Add a warning like "this test was skipped
> - WebGL is required".
> 
> Currently when I run all the tests from browser/devtools I get numerous
> failures. This should be fixed before Tilt lands.
> 
> Thank you!

Hmm, this shouldn't happen, because I'm testing for WebGL capabilities first. Thanks for noticing it!
I guess that marking WebGL disabled in about:config doesn't handle all cases. I'll look more into it.

Comment 66

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

Attachment: v3.0

Comment 67

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

Attachment: v3.1

Fixed a few typos in tests and added a couple of new ones handling initialization, tab switches and notification handling.

Comment 68

[Mihai Sucan [:msucan]]

Comment on attachment 577279 [details] [diff] [review]
v3.1

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

Thanks for your updates Victor!

Giving r+ even if the tests in this patch still fail. Over IRC we found the fix and that will be included in the next patch.

Good work!

::: browser/base/content/browser.js
@@ +1699,5 @@
> +  // Enable Tilt?
> +  let tEnabled = gPrefService.getBoolPref("devtools.tilt.enabled");
> +  let tForceEnabled = gPrefService.getBoolPref("devtools.tilt.force-enabled");
> +  if (tEnabled) {
> +    if (tForceEnabled || Tilt.isWebGLSupported()) {

IsWebGLSupported() is something we shouldn't call on browser startup. That function is probably not as lightweight as reading a pref.

Please ping Rob if this is acceptable to be called on browser startup. I would suggest this gets called only when the Inspector is first opened.

::: browser/devtools/tilt/TiltWorkerCrafter.js
@@ +46,5 @@
> +/**
> + * This worker constructs the arrays representing a vertices, texture coords,
> + * colors, indices etc. necessary for a DOM visualization mesh. Given a set of
> + * properties and initialization data (thickness, sizes, and nodes information)
> + * it posts back the created arrays.

/**
 * Given the initialization data (thickness, sizes and information about
 * each DOM node) this worker sends back the arrays representing
 * vertices, texture coords, colors, indices and all the needed data for
 * rendering the DOM visualization mesh.

(hopefully clearer rewording)

Comment 69

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

(In reply to Mihai Sucan [:msucan] from comment #68)
> 
> ::: browser/base/content/browser.js
> @@ +1699,5 @@
> > +  // Enable Tilt?
> > +  let tEnabled = gPrefService.getBoolPref("devtools.tilt.enabled");
> > +  let tForceEnabled = gPrefService.getBoolPref("devtools.tilt.force-enabled");
> > +  if (tEnabled) {
> > +    if (tForceEnabled || Tilt.isWebGLSupported()) {
> 
> IsWebGLSupported() is something we shouldn't call on browser startup. That
> function is probably not as lightweight as reading a pref.

Yeah, that shouldn't be done there. I'll add a call in inspector.jsm in openInspectorUI to handle this.

Comment 70

[Benoit Jacob [:bjacob] (mostly away)]

Indeed, querying GfxInfo is more expensive than just querying a pref, so might be worth avoiding doing on browser startup.

Comment 71

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

Attachment: v3.3

Depends on 705731 to make sense and on 706734 for tests to run properly.

Comment 75

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

Attachment: v4

No longer depends on bug 706734.
Also fixed bug 703907, bug 703908 and bug 703909 in here to centralize things since this hasn't landed yet.

Comment 76

[Mihai Sucan [:msucan]]

Comment on attachment 579654 [details] [diff] [review]
v4

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

Thanks for your updates Victor! I like the Inspector integration - really good stuff.

I was about to sign off with an r+ for the patch, but when I ran all the tests, I got test failures. See:

http://img.i7m.de/show/rpuwq.png

mochitest-browser-chrome failed:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_618311_close_panels.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_618311_close_panels.js | correct number of popups shown - Got 1, expected 2
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_618311_private_browsing.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_618311_private_browsing.js | correct number of popups shown - Got 1, expected 2
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_618311_private_browsing.js | Found a tab after previous test timed out: about:blank

The browser window is in a weird state, for some reason. These are Web Console tests that break only with the Tilt patch applied. Can you please check what's the problem?

If I run the Web Console tests alone, they run fine.

Giving an r- only for the test failures.

::: browser/devtools/tilt/TiltUtils.jsm
@@ +665,5 @@
> + */
> +TiltUtils.gc = function TU_gc()
> +{
> +  var browserWindow = Cc["@mozilla.org/appshell/window-mediator;1"]
> +    .getService(Components.interfaces.nsIWindowMediator)

s/Components.interfaces/Ci/

Comment 77

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

Attachment: v4.1

Fixed failing tests and some other small typos.
I forgot to do a removeChild for a canvas in test_utils04 which made the webconsole panic.

Comment 78

[Mihai Sucan [:msucan]]

Comment on attachment 579797 [details] [diff] [review]
v4.1

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

Thanks for the update, Victor! All tests pass now. Obligatory r+!

Comment 79

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

\0/

Comment 80

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

Attachment: v4.2

Removed a couple of unused params in gl functions and made a few minor modifications to allow the Tilt extension to automatically start native Tilt when requested. For the record, the changes made in the extension are
http://bit.ly/vQTsid and http://bit.ly/uEaR3B

Comment 81

[Rob Campbell [:rc] (:robcee)]

weirdly, while testing the addon changes, opening Tilt on this bug (whether the addon is installed or not) produces a blank visualization on this machine. Need to verify elsewhere, but I have a sneaky suspicion something's Not Quite Right with Tilt on bugzilla pages.

Nothing on the error console.

Comment 82

[Rob Campbell [:rc] (:robcee)]

and now it's working. Odd.

Comment 83

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 580394 [details] [diff] [review]
v4.2

This looks great! I think we're ready to try landing it.

Comment 84

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

(In reply to Rob Campbell [:rc] (robcee) from comment #81)
> weirdly, while testing the addon changes, opening Tilt on this bug (whether
> the addon is installed or not) produces a blank visualization on this
> machine. Need to verify elsewhere, but I have a sneaky suspicion something's
> Not Quite Right with Tilt on bugzilla pages.
> 
> Nothing on the error console.

That's weird.. I *never* had this problem. I'll look into it.

(In reply to Rob Campbell [:rc] (robcee) from comment #83)
> Comment on attachment 580394 [details] [diff] [review]
> v4.2
> 
> This looks great! I think we're ready to try landing it.

Awesome!

Comment 85

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

(In reply to Rob Campbell [:rc] (robcee) from comment #82)
> and now it's working. Odd.

Found the "problem": if you're inspecting pages with > MAX_TEXTURE_SIZE dimensions (usually 8192), only a part of the mesh will be textured (hardware limitations, meh). If you start Tilt on a giant page scrolled to the bottom (like this one) you won't see anything (because the visualization is also translated on initialization at the x,y scroll offset). If you scroll back to < 8192px (whether in Tilt or on the page itself), you won't notice any problems.

Quick and easy fix: if you scrolled past MAX_TEXTURE_SIZE, don't offset the mesh.
Better (harder) fix: create as many textures as needed, split the page in chunks.

I can do the easy fix here. The harder fix would take longer.

Comment 86

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

Attachment: v4.3

Made the easy fix.

Comment 87

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

Filed bug 710565 for the harder fix.

Comment 88

[Ed Morley [:emorley]]

(Sorry for the noise - but anyone looking for:
* cfe788512a5d : Remove build support for old Unix platforms
* 78af9261582f : Remove remaining Tru64/Alpha support
...it was landed with the wrong bug number, so please see bug 682920 instead.)

Comment 89

[Rob Campbell [:rc] (:robcee)]

Nice deduction from my very cryptic STR, Victor. :)

Comment 90

[Rob Campbell [:rc] (:robcee)]

Interdiffed v4.2 and 4.3. Changes look reasonable.

Comment 91

[Mozilla RelEng Bot]

Try run for 80f3424d5240 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=80f3424d5240
Results (out of 206 total builds):
    success: 169
    warnings: 34
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-80f3424d5240

Comment 92

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

Found the reason for the failures: tests don't handle the case in which webgl is available, but disabled (but which is handled when you actually start Tilt). This sounds like an easy fix, I'm on it :) The other warnings are unrelated.

Comment 93

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

Attachment: v4.4

Added a check in the tests to test both if WebGL is supported and a context can be created (should get null if disabled).

Also, while testing a debug build, some colors looked a bit weird so I changed the conversion algorithm to not use bit shifting anymore.

Comment 94

[Mozilla RelEng Bot]

Try run for b2b8caa5d740 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b2b8caa5d740
Results (out of 153 total builds):
    exception: 46
    success: 97
    warnings: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b2b8caa5d740

Comment 95

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

Attachment: v4.4fix

Fixed typo: forgot to add a param to the create3DContext function when checking if isWebGLSupported in tests.

I'm tired.

Comment 96

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 581919 [details] [diff] [review]
v4.4fix

ok.

is this landable or do we need another try run?

Comment 97

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

(In reply to Rob Campbell [:rc] (robcee) from comment #96)
> Comment on attachment 581919 [details] [diff] [review]
> v4.4fix
> 
> ok.
> 
> is this landable or do we need another try run?

I say landable. Try run looks green: https://tbpl.mozilla.org/?tree=Try&rev=3763055d5985

Comment 98

[Rob Campbell [:rc] (:robcee)]

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

Comment 99

[Mozilla RelEng Bot]

Try run for 3763055d5985 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3763055d5985
Results (out of 206 total builds):
    exception: 1
    success: 179
    warnings: 26
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-3763055d5985

Comment 100

[Rob Campbell [:rc] (:robcee)]

!!!!!

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

Comment 101

[Francesco Lodolo [:flod]]

Silly question: why using an external accesskey (M) when "3" is probably free?
https://hg.mozilla.org/mozilla-central/diff/cb4feeed6ac5/browser/locales/en-US/chrome/browser/browser.dtd

Comment 102

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

(In reply to flod (Francesco Lodolo) from comment #101)
> Silly question: why using an external accesskey (M) when "3" is probably
> free?
> https://hg.mozilla.org/mozilla-central/diff/cb4feeed6ac5/browser/locales/en-
> US/chrome/browser/browser.dtd

Legacy reasons, as the addon also used M.
https://addons.mozilla.org/en-US/firefox/addon/tilt

Comment 103

[Axel Hecht [:Pike]]

(In reply to Victor Porof from comment #102)
> (In reply to flod (Francesco Lodolo) from comment #101)
> > Silly question: why using an external accesskey (M) when "3" is probably
> > free?
> > https://hg.mozilla.org/mozilla-central/diff/cb4feeed6ac5/browser/locales/en-
> > US/chrome/browser/browser.dtd
> 
> Legacy reasons, as the addon also used M.
> https://addons.mozilla.org/en-US/firefox/addon/tilt

That would be an explenation. I don't see that as a reason, though, in particular as you're the original author, afaict, so you could share why you chose 'M' to begin with.

Or, to phrase it the other way around: In general, accesskeys are chosen to be a letter within the string. If there are no reasons to not do that here, we should fix that.

Comment 104

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

(In reply to Axel Hecht [:Pike] from comment #103) 
> That would be an explenation. I don't see that as a reason, though, in
> particular as you're the original author, afaict, so you could share why you
> chose 'M' to begin with.
> 
> Or, to phrase it the other way around: In general, accesskeys are chosen to
> be a letter within the string. If there are no reasons to not do that here,
> we should fix that.

Ctrl+3 switches workspaces on Mac OS (and Ctrl+n is a generally accepted shortcut for doing this on all OS).

Comment 105

[Francesco Lodolo [:flod]]

(In reply to Victor Porof from comment #104)
> Ctrl+3 switches workspaces on Mac OS (and Ctrl+n is a generally accepted
> shortcut for doing this on all OS).

AFAIK access keys are not relevant on Mac OS X (they're not part of UI), are we talking about an accesskey or a commandkey here?

Comment 106

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

(In reply to flod (Francesco Lodolo) from comment #105)
> (In reply to Victor Porof from comment #104)
> > Ctrl+3 switches workspaces on Mac OS (and Ctrl+n is a generally accepted
> > shortcut for doing this on all OS).
> 
> AFAIK access keys are not relevant on Mac OS X (they're not part of UI), are
> we talking about an accesskey or a commandkey here?

We're talking about access keys, which are accessible via Ctrl+Key. Ctrl+3 is not a good option. File a bug and cc me if you feel strongly about this.

Comment 107

[Francesco Lodolo [:flod]]

Sorry but I don't get it...

Access keys are not supposed to work on Mac, in fact if I press CTRL+F nothing happens (according to your comment it should open the File menu).

I downloaded Tilt from AMO and there's no access key, there's an hard-coded command key set to "M" and to get that you have to press command+shift+M on a Mac.

Comment 108

[Axel Hecht [:Pike]]

(In reply to Victor Porof from comment #106)
> (In reply to flod (Francesco Lodolo) from comment #105)
> > (In reply to Victor Porof from comment #104)
> We're talking about access keys, which are accessible via Ctrl+Key. Ctrl+3
> is not a good option. File a bug and cc me if you feel strongly about this.

That's not true, the "accesskeys" are actually accesskeys, and are accessed through alt-key. Just tested on linux.

Rather funky concept for a11y, but that's how it works here.

In case other folks want to test, if you open inspect, we're talking about the buttons on the right.

Comment 109

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

(In reply to Axel Hecht [:Pike] from comment #108)
> > We're talking about access keys, which are accessible via Ctrl+Key. Ctrl+3
> > is not a good option. File a bug and cc me if you feel strongly about this.
> 
> That's not true, the "accesskeys" are actually accesskeys, and are accessed
> through alt-key. Just tested on linux.
> 
> Rather funky concept for a11y, but that's how it works here.
> 
> In case other folks want to test, if you open inspect, we're talking about
> the buttons on the right.

I'm referring to the Mac OS case specifically. After opening Inspector, pressing Ctrl+M opens the integrated Tilt. Alt+M does not. If I were to use 3 instead of M, Ctrl+3 would switch workspaces.

Please file a separate bug for this.

Comment 110

[Rob Campbell [:rc] (:robcee)]

yeah, for whatever reason, these toolbar accesskeys work with the ctrl key on the mac. We also don't get the accelerator text (M) like we do on Windows and Linux.

It might make sense to use the '3' on Windows and Linux and stick with 'M' on the mac.

As for "Why did we choose M in the first place?" if memory serves, it was one of the few shortcuts available to all three platforms.

filed bug 711966.

Comment 111

[Rimas Kudelis [use needinfo to ping]]

(In reply to Rob Campbell [:rc] (robcee) from comment #110)
> yeah, for whatever reason, these toolbar accesskeys work with the ctrl key
> on the mac.

Do you really mean Ctrl key here, or Cmd (by default)? If it's the latter, then wow, maybe this functionality (accesskeys acting as commandkeys on mac) should be "fixed: by removing it? In my locale, toolbar accesskeys are T, K, V, H, S, and now M. Of all these, only K and M aren't used with Cmd for other stuff. Knowing that those shortcuts aren't even advertised anywhere (are they?), I'd see that functionality as a bug.

Comment 112

[Rob Campbell [:rc] (:robcee)]

Hi Rimas,

(In reply to Rimas Kudelis from comment #111)
> (In reply to Rob Campbell [:rc] (robcee) from comment #110)
> > yeah, for whatever reason, these toolbar accesskeys work with the ctrl key
> > on the mac.
> 
> Do you really mean Ctrl key here, or Cmd (by default)? If it's the latter,
> then wow, maybe this functionality (accesskeys acting as commandkeys on mac)
> should be "fixed: by removing it? In my locale, toolbar accesskeys are T, K,
> V, H, S, and now M. Of all these, only K and M aren't used with Cmd for
> other stuff. Knowing that those shortcuts aren't even advertised anywhere
> (are they?), I'd see that functionality as a bug.

I really do mean that Ctrl keys work as accelerators on toolbar buttons on the Mac! I was surprised as well when I discovered that. I'm not sure whence that behavior came from, but I bet it's defined somewhere in toolkit/widgets.

I'm not sure they'd be considered "command keys" or not. May be worth raising a bug to re-evaluate that behavior. They definitely don't show up in UI so they're totally undiscoverable.

Comment 113

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 579797 [details] [diff] [review]
v4.1

clearing old review requests.

Comment 114

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 576833 [details]
screenshot

I think this was implicitly ui+.

Comment 115

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/Tools/Page_Inspector/3D_view

And mentioned on Firefox 11 for developers.