Closed Bug 473398 Opened 11 years ago Closed 11 years ago

minor cleanup for nsCSSRendering::PaintBackground

Categories

(Core :: Web Painting, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: zwol, Assigned: zwol)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 1 obsolete file)

The code in nsCSSRendering::PaintBackground that tries to ensure an opaque background for the canvas frame is unnecessary now that PresShell::Paint does the same work.  This was noted during the discussion of bug 469170 but does not fix that bug.
Attached patch patch (obsolete) — Splinter Review
Attachment #356736 - Flags: superreview?(roc)
Attachment #356736 - Flags: review?(roc)
Attachment #356736 - Flags: approval1.9.1?
We should just do a straight backout of bug 453566. See my comment in bug 469170. In particular this block
+  if (isCanvas) {
+    nsStyleBackground canvasColor(*color);
+    nsIViewManager* vm = aPresContext->GetViewManager();
+    vm->SetDefaultBackgroundColor(canvasColor.mBackgroundColor);
   }
is not wanted.
That block predates bug 453566, though!  And a straight backout would leave us with special casing for frames where isCanvas is true and the alpha component of the background color is exactly zero, which I don't think makes sense anymore either.

If what you're saying is PaintBackground no longer needs to special case canvas frames at all, I believe you, but a straight backout of 453566 won't get us there.
ok. I was wrong in comment #2 though, we need to keep that code.

So can you write a patch that removes the special casing of exactly-zero but otherwise reverts 453566?
Blocks: 453566
Comment on attachment 356736 [details] [diff] [review]
patch

This patch is almost right, but we don't want to put a non-opaque color into SetDefaultBackgroundColor. So I'd just make the call to SetDefaultBackgroundColor conditional on the alpha channel == 255. For transparent backgrounds, that's equivalent to the 1.9.0 code --- we're just leaving the current default-background-color as-is, which makes sense.
So, like this?
Attachment #356736 - Attachment is obsolete: true
Attachment #357189 - Flags: superreview?(roc)
Attachment #357189 - Flags: review?(roc)
Attachment #357189 - Flags: approval1.9.1?
Attachment #356736 - Flags: superreview?(roc)
Attachment #356736 - Flags: review?(roc)
Attachment #356736 - Flags: approval1.9.1?
Attachment #357189 - Flags: superreview?(roc)
Attachment #357189 - Flags: superreview+
Attachment #357189 - Flags: review?(roc)
Attachment #357189 - Flags: review+
Pushed http://hg.mozilla.org/mozilla-central/rev/5f9daba6c730
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 approval]
Comment on attachment 357189 [details] [diff] [review]
(rev 2) only set view manager default if opaque

We need this on 1.9.1 because it fixes bug 469170
Actually, since 469170 is blocking and this fixes it, we shouldn't beat about the bush
Flags: blocking1.9.1+
Whiteboard: [needs 191 approval] → [needs 191 landing]
I'm not sure this fixes it all by itself...
... ok, unmodified trunk as of now does pass the test in bug 469170.
Depends on: 474201
This causes the regression in bug 474201, so I'm not sure landing it in 1.9.1 is the right idea at this time.
Comment on attachment 357189 [details] [diff] [review]
(rev 2) only set view manager default if opaque

This is a blocker, so it doesn't need approval. Don't forget to land the fix for bug 474201 on the branch as well.
Attachment #357189 - Flags: approval1.9.1?
(In reply to comment #12)
> This causes the regression in bug 474201, so I'm not sure landing it in 1.9.1
> is the right idea at this time.

And while that bug's now been fixed, *its* fix apparently caused a number of other regressions, too.  So I'm guessing it's still probably not a good idea to land this on 1.9.1 right now.
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
Depends on: 542970
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.