748116 - [Azure] Use Azure canvas for taskbar tab previews

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nick Cameron [:nrc]]

Requires using a graphics surface with Azure canvas

Comment 1

[Nick Cameron [:nrc]]

The way this works at the momment, we can't enable this until Azure/Cairo Canvases are working (Bug 694351), because we create a canvas on an existing surface which means we end up with a Cairo/Azure canvas (and a crash at the moment). I'm not sure if we can use an Azure/Skia canvas with a gfxASurface, but if we can, then we don't need to depend on 694351.

Comment 2

[Nick Cameron [:nrc]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=8bf70b4996ac

Comment 3

[Nick Cameron [:nrc]]

Attachment: patch

Comment 4

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

Comment on attachment 622539 [details] [diff] [review]
patch

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

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +1011,5 @@
>        !gfxWindowsPlatform::GetPlatform()->DWriteEnabled()) &&
>        !Preferences::GetBool("gfx.canvas.azure.prefer-skia", false)) {
>      return NS_ERROR_NOT_AVAILABLE;
>    }
> +  */

Don't comment out code!!! Either remove it or leave it. In this case I think you need to leave it?

@@ +1376,1 @@
>    }

mValid = mTarget != nsnull;

::: gfx/2d/DrawTargetCairo.cpp
@@ +338,5 @@
> +    cairo_surface_t* surf = cairo_image_surface_create_for_data(data->GetData(),
> +                                               GfxFormatToCairoFormat(data->GetFormat()),
> +                                               data->GetSize().width,
> +                                               data->GetSize().height,
> +                                               data->Stride());

The indentation would look better if you break the line after the '='

@@ +340,5 @@
> +                                               data->GetSize().width,
> +                                               data->GetSize().height,
> +                                               data->Stride());
> +    pat = cairo_pattern_create_for_surface(surf);
> +    cairo_surface_destroy(surf);

There's a problem here, which is that nothing keeps the DataSourceSurface alive while the cairo surface is alive. So it could go away, making the underlying data buffer invalid.

Best way to fix this is probably to attach the DataSourceSurface as user-data to the cairo surface.

Also, I'd put all this in a helper function GetCairoSurfaceForSourceSurface because we'll want to call it elsewhere as well.

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +506,5 @@
>      }
>      */
> +    //TODO[nrc]
> +        mPreferredDrawTargetBackend = BACKEND_SKIA;
> +    /*

Do you really want to land this change?

::: widget/windows/TaskbarPreview.cpp
@@ +95,5 @@
>  
>    if (!ctx) {
>      // create the canvas rendering context
> +    ctx = do_CreateInstance("@mozilla.org/content/canvas-rendering-context;1?id=2d", &rv);
> +    //ctx = do_CreateInstance("@mozilla.org/content/2dthebes-canvas-rendering-context;1", &rv);

Don't leave commented-out code lying around.

Comment 5

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

Please break down this patch into separate pieces as far as possible. Certainly the DrawTargetCairo change should be in its own patch. The TaskbarPreview change should also be in its own patch.

Comment 6

[Nick Cameron [:nrc]]

Attachment: patch 1: CairoDrawTarget::DrawSurface to accept non-cairo surfaces

Comment 7

[Nick Cameron [:nrc]]

Attachment: patch 2: Azure canvas ::InitializeWithSurface

Comment 8

[Nick Cameron [:nrc]]

Attachment: patch 3: Azure canvas for tab previews

Comment 9

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

Comment on attachment 624643 [details] [diff] [review]
patch 2: Azure canvas ::InitializeWithSurface

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

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +1296,5 @@
>    return InitializeWithTarget(target, width, height);
>  }
>  
>  nsresult
> +nsCanvasRenderingContext2DAzure::Initialize(PRInt32 width, PRInt32 height) {

{ on next line

@@ +1374,1 @@
>    }

mValid = mTarget != nsnull;

Comment 10

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

Comment on attachment 624642 [details] [diff] [review]
patch 1: CairoDrawTarget::DrawSurface to accept non-cairo surfaces

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

In a separate patch, make GfxPatternToCairoPattern use your new function.

::: gfx/2d/DrawTargetCairo.cpp
@@ +334,5 @@
> +  if (aSurface->GetType() == SURFACE_CAIRO) {
> +    cairo_surface_t* surf = static_cast<SourceSurfaceCairo*>(aSurface)->GetSurface();
> +    cairo_surface_reference(surf);
> +    return surf;
> +  } else {

no else after return

@@ +342,5 @@
> +                                          GfxFormatToCairoFormat(data->GetFormat()),
> +                                          data->GetSize().width,
> +                                          data->GetSize().height,
> +                                          data->Stride());
> +    data->AddRef();

instead of doing this, just pass data.forget() instead of data.get() below.

Comment 11

[Nick Cameron [:nrc]]

Attachment: patch 1: CairoDrawTarget::DrawSurface to accept non-cairo surfaces

addressed comments; carrying r=roc

Comment 12

[Nick Cameron [:nrc]]

Attachment: patch 2: Azure canvas ::InitializeWithSurface

addressed comments; carrying r=roc

Comment 13

[Nick Cameron [:nrc]]

Attachment: patch 4: refactor GfxPatternToCairoPattern

Comment 14

[Nick Cameron [:nrc]]

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

Comment 15

[Nick Cameron [:nrc]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=eb4f2872a6ca

Comment 16

[Nick Cameron [:nrc]]

backed out: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ff7d09c5c945

Comment 17

[Nick Cameron [:nrc]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c5125dde4bbf

Comment 18

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/1bb02cc7e1ab
https://hg.mozilla.org/mozilla-central/rev/7378179b373e
https://hg.mozilla.org/mozilla-central/rev/95f261232c71
https://hg.mozilla.org/mozilla-central/rev/28ba7ce0a197