Closed Bug 631649 Opened 10 years ago Closed 9 years ago

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

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 4.0b12

People

(Reporter: msucan, Assigned: msucan)

References

Details

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

Attachments

(1 file, 3 obsolete files)

We need to fix the -moz-column-count error that keeps showing up in the error console since bug 595965 landed.
Attached 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-
(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?
Attached patch updated fix (obsolete) — Splinter Review
Updated patch, with a better fix.
Attachment #509869 - Attachment is obsolete: true
Attachment #510024 - Flags: review?(ian)
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-
Attached 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)
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
(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+!
Attached patch patch update 3Splinter Review
Fixed the comment.
Attachment #510355 - Attachment is obsolete: true
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+
Thanks for the approval Dolske!
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/44dac293dee8
Status: ASSIGNED → RESOLVED
Closed: 9 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.