Last Comment Bug 748116 - [Azure] Use Azure canvas for taskbar tab previews
: [Azure] Use Azure canvas for taskbar tab previews
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: All Windows 7
: -- normal with 1 vote (vote)
: mozilla17
Assigned To: Nick Cameron [:nrc]
:
Mentors:
Depends on: 694351 746883 748113
Blocks: 752812 734668 773460
  Show dependency treegraph
 
Reported: 2012-04-23 14:05 PDT by Nick Cameron [:nrc]
Modified: 2012-12-05 14:29 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (12.09 KB, patch)
2012-05-09 15:25 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Review
patch 1: CairoDrawTarget::DrawSurface to accept non-cairo surfaces (4.85 KB, patch)
2012-05-16 21:34 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Review
patch 2: Azure canvas ::InitializeWithSurface (5.93 KB, patch)
2012-05-16 21:36 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Review
patch 3: Azure canvas for tab previews (2.57 KB, patch)
2012-05-16 21:36 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Review
patch 1: CairoDrawTarget::DrawSurface to accept non-cairo surfaces (4.83 KB, patch)
2012-05-16 22:23 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Review
patch 2: Azure canvas ::InitializeWithSurface (5.92 KB, patch)
2012-05-16 22:23 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Review
patch 4: refactor GfxPatternToCairoPattern (6.24 KB, patch)
2012-05-16 22:56 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Review

Description Nick Cameron [:nrc] 2012-04-23 14:05:52 PDT
Requires using a graphics surface with Azure canvas
Comment 1 Nick Cameron [:nrc] 2012-05-06 22:29:31 PDT
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] 2012-05-09 15:21:23 PDT
Try run: https://tbpl.mozilla.org/?tree=Try&rev=8bf70b4996ac
Comment 3 Nick Cameron [:nrc] 2012-05-09 15:25:20 PDT
Created attachment 622539 [details] [diff] [review]
patch
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-09 15:47:17 PDT
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) (Exited; email my personal email if necessary) 2012-05-09 15:53:25 PDT
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] 2012-05-16 21:34:55 PDT
Created attachment 624642 [details] [diff] [review]
patch 1: CairoDrawTarget::DrawSurface to accept non-cairo surfaces
Comment 7 Nick Cameron [:nrc] 2012-05-16 21:36:09 PDT
Created attachment 624643 [details] [diff] [review]
patch 2: Azure canvas ::InitializeWithSurface
Comment 8 Nick Cameron [:nrc] 2012-05-16 21:36:44 PDT
Created attachment 624644 [details] [diff] [review]
patch 3: Azure canvas for tab previews
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-16 21:58:28 PDT
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) (Exited; email my personal email if necessary) 2012-05-16 22:03:40 PDT
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] 2012-05-16 22:23:02 PDT
Created attachment 624658 [details] [diff] [review]
patch 1: CairoDrawTarget::DrawSurface to accept non-cairo surfaces

addressed comments; carrying r=roc
Comment 12 Nick Cameron [:nrc] 2012-05-16 22:23:53 PDT
Created attachment 624659 [details] [diff] [review]
patch 2: Azure canvas ::InitializeWithSurface

addressed comments; carrying r=roc
Comment 13 Nick Cameron [:nrc] 2012-05-16 22:56:27 PDT
Created attachment 624663 [details] [diff] [review]
patch 4: refactor GfxPatternToCairoPattern
Comment 14 Nick Cameron [:nrc] 2012-05-17 20:12:23 PDT
Try push: https://tbpl.mozilla.org/?tree=Try&rev=bef46ac46b23
Comment 16 Nick Cameron [:nrc] 2012-07-25 23:51:22 PDT
backed out: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ff7d09c5c945

Note You need to log in before you can comment on or make changes to this bug.