582023 - Land TabCandy Utility Code (utils.jsm, iq.js)

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Ian Gilman [:iangilman]]

Attachment: Patch v1

This bug is for reviewing Tab Candy's core utility files, utils.js and iq.js. 

Utils.js encapsulates a number of common methods used throughout Tab Candy, as well as providing several geometry classes. Documentation is provided in comment form and can be seen in HTML form (for the code version that matches the attached patch) here: 

http://hg.mozilla.org/labs/tabcandy/raw-file/b80bd1fe1f79/content/doc/files2/core/utils-js.html

Iq.js helps with DOM manipulation, with an API similar to jQuery. You can see the HTML version of its comment documentation here:

http://hg.mozilla.org/labs/tabcandy/raw-file/b80bd1fe1f79/content/doc/files2/core/iq-js.html

Note that both of these files are used solely within the Tab Candy frame.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Looks like bug 582200 has patches meant to land on top of these. Can you guys perhaps work together to get them merged and then re-request review on the whole?

Comment 2

[Ian Gilman [:iangilman]]

Attachment: Patch v2

Replacement patch, incorporating Frank Yan's fixes, as well as some additional cleanup.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 460574 [details] [diff] [review]
Patch v2

I really wish the iQ code in particular was reformatted to be readable using something resembling typical Mozilla coding style. As it is it's nearly obfuscated (I imagine this is inherited from jQuery).

Not squishing multiple declarations into a single var statement, using "if (foo)" spacing for conditions/control statements, normal indentation, etc. would make this code much more approachable.

I didn't review iq.js too closely, since its use is going to be limited to the tabcandy iframe. Has it been audited to ensure that all of the code is used? I'd like to minimize its size if at all possible.

>diff -r 58175e77c460 browser/base/content/tabcandy/core/iq.js

>+(function( window, undefined ) {

I don't understand why this complication is needed. It seems like the intent is to avoid scope pollution or conflicts with other libraries, but neither of those are concerns for us, so this seems like this could all just be in a "var iQ = {...}".

>+  get: function( num ) {
>+    return num == null ?
>+
>+      // Return a 'clean' array
>+      // was toArray
>+      Array.prototype.slice.call( this, 0 ) :
>+
>+      // Return just the object
>+      ( num < 0 ? this[ num + this.length ] : this[ num ] );

if (num == null)
  return Array.prototype.slice.call( this, 0 );

var index = num < 0 ? num + this.length : num;
return this[index];

Is much more readable, IMO.

>+  addClass: function( value ) {

>+    if ( value && typeof value === "string" ) {
>+      for ( var i = 0, l = this.length; i < l; i++ ) {
>+        var elem = this[i];
>+        if ( elem.nodeType === 1 ) {
>+          (value || "").split( rspace ).forEach(function(className) {

value can't be false-y here, given the check above.

>+          if ( value ) {
>+            (value || "").split(rspace).forEach(function(className) {

same here.

>+  // ----------
>+  // Function: html
>+  // Given a value, sets the receiver's innerHTML to it; otherwise returns what's already there.
>+  // TODO: security

What's this TODO about?

>+// Give the init function the iQ prototype for later instantiation
>+iQ.fn.init.prototype = iQ.fn;

my brain hurts

>diff -r 58175e77c460 browser/base/content/tabcandy/core/utils.js

This does seem like it should be a module (in which case "Utils" could be a new-able object that keeps a reference to a window in its constructor). That also lets you get rid of the (function(){}) wrapping and such.

>+XPCOMUtils.defineLazyGetter(this, "gWindow", function() {

If this doesn't become a module, this should just use:
window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
      .getInterface(Components.interfaces.nsIWebNavigation)
      .QueryInterface(Components.interfaces.nsIDocShell)
      .chromeEventHandler.ownerDocument.defaultView;

Fennec has Rect/Point classes that are very similar to the ones in this file already:

http://mxr.mozilla.org/mobile-browser/source/chrome/content/Util.js#310

Their code is in a separate repository, so sharing it directly isn't possible at the moment, but perhaps it would be worth copying it over rather than having a slightly different (new) implementation here? I think switching wouldn't require many changes, and it would make it possible to move the code into a shared location (toolkit) in the future.

>+window.Point = function(a, y) {

>+    // Variable: x
>+    // Variable: y

I don't understand the purpose of these comments (same applies to Rect).

>+  // You could of course just pass the rectangle straight in, but this is cleaner.
>+  css: function() {

I don't really understand why this is cleaner.

>+  _sendToSubscribers: function(eventName, eventInfo) {

>+      var subsCopy = Utils.merge([], this.subscribers[eventName]);

var subsCopy = this.subscribers[eventName].concat();

>+window.Utils = {
>+  consoleService: null, 

Everywhere this is used should just use Services.console instead (import XPCOMUtils as needed).

>+  isDOMElement: function(object) {
>+    return (object && typeof(object.nodeType) != 'undefined' ? true : false);
>+  },

instanceof DOMElement?

>+  // ----------
>+  // Function: merge
>+  // Merge two arrays and return the result.
>+  merge: function( first, second ) {

I'd use:

Array.forEach(second, function (e) Array.push(first, e));
return first;

assuming it's important to actually modify first and also to avoid converting it to a real array. If it's not, I'd use:

return Array.slice(first).concat(Array.slice(second));





We need to get the licensing question sorted out (maybe try poking [e-mail] directly, pointing to your bug?), but otherwise I think this should be good to land with these comments addressed.

Comment 4

[Michael Yoshitaka Erlewine [:mitcho]]

Revised iQ (and a little of utils) based on comments from gavin:

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/15a0872846d4

- let is the new var, no multiple declarations on the same line.
- renamed iQ.fn.init to be iQClass, iQ.fn to be iQClass.prototype, to look much more normal. iQ now just creates a new iQClass. No more brain hurt.
- no more (function(){...}) closure
- Array.prototype.xxx.call replaced by Array.xxx
- a couple other changes recommended by gavin.
- Utils: rm some comments, but haven't touched JSM, isDOMElement, or merge.

Haven't touched Utils much yet (as noted in the commit notes above) and haven't looked into the Fennec geometry utils yet.

Mardak, feel free to improve on this and/or utils as you see fit and rerequest review.

Comment 5

[Ed Lee :Mardak]

mitcho: You changed some merges to just call concat.

Comment 6

[Ed Lee :Mardak]

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/38687a9c24e2
Move lazy gWindow, gBrowser, etc into tabcandy.js out of utils.js for bug 582023.

Comment 7

[Michael Yoshitaka Erlewine [:mitcho]]

Removed iQ get method.

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/1a438ce6bc9e

Comment 8

[Ed Lee :Mardak]

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/c65c28b4af0a
Move utils.js into a javascript module that exports Point, Rect, Range, Subscribable, and Utils for bug 582023.

Comment 9

[Ed Lee :Mardak]

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/39adb0e18d0e
Simplify Utils.merge to use Array.forEach and Array.push for bug 582023.

Comment 10

[Ed Lee :Mardak]

Attachment: v2 -> v3 hg diff -r 9517ed327dd4 modules/utils.js iq.js

Comment 11

[Ed Lee :Mardak]

Attachment: v3

Comment 12

[Ed Lee :Mardak]

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/2ed5b7a3764a
Just use instanceof Ci.nsIDOMElement for Utils.isDOMElement for bug 582023.

Comment 13

[Aza Raskin [:aza]]

We have now got all of our licensing ducks in a row (bug 582025) as of commit 7b9e1f5b4d9b.

Comment 14

[Ed Lee :Mardak]

Attachment: v2 -> v3.1 hg diff -r 9517ed327dd4 modules/utils.js iq.js

Comment 15

[Ed Lee :Mardak]

Attachment: v3.1

Updated patch with diff of interdiff:
https://bugzilla.mozilla.org/attachment.cgi?oldid=461312&action=interdiff&newid=462954&headers=1

Main things are license changes and isDOMElement

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 462954 [details] [diff] [review]
v2 -> v3.1 hg diff -r 9517ed327dd4 modules/utils.js iq.js

>   timeout: function(func, delay) {
>-    setTimeout(function() {
>-      try {
>-        func();
>-      } catch(e) {
>-        Utils.log(e);
>+    let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>+    timer.initWithCallback({
>+      notify: function notify() {
>+        try {
>+          func();
>+        }
>+        catch(ex) {
>+          Utils.log(timer, ex);
>+        }
>       }
>-    }, delay);
>+    }, delay, timer.TYPE_ONE_SHOT);
>   }

Why this change?

Also I still don't really see the point of the try/catch...

Comment 17

[Ian Gilman [:iangilman]]

(In reply to comment #16)
> Why this change?
> 
> Also I still don't really see the point of the try/catch...

I'm curious about that change as well. Looks like it was Mardak's. 

The purpose of the try/catch is that exceptions thrown inside a setTimeout just disappear into the ether without being reported (as far as I can tell). We want to know when something goes wrong. If there's a better way, I'd love to hear it.

Comment 18

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #17)
> (In reply to comment #16)
> > Why this change?
> > 
> > Also I still don't really see the point of the try/catch...
> 
> I'm curious about that change as well. Looks like it was Mardak's. 

As Utils is a JSM, we no longer have access to the local window, so we can't use setTimeout and instead have to use the nsITimer... is my interpretation.

Comment 19

[Ed Lee :Mardak]

Uncaught exceptions should still show up in the error console. And yes the switch to nsITimer is because it's moved to a js module.

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #19)
> Uncaught exceptions should still show up in the error console.

Exactly.

Comment 21

[Ed Lee :Mardak]

Attachment: v4

Comment 22

[Ed Lee :Mardak]

Attachment: v2 -> v4 hg diff -r 9517ed327dd4:414480c53b02 modules/utils.js iq.js

Comment 23

[Dão Gottwald [:dao]]

Are there going to be users of Utils.timeout without a window object, i.e. JS modules? iq.js should use setTimeout, which provides the same API.

Comment 24

[Dão Gottwald [:dao]]

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

>+  // ----------
>+  // Function: assert

:( ... Can we drop this? "// Function: assert" right above "assert: function" seems slightly pointless.

>+  // Prints a stack trace along with label (as a console message) if condition is false.
>+  assert: function Utils_assert(label, condition) {

This looks backwards; the traditional argument order would be (condition, label), I think.

Comment 25

[Ian Gilman [:iangilman]]

I believe Gavin has already given this patch a verbal rubberstamp (unfortunately he didn't do it officially here ... aza's working on sorting that out). Can we land this as approved and then continue addressing Dao's comments?

Also, dolske, please prioritize reviewing bug 574217 before this one; in fact reviewing this one may not be needed (with the possible exception of minor changes).

Comment 26

[:Gavin Sharp [email: [e-mail]]]

I didn't stamp it because I wanted to give it another quick look, and didn't get a chance. Dao's comments would be good to address.

Comment 27

[Ian Gilman [:iangilman]]

(In reply to comment #26)
> I didn't stamp it because I wanted to give it another quick look, and didn't
> get a chance. Dao's comments would be good to address.

Gavin, we should address them by making the changes and resubmitting a patch? Or we should address them through discussion with an eye towards updating after landing?

I just want to streamline this process as much as possible, as we've got less than a week before our beta4 window closes, and both dao and dolske have other patches of ours to review. 

At any rate, it looks like the outstanding issues are timeout, assert call order, and documentation comments:  

* I can remove Utils.timeout and replace it with setTimeout wherever it's used (though in my tests, even though the uncaught exceptions are indeed supposed to show up in the error console, they do not).

* I can flip the arguments on the assert methods (I just used the order we had on my last team; happy to use the preferred mozilla order). 

* Comments like "Function: assert" are for Natural Docs; they're how we can provide you with this: 

http://hg.mozilla.org/labs/tabcandy/raw-file/tip/content/doc/files2/modules/utils-js.html

... which seems like a good thing to me. I hope we don't have to redo all the comments before we can land.

Comment 28

[Dão Gottwald [:dao]]

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

>+        let substitutions = {
>+          'MozTransform': '-moz-transform',
>+          'zIndex': 'z-index'
>+        };
>+
>+        return window.getComputedStyle(this[0], null).getPropertyValue(substitutions[key] || key);

return window.getComputedStyle(this[0], null).key;

Comment 29

[Dão Gottwald [:dao]]

(In reply to comment #28)
> Comment on attachment 463719 [details] [diff] [review]
> v4
> 
> >+        let substitutions = {
> >+          'MozTransform': '-moz-transform',
> >+          'zIndex': 'z-index'
> >+        };
> >+
> >+        return window.getComputedStyle(this[0], null).getPropertyValue(substitutions[key] || key);
> 
> return window.getComputedStyle(this[0], null).key;

Err, return window.getComputedStyle(this[0], null)[key];

Comment 30

[Dão Gottwald [:dao]]

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

>+  // ----------
>+  // Function: width
>+  // Returns the width of the receiver.
>+  width: function() {
>+    return parseInt(this.css('width'));
>+  },
>+
>+  // ----------
>+  // Function: height
>+  // Returns the height of the receiver.
>+  height: function() {
>+    return parseInt(this.css('height'));
>+  },
>+
>+  // ----------
>+  // Function: position
>+  // Returns an object with the receiver's position in left and top
>+  // properties.
>+  position: function() {
>+    return {
>+      left: parseInt(this.css('left')),
>+      top: parseInt(this.css('top'))
>+    };
>+  },
>+
>+  // ----------
>+  // Function: bounds
>+  // Returns a <Rect> with the receiver's bounds.
>+  bounds: function() {
>+    let p = this.position();
>+    return new Rect(p.left, p.top, this.width(), this.height());
>+  },

Won't this.css('left') be "auto" for non-positioned elements?

Are .width/.height expected to exclude padding and border? If so, they shouldn't be used for .bounds. Seems like getBoundingClientRect should be used throughout this code.

Comment 31

[Dão Gottwald [:dao]]

(In reply to comment #27)
> * Comments like "Function: assert" are for Natural Docs; they're how we can
> provide you with this: 
> 
> http://hg.mozilla.org/labs/tabcandy/raw-file/tip/content/doc/files2/modules/utils-js.html
> 
> ... which seems like a good thing to me. I hope we don't have to redo all the
> comments before we can land.

I'm not sure whether these docs are going to be relevant and kept up to date for mozilla-central, but you can leave the comments for now.

Comment 32

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #29)
> Err, return window.getComputedStyle(this[0], null)[key];

(In reply to comment #30)
> Won't this.css('left') be "auto" for non-positioned elements?
> 
> Are .width/.height expected to exclude padding and border? If so, they
> shouldn't be used for .bounds. Seems like getBoundingClientRect should be used
> throughout this code.

Ian, I tried to make these changes, but they resulted in some major geometry quirks (weird sizes for tab items). I'd appreciate it if you could take a look at this tomorrow or if we can talk it through.

Comment 33

[Dão Gottwald [:dao]]

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

>+        let substitutions = {
>+          'MozTransform': '-moz-transform',
>+          'zIndex': 'z-index'
>+        };
>+
>+        return window.getComputedStyle(this[0], null).getPropertyValue(substitutions[key] || key);

Alternatively to what I wrote earlier, you could always pass "z-index" & co. and use getPropertyValue(key). Either way the substitutions object should go away, as it's unnecessary and its content appears to be an arbitrary selection.

Comment 34

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #33)
> Alternatively to what I wrote earlier, you could always pass "z-index" & co.
> and use getPropertyValue(key). Either way the substitutions object should go
> away, as it's unnecessary and its content appears to be an arbitrary selection.

Yep, thanks. I went ahead and did that, as well as redid the move to using getBoundingClientRect: (So Ian, I did end up resolving this)

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/a7fb462bf45c

Comment 35

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #23)
> Are there going to be users of Utils.timeout without a window object, i.e. JS
> modules? iq.js should use setTimeout, which provides the same API.

Utils.timeout has been removed:

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/c28dff002782

Comment 36

[Ian Gilman [:iangilman]]

Looks like the remaining item to address is the assert call order. I'll take care of that. Then should we produce another patch?

Comment 37

[Ian Gilman [:iangilman]]

Ok, asserts fixed:

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/afe85ee5085d

Mardak, can you generate a new version of the patch? I guess it should be flagged for Dao, since he's the one who's been reviewing it?

Comment 38

[Ed Lee :Mardak]

Attachment: v4 -> v5 hg diff -r 414480c53b02:afe85ee5085d iq.js modules/utils.jsm

Comment 39

[Ed Lee :Mardak]

Attachment: v5

Comment 40

[Dão Gottwald [:dao]]

Comment on attachment 464496 [details] [diff] [review]
v5

>+  // The current version of iQ being used
>+  iq: "1.4.2",

?

>+  width: function() {
>+    let bounds = this.bounds();
>+    return bounds.width

add semicolon

>+  attr: function(key, value) {
>+    try {
>+      Utils.assert(typeof key === 'string', 'string key');
>+      if (typeof value === "undefined") {
>+        Utils.assert(this.length == 1, 'retrieval does not support multi-objects (or null objects)');
>+        return this[0].getAttribute(key);
>+      }
>+      for (let i = 0; this[i] != null; i++) {
>+        this[i].setAttribute(key, value);
>+      }
>+    } catch(e) {
>+      Utils.log(e);
>+    }

Why would this throw?

>+      this.css({
>+        '-moz-transition-property': 'all', // TODO: just animate the properties we're changing
>+        '-moz-transition-duration': (duration / 1000) + 's',
>+        '-moz-transition-timing-function': easing
>+      });
>+
>+      this.css(css);
>+
>+      let self = this;
>+      setTimeout(function() {
>+        self.css({
>+          '-moz-transition-property': 'none',
>+          '-moz-transition-duration': '',
>+          '-moz-transition-timing-function': ''
>+        });
>+
>+        if (typeof options.complete == "function")
>+          options.complete.apply(self);
>+      }, duration);

How about using the transitionend event?

>+  fadeIn: function() {
>+    try {
>+      this.css({display: ''});
>+      this.animate({
>+        opacity: 1
>+      }, {
>+        duration: 400
>+      });
>+    } catch(e) {
>+      Utils.log(e);
>+    }

Why would this throw?

>+  hide: function() {
>+    try {
>+      this.css({display: 'none', opacity: 0});
>+    } catch(e) {
>+      Utils.log(e);
>+    }

>+  show: function() {
>+    try {
>+      this.css({display: '', opacity: 1});
>+    } catch(e) {
>+      Utils.log(e);
>+    }

And these?

>+ * The Initial Developer of the Original Code is
>+ * Aza Raskin <[e-mail]>

Make this "the Mozilla Foundation", add Aza as a contributor.

>+  isNumber: function(n) {
>+    return (typeof(n) == 'number' && !isNaN(n));

here and elsewhere: "typeof n". (typeof is not a function.)

>+    if (obj.constructor
>+      && !hasOwnProperty.call(obj, "constructor")
>+      && !hasOwnProperty.call(obj.constructor.prototype, "isPrototypeOf")) {

here and elsewhere:

(foo &&
 bar &&
 baz)

>+  extend: function() {
>+    // copy reference to target object
>+    var target = arguments[0] || {}, i = 1, length = arguments.length, options, name, src, copy;
>+
>+    // Deep copy is not supported
>+    if (typeof target === "boolean") {
>+      this.assert(false, "The first argument of extend cannot be a boolean." +
>+          "Deep copy is not supported.");
>+      return target;
>+    }
>+
>+    // Back when this was in iQ + iQ.fn, so you could extend iQ objects with it.
>+    // This is no longer supported.
>+    if (length === 1) {
>+      this.assert(false, "Extending the iQ prototype using extend is not supported.");
>+      return target;
>+    }
>+
>+    // Handle case when target is a string or something
>+    if (typeof target != "object" && typeof target != "function") {
>+      target = {};
>+    }
>+
>+    for (; i < length; i++) {

Declare the variables where you use them, each declaration on a single line.

Comment 41

[Dão Gottwald [:dao]]

There's also NS_ASSERT provided by debug.js.

Comment 42

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #40)
> >+ * The Initial Developer of the Original Code is
> >+ * Aza Raskin <[e-mail]>
> 
> Make this "the Mozilla Foundation", add Aza as a contributor.
> 
> >+  isNumber: function(n) {
> >+    return (typeof(n) == 'number' && !isNaN(n));
> 
> here and elsewhere: "typeof n". (typeof is not a function.)
> 
> >+    if (obj.constructor
> >+      && !hasOwnProperty.call(obj, "constructor")
> >+      && !hasOwnProperty.call(obj.constructor.prototype, "isPrototypeOf")) {
> 
> here and elsewhere:
> 
> (foo &&
>  bar &&
>  baz)
> 
> >+  extend: function() {
> >+    // copy reference to target object
> >+    var target = arguments[0] || {}, i = 1, length = arguments.length, options, name, src, copy;
> >+
> >+    // Deep copy is not supported
> >+    if (typeof target === "boolean") {
> >+      this.assert(false, "The first argument of extend cannot be a boolean." +
> >+          "Deep copy is not supported.");
> >+      return target;
> >+    }
> >+
> >+    // Back when this was in iQ + iQ.fn, so you could extend iQ objects with it.
> >+    // This is no longer supported.
> >+    if (length === 1) {
> >+      this.assert(false, "Extending the iQ prototype using extend is not supported.");
> >+      return target;
> >+    }
> >+
> >+    // Handle case when target is a string or something
> >+    if (typeof target != "object" && typeof target != "function") {
> >+      target = {};
> >+    }
> >+
> >+    for (; i < length; i++) {
> 
> Declare the variables where you use them, each declaration on a single line.

Thanks Dão. I just committed the changes you recommended to all non-iQ code. Ian is working on iq.js.

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/5a46a7680fd2

Comment 43

[Mike Beltzner [:beltzner, not reading bugmail]]

Dao: figure those nits will be the last? We're trying to determine viability of this making it into beta 4.

Comment 44

[Ian Gilman [:iangilman]]

Dao, I've addressed the recent comments in iQ. 

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/09bc63c0e629

... with the exception of the transitionend event. We originally tried transitionend but unfortunately it doesn't give us what we need; it's too easy to get too many or too few callbacks. setTimeout is much more reliable for our needs.

NS_ASSERT sounds interesting; I'll take a look at it. I'd like to avoid making that change for now, though, if possible. 

Should we make another patch?

Comment 45

[Ed Lee :Mardak]

Attachment: v5 -> v6 hg di -r afe85ee5085d:09bc63c0e629 iq.js modules/utils.jsm

Comment 46

[Ed Lee :Mardak]

Attachment: v6

Comment 47

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #44)
> transitionend but unfortunately it doesn't give us what we need; it's too easy
> to get too many or too few callbacks. setTimeout is much more reliable for our
> needs.

I'm not sure what exactly you're trying to do, but this doesn't sound right ("setTimeout" and "reliable" in the same sentence!). Worth elaborating with a CSS transition expert in a followup bug, perhaps?

Comment 48

[Aza Raskin [:aza]]

If I remember correctly, in our tests setting a CSS transition to take a certain amount of time didn't actually guarantee that it would last that long. Sometimes it would go longer than the time specified, and sometimes shorter. Using a setTimeout reduced time variability.

Comment 49

[Dão Gottwald [:dao]]

Comment on attachment 464641 [details] [diff] [review]
v6

>+++ b/browser/base/content/tabview/iq.js

>+ * The Initial Developer of the Original Code is
>+ * the Mozilla Foundation

add period

>+        } else {
>+            Utils.assert(false, 'does not support complex HTML creation');
>+        }

reduce indentation

>+  addClass: function(value) {
>+    if (typeof value != "string" || !value) {
>+      Utils.assert(false, 'requires a valid string argument');
>+      return null;
>+    }

You don't check arguments like this in all possible places. I don't see what makes you decide doing it or not doing it. Doing it in all possible places would bloat the code to an annoying level, so I'm not asking you to do this. Instead I'd like to question whether the current instances are worthwhile.

At least I think you should flatten those four lines to:

Utils.assertThrow(typeof value != "string" || !value, 'requires a valid string argument');

(yes, without try/catch)

>+  animate: function(css, options) {
>+    try {
>+      Utils.assert(this.length == 1, 'does not yet support multi-objects (or null objects)');
>+
>+      if (!options)
>+        options = {};
>+
>+      let easings = {
>+        tabviewBounce: "cubic-bezier(0.0, 0.63, .6, 1.0)", 
>+        // TODO: change 1.0 above to 1.29 after bug 575672 is fixed
>+        
>+        easeInQuad: 'ease-in', // TODO: make it a real easeInQuad, or decide we don't care
>+        fast: 'cubic-bezier(0.7,0,1,1)'
>+      };
>+
>+      let duration = (options.duration || 400);
>+      let easing = (easings[options.easing] || 'ease');
>+
>+      // The latest versions of Firefox do not animate from a non-explicitly
>+      // set css properties. So for each element to be animated, go through
>+      // and explicitly define 'em.
>+      let rupper = /([A-Z])/g;
>+      this.each(function(elem) {
>+        let cStyle = window.getComputedStyle(elem, null);
>+        for (let prop in css) {
>+          prop = prop.replace(rupper, "-$1").toLowerCase();
>+          iQ(elem).css(prop, cStyle.getPropertyValue(prop));
>+        }
>+      });
>+
>+      this.css({
>+        '-moz-transition-property': 'all', // TODO: just animate the properties we're changing
>+        '-moz-transition-duration': (duration / 1000) + 's',
>+        '-moz-transition-timing-function': easing
>+      });
>+
>+      this.css(css);
>+
>+      let self = this;
>+      setTimeout(function() {
>+        self.css({
>+          '-moz-transition-property': 'none',
>+          '-moz-transition-duration': '',
>+          '-moz-transition-timing-function': ''
>+        });
>+
>+        if (typeof options.complete == "function")
>+          options.complete.apply(self);
>+      }, duration);
>+    } catch(e) {
>+      Utils.log(e);
>+    }

remove try/catch

>+  // Binds the given function to the given event type. Also wraps the function
>+  // in a try/catch block that does a Utils.log on any errors.
>+  bind: function(type, func) {
>+    Utils.assert(typeof func == "function", 'does not support eventData argument');
>+
>+    let handler = function(event) {
>+      try {
>+        return func.apply(this, [event]);
>+      } catch(e) {
>+        Utils.log(e);
>+      }
>+    };

Again, error logging should already happen automatically.

Regarding the assert, I'm not sure what eventData is and why someone would want to pass this.

>+++ b/browser/base/content/tabview/modules/utils.jsm

>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+const Cu = Components.utils;
>+const Cr = Components.results;

remove unused constants

>+  intersects: function(rect) {
>+    return (rect.right > this.left &&
>+           rect.left < this.right &&
>+           rect.bottom > this.top &&
>+           rect.top < this.bottom);

>+  contains: function(rect) {
>+    return (rect.left > this.left &&
>+           rect.right < this.right &&
>+           rect.top > this.top &&
>+           rect.bottom < this.bottom);
>+  },

>+  equals: function(rect) {
>+    return (rect.left == this.left &&
>+           rect.top == this.top &&
>+           rect.width == this.width &&
>+           rect.height == this.height);
>+  },

more indentation needed (one space each line)

>+  contains: function(value) {
>+    return Utils.isNumber(value) ?
>+      value >= this.min && value <= this.max :
>+      Utils.isRange(value) ?
>+        (value.min <= this.max && this.min <= value.max) :
>+        false;
>+  },

This looks wrong:
value.min <= this.max && this.min <= value.max

Should probably be this instead:
value.min >= this.min && value.max <= this.max

Also, how about one of these:

if (Utils.isNumber(value))
  return value >= this.min && value <= this.max;
if (Utils.isRange(value))
  return value.min >= this.min && value.max <= this.max;
return false;

if (Utils.isNumber(value))
  value = new Range(value, value);
if (Utils.isRange(value))
  return value.min >= this.min && value.max <= this.max;
return false;

>+  addSubscriber: function(refObject, eventName, callback) {
>+    try {
>+      Utils.assertThrow(refObject, "refObject");
>+      Utils.assertThrow(typeof callback == "function", "callback must be a function");
>+      Utils.assertThrow(eventName && typeof eventName == "string",
>+          "eventName must be a non-empty string");
>+
>+      if (!this.subscribers)
>+        this.subscribers = {};
>+
>+      if (!this.subscribers[eventName])
>+        this.subscribers[eventName] = [];
>+
>+      var subs = this.subscribers[eventName];
>+      var existing = subs.filter(function(element) {
>+        return element.refObject == refObject;
>+      });
>+
>+      if (existing.length) {
>+        Utils.assert(existing.length == 1, 'should only ever be one');
>+        existing[0].callback = callback;
>+      } else {
>+        subs.push({
>+          refObject: refObject,
>+          callback: callback
>+        });
>+      }
>+    } catch(e) {
>+      Utils.log(e);
>+    }

Don't wrap everything in try/catch, only the assertThrows with a return in the catch block, if at all; just letting the exceptions be thrown seems appropriate too. Also as said before, checking each and every argument seems a little bit over the top.

>+  removeSubscriber: function(refObject, eventName) {
>+    try {
>+      Utils.assertThrow(refObject, "refObject");
>+      Utils.assertThrow(eventName && typeof eventName == "string",
>+          "eventName must be a non-empty string");
>+
>+      if (!this.subscribers || !this.subscribers[eventName])
>+        return;
>+
>+      this.subscribers[eventName] = this.subscribers[eventName].filter(function(element) {
>+        return element.refObject != refObject;
>+      });
>+    } catch(e) {
>+      Utils.log(e);
>+    }

ditto

>+  _sendToSubscribers: function(eventName, eventInfo) {
>+    try {
>+      Utils.assertThrow(eventName && typeof eventName == "string",
>+          "eventName must be a non-empty string");
>+
>+      if (!this.subscribers || !this.subscribers[eventName])
>+        return;
>+
>+      var self = this;
>+      var subsCopy = this.subscribers[eventName].concat();
>+      subsCopy.forEach(function(object) {
>+        object.callback(self, eventInfo);
>+      });
>+    } catch(e) {
>+      Utils.log(e);
>+    }

ditto

Remove var self = this, pass 'this' as the second argument to forEach instead.

You might want to try/catch within the forEach callback so that all subscribers get called.

>+  isRect: function(r) {
>+    return (r &&
>+           this.isNumber(r.left) &&
>+           this.isNumber(r.top) &&
>+           this.isNumber(r.width) &&
>+           this.isNumber(r.height));

>+  isRange: function(r) {
>+    return (r &&
>+           this.isNumber(r.min) &&
>+           this.isNumber(r.max));

>+    if (obj.constructor &&
>+       !hasOwnProperty.call(obj, "constructor") &&
>+       !hasOwnProperty.call(obj.constructor.prototype, "isPrototypeOf")) {

more indentation needed

file followups on NS_ASSERT and transitionend

r=me with that

Comment 50

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #49)
> Comment on attachment 464641 [details] [diff] [review]
> v6
...
> r=me with that

Thanks Dao! Changes made!

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/292c847cf3a0

Mardak, push to m-c?

Comment 51

[Ed Lee :Mardak]

http://hg.mozilla.org/mozilla-central/rev/77cd3fbaf973

With history:
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=77cd3fbaf973

Comment 52

[Shawn Wilsher :sdwilsh]

These new APIs should have had sr before landing on mozilla-central per the sr policy.  Any reason why they didn't?

Comment 53

[:Gavin Sharp [email: [e-mail]]]

It's not adding general purpose APIs for use by external consumers, this is just utility code for tabcandy. I don't think getting SR on this patch would serve a useful purpose - in fact the review hurdles may have already been unreasonably onerous.