> // ___ Children > Array.prototype.forEach.call(listOfEls, function(el) { > self.add(el, options); > }); > > // ___ Finish Up > this._addHandlers($container); > > this.setResizable(true, immediately); > > GroupItems.register(this); > > // ___ Position > this.setBounds(rectToBe, immediately); So, if we create a new group with some children, we seem to add those children, sometimes with immediately=true, and *then* set the bounds on the group itself. It's possible that this means that the child tabs actually get setBounds-ed twice during this type of a call. Investigate this.

bugspam

bugspam

bugspam

Comment on attachment 541005 [details] [diff] [review] patch v1 >+ let addOptions = Utils.merge(options, {dontArrange: true}); > Array.prototype.forEach.call(listOfEls, function(el) { >- self.add(el, options); >+ self.add(el, addOptions); > }); Can you replace "Array.prototype.forEach.call(listOfEls, " with "listOfEls.forEach(" while you're at it?

(In reply to comment #5) > Can you replace "Array.prototype.forEach.call(listOfEls, " with > "listOfEls.forEach(" while you're at it? Of course, good catch.

Comment on attachment 541009 [details] [diff] [review] patch v2 I don't see how this patch can prevent the child tabs get setBounds-ed twice mentioned in comment 0. Could you explain please?

(In reply to comment #7) > I don't see how this patch can prevent the child tabs get setBounds-ed twice > mentioned in comment 0. Could you explain please? The call order is: groupItem.add(tabItem) [add a tab] -> groupItem.arrange() [position that new tab in the group] -> tabItem.setBounds() [for every child] With {dontArrange: true} we prevent the second step [the call to .arrange()]. So when a groupItem is initialized with 3 children we're even saving 6 TabItem.setBounds() calls here, not to forget the three GroupItem.arrange() calculations...

Comment on attachment 541009 [details] [diff] [review] patch v2 Thanks for the explanation! :)

Comment on attachment 541009 [details] [diff] [review] patch v2 Review of attachment 541009 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits fixed. ::: browser/base/content/tabview/groupitems.js @@ +255,2 @@ > }); > even though you gave your explanation in the bug, this is non-obvious to someone who's only reading the code. please add a comment here about why it's important to use dontArrange. think of the future generations of Firefox developers ;) nit: do you ever want callers to be able to do dontArrange=false in GroupItem ctor? if no, then merge() seems overkill here. just set the property in options itself. question not really related: have dontArrange default to false seems like a potential footgun due to side effects. why not default to true, and put the onus on the caller to trigger arrange manually? are there too many call sites for this approach?

(In reply to comment #10) > even though you gave your explanation in the bug, this is non-obvious to > someone who's only reading the code. please add a comment here about why > it's important to use dontArrange. think of the future generations of > Firefox developers ;) Yeah, good idea, done. > nit: do you ever want callers to be able to do dontArrange=false in > GroupItem ctor? if no, then merge() seems overkill here. just set the > property in options itself. Fixed. > question not really related: have dontArrange default to false seems like a > potential footgun due to side effects. why not default to true, and put the > onus on the caller to trigger arrange manually? are there too many call > sites for this approach? There are actually many call sites that rely on groups' children being re-arranged when adding a new child. There are only rare cases where we don't want a group to re-arrange (like here) - so IMHO the default behavior is correct.

backed out from mozilla-inbound because part of a push that increased number of random failures in Panorama browser-chrome tests. Please reland in smaller chunks when ready.

The modifications were updated on mozilla-central repository. Setting as Verified Fixed.