820246 - Split nsDisplayCanvasBackground into separate color/image items.

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: Split them!

https://tbpl.mozilla.org/?tree=Try&rev=fef1a9726b47

Comment 1

[Matt Woodrow (:mattwoodrow)]

Sorry bjacob, decided we wanted this done asap.

Also ignore the move of ShouldFixToViewport, i've reverted that change locally.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Sorry, I have been more busy than expected with B2G, security and WebGL bugs that I couldn't de-prioritize. But I still would like very much to try some layout bug. If you can think of another one, please let me know, and sorry that I didn't actually do this one!

Comment 3

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

Comment on attachment 690700 [details] [diff] [review]
Split them!

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

This will need to be rebased over bug 818643

::: layout/base/nsDisplayList.h
@@ +1875,5 @@
> +    // compositing layer. Since we know their background painting area can't
> +    // change (unless the viewport size itself changes), async scrolling
> +    // will work well.
> +    return mFrame->GetType() == nsGkAtoms::canvasFrame && mBackgroundStyle &&
> +      mBackgroundStyle->mLayers[mLayer].mAttachment == NS_STYLE_BG_ATTACHMENT_FIXED &&

Reorder these to avoid the GetType() call when possible.

But why not just put this on nsDisplayCanvasBackgroundImage?

::: layout/generic/nsCanvasFrame.h
@@ +121,5 @@
>   * PaintBackground, covering the whole overflow area.
>   * We can also paint an "extra background color" behind the normal
>   * background.
>   */
> +class nsDisplayCanvasBackground : public nsDisplayItem {

nsDisplayCanvasBackgroundColor

Comment 4

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> Also ignore the move of ShouldFixToViewport, i've reverted that change
> locally.

OK. But I'd like to see the new patch.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Split v2

Fixed review comments and tidied a few things up.

Passes all but a single reftest on android:

https://tbpl.mozilla.org/?tree=Try&rev=9f7187eb2e74

Comment 6

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

Comment on attachment 691233 [details] [diff] [review]
Split v2

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

::: layout/generic/nsCanvasFrame.h
@@ +201,4 @@
>   
>    // We still need to paint a background color as well as an image for this item, 
>    // so we can't support this yet.
> +  virtual bool SupportsOptimizingToImage() MOZ_OVERRIDE { return false; }

Remove this comment. In fact, I think in a separate patch you should allow this to return true.

Comment 7

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4777ad9776ec

Comment 8

[Ed Morley [:emorley]]

Sorry had to back this out for frequent failures in background-image-zoom-1.html on Android, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=17899647&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/e543e17b9824

Comment 9

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afc699fc7e65

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/afc699fc7e65