The default bug view has changed. See this FAQ.

Why does GroupItem constructor add children before it sets its bounds?

VERIFIED FIXED in Firefox 7

Status

Firefox Graveyard
Panorama
VERIFIED FIXED
6 years ago
a year ago

People

(Reporter: mitcho, Assigned: ttaubert)

Tracking

Trunk
Firefox 7

Details

(Whiteboard: [cleanup], [inbound])

Attachments

(1 attachment, 1 obsolete attachment)

>  // ___ 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

6 years ago
bugspam
Target Milestone: Future → ---
(Assignee)

Updated

6 years ago
No longer blocks: 603789
(Assignee)

Updated

6 years ago
Blocks: 653099
(Assignee)

Comment 2

6 years ago
bugspam
No longer blocks: 653099
(Assignee)

Comment 3

6 years ago
bugspam
Blocks: 660175
(Assignee)

Updated

6 years ago
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
Whiteboard: [cleanup][investigate] → [cleanup]
Version: unspecified → Trunk
(Assignee)

Comment 4

6 years ago
Created attachment 541005 [details] [diff] [review]
patch v1
Attachment #541005 - Flags: feedback?(raymond)
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?
(Assignee)

Comment 6

6 years ago
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.
Attachment #541005 - Attachment is obsolete: true
Attachment #541009 - Flags: feedback?(raymond)
Attachment #541005 - Flags: feedback?(raymond)
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?
(Assignee)

Comment 8

6 years ago
(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! :)
Attachment #541009 - Flags: feedback?(raymond) → feedback+
(Assignee)

Updated

6 years ago
Attachment #541009 - Flags: review?(dietrich)
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?
Attachment #541009 - Flags: review?(dietrich) → review+
(Assignee)

Comment 11

6 years ago
(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.
(Assignee)

Comment 12

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/d697b0b26227
Whiteboard: [cleanup] → [cleanup], [inbound]
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.
Whiteboard: [cleanup], [inbound] → [cleanup]
(Assignee)

Comment 14

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/141336f727f0
Whiteboard: [cleanup] → [cleanup], [inbound]
http://hg.mozilla.org/mozilla-central/rev/141336f727f0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
The modifications were updated on mozilla-central repository.
Setting as Verified Fixed.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.