Closed Bug 480233 Opened 15 years ago Closed 15 years ago

Make the display list item for the canvas know about the opaque setting of the canvas

Categories

(Core :: Graphics: Canvas2D, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: pavlov, Assigned: pavlov)

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 4 obsolete files)

Attached patch v1.0 (obsolete) — Splinter Review
Canvas currently uses an nsDisplayGeneric item that always say it isn't opaque.  The canvas has a moz-opaque property which says if it is, and we should use that hint to be a lot smarter when doing display list optimizations.
Attachment #364228 - Flags: superreview?(roc)
Attachment #364228 - Flags: review?(vladimir)
Instead of inheriting from nsDisplayGeneric, just clone nsDisplayGeneric and customize it for your needs so it inherits directly from nsDisplayItem. That's slightly more efficient because the 'name' string becomes debug-only and there's one less level of function calls while painting.
Oh, another thing you need to do is override GetBounds. The default GetBounds returns the area of the frame including borders. But the borders might not be opaque. You should return the content area of the frame --- use nsHTMLCanvasFrame::GetInnerArea --- because that's what the canvas paints and that's the area that's going to be opaque.
Attached patch v1.1 (obsolete) — Splinter Review
this seems to work, but i get some weird repainting effects occasionally.. trying to figure out how to screencast it
Attachment #365608 - Flags: review?(vladimir)
Attachment #364228 - Attachment is obsolete: true
Attachment #364228 - Flags: superreview?(roc)
Attachment #364228 - Flags: review?(vladimir)
Attached patch v1.2 (obsolete) — Splinter Review
Attachment #365608 - Attachment is obsolete: true
Attachment #365608 - Flags: review?(vladimir)
Attached patch v1.3 (obsolete) — Splinter Review
Assignee: nobody → pavlov
Attachment #365611 - Attachment is obsolete: true
Attachment #365613 - Flags: superreview?(vladimir)
Attachment #365613 - Flags: review?(roc)
(In reply to comment #5)
> Created an attachment (id=365613) [details]
> v1.3

ignore the nsWindow.cpp changes
Looks good but you can remove PaintCallback and ::PaintCanvas, and just have nsDisplayItemCanvas::Paint call nsHTMLCanvasFrame::PaintCanvas directly.
Attached patch v1.5Splinter Review
ok, this should be good to go
Attachment #365613 - Attachment is obsolete: true
Attachment #365705 - Flags: review?(roc)
Attachment #365613 - Flags: superreview?(vladimir)
Attachment #365613 - Flags: review?(roc)
Attachment #365705 - Flags: approval1.9.1?
Comment on attachment 365705 [details] [diff] [review]
v1.5

Trunk, then branch.
Attachment #365705 - Flags: approval1.9.1? → approval1.9.1-
Any reason this doesn't have tests?
Flags: in-testsuite?
Comment on attachment 365705 [details] [diff] [review]
v1.5

sorry, this landed a while ago
Attachment #365705 - Flags: approval1.9.1- → approval1.9.1?
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #365705 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 365705 [details] [diff] [review]
v1.5

a191, though yeah, this should have tests. I'll trust you to check in with those :)
It's a performance optimization so it's hard to write a specific test.
Do we have a Twhatever that exercises the motivating use case, so that we don't accidentally regress it with other changes? (cairo landing, compositor work, etc.)

If not, could someone with a good understanding of the details file a bug to get one?
(In reply to comment #15)
> If not, could someone with a good understanding of the details file a bug to
> get one?
and CC me on it in case you need Test Dev help.
Is this going to land on 1.9.1?  (Or not because we don't have tests?)
It should land on 1.9.1, but where's the bug to figure out how to get perf coverage for it?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: