[Azure] Use Azure canvas for taskbar tab previews

RESOLVED FIXED in mozilla17

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nrc, Assigned: nrc)

Tracking

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

unspecified
mozilla17
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Requires using a graphics surface with Azure canvas
(Assignee)

Updated

5 years ago
Depends on: 694351
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 752812
(Assignee)

Comment 2

5 years ago
Try run: https://tbpl.mozilla.org/?tree=Try&rev=8bf70b4996ac
(Assignee)

Comment 3

5 years ago
Created attachment 622539 [details] [diff] [review]
patch
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.
(Assignee)

Updated

5 years ago
Depends on: 748113
(Assignee)

Comment 6

5 years ago
Created attachment 624642 [details] [diff] [review]
patch 1: CairoDrawTarget::DrawSurface to accept non-cairo surfaces
Attachment #624642 - Flags: review?(roc)
(Assignee)

Comment 7

5 years ago
Created attachment 624643 [details] [diff] [review]
patch 2: Azure canvas ::InitializeWithSurface
Attachment #624643 - Flags: review?(roc)
(Assignee)

Comment 8

5 years ago
Created attachment 624644 [details] [diff] [review]
patch 3: Azure canvas for tab previews
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+
Attachment #624644 - Flags: review?(roc) → review+
(Assignee)

Comment 11

5 years ago
Created attachment 624658 [details] [diff] [review]
patch 1: CairoDrawTarget::DrawSurface to accept non-cairo surfaces

addressed comments; carrying r=roc
Attachment #624642 - Attachment is obsolete: true
Attachment #624658 - Flags: review+
(Assignee)

Comment 12

5 years ago
Created attachment 624659 [details] [diff] [review]
patch 2: Azure canvas ::InitializeWithSurface

addressed comments; carrying r=roc
Attachment #624643 - Attachment is obsolete: true
Attachment #624659 - Flags: review+
(Assignee)

Comment 13

5 years ago
Created attachment 624663 [details] [diff] [review]
patch 4: refactor GfxPatternToCairoPattern
Attachment #624663 - Flags: review?(roc)
Attachment #624663 - Flags: review?(roc) → review+
(Assignee)

Comment 14

5 years ago
Try push: https://tbpl.mozilla.org/?tree=Try&rev=bef46ac46b23
(Assignee)

Updated

5 years ago
Blocks: 773460
(Assignee)

Comment 15

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=eb4f2872a6ca
(Assignee)

Comment 16

5 years ago
backed out: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ff7d09c5c945
(Assignee)

Comment 17

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c5125dde4bbf
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.