Closed Bug 748116 Opened 7 years ago Closed 7 years ago

[Azure] Use Azure canvas for taskbar tab previews

Categories

(Core :: Graphics, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: nrc, Assigned: nrc)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

Requires using a graphics surface with Azure canvas
Depends on: 694351
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.
Blocks: 752812
Attached patch patch (obsolete) — Splinter Review
Attachment #622539 - Flags: review?(roc)
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.
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.
Depends on: 748113
Attachment #624643 - Flags: review?(roc)
Attachment #622539 - Attachment is obsolete: true
Attachment #622539 - Flags: review?(roc)
Attachment #624644 - Flags: review?(roc)
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;
Attachment #624643 - Flags: review?(roc) → review+
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.
Attachment #624642 - Flags: review?(roc) → review+
addressed comments; carrying r=roc
Attachment #624642 - Attachment is obsolete: true
Attachment #624658 - Flags: review+
addressed comments; carrying r=roc
Attachment #624643 - Attachment is obsolete: true
Attachment #624659 - Flags: review+
Blocks: 773460
You need to log in before you can comment on or make changes to this bug.