Closed Bug 499377 Opened 11 years ago Closed 11 years ago

Store float region on frame

Categories

(Core :: Layout: Floats, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: fantasai.bugs, Assigned: fantasai.bugs)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

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.
Assignee: nobody → fantasai.bugs
Status: NEW → ASSIGNED
dbaron, why is mFloatManager translated minus borderPadding in the
nsBlockReflowState constructor?
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.
Why do they use content rect-based coordinates? ("They" doesn't include any frame rects, does it?)
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?
Attached patch first pass (obsolete) — Splinter Review
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?
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.
Attached patch patch (obsolete) — Splinter Review
Attachment #384563 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
This is Part II of bug 492627. The included testcase should also cover bug 503183.
Attachment #384656 - Attachment is obsolete: true
Attachment #387795 - Flags: review?(roc)
+  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.
> +  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.)
Attached patch updated patch (obsolete) — Splinter Review
Attachment #387795 - Attachment is obsolete: true
Attachment #388000 - Flags: superreview?(bzbarsky)
Attachment #388000 - Flags: review?(roc)
Attachment #387795 - Flags: review?(roc)
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)
?
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.
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.
Attachment #388000 - Attachment is obsolete: true
Attachment #388365 - Flags: superreview?(bzbarsky)
Attachment #388365 - Flags: review?(roc)
Attachment #388000 - Flags: superreview?(bzbarsky)
Attachment #388000 - Flags: review?(roc)
This patch completes margin arithmetic. If you want it, mark r+.
Attachment #388365 - Attachment is patch: true
Attachment #388365 - Attachment mime type: application/octet-stream → text/plain
Attachment #388365 - Flags: superreview?(bzbarsky)
Attachment #388365 - Flags: superreview+
Attachment #388365 - Flags: review?(roc)
Attachment #388365 - Flags: review+
Comment on attachment 388365 [details] [diff] [review]
patch with operator-

I think we don't need bz's time here
Fix checked in. http://hg.mozilla.org/mozilla-central/rev/b24569bfd574 http://hg.mozilla.org/mozilla-central/rev/30f5342999ae
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 507495
You need to log in before you can comment on or make changes to this bug.