622631 - JavaScript Warning: "Error in parsing value for 'width'/'height'. Declaration dropped." {file: "chrome://browser/content/tabview.html" line: 0}

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Michael Yoshitaka Erlewine [:mitcho]]

We see warnings like this throughout our tests, and while using Panorama. It looks like some functions, mostly zoom-related, are giving bad rects to setBounds and animate, with negative widths or heights. Most likely this means we are doing something wrong.

Comment 1

[Michael Yoshitaka Erlewine [:mitcho]]

Turns out most of these issues seem to be stemming from a problem with pushAway... investigating...

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

pushAway is the main source of errors; the other is stackArrange. Working on fixes...

Comment 3

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v1

The fix for this involved creating a general Items_enforceMinimumItemSize function and using it in a few strategic places where we were apt to creating too-small-bounds, in which case our current codebase was recording that as the item's bounds but then ignoring the CSS call: i.e., the real CSS and the bounds were not in sync. Bad news.

This patch fixes all such syncing issues. Ran tabview tests locally and produced no such warnings any more. Pushed to try as well.

Comment 4

[Ian Gilman [:iangilman]]

Comment on attachment 501265 [details] [diff] [review]
Patch v1

Thanks for tracking this down! Comments:

* Instead of an Items.enforceMinimumItemSize with switches for tab or group, it would be cleaner if .enforceMinimumSize (or even .enforceMinSize) was a method on the individual concrete classes (TabItem and GroupItem). Actually, "enforce" kind of sounds like it's going to go ahead and change the item's bounds, whereas it's just modifying its input... maybe .clampToMinSize(rect) is better.

* Looks like this patch fixes bug 609388 (which was already half fixed: the numbers are now the same, but they're duplicated as literals); we should resolve it when this lands. 

>+  // Function: enforceMinimumItemSize
>+  // Takes a <Rect> and returns that <Rect>, possibly modified in case it is too small to be
>+  // the bounds of an <Item>.

Not only does it return the rect, but it modifies the original; it's worth making that clear in the comments.

>+      bounds.width = Math.max(bounds.width, TabItems.minTabWidth);
>+      bounds.height = Math.max(bounds.height,
>+                               TabItems.minTabWidth * (TabItems.tabHeight / TabItems.tabWidth));

We should pre-calculate the minTabHeight in TabItems so we don't have to do this math every time; this code is called from high-performance areas, such as dragging, and every bit counts.

Comment 5

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #4)
> Comment on attachment 501265 [details] [diff] [review]
> Patch v1
> 
> Thanks for tracking this down! Comments:
> 
> * Instead of an Items.enforceMinimumItemSize with switches for tab or group, it
> would be cleaner if .enforceMinimumSize (or even .enforceMinSize) was a method
> on the individual concrete classes (TabItem and GroupItem). Actually, "enforce"
> kind of sounds like it's going to go ahead and change the item's bounds,
> whereas it's just modifying its input... maybe .clampToMinSize(rect) is better.

Given that this doesn't actually do something particular to that Item, I'd prefer to put it in TabItems and GroupItems. Not sure about the name .clampToMinSize, though...

> * Looks like this patch fixes bug 609388 (which was already half fixed: the
> numbers are now the same, but they're duplicated as literals); we should
> resolve it when this lands. 

Yes indeed. Cool. :)

> >+  // Function: enforceMinimumItemSize
> >+  // Takes a <Rect> and returns that <Rect>, possibly modified in case it is too small to be
> >+  // the bounds of an <Item>.
> 
> Not only does it return the rect, but it modifies the original; it's worth
> making that clear in the comments.

Indeed. Updated usage appropriately as well. Do you think keeping the return value is good?

> >+      bounds.width = Math.max(bounds.width, TabItems.minTabWidth);
> >+      bounds.height = Math.max(bounds.height,
> >+                               TabItems.minTabWidth * (TabItems.tabHeight / TabItems.tabWidth));
> 
> We should pre-calculate the minTabHeight in TabItems so we don't have to do
> this math every time; this code is called from high-performance areas, such as
> dragging, and every bit counts.

Done.

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v1.1

Comment 7

[Ian Gilman [:iangilman]]

Comment on attachment 501562 [details] [diff] [review]
Patch v1.1

(In reply to comment #5)
> Given that this doesn't actually do something particular to that Item, I'd
> prefer to put it in TabItems and GroupItems. Not sure about the name
> .clampToMinSize, though...

Good point. I was just thinking that that would save you from having to check whether it's a group or a tab in Items.arrange. I see you solved that by just assuming it's a tab, which I suppose is always true now, though it makes me a little uncomfortable, like maybe it needs an assert or a comment, or to do the test and handle both cases.

> Indeed. Updated usage appropriately as well. Do you think keeping the return
> value is good?

Actually, now that you mention it, let's kill the return; having the single usage pattern clears up confusion.
 
> > We should pre-calculate the minTabHeight in TabItems so we don't have to do
> > this math every time; this code is called from high-performance areas, such as
> > dragging, and every bit counts.
>
> let TabItems = {
>   minTabWidth: 40,
>+  minTabHeight: 30,
>   tabWidth: 160,
>   tabHeight: 120,

This is a minor point, I suppose, but writing the minTabHeight explicitly isn't DRY; it doesn't show the relationship. Better to do the math here (or in the init routine). 

R+ with the above addressed.

Comment 8

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v1.2

Fixed those points.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 502055 [details] [diff] [review]
Patch v1.2

a=beltzner

Comment 10

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin

Comment 11

[Ian Gilman [:iangilman]]

Does this need a try run?

Comment 12

[Michael Yoshitaka Erlewine [:mitcho]]

Try run of this today was botched as it was applied on an orange rev of m-c. Sending again.

Comment 13

[Michael Yoshitaka Erlewine [:mitcho]]

Passed, modulo two known intermittent oranges.

Comment 14

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/d6eac3f4a14a