659807 - Implement Tilt: a WebGL-based 3D visualization of a Webpage

Status: VERIFIED FIXED

(DevTools :: General, enhancement)

Description

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

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: 

https://wiki.mozilla.org/Tilt_Project_Page

Tilt represents a new way of visualizing a web page. This tool creates a 3D representation of the document, with the purpose of displaying, understanding and easily analyzing the DOM. It will take advantage of the great tools Firefox has to offer, as it is an extension which contains a WebGL implementation, providing rich user-experience, fun interaction and useful information, while taking full advantage of 3D hardware acceleration, GLSL shaders and what OpenGL ES 2.0 has to offer.

Reproducible: Always

Comment 1

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

Unittests Unittests Unittests!
since we're targeting Fx6 or so, this should live in the Web Developer submenu

browser.xul
  <menupopup id="menu_ToolsPopup">
    <menu class="menu-iconic"
          id="tilt-menu"
          label="&tilt.menu.label;"
          accesskey="&tilt.menu.accesskey;"
          insertafter="devToolsSeparator">

not sure we need a menu-iconic class, but ok.

 <menu id="tilt-menuItemOptions"
              label="&tilt.menuItemOptions.label;"
              accesskey="&tilt.menuItemOptions.accesskey;"]]>
          <menupopup>
            <menuitem id="tilt-menuItemOptionsOption1"
                      label="&tilt.menuItemOptionsOption1.label;"
                      type="checkbox"
                      checked="true"/>
            <menuitem id="tilt-menuItemOptionsOption2"
                      label="&tilt.menuItemOptionsOption2.label;"
                      type="checkbox"/>
            <menuitem id="tilt-menuItemOptionsOption3"
                      label="&tilt.menuItemOptionsOption3.label;"
                      type="checkbox"/>

what are these for? If they're not being used, get rid of 'em!

in browserOverlay.js:

line 52: var label = "Initialize Tilt";

if we want this to be localizable, we should use a .properties file and a stringbundle. see https://developer.mozilla.org/en/Localizing_an_extension#Localizing_strings_in_JavaScript_code

You're also using var here, and then re-using the variable declaration in the else statement. In Firefox, we get to use the let keyword to allow block-scope declarations. You should too.

For your documentation comments, please include @param and @return for your functions.

line 80: this.iframe = undefined;

we typically use null to clean up in destructor methods, but that may be a matter of taste.

Did a quick sweep through glMatrix-0.9.5.js. Looks decent.

jTiltDraw.js
line 136: cubeVertices = engine.initBuffer ...

