1296658 - BasicLayerManager disregards its final destination when creating buffer surfaces

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bas Schouten (:bas.schouten)]

This can cause it to create D2D surfaces which it then has to draw to a non-D2D target.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1296658: Attempt to create the correct BackendType buffers for a LayerManager.

Review commit: https://reviewboard.mozilla.org/r/72944/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72944/

Comment 2

[Bas Schouten (:bas.schouten)]

Comment on attachment 8782945 [details]
Bug 1296658: Attempt to create the correct BackendType buffers for a LayerManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72944/diff/1-2/

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8782945 [details]
Bug 1296658: Attempt to create the correct BackendType buffers for a LayerManager.

https://reviewboard.mozilla.org/r/72944/#review70724

::: gfx/layers/basic/BasicPaintedLayer.cpp:232
(Diff revision 2)
>  already_AddRefed<PaintedLayer>
>  BasicLayerManager::CreatePaintedLayer()
>  {
>    NS_ASSERTION(InConstruction(), "Only allowed in construction phase");
> -  RefPtr<PaintedLayer> layer = new BasicPaintedLayer(this);
> +
> +  BackendType backend = gfxPlatform::GetPlatform()->GetDefaultContentBackend();

You're clobbering this assignment on both branches of the if below. Is this intentional?

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 8782945 [details]
Bug 1296658: Attempt to create the correct BackendType buffers for a LayerManager.

https://reviewboard.mozilla.org/r/72944/#review70724

> You're clobbering this assignment on both branches of the if below. Is this intentional?

Yes, because there's a default branch where the original assignment is used, where there's !mDefaultTarget and mType != BLM_WIDGET, i.e. a regular browser window on first draw.

Comment 5

[Bas Schouten (:bas.schouten)]

See reply.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8782945 [details]
Bug 1296658: Attempt to create the correct BackendType buffers for a LayerManager.

https://reviewboard.mozilla.org/r/72944/#review71708

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Bas, this has r+, is it ready to land?

Comment 8

[Bas Schouten (:bas.schouten)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Bas, this has r+, is it ready to land?

Yes but another bug (1296657) isn't reviewed yet.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8782945 [details]
Bug 1296658: Attempt to create the correct BackendType buffers for a LayerManager.

https://reviewboard.mozilla.org/r/72944/#review76738

::: gfx/layers/client/ContentClient.cpp:117
(Diff revision 2)
>    }
>  }
>  
>  // We pass a null pointer for the ContentClient Forwarder argument, which means
>  // this client will not have a ContentHost on the other side.
> -ContentClientBasic::ContentClientBasic()
> +ContentClientBasic::ContentClientBasic(gfx::BackendType aBackend)

ContentClient.h needs to be updated to include this argument in the function signature.

::: gfx/layers/client/ContentClient.cpp:120
(Diff revision 2)
>  // We pass a null pointer for the ContentClient Forwarder argument, which means
>  // this client will not have a ContentHost on the other side.
> -ContentClientBasic::ContentClientBasic()
> +ContentClientBasic::ContentClientBasic(gfx::BackendType aBackend)
>    : ContentClient(nullptr)
>    , RotatedContentBuffer(ContainsVisibleBounds)
> +  , mBackend(aBackend)

The member variable definition for mBackend is missing from ContentClient.h.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Can this and the bug that it depends on now land with the noted issues fixed?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2372a86671c
Attempt to create the correct BackendType buffers for a LayerManager. r=jrmuizel

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1032461dcb88
Followup: Mark constructor explicit. r=bustage

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b2372a86671c
https://hg.mozilla.org/mozilla-central/rev/1032461dcb88