Closed Bug 592493 Opened 9 years ago Closed 9 years ago

Register border-images with the document image tracker

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(1 file, 3 obsolete files)

Alon needs this for bug 359608, which is a beta-6 blocker.
Nominating for beta-6 blocking status, since it blocks a beta-6 blocker.
blocking2.0: --- → ?
Attached patch patch v1 (obsolete) — Splinter Review
Added a patch. Flagging dbaron for review.
Attachment #470980 - Flags: review?(dbaron)
Blocks: 359608
Comment on attachment 470980 [details] [diff] [review]
patch v1

This isn't going to work because there are two places outside the style system that use nsStyleBorder's copy constructor for other things: one in nsTableCellFrame.cpp and one in nsCSSRendering.cpp.

The simple but ugly way of dealing with that is adding appropriate TrackImage calls *and* UntrackImage calls (since they're allocating on the stack and thus not calling Destroy) at those two sites so the UntrackImage calls don't get confused.  That's probably best, since it has the least performance penalty, although this code is getting rather ugly.

In nsStyleStruct.h, could you return !!mBorderImage instead of casting to bool?

Other than that this looks ok, but I'd like to look at the revisions.
Attachment #470980 - Flags: review?(dbaron) → review-
Perhaps an even better approach, though, would be to simply copy the mImageTracked at those two locations, since they're never changing the image; they're just copying it from one nsStyleBorder to another, and it should be registered in the first.  And in both cases, with the current code, they'd neither register nor unregister, which is fine, since it's already registered for the nsStyleBorder being copy-constructed from.
(In reply to comment #4)
> Perhaps an even better approach, though, would be to simply copy the
> mImageTracked at those two locations, since they're never changing the image;
> they're just copying it from one nsStyleBorder to another, and it should be
> registered in the first.  And in both cases, with the current code, they'd
> neither register nor unregister, which is fine, since it's already registered
> for the nsStyleBorder being copy-constructed from.

How would you feel about adding a DoCopy(nsPresContext* aContext) method and hiding the copy constructor? It seems like this might be an accident waiting to happen otherwise...
I'd rather not make the style structs different from each other, and the copy constructor usage in nsRuleNode is fine.

Really, the only practical problem this causes is hitting the NS_ABORT_IF_FALSE in GetBorderImage() when things are actually just fine.
(In reply to comment #6)
> Really, the only practical problem this causes is hitting the NS_ABORT_IF_FALSE
> in GetBorderImage() when things are actually just fine.

Well, we'd also run into trouble when we tried to Destroy() those nsStyleBorders, because they'd call RemoveImage() on their non-null imgIRequests despite never having called AddImage().

If we solved that issue (presumably by making mImageTracked non-#ifdef and using it), another problem would be if the original nsStyleBorder went away but the others stuck around. Or if it was ever possible for one of those copies to migrate to another document context.

Thoughts?
(In reply to comment #7)
> (In reply to comment #6)
> > Really, the only practical problem this causes is hitting the NS_ABORT_IF_FALSE
> > in GetBorderImage() when things are actually just fine.
> 
> Well, we'd also run into trouble when we tried to Destroy() those
> nsStyleBorders, because they'd call RemoveImage() on their non-null
> imgIRequests despite never having called AddImage().

No, we won't, because we don't call Destroy on them.  They're on the stack, so we call the destructor without calling Destroy.
Attached patch patch v2 (obsolete) — Splinter Review
Added an updated patch. Flagging dbaron for review.
Attachment #470980 - Attachment is obsolete: true
Attachment #471359 - Flags: review?(dbaron)
Comment on attachment 471359 [details] [diff] [review]
patch v2

oh hang on - this patch asserts where the old one didn't and I have no idea why. I'll investigate when I get into the office.
Attachment #471359 - Flags: review?(dbaron)
Attached patch patch v3 (obsolete) — Splinter Review
Added a patch that fixes the assertion (I swear I didn't hit it on patch v1, but maybe I screwed up). Also ifdef-ed the copying of mImageTracked.

Flagging dbaron for review.
Attachment #471359 - Attachment is obsolete: true
Attachment #471589 - Flags: review?(dbaron)
Comment on attachment 471589 [details] [diff] [review]
patch v3

>+  // We're making an ephemeral stack copy here, so just copy this debug-only
>+  // member to prevent assertions.
>+#ifdef DEBUG
>+  newStyleBorder.mImageTracked = styleBorder->mImageTracked;
>+#endif
>+


You also need an

#ifdef DEBUG
  newStyleBorder.mImageTracked = false;
#endif

after we're done with newStyleBorder but before it goes away.  (Same at the other location.)

r=dbaron with that
Attachment #471589 - Flags: review?(dbaron) → review+
Attached patch patch v4Splinter Review
Added a patch, addressing dbaron's final comments. Carrying over review.

Pushed to try: http://hg.mozilla.org/try/rev/38a0c895bf9d
Attachment #471589 - Attachment is obsolete: true
Attachment #471631 - Flags: review+
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/d35c0d22fa71

Resolving fixed.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.