From 2a95affe4dabcd107f8a1356359d4ab594a56392 Mon Sep 17 00:00:00 2001
From: Ian Gilman <iangilman@gmail.com>
Date: Tue, 11 May 2010 12:03:31 -0700
Subject: [PATCH] + Fixed an extremely subtle bug that would cause groups that
 you hadn't manually resized to start falling apart after a refresh. + Added
 sanity checks on data going to and from storage (still some work to be done
 there); if the check doesn't pass, the data isn't loaded/saved + Added a
 manual save option to the dev menu + Added Utils.isNumber, as well as
 standalone isRect and isPoint routines + Miscellaneous double checks and
 assertions

---
 browser/base/content/tabcandy/app/groups.js   | 36 ++++++++++--
 browser/base/content/tabcandy/app/items.js    | 12 +++-
 browser/base/content/tabcandy/app/tabitems.js | 29 +++++++++-
 browser/base/content/tabcandy/app/ui.js       | 58 ++++++++++++++-----
 browser/base/content/tabcandy/core/utils.js   | 33 ++++++++---
 5 files changed, 135 insertions(+), 33 deletions(-)

diff --git a/browser/base/content/tabcandy/app/groups.js b/browser/base/content/tabcandy/app/groups.js
index 3afc2539188e..1ebed36427b4 100644
--- a/browser/base/content/tabcandy/app/groups.js
+++ b/browser/base/content/tabcandy/app/groups.js
@@ -38,13 +38,13 @@ window.Group = function(listOfEls, options) {
   this.defaultSize = new Point(TabItems.tabWidth * 1.5, TabItems.tabHeight * 1.5);
   this.isAGroup = true;
   this.id = options.id || Groups.getNextID();
-  this.userSize = options.userSize || null;
   this._isStacked = false;
   this._stackAngles = [0];
   this.expanded = null;
+  this.locked = (options.locked ? $.extend({}, options.locked) : {});
   
-  if(typeof(options.locked) == 'object')  
-    this.locked = $.extend({}, options.locked);
+  if(isPoint(options.userSize))  
+    this.userSize = new Point(options.userSize);
 
   var self = this;
 
@@ -231,12 +231,15 @@ window.Group.prototype = $.extend(new Item(), new Subscribable(), {
   getStorageData: function() {
     var data = {
       bounds: this.getBounds(), 
-      userSize: $.extend({}, this.userSize),
+      userSize: null,
       locked: $.extend({}, this.locked), 
       title: this.getTitle(),
       id: this.id
     };
     
+    if(isPoint(this.userSize))  
+      data.userSize = new Point(this.userSize);
+  
     return data;
   },
   
@@ -297,6 +300,11 @@ window.Group.prototype = $.extend(new Item(), new Subscribable(), {
   
   // ----------  
   setBounds: function(rect, immediately) {
+    if(!isRect(rect)) {
+      Utils.trace('Group.setBounds: rect is not a real rectangle!', rect);
+      return;
+    }
+    
     var titleHeight = this.$titlebar.height();
     
     // ___ Determine what has changed
@@ -358,6 +366,9 @@ window.Group.prototype = $.extend(new Item(), new Subscribable(), {
     this.adjustTitleSize();
 
     this._updateDebugBounds();
+
+    if(!isRect(this.bounds))
+      Utils.trace('Group.setBounds: this.bounds is not a real rectangle!', this.bounds);
   },
   
   // ----------
@@ -1127,6 +1138,23 @@ window.Groups = {
     } 
   },
   
+  // ----------
+  storageSanity: function(data) {
+    // TODO: check everything 
+    if(!data.groups)
+      return false;
+      
+    var sane = true;
+    $.each(data.groups, function(index, group) {
+      if(!isRect(group.bounds)) {
+        Utils.log('Groups.storageSanity: bad bounds', group.bounds);
+        sane = false;
+      }
+    });
+    
+    return sane;
+  },
+  
   // ----------
   getNewTabGroup: function() {
     var groupTitle = 'New Tabs';
diff --git a/browser/base/content/tabcandy/app/items.js b/browser/base/content/tabcandy/app/items.js
index 9c3c01108913..88be6821c0df 100644
--- a/browser/base/content/tabcandy/app/items.js
+++ b/browser/base/content/tabcandy/app/items.js
@@ -13,6 +13,7 @@
 //   removeOnClose: function(referenceObject)
 // ... and this property: 
 //   defaultSize, a Point
+//   <locked>, an object 
 // Make sure to call _init() from your subclass's constructor. 
 window.Item = function() {
   // Variable: isAnItem
@@ -42,7 +43,7 @@ window.Item = function() {
   //   .bounds is true if it can't be pushed, dragged, resized, etc
   //   .close is true if it can't be closed
   //   .title is true if it can't be renamed
-  this.locked = {};
+  this.locked = null;
   
   // Variable: parent
   // The group that this item is a child of
@@ -69,7 +70,8 @@ window.Item.prototype = {
     Utils.assert('Subclass must provide close', typeof(this.close) == 'function');
     Utils.assert('Subclass must provide addOnClose', typeof(this.addOnClose) == 'function');
     Utils.assert('Subclass must provide removeOnClose', typeof(this.removeOnClose) == 'function');
-    Utils.assert('Subclass must provide defaultSize', this.defaultSize);
+    Utils.assert('Subclass must provide defaultSize', isPoint(this.defaultSize));
+    Utils.assert('Subclass must provide locked', this.locked);
     
     this.container = container;
     
@@ -93,6 +95,7 @@ window.Item.prototype = {
   // Function: getBounds
   // Returns a copy of the Item's bounds as a <Rect>.
   getBounds: function() {
+    Utils.assert('this.bounds', isRect(this.bounds));
     return new Rect(this.bounds);    
   },
   
@@ -106,6 +109,7 @@ window.Item.prototype = {
   //   immediately - if false or omitted, animates to the new position;
   //   otherwise goes there immediately
   setPosition: function(left, top, immediately) {
+    Utils.assert('this.bounds', isRect(this.bounds));
     this.setBounds(new Rect(left, top, this.bounds.width, this.bounds.height), immediately);
   },
 
@@ -119,6 +123,7 @@ window.Item.prototype = {
   //   immediately - if false or omitted, animates to the new size;
   //   otherwise resizes immediately
   setSize: function(width, height, immediately) {
+    Utils.assert('this.bounds', isRect(this.bounds));
     this.setBounds(new Rect(this.bounds.left, this.bounds.top, width, height), immediately);
   },
 
@@ -126,6 +131,7 @@ window.Item.prototype = {
   // Function: setUserSize
   // Remembers the current size as one the user has chosen. 
   setUserSize: function() {
+    Utils.assert('this.bounds', isRect(this.bounds));
     this.userSize = new Point(this.bounds.width, this.bounds.height);
   },
   
@@ -530,7 +536,7 @@ window.Items = {
       var newBounds = new Rect(bounds);
 
       var newSize;
-      if(item.userSize) 
+      if(isPoint(item.userSize)) 
         newSize = new Point(item.userSize);
       else
         newSize = new Point(TabItems.tabWidth, TabItems.tabHeight);
diff --git a/browser/base/content/tabcandy/app/tabitems.js b/browser/base/content/tabcandy/app/tabitems.js
index de548c618c2e..ef9112a1ec03 100644
--- a/browser/base/content/tabcandy/app/tabitems.js
+++ b/browser/base/content/tabcandy/app/tabitems.js
@@ -1,7 +1,10 @@
 // ##########
 window.TabItem = function(container, tab) {
   this.defaultSize = new Point(TabItems.tabWidth, TabItems.tabHeight);
+  this.locked = {};
+
   this._init(container);
+
   this._hasBeenDrawn = false;
   this.tab = tab;
   this.setResizable(true);
@@ -11,8 +14,8 @@ window.TabItem.prototype = $.extend(new Item(), {
   // ----------  
   getStorageData: function() {
     return {
-      bounds: this.bounds, 
-      userSize: this.userSize,
+      bounds: this.getBounds(), 
+      userSize: (isPoint(this.userSize) ? new Point(this.userSize) : null),
       url: this.tab.url,
       groupID: (this.parent ? this.parent.id : 0)
     };
@@ -383,6 +386,23 @@ window.TabItems = {
     }
   },
   
+  // ----------
+  storageSanity: function(data) {
+    // TODO: check everything 
+    if(!data.tabs)
+      return false;
+      
+    var sane = true;
+    $.each(data.tabs, function(index, tab) {
+      if(!isRect(tab.bounds)) {
+        Utils.log('TabItems.storageSanity: bad bounds', tab.bounds);
+        sane = false;
+      }
+    });
+    
+    return sane;
+  },
+
   // ----------
   reconnect: function(item) {
     if(item.reconnected)
@@ -400,7 +420,10 @@ window.TabItems = {
             item.parent.remove(item);
             
           item.setBounds(tab.bounds, true);
-          item.userSize = tab.userSize;
+          
+          if(isPoint(tab.userSize))
+            item.userSize = new Point(tab.userSize);
+            
           if(tab.groupID) {
             var group = Groups.group(tab.groupID);
             group.add(item);
diff --git a/browser/base/content/tabcandy/app/ui.js b/browser/base/content/tabcandy/app/ui.js
index 9cd208d0d578..e2803005b9ce 100644
--- a/browser/base/content/tabcandy/app/ui.js
+++ b/browser/base/content/tabcandy/app/ui.js
@@ -351,27 +351,22 @@ function UIClass(){
   
   // ___ Storage
   var data = Storage.read();
-  this.storageSanity(data);
-    
-  if(data.dataVersion < 2) {
+  var sane = this.storageSanity(data);
+  if(!sane || data.dataVersion < 2) {
     data.groups = null;
     data.tabs = null;
+    data.pageBounds = null;
+    
+    if(!sane)
+      alert('storage data is bad; starting fresh');
   }
-      
+   
   Groups.reconstitute(data.groups);
   TabItems.reconstitute(data.tabs);
   
   $(window).bind('beforeunload', function() {
-    if(self.initialized) {
-      var data = {
-        dataVersion: 2,
-        groups: Groups.getStorageData(),
-        tabs: TabItems.getStorageData(), 
-        pageBounds: Items.getPageBounds()
-      };
-      
-      Storage.write(data);
-    }
+    if(self.initialized) 
+      self.save();
   });
   
   // ___ resizing
@@ -464,12 +459,15 @@ UIClass.prototype = {
   
   // ----------
   addDevMenu: function() {
+    var self = this;
+    
     var html = '<select style="position:absolute; top:5px;">'; 
     var $select = $(html)
       .appendTo('body')
       .change(function () {
         var index = $(this).val();
         commands[index].code();
+        $(this).val(0);
       });
       
     var commands = [{
@@ -481,6 +479,11 @@ UIClass.prototype = {
       code: function() {
         location.href = '../../index.html';
       }
+    }, {
+      name: 'save', 
+      code: function() {
+        self.save();
+      }
     }];
       
     var count = commands.length;
@@ -496,11 +499,36 @@ UIClass.prototype = {
     }
   },
 
+  // ----------
+  save: function() {  
+    var data = {
+      dataVersion: 2,
+      groups: Groups.getStorageData(),
+      tabs: TabItems.getStorageData(), 
+      pageBounds: Items.getPageBounds()
+    };
+    
+    if(this.storageSanity(data))
+      Storage.write(data);
+    else
+      alert('storage data is bad; reverting to previous version');
+  },
+
   // ----------
   storageSanity: function(data) {
+    var sane = true;
     if(data) {
-      // TODO: cleanliness check
+      sane = sane && typeof(data.dataVersion) == 'number';
+      sane = sane && isRect(data.pageBounds);
+      
+      if(data.tabs)
+        sane = sane && TabItems.storageSanity(data.tabs);
+        
+      if(data.groups)
+        sane = sane && Groups.storageSanity(data.groups);
     }
+    
+    return sane;
   },
   
   // ----------
diff --git a/browser/base/content/tabcandy/core/utils.js b/browser/base/content/tabcandy/core/utils.js
index 7fc9c1bd119d..1eb313a4591c 100644
--- a/browser/base/content/tabcandy/core/utils.js
+++ b/browser/base/content/tabcandy/core/utils.js
@@ -27,14 +27,19 @@ var extensionManager = Cc["@mozilla.org/extensions/manager;1"]
 // and creates a Point with it along with y. If either a or y are omitted, 
 // 0 is used in their place. 
 window.Point = function(a, y) {
-  if(a && typeof(a.x) != 'undefined' && typeof(a.y) != 'undefined') {
+  if(isPoint(a)) {
     this.x = a.x;
     this.y = a.y;
   } else {
-    this.x = (typeof(a) == 'undefined' ? 0 : a);
-    this.y = (typeof(y) == 'undefined' ? 0 : y);
+    this.x = (Utils.isNumber(a) ? a : 0);
+    this.y = (Utils.isNumber(y) ? y : 0);
   }
-}
+};
+
+// ----------
+window.isPoint = function(p) {
+  return (p && Utils.isNumber(p.x) && Utils.isNumber(p.y));
+};
 
 window.Point.prototype = { 
   // ---------- 
@@ -59,8 +64,7 @@ window.Point.prototype = {
 // and creates a Rect with it along with top, width, and height. 
 window.Rect = function(a, top, width, height) {
   // Note: perhaps 'a' should really be called 'rectOrLeft'
-  if(typeof(a.left) != 'undefined' && typeof(a.top) != 'undefined'
-      && typeof(a.width) != 'undefined' && typeof(a.height) != 'undefined') {
+  if(isRect(a)) {
     this.left = a.left;
     this.top = a.top;
     this.width = a.width;
@@ -71,7 +75,15 @@ window.Rect = function(a, top, width, height) {
     this.width = width;
     this.height = height;
   }
-}
+};
+
+// ----------
+window.isRect = function(r) {
+  return (Utils.isNumber(r.left)
+      && Utils.isNumber(r.top)
+      && Utils.isNumber(r.width)
+      && Utils.isNumber(r.height));
+};
 
 window.Rect.prototype = {
   // ----------
@@ -512,7 +524,12 @@ var Utils = {
   isDOMElement: function(object) {
     // TODO: need more robust way 
     return (object && typeof(object.tagName) != 'undefined' ? true : false);
-  }   
+  },
+ 
+  // ----------
+  isNumber: function(n) {
+    return (typeof(n) == 'number' && !isNaN(n));
+  }
 };
 
 window.Utils = Utils;