Items.arrange gets called in a number of contexts: 1) from GroupItem.shouldStack, where we simply want to know what the child width is 2) from GroupItem.arrange, when expanded, when we want it to actually move the children 3) from GroupItem.arrange, after running shouldStack, actually getting the boxes to arrange them. In particular, in GroupItem.arrange, if we end up not stacking, we'll have called Items.arrange twice: once to establish that we should not stack, and then again to actually arrange the items. So we (a) have some redundant code between GroupItems.arrange and Items.arrange and (b) are actually working too hard in a few situations. I propose the following optimizations: 1. Replace the "pretend" option with a "return" option which can be "widthAndColumns", where we simply return an object with the tab width size and the number of columns of tabs we require, without computing the boxes at all (which we currently are doing, and then not using) 2. In GroupItem.shouldStack, save the return value from Items.arrange in widthAndColumns mode as the GroupItem's _columns and _shouldStack parameters. 3. Add a "columns" option to Items.arrange so we can specify a columns number and bypass the computation, if we already have a value for it. 4. In GroupItem.arrange, if we're doing a non-stacking arrangement, just let Items.arrange do the arrangement instead of doing it ourselves based on the returned boxes. 5. Have Items.arrange always return the computed boxes (unless in widthAndColumns mode), moving the children if children were specified (i.e., abolishing the pretend mode). Perhaps I'll think of something else down the line as well...

Comment on attachment 481755 [details] [diff] [review] Patch v1 Looks good. + // return - if set to 'childWidth', it'll simply return the width of children without + // moving any. This should say 'widthAndColumns'. - var animate; - if (!options || typeof options.animate == 'undefined') - animate = true; - else - animate = options.animate; - if (typeof options == 'undefined') options = {}; - var rects = null; - if (options.pretend) - rects = []; + var animate = options.animate || false; This changes the logic of .animate; before it was true by default, but with this code it's false by default. Is this intentional? I assume not (and if it is, it goes against the logic used for .animate elsewhere); please fix. + var immediately = !animate; + + var rects = []; You've got a tab character in here. + var columns = options.columns || 1; I suppose you could also skip the while loop that refines the columns? + for (let a = 0; a < count; a++) { + rects.push(new Rect(box)); Another tab character. F+ with those.