long arrays like this should be broken into multiple lines (moz code style recommends 80 chars/line used to be 72 for C code and 76 for JS, see https://developer.mozilla.org/En/Developer_Guide/Coding_Style#JavaScript_Practices). Better, put the arrays in another variable and pass them in. What do these arrays represent? 

in jTiltEngine.js

line: 84: vertShader = that.compileShader(http[0].responseText,
                                      "x-shader/x-vertex");

"x-shader/x-vertex" should probably be documented and have a const declaration at the start of your function.

likewise for x-fragment and so on.

line 107: alert("Could not create shader: shader source is null.")

use your stringbundle

same elsewhere.

Also, might not want to use alerts for debugging. We have the nsIConsoleService for that.

http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsIConsoleService.idl#51

You might want to create your own reportError() message in your code that wraps this.

line 242: function applyTextureImage() {

this could use a docstring.

line 444: Image.resizeToPowerOfTwo

Please don't add methods to global objects. That can have serious repercussions in browser world.

Same goes for the additions to Math. You should create your own objects to contain these.

file webgl-utils.js

Do we need this? I'm thinking not. Ditch it!

Overall this is looking really good. We can do another review pass when you have more working.

Also, for future reviews, we should probably attach a patch to attach the review request to. Your repo is a moving target so a reviewer will never be able to be sure they're reviewing the right point in time. Alternatively, you could cut tags for review points and attach the link here.

Comment 2

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

Attachment: Version 0.82, candidate for submitting to addons.mozilla.org

The branch can be also found on github at https://github.com/victorporof/Tilt/tree/v82

Comment 3

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

in TiltChromeUI.js

line 98,
	iframeStripButton = null,

Is that a tab I see? Don't mix your whitespace!

line 150,
  this.init = function(canvas) { ...

I'd kind of like to see the UI construction broken up somewhat.

You've got a row of buttons at the top, arcball construction, domstrips and color picker controls all piled into one method. Some organization or at least comments around the different groupings could help a bit.

line 786,
			iframeStripButton,

tabs! :)

line 852, (in meshNodeCallback):

    var stripNo = this.stripNo++,
      x = 3 + depth * 6,
      y = 3 + stripNo * 10,
      height = 7,

These numbers could use some comments. Appear to be dimensions in pixels.

    clsx = x + namelength * 10 + 3,
    idx = clsx + clslength * 3 + 3;

Same here. Document these numbers!

line 877, same method:
    if (node.className) {
      stripClassButton = new Tilt.Button(null, { 

I'd prefer to see id listed before className in the stripButton. ID is typically the first attribute listed in a list of node attributes, we should do the same thing here.

Also, since classNames is variable length, we don't have to worry about positioning the classNames portion in different relative horizontal positions. They'll always be the id's width + padding to the right.

line 989,
  this.resize = function(width, height) {

more magic numbers here. You might want to declare these somewhere as constants (use the const keyword).

Man, that's a lot of stuff in that destroy() method. (and more tabs before iframeStripButton) One issue with doing all of this construction and destruction in a single object is you have to keep track of everything here. I'm not sure breaking it up up into create{UIComponent}/destroy{UIComponent} bits would make these easier to keep track of or not, but it couldn't hurt.

Comment 4

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

tiltBrowserOverlay.js/xul and browserEntryPoint.js look fine.

Same goes for tiltChromeConfig.js, though some of those values might benefit from the declaration of some constants.

Comment 5

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

tiltChromeController.js is awesome.

Comment 6

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

TiltChromeVisualization.js

line 57,
  var tilt = new Tilt.Renderer(canvas, {
    // if initialization fails because WebGL context coulnd't be created,

typo (coulnd't).

line 63,
      Tilt.Console.alert("Firefox", Tilt.StringBundle.get("initWebGL.error"));

I'd prefer to see some other notification than alert(). Recommend using nsIPromptService instead.

line 75,
  thickness = 15,

magic number! Should be a const, or possibly a configuration paramater. Ideally, I'd like to be able to vary this value if it's what I think it is.

... What is it? :) Might want some comments for these variables.

line 88,
  /**
   * A highlight quad drawn over a stacked dom node.
   */
  highlightQuad = {
    index: -1,
    fill: "#fff",
    stroke: "000",
    strokeWeight: 3,

more constants. (these comments are getting old, I know...)

line 125,
    // use an extension to get the image representation of the document
    // this will be removed once the MOZ_window_region_texture WebGL extension
    // is finished; currently converting the document image to a texture

Might want to reference the bug number for this in the comment.


I like the draw() function. nice'n'tidy!

line 258,
    if ((tilt.frameCount + 1) % 1000 === 0) {
      TiltChrome.BrowserOverlay.performGC();
    }

tilt.frameCount++ ?

running a GC every 1000 frames seems overly excessive. Did you do any tuning of this value? This should probably be another constant or even a preference so we can tweak it and experiment.

...

        indices.push(i + 0,  i + 1,  i + 2,  i + 0,  i + 2,  i + 3,
                     i + 4,  i + 5,  i + 6,  i + 4,  i + 6,  i + 7,
                     i + 8,  i + 9,  i + 10, i + 8,  i + 10, i + 11,
                     i + 10, i + 9,  i + 6,  i + 10, i + 6,  i + 5,
                     i + 8,  i + 11, i + 4,  i + 8,  i + 4,  i + 7);
                     
oof. :)

no further comments on this file. Really beautiful stuff.

Comment 7

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

TiltChromeVisualizationShaders.js

line 60,
  vs: [
"attribute vec3 vertexPosition;",
"attribute vec2 vertexTexCoord;",

funny lack of indentation here. Probably should be indented a couple of levels.

Comment 8

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

Arcball.js

line 241,
    if (keyCode[65]) { // w
      addKeyRot[0] -= frameDelta / 5;
    }

seeing a bunch of these. Division is (I'm told) slower in JS (and most other languages, [citation needed!]) than multiplication. Since these are happening in loops, they might be more costly than doing:

addKeyRot[0] -= frameDelta * 0.2;

Do as much as you can without division. Everywhere!

Be curious to see some profiling on this.

Comment 9

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

sqrt is pretty expensive too. These are happening in loops, (in pointToSphere) though admittedly, probably at low frequency. If you can avoid them, you might get some slightly quicker execution.

Hard to find anything worse than a comment nit in most of this.

Really nice work, Victor. A pleasure to read through. :)

Comment 10

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

Thanks a lot for the code review! It's simply great!
A few things I want to mention (just for the sake of mentioning):

1. Sorry about those couple of tabs.. most probable reason: installed Lion :)

2. "I'd prefer to see some other notification than alert(). Recommend using nsIPromptService": I am doing just that :) See the Tilt.Console.alert (wrapper) function in Console.js line 54, where I use the nsIPromptService. I should probably rename that to "prompt" though..

3. "thickness = 15": It's the thickness of each stack in the 3D mesh. True, should be in a config. It will be eventually changeable at runtime.

4. "running a GC every 1000 frames seems overly excessive": I don't think it's the case (although, indeed, maybe a slightly bit excessive). Since rendering isn't done continuously (if there's nothing new or different to draw a frame, the rendering effectively pauses, thus frameCount isn't incremented). From my experiments, this means that 1000 frames is ~3, 4 minutes, probably the rate in which a normal user opens and closes tabs. The rendering is also not slowed down by this. Anyway, I could increase the value.

5. "sqrt is pretty expensive too": there is some 3D vector algebra that simply has to use sqrt, but there are quite a few optimizations that can be done (most of them involving playing with bits). I'll try to port as much as I can from my dusty, highly unreadable, optimized math functions :)

Comment 11

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

(In reply to Victor Porof from comment #10)
> Thanks a lot for the code review! It's simply great!
> A few things I want to mention (just for the sake of mentioning):
> 
> 1. Sorry about those couple of tabs.. most probable reason: installed Lion :)

hehe, no worries. :)

> 2. "I'd prefer to see some other notification than alert(). Recommend using
> nsIPromptService": I am doing just that :) See the Tilt.Console.alert
> (wrapper) function in Console.js line 54, where I use the nsIPromptService.
> I should probably rename that to "prompt" though..

ah! Ok, my mistake for not looking at the implementation.

> 3. "thickness = 15": It's the thickness of each stack in the 3D mesh. True,
> should be in a config. It will be eventually changeable at runtime.

That's what I thought. I crave a slider to make the stacks towering structures.

> 4. "running a GC every 1000 frames seems overly excessive": I don't think
> it's the case (although, indeed, maybe a slightly bit excessive). Since
> rendering isn't done continuously (if there's nothing new or different to
> draw a frame, the rendering effectively pauses, thus frameCount isn't
> incremented). From my experiments, this means that 1000 frames is ~3, 4
> minutes, probably the rate in which a normal user opens and closes tabs. The
> rendering is also not slowed down by this. Anyway, I could increase the
> value.

If you've already done some testing with it, that's fine. The number seemed low to me, but hadn't done a lot of analysis.

I wonder if it would be better to get the GC out of the render loop entirely and run it in a timeout?

> 5. "sqrt is pretty expensive too": there is some 3D vector algebra that
> simply has to use sqrt, but there are quite a few optimizations that can be
> done (most of them involving playing with bits). I'll try to port as much as
> I can from my dusty, highly unreadable, optimized math functions :)

Yeah, I'm not sure how often you're calling those sqrts, but you might get a little extra performance out of the arcball by removing them. I know the math is a little funky, so I'm not sure it's worth it.

Comment 12

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

Comment on attachment 554418 [details]
Version 0.82, candidate for submitting to addons.mozilla.org

At a quick glance I couldn't see anything wrong with this code, WebGL-wise. Let me know if you have a concern about a precise area, and I'll have a closer look.

Comment 13

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

Awesome. Thanks Benoit!

Comment 14

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

Tilt is up on AMO, I think we can close this bug!

Victor, I'll let you do the honors.

Comment 15

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

woot!