499377 - Store float region on frame

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect)

Description

[fantasai]

We're currently using the float cache to store the float frame's "region" (i.e. its margin box nsRect). This should be stored on the frame itself.

Comment 1

[fantasai]

dbaron, why is mFloatManager translated minus borderPadding in the
nsBlockReflowState constructor?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So that block reflow can interact with the float manager using coordinates in the block's content rect.  A bunch of things in block reflow use content rect-based coordinates.

Comment 3

[fantasai]

Why do they use content rect-based coordinates? ("They" doesn't include any frame rects, does it?)

Comment 4

[fantasai]

Also, in this bit...

http://mxr.mozilla.org/mozilla1.9.1/source/layout/generic/nsBlockReflowState.cpp#954

  mFloatManager->AddFloat(floatFrame, region);
  // Save away the floats region in the spacemanager, after making
  // it relative to the containing block's frame instead of relative
  // to the spacemanager translation (which is inset by the
  // border+padding).
  // XXX Maybe RecoverFloats should calc/add in the borderPadding itself?
  // It's kind of confusing to have the spacemanager translation be different
  // depending on what stage of reflow we're in.
  aFloatCache->mRegion = region +
                         nsPoint(borderPadding.left, borderPadding.top);
  // If the float's dimensions have changed, note the damage in the
  // float manager.
  if (aFloatCache->mRegion != oldRegion) {
    // XXXwaterson ...
    nscoord top = NS_MIN(region.y, oldRegion.y);
    nscoord bottom = NS_MAX(region.YMost(), oldRegion.YMost());
    mFloatManager->IncludeInDamage(top, bottom);
  }

The AddFloat and IncludeInDamage calls seem to assume different coord systems. Is the IncludeInDamage call wrong, then, or am I misunderstanding something?

Comment 5

[fantasai]

Attachment: first pass

Built clobber, but I still can't get this to link. :/ Here's the error:

../generic/libgkgeneric_s.a(nsFloatManager.o): In function `nsFloatManager::StoreRegionFor(nsIFrame*, nsRect&)':
/home/fantasai/moz/trunk/layout/generic/nsFloatManager.cpp:352: undefined reference to `nsMargin::DeltaRect(nsRect&, nsRect&)'
/usr/bin/ld: libgklayout.so: hidden symbol `nsMargin::DeltaRect(nsRect&, nsRect&)' isn't defined
/usr/bin/ld: final link failed: Nonrepresentable section on output

Any idea what I'm doing wrong?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You didn't include nsMargin.cpp in the patch.

But I think I see what's going on. You need the nsMargin symbols to be exported. nsRect.h has

struct NS_GFX nsRect {

You need to add NS_GFX to nsMargin.

Comment 7

[fantasai]

Attachment: patch

Comment 8

[fantasai]

Attachment: patch

This is Part II of bug 492627. The included testcase should also cover bug 503183.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  void DeltaRect(nsRect& aFrom, nsRect& aTo);

Wouldn't it be clearer to use operator-?

In nsFloatManager::FindRegionFor, it's a bit weird that the behaviour is completely different depending on whether aMargin is null. Should we break into two functions with different names, one to get the stored region, one to compute it?

Otherwise looks good.

Comment 10

[fantasai]

> +  void DeltaRect(nsRect& aFrom, nsRect& aTo);
> Wouldn't it be clearer to use operator-?

Since we use an Inflate method instead of operator+ on nsRect to adds an nsMargin to an nsRect, I'm leaning towards leaving it as is. Ok for you?

(I'll split FindRegionFor, though; that makes sense.)

Comment 11

[fantasai]

Attachment: updated patch

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

DeltaRect is just not a good name. Let's call it something else. In fact, how about making it a method of nsRect?

  nsMargin SubtractRect(const nsRect& aInnerRect)
?

Comment 13

[fantasai]

The other methods on nsRect don't follow that pattern, though; they set 'this' to the result of some operation, here we're returning the result.

I don't have an objection to operator-, I just find it inconsistent that there isn't an opposing operator+. I suppose we could make one that's just an alias for Inflate.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The methods that modify in place are awful. ToNearestPixels/ToOutsidePixels/ToInsidePixels return a new rect. I don't think there will be much confusion since we're returning an nsMargin, not an nsRect.

Comment 15

[fantasai]

Attachment: patch with operator-

Comment 16

[fantasai]

Attachment: patch to complete margin arithmetic API

This patch completes margin arithmetic. If you want it, mark r+.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 388365 [details] [diff] [review]
patch with operator-

I think we don't need bz's time here

Comment 18

[fantasai]

Fix checked in. http://hg.mozilla.org/mozilla-central/rev/b24569bfd574 http://hg.mozilla.org/mozilla-central/rev/30f5342999ae