(In reply to comment #2) > I suppose you could also skip the while loop that refines the columns? I think just having the options.columns, if set, be a lower limit, but not necessarily fix it, is a good thing. This ensures that, if for some reason some of our data isn't in sync (as with bug 591711 pre-bug-590268-fix, where we have group data but no individual tab data) we'll never end up in a situation where we end up with a preset columns value which is actually too small for the number of children we have, in which case we would overflow the group. While we would like to ultimately make sure all our data is in sync at all times, I think this is a good, pragmatic step, and in most cases just means we keep one extra boolean check.

Stacking was broken: fixed.

(In reply to comment #3) > (In reply to comment #2) > > I suppose you could also skip the while loop that refines the columns? > > I think just having the options.columns, if set, be a lower limit, but not > necessarily fix it, is a good thing. This ensures that, if for some reason some > of our data isn't in sync (as with bug 591711 pre-bug-590268-fix, where we have > group data but no individual tab data) we'll never end up in a situation where > we end up with a preset columns value which is actually too small for the > number of children we have, in which case we would overflow the group. While we > would like to ultimately make sure all our data is in sync at all times, I > think this is a good, pragmatic step, and in most cases just means we keep one > extra boolean check. Makes sense. Perhaps it should be called .minColumns instead?

This blocks b8 as it is part of the solution for Panorama being slow to load many tabs.

Comment on attachment 482059 [details] [diff] [review] Patch v3 >+ if (typeof options.animate != 'undefined') >+ animate = options.animate; This would be a bit clearer as: if ("animate" in options) Similarly (in other places), instead of |if (typeof foo == "undefined")|, it would be a bit more efficient to do |if (foo === undefined)|. [sic, note triple-equal strict equality check.] I think you're mostly just shuffling around existing code here, so maybe look at doing this for a followup?

Comment on attachment 482059 [details] [diff] [review] Patch v3 >diff --git a/browser/base/content/tabview/groupitems.js b/browser/base/content/tabview/groupitems.js >--- a/browser/base/content/tabview/groupitems.js >+++ b/browser/base/content/tabview/groupitems.js >@@ -956,103 +956,93 @@ GroupItem.prototype = Utils.extend(new I > // But who can blame an object for being a bit optimistic when self-reporting size. > // It has to impress the ladies somehow. Bug 586549 > left: dL + childBB.width/2 - this.$expander.width()/2 - 6, > }); > }, > > // ---------- > // Function: shouldStack >- // Returns true if the groupItem, given "count", should stack (instead of grid). >+ // Returns true if the groupItem should stack (instead of grid). > shouldStack: function GroupItem_shouldStack(count) { > if (count <= 1) > return false; > > var bb = this.getContentBounds(); > var options = { >- pretend: true, >- count: count >+ return: 'widthAndColumns', >+ count: count || this._children.length > }; >+ let {childWidth, columns} = Items.arrange(null, bb, options); >+ >+ let shouldStack = childWidth < TabItems.minTabWidth * 1.35; >+ this._columns = shouldStack ? null : columns; > >- var rects = Items.arrange(null, bb, options); >- return (rects[0].width < TabItems.minTabWidth * 1.35); >+ return shouldStack; > }, > > // ---------- > // Function: arrange > // Lays out all of the children. > // > // Parameters: > // options - passed to <Items.arrange> or <_stackArrange> > arrange: function GroupItem_arrange(options) { > if (this.expanded) { > this.topChild = null; > var box = new Rect(this.expanded.bounds); > box.inset(8, 8); > Items.arrange(this._children, box, Utils.extend({}, options, {z: 99999})); > } else { > var bb = this.getContentBounds(); >- var count = this._children.length; >- if (!this.shouldStack(count)) { >- var animate; >- if (!options || typeof options.animate == 'undefined') >- animate = true; >- else >- animate = options.animate; >- >+ if (!this.shouldStack()) { > if (typeof options == 'undefined') > options = {}; > > this._children.forEach(function(child) { > child.removeClass("stacked") > }); > > this.topChild = null; > >- var arrangeOptions = Utils.copy(options); >- Utils.extend(arrangeOptions, { >- pretend: true, >- count: count >- }); >- >- if (!count) { >+ if (!this._children.length) { > this.xDensity = 0; > this.yDensity = 0; > return; > } >- >+ >+ var arrangeOptions = Utils.copy(options); >+ Utils.extend(arrangeOptions, { >+ columns: this._columns >+ }); >+ >+ // Items.arrange will rearrange the children, but also return an array >+ // of the Rect's used. > var rects = Items.arrange(this._children, bb, arrangeOptions); > >- // yDensity = (the distance of the bottom of the last tab to the top of the content area) >+ // yDensity = >+ // (the distance of the bottom of the last tab to the top of the content area) > // / (the total available content height) > this.yDensity = (rects[rects.length - 1].bottom - bb.top) / (bb.height); > >- // xDensity = (the distance from the left of the content area to the right of the rightmost >+ // xDensity = >+ // (the distance from the left of the content area to the right of the rightmost > // tab) / (the total available content width) > > // first, find the right of the rightmost tab! luckily, they're in order. > // TODO: does this change for rtl? > var rightMostRight = 0; > for each (var rect in rects) { > if (rect.right > rightMostRight) > rightMostRight = rect.right; > else > break; > } > this.xDensity = (rightMostRight - bb.left) / (bb.width); > >- this._children.forEach(function(child, index) { >- if (!child.locked.bounds) { >- child.setBounds(rects[index], !animate); >- child.setRotation(0); >- if (options.z) >- child.setZ(options.z); >- } >- }); >- > this._isStacked = false; > } else > this._stackArrange(bb, options); > } > > if (this._isStacked && !this.expanded) this.showExpandControl(); > else this.hideExpandControl(); > }, >diff --git a/browser/base/content/tabview/items.js b/browser/base/content/tabview/items.js >--- a/browser/base/content/tabview/items.js >+++ b/browser/base/content/tabview/items.js >@@ -590,17 +590,17 @@ Item.prototype = { > var startSent; > var startEvent; > var droppables; > var dropTarget; > > // ___ mousemove > var handleMouseMove = function(e) { > // positioning >- var mouse = new Point(e.pageX, e.pageY); >+ var mouse = new Point(e.pageX, e.pageY); > if (!startSent) { > if(Math.abs(mouse.x - startMouse.x) > self.dragOptions.minDragDistance || > Math.abs(mouse.y - startMouse.y) > self.dragOptions.minDragDistance) { > if (typeof self.dragOptions.start == "function") > self.dragOptions.start.apply(self, > [startEvent, {position: {left: startPos.x, top: startPos.y}}]); > startSent = true; > } >@@ -883,49 +883,51 @@ let Items = { > }, > > // ---------- > // Function: arrange > // Arranges the given items in a grid within the given bounds, > // maximizing item size but maintaining standard tab aspect ratio for each > // > // Parameters: >- // items - an array of <Item>s. Can be null if the pretend and count options are set. >+ // items - an array of <Item>s. Can be null, in which case we won't >+ // actually move anything. > // bounds - a <Rect> defining the space to arrange within > // options - an object with various properites (see below) > // > // Possible "options" properties: > // animate - whether to animate; default: true. > // z - the z index to set all the items; default: don't change z. >- // pretend - whether to collect and return the rectangle rather than moving the items; default: false >- // count - overrides the item count for layout purposes; default: the actual item count >+ // return - if set to 'widthAndColumns', it'll return an object with the >+ // width of children and the columns. >+ // count - overrides the item count for layout purposes; >+ // default: the actual item count > // padding - pixels between each item >+ // columns - (int) a preset number of columns to use > // > // Returns: >- // the list of rectangles if the pretend option is set; otherwise null >+ // an object with the width value of the child items and the number of columns, >+ // if the return option is set to 'widthAndColumns'; otherwise the list of <Rect>s > arrange: function Items_arrange(items, bounds, options) { >- var animate; >- if (!options || typeof options.animate == 'undefined') >- animate = true; >- else >- animate = options.animate; >- > if (typeof options == 'undefined') > options = {}; > >- var rects = null; >- if (options.pretend) >- rects = []; >+ var animate = true; >+ if (typeof options.animate != 'undefined') >+ animate = options.animate; >+ var immediately = !animate; >+ >+ var rects = []; > > var tabAspect = TabItems.tabHeight / TabItems.tabWidth; > var count = options.count || (items ? items.length : 0); > if (!count) > return rects; > >- var columns = 1; >+ var columns = options.columns || 1; > // We'll assume for the time being that all the items have the same styling > // and that the margin is the same width around. > var itemMargin = items && items.length ? > parseInt(iQ(items[0].container).css('margin-left')) : 0; > var padding = itemMargin * 2; > var yScale = 1.1; // to allow for titles > var rows; > var tabWidth; >@@ -945,45 +947,41 @@ let Items = { > columns++; > figure(); > } > > if (rows == 1) { > tabWidth = Math.min(tabWidth, (bounds.height - 2 * itemMargin) / tabAspect); > tabHeight = tabWidth * tabAspect; > } >+ >+ if (options.return == 'widthAndColumns') >+ return {childWidth: tabWidth, columns: columns}; > > var box = new Rect(bounds.left, bounds.top, tabWidth, tabHeight); >- var row = 0; > var column = 0; >- var immediately; > >- var a; >- for (a = 0; a < count; a++) { >- immediately = !animate; >- >- if (rects) >- rects.push(new Rect(box)); >- else if (items && a < items.length) { >- var item = items[a]; >+ for (let a = 0; a < count; a++) { >+ rects.push(new Rect(box)); >+ if (items && a < items.length) { >+ let item = items[a]; > if (!item.locked.bounds) { > item.setBounds(box, immediately); > item.setRotation(0); > if (options.z) > item.setZ(options.z); > } > } > > box.left += box.width + padding; > column++; > if (column == columns) { > box.left = bounds.left; > box.top += (box.height * yScale) + padding; > column = 0; >- row++; > } > } > > return rects; > }, > > // ---------- > // Function: unsquish

Requesting approval2.0. Note this bug is part of the meta bug 591704, which is blocking betaN+.

(In reply to comment #8) > Comment on attachment 482059 [details] [diff] [review] > Patch v3 > > >+ if (typeof options.animate != 'undefined') > >+ animate = options.animate; > > This would be a bit clearer as: if ("animate" in options) > > Similarly (in other places), instead of |if (typeof foo == "undefined")|, it > would be a bit more efficient to do |if (foo === undefined)|. [sic, note > triple-equal strict equality check.] > > I think you're mostly just shuffling around existing code here, so maybe look > at doing this for a followup? Followup: bug 606824. Sorry all about the horrible long comment above. :'(

Requesting blocking (as the meta bug is already blocking)

Please prep for check in (including try test if still needed), Mitcho.

Pushed to try just in case as I had to battle some patch rot. Try rev 9728e21406d9.

Passed try! Checkin needed.

Landed: http://hg.mozilla.org/mozilla-central/rev/f11c25226916 Mitcho, the commit message needs to literally have "a=" somewhere in it, otherwise the bot on the server cancels the push. I fixed it for this patch, but just so you know for future reference.

(In reply to comment #16) > Mitcho, the commit message needs to literally have "a=" somewhere in it, > otherwise the bot on the server cancels the push. I fixed it for this patch, > but just so you know for future reference. Good to know. Thanks!