473398 - minor cleanup for nsCSSRendering::PaintBackground

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Zack Weinberg (:zwol)]

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.

Comment 1

[Zack Weinberg (:zwol)]

Attachment: patch

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 3

[Zack Weinberg (:zwol)]

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.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[Zack Weinberg (:zwol)]

Attachment: (rev 2) only set view manager default if opaque

So, like this?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed http://hg.mozilla.org/mozilla-central/rev/5f9daba6c730

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually, since 469170 is blocking and this fixes it, we shouldn't beat about the bush

Comment 10

[Zack Weinberg (:zwol)]

I'm not sure this fixes it all by itself...

Comment 11

[Zack Weinberg (:zwol)]

... ok, unmodified trunk as of now does pass the test in bug 469170.

Comment 12

[Bill Gianopoulos [:WG9s]]

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 13

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 14

[Daniel Holbert [:dholbert]]

(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.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/03318e9d68d5