Closed Bug 582023 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 4.0b4

People

(Reporter: iangilman, Assigned: Mardak)

References

Details

Attachments

(2 files, 10 obsolete files)

Attached patch Patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → gavin.sharp
Blocks: 574217
Attachment #460293 - Flags: review?(gavin.sharp)
Attachment #460293 - Attachment is patch: true
Attachment #460293 - Attachment mime type: application/x-javascript → text/plain
Assignee: gavin.sharp → ian
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?
Assignee: ian → nobody
Component: TabCandy → General
Product: Mozilla Labs → Firefox
QA Contact: tabcandy → general
Summary: Tab Candy Utility Code → Land TabCandy Utility Code (iq.js)
Target Milestone: -- → ---
Attached patch Patch v2 (obsolete) — Splinter Review
Replacement patch, incorporating Frank Yan's fixes, as well as some additional cleanup.
Attachment #460293 - Attachment is obsolete: true
Attachment #460574 - Flags: review?(gavin.sharp)
Attachment #460293 - Flags: review?(gavin.sharp)
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 licensing@mozilla.org directly, pointing to your bug?), but otherwise I think this should be good to land with these comments addressed.
Attachment #460574 - Flags: review?(gavin.sharp)
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.
mitcho: You changed some merges to just call concat.
Depends on: 577968
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.
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.
Blocks: 582865
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.
Attachment #460574 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
Attachment #461314 - Flags: review?(gavin.sharp)
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.
We have now got all of our licensing ducks in a row (bug 582025) as of commit 7b9e1f5b4d9b.
Attachment #461312 - Attachment is obsolete: true
Attached patch v3.1 (obsolete) — Splinter Review
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
Attachment #461314 - Attachment is obsolete: true
Attachment #462956 - Flags: review?(gavin.sharp)
Attachment #461314 - Flags: review?(gavin.sharp)
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...
(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.
(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.
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.
(In reply to comment #19)
> Uncaught exceptions should still show up in the error console.

Exactly.
Attached patch v4 (obsolete) — Splinter Review
Attachment #462956 - Attachment is obsolete: true
Attachment #463719 - Flags: review?(dolske)
Attachment #462956 - Flags: review?(gavin.sharp)
Attachment #462954 - Attachment is obsolete: true
Attachment #463721 - Attachment is obsolete: true
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 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.
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).
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.
(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 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;
(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 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.
(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.
(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 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.
(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
(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
Looks like the remaining item to address is the assert call order. I'll take care of that. Then should we produce another patch?
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?
Attached patch v5 (obsolete) — Splinter Review
Attachment #463719 - Attachment is obsolete: true
Attachment #464496 - Flags: review?(dao)
Attachment #463719 - Flags: review?(dolske)
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 <aza@mozilla.com>

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.
There's also NS_ASSERT provided by debug.js.
(In reply to comment #40)
> >+ * The Initial Developer of the Original Code is
> >+ * Aza Raskin <aza@mozilla.com>
> 
> 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
Dao: figure those nits will be the last? We're trying to determine viability of this making it into beta 4.
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?
Attached patch v6Splinter Review
Attachment #464496 - Attachment is obsolete: true
Attachment #464641 - Flags: review?(dao)
Attachment #464496 - Flags: review?(dao)
(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?
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 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
Attachment #464641 - Flags: review?(dao) → review+
Assignee: nobody → edilee
(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?
Status: NEW → ASSIGNED
http://hg.mozilla.org/mozilla-central/rev/77cd3fbaf973

With history:
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=77cd3fbaf973
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Summary: Land TabCandy Utility Code (iq.js) → Land TabCandy Utility Code (utils.jsm, iq.js)
Target Milestone: --- → Firefox 4.0b4
These new APIs should have had sr before landing on mozilla-central per the sr policy.  Any reason why they didn't?
Blocks: 586347
Blocks: 586355
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.
You need to log in before you can comment on or make changes to this bug.