Last Comment Bug 632293 - Why does GroupItem constructor add children before it sets its bounds?
: Why does GroupItem constructor add children before it sets its bounds?
Status: VERIFIED FIXED
[cleanup], [inbound]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 7
Assigned To: Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
:
Mentors:
Depends on:
Blocks: 660175
  Show dependency treegraph
 
Reported: 2011-02-07 20:25 PST by Michael Yoshitaka Erlewine [:mitcho]
Modified: 2016-04-12 14:00 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (758 bytes, patch)
2011-06-22 03:18 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
no flags Details | Diff | Splinter Review
patch v2 (795 bytes, patch)
2011-06-22 03:40 PDT, Tim Taubert [:ttaubert] (on PTO, back Aug 29th)
dietrich: review+
raymond: feedback+
Details | Diff | Splinter Review

Description Michael Yoshitaka Erlewine [:mitcho] 2011-02-07 20:25:00 PST
>  // ___ 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.
Comment 1 Kevin Hanes 2011-03-31 10:51:48 PDT
bugspam
Comment 2 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-05-27 02:22:47 PDT
bugspam
Comment 3 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-05-27 02:27:40 PDT
bugspam
Comment 4 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-22 03:18:42 PDT
Created attachment 541005 [details] [diff] [review]
patch v1
Comment 5 Dão Gottwald [:dao] 2011-06-22 03:28:58 PDT
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?
Comment 6 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-22 03:40:36 PDT
Created attachment 541009 [details] [diff] [review]
patch v2

(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 7 Raymond Lee [:raymondlee] 2011-06-22 07:32:07 PDT
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?
Comment 8 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-22 07:46:43 PDT
(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 9 Raymond Lee [:raymondlee] 2011-06-22 07:50:27 PDT
Comment on attachment 541009 [details] [diff] [review]
patch v2

Thanks for the explanation! :)
Comment 10 Dietrich Ayala (:dietrich) 2011-06-22 09:03:49 PDT
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?
Comment 11 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-23 07:52:44 PDT
(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.
Comment 12 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-24 01:31:41 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/d697b0b26227
Comment 13 Marco Bonardo [::mak] 2011-06-24 05:52:45 PDT
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.
Comment 14 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-06-27 14:44:57 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/141336f727f0
Comment 15 Marco Bonardo [::mak] 2011-06-28 02:28:24 PDT
http://hg.mozilla.org/mozilla-central/rev/141336f727f0
Comment 16 Virgil Dicu [:virgil] [QA] 2011-08-31 04:36:48 PDT
The modifications were updated on mozilla-central repository.
Setting as Verified Fixed.

Note You need to log in before you can comment on or make changes to this bug.