Closed
Bug 631649
Opened 12 years ago
Closed 12 years ago
CSS error -moz-column-count since bug 595965 landed
Categories
(Firefox Graveyard :: Panorama, defect)
Firefox Graveyard
Panorama
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)
5.96 KB,
patch
|
Dolske
:
approval2.0+
|
Details | Diff | Splinter Review |
We need to fix the -moz-column-count error that keeps showing up in the error console since bug 595965 landed.
Assignee | ||
Comment 1•12 years ago
|
||
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!
Comment 2•12 years ago
|
||
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•12 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•12 years ago
|
||
Updated patch, with a better fix.
Attachment #509869 -
Attachment is obsolete: true
Attachment #510024 -
Flags: review?(ian)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [patchclean:0205]
Comment 5•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
Whiteboard: [patchclean:0205] → [patchclean:0207]
Comment 7•12 years ago
|
||
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+
Assignee | ||
Comment 8•12 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•12 years ago
|
||
Fixed the comment.
Attachment #510355 -
Attachment is obsolete: true
Assignee | ||
Comment 10•12 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?
Comment 11•12 years ago
|
||
(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.
Comment 12•12 years ago
|
||
Didn't see try results link, so sent to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=1f4b4606ffde
Comment 13•12 years ago
|
||
(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 :)
Comment 14•12 years ago
|
||
(In reply to comment #13) > Passed try perfectly, please approve :) WOW! That's impressively green! :D
Updated•12 years ago
|
Attachment #510563 -
Flags: approval2.0? → approval2.0+
Comment 16•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/44dac293dee8
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12
Updated•12 years ago
|
Keywords: checkin-needed
Updated•12 years ago
|
Whiteboard: [patchclean:0207] → [patchclean:0207][qa-]
Updated•7 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•