Closed Bug 1031576 Opened 5 years ago Closed 5 years ago

Add a new Image subclass for dynamically-created images and use it for -moz-element

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(3 files, 6 obsolete files)

Right now, -moz-element is a special case that reimplements some things that are already implemented elsewhere (such as clipping for border-image) and has a special drawing path that nothing else uses.

In this bug we'll add a new Image subclass that wraps a gfxDrawable. -moz-element code can then use the normal image drawing path. This will ensure that when bugs are encountered in the border-image or pixel snapping code, -moz-element gets the benefits, without any further work. (It's especially important that this not be a special path because -moz-element is used so rarely, so regressions may not be noticed immediately.)

In addition, this will support upcoming imagelib refactorings that require more imagelib involvement in the pixel snapping code. This was quite awkward for the -moz-element case since right now there is no Image/imgIContainer object in that situation.
This patch adds a new Image subclass, DynamicImage, intended for use for emphemeral, dynamically-generated images. Such images are backed by a gfxDrawable.

The implementation is verbose just because there are so many methods on Image, but most of them are irrelevant for this use, so there's fairly little non-boilerplate code here.
Attachment #8447528 - Flags: review?(tnikkel)
This patch replaces the current -moz-element drawing code with code that uses DynamicImage.

This should be at least as performant as the old code and sometimes better, since ClippedImage can often avoid copying.

To avoid issues with cache invalidation, I still don't cache the ClippedImage instances that get created.
Attachment #8447530 - Flags: review?(ncameron)
After part 2, nsLayoutUtils::DrawPixelSnapped is dead code. This patch removes it.

Eliminating this code path should both make maintenance easier and make the soon-to-arrive imagelib refactorings much cleaner.
Attachment #8447531 - Flags: review?(dbaron)
Attachment #8447528 - Flags: review?(tnikkel) → review+
Hmm, looks like this patch busted something. I suspect I know exactly what the problem is; should be a quick fix. I'll take a look after dinner.
Comment on attachment 8447530 [details] [diff] [review]
(Part 2) - Use dynamic images in the implementation of -moz-element

Review of attachment 8447530 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm
Attachment #8447530 - Flags: review?(ncameron) → review+
Looks like we just needed to call DrawImage instead of DrawSingleImage in the Element case. This is odd because I just copied the DrawSingleImage call from the Image case.

IRC discussions between Nick and myself have led to the conclusion that the Image case may actually be dead code. If this turns out to be true, it'll be dealt with in a followup bug.
Attachment #8447530 - Attachment is obsolete: true
Comment on attachment 8447531 [details] [diff] [review]
(Part 3) - Remove nsLayoutUtils::DrawPixelSnapped, since it's dead code now

r=dbaron; sorry for the delay getting to this
Attachment #8447531 - Flags: review?(dbaron) → review+
This patch got build failures on B2G because the 'using' directives needed a tweak. Not sure why this *doesn't* fail on most platforms!
Attachment #8447528 - Attachment is obsolete: true
Thanks for the review, David. This can land if B2G is green in this try job:

https://tbpl.mozilla.org/?tree=Try&rev=02c9d5addf03
Sigh. Fixing one set of problems revealed new problems. I'm pretty sure the difference in behavior here is that the B2G builds aren't running in unified mode.

Hopefully all the needed headers are present now. Another try push:

https://tbpl.mozilla.org/?tree=Try&rev=b06968b07c73
Attachment #8449907 - Attachment is obsolete: true
Blocks: 1034209
Blocks: 1034345
Attachment #8450400 - Attachment is obsolete: true
Attachment #8448358 - Attachment is obsolete: true
Attachment #8447531 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.