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

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Mihai Sucan [:msucan]]

We need to fix the -moz-column-count error that keeps showing up in the error console since bug 595965 landed.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed fix

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

[Ian Gilman [:iangilman]]

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.

Comment 3

[Mihai Sucan [:msucan]]

(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?

Comment 4

[Mihai Sucan [:msucan]]

Attachment: updated fix

Updated patch, with a better fix.

Comment 5

[Ian Gilman [:iangilman]]

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.

Comment 6

[Mihai Sucan [:msucan]]

Attachment: patch update 2

Updated the patch according to your review. Thanks!

Please let me know if you want further changes.

Comment 7

[Ian Gilman [:iangilman]]

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.

Comment 8

[Mihai Sucan [:msucan]]

(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+!

Comment 9

[Mihai Sucan [:msucan]]

Attachment: patch update 3

Fixed the comment.

Comment 10

[Mihai Sucan [:msucan]]

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!

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

(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

[Michael Yoshitaka Erlewine [:mitcho]]

Didn't see try results link, so sent to try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=1f4b4606ffde

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(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

[Michael Yoshitaka Erlewine [:mitcho]]

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

WOW! That's impressively green! :D

Comment 15

[Mihai Sucan [:msucan]]

Thanks for the approval Dolske!

Comment 16

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/44dac293dee8