CSS error -moz-column-count since bug 595965 landed

RESOLVED FIXED in Firefox 4.0b12

Status

defect
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: msucan, Assigned: msucan)

Tracking

Trunk
Firefox 4.0b12
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patchclean:0207][qa-])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

8 years ago
We need to fix the -moz-column-count error that keeps showing up in the error console since bug 595965 landed.
(Assignee)

Comment 1

8 years ago
Posted patch proposed fix (obsolete) — Splinter Review
Fix for the error. Didn't notice it when I was working on bug 595965 - had Error console filter set to only show JS errors, hehe. Sorry!
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Attachment #509869 - Flags: review?(ian)
Comment on attachment 509869 [details] [diff] [review]
proposed fix

>+    // Sometimes we get wrong icon and box bounds, which means we also get the
>+    // numbers of rows, columns and maxColumns wrong. When that happens, we have
>+    // to stop.
>+    if (rows < 1 || columns < 1 || maxColumns < 1)
>+      return;

How does this happen? The comment, as written, sounds like "sometimes our code screws up randomly, and rather than figure it out we just bail", but I assume that's not what you really mean.
Attachment #509869 - Flags: review?(ian) → review-
(Assignee)

Comment 3

8 years ago
(In reply to comment #2)
> Comment on attachment 509869 [details] [diff] [review]
> proposed fix
> 
> >+    // Sometimes we get wrong icon and box bounds, which means we also get the
> >+    // numbers of rows, columns and maxColumns wrong. When that happens, we have
> >+    // to stop.
> >+    if (rows < 1 || columns < 1 || maxColumns < 1)
> >+      return;
> 
> How does this happen? The comment, as written, sounds like "sometimes our code
> screws up randomly, and rather than figure it out we just bail", but I assume
> that's not what you really mean.

This happens when the GroupItem constructor adds all of the app tabs. During construction the bounds are not available.

I can fix that by checking this._inited in addAppTab(). Will submit an updated patch.

I think the check in adjustAppTabTray() should still stay, because there might be other unknown cases when the bounds are not available, and the call to this method is superfluous anyway. If we want to know those cases, we can make the error fatal by throwing an exception. Thoughts?
(Assignee)

Comment 4

8 years ago
Posted patch updated fix (obsolete) — Splinter Review
Updated patch, with a better fix.
Attachment #509869 - Attachment is obsolete: true
Attachment #510024 - Flags: review?(ian)
(Assignee)

Updated

8 years ago
Whiteboard: [patchclean:0205]
Comment on attachment 510024 [details] [diff] [review]
updated fix

I think I'd rather you modify the addAppTab call in GroupItem's constructor like so:

  self.addAppTab(xulTab, {dontAdjustTray: true});
  
... since that's the problem area. 

If you're still concerned about the numbers being wrong for some other reason, go ahead and put an assert in; I don't want it just failing silently.

Thank you.
Attachment #510024 - Flags: review?(ian) → review-
(Assignee)

Comment 6

8 years ago
Posted patch patch update 2 (obsolete) — Splinter Review
Updated the patch according to your review. Thanks!

Please let me know if you want further changes.
Attachment #510024 - Attachment is obsolete: true
Attachment #510355 - Flags: review?(ian)
(Assignee)

Updated

8 years ago
Whiteboard: [patchclean:0205] → [patchclean:0207]
Comment on attachment 510355 [details] [diff] [review]
patch update 2

Looks great, thanks!

>   // Adds the given xul:tab as an app tab in this group's apptab tray
>+  // options
>+  //
>+  // Parameters:
>+  //   options - change how the app tab is added.
>+  //
>+  // Options:
>+  //   dontAdjustTray - (boolean) if true, the $appTabTray size is not adjusted,
>+  //                    which means that the adjustAppTabTray() method is not
>+  //                    called.

Thank you for documenting the options so nicely! I don't think you meant to have that lone "options" line at the top, though.

R+ with that.
Attachment #510355 - Flags: review?(ian) → review+
Blocks: 631730
(Assignee)

Comment 8

8 years ago
(In reply to comment #7)
> Comment on attachment 510355 [details] [diff] [review]
> patch update 2
> 
> Looks great, thanks!
> 
> >   // Adds the given xul:tab as an app tab in this group's apptab tray
> >+  // options
> >+  //
> >+  // Parameters:
> >+  //   options - change how the app tab is added.
> >+  //
> >+  // Options:
> >+  //   dontAdjustTray - (boolean) if true, the $appTabTray size is not adjusted,
> >+  //                    which means that the adjustAppTabTray() method is not
> >+  //                    called.
> 
> Thank you for documenting the options so nicely! I don't think you meant to
> have that lone "options" line at the top, though.
> 
> R+ with that.

Hah, that slipped by mistake. Thanks for the review+!
(Assignee)

Comment 9

8 years ago
Fixed the comment.
Attachment #510355 - Attachment is obsolete: true
(Assignee)

Comment 10

8 years ago
Comment on attachment 510563 [details] [diff] [review]
patch update 3

Asking for approval2.0+, this patch fixes a CSS error that shows up quite often when Panorama is used. Thanks!
Attachment #510563 - Flags: approval2.0?
(In reply to comment #10)
> Comment on attachment 510563 [details] [diff] [review]
> patch update 3
> 
> Asking for approval2.0+, this patch fixes a CSS error that shows up quite often
> when Panorama is used. Thanks!

+1. Looks very minimal risk to me, and does fix (very unfortunate) error log spam.
(In reply to comment #12)
> Didn't see try results link, so sent to try:
> http://tbpl.mozilla.org/?tree=MozillaTry&rev=1f4b4606ffde

Passed try perfectly, please approve :)
Blocks: 595965
(In reply to comment #13)
> Passed try perfectly, please approve :)

WOW! That's impressively green! :D
Attachment #510563 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 15

8 years ago
Thanks for the approval Dolske!
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/44dac293dee8
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12
Whiteboard: [patchclean:0207] → [patchclean:0207][qa-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.