Last Comment Bug 711063 - [Azure] Create Thebes Wrapper around Azure
: [Azure] Create Thebes Wrapper around Azure
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla12
Assigned To: Bas Schouten (:bas.schouten)
:
:
Mentors:
Depends on: 780018 840869 854296 1131264
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-15 07:40 PST by Bas Schouten (:bas.schouten)
Modified: 2015-07-13 14:26 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Add gfx2DGlue code to Thebes for Azure wrapper. (5.13 KB, patch)
2011-12-15 08:15 PST, Bas Schouten (:bas.schouten)
joe: review+
Details | Diff | Splinter Review
Part 2: Add wrapper for for gfxContext (67.02 KB, patch)
2011-12-15 08:17 PST, Bas Schouten (:bas.schouten)
joe: feedback+
Details | Diff | Splinter Review
Part 3: Adjust font code to be compatible with Azure. (22.15 KB, patch)
2011-12-15 09:02 PST, Bas Schouten (:bas.schouten)
joe: review+
Details | Diff | Splinter Review
Part 4: Adjust gfxUtils to be compatible with the Azure Wrapper. (1.26 KB, patch)
2011-12-15 09:07 PST, Bas Schouten (:bas.schouten)
joe: review-
Details | Diff | Splinter Review
Part 5: Adjust gfxWindowsNativeDrawing to be compatible with the Azure Wrapper. (3.55 KB, patch)
2011-12-15 09:09 PST, Bas Schouten (:bas.schouten)
jmuizelaar: review-
Details | Diff | Splinter Review
Part 6: Adjust nsRenderingContext to be compatible with the Azure Wrapper. (831 bytes, patch)
2011-12-15 09:10 PST, Bas Schouten (:bas.schouten)
joe: review+
Details | Diff | Splinter Review
Part 2: Add wrapper for for gfxContext v2 (68.64 KB, patch)
2011-12-27 15:28 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 2: Add wrapper for for gfxContext v3 (68.74 KB, patch)
2011-12-27 18:09 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 4: Adjust gfxUtils to be compatible with the Azure Wrapper. v2 (2.61 KB, patch)
2011-12-29 17:26 PST, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review
Part 7: Adjust border rendering code to be compatible with the Azure Wrapper. (1.74 KB, patch)
2011-12-29 17:52 PST, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review
Part 8: Adjust SVG code to be compatible with the Azure Wrapper. (2.45 KB, patch)
2011-12-29 18:06 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 8: Adjust SVG code to be compatible with the Azure Wrapper. (2.45 KB, patch)
2011-12-29 18:08 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 2: Add wrapper for for gfxContext v4 (68.83 KB, patch)
2011-12-29 18:52 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 9: Adjust gfxDrawable code to be compatible with the Azure Wrapper. (1.23 KB, patch)
2011-12-29 20:15 PST, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review
Part 10: Adjust BasicLayers code to be compatible with the Azure Wrapper. (5.65 KB, patch)
2011-12-29 20:15 PST, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review
Part 11: Allow using Azure for content drawing with D3D10 layers. (8.62 KB, patch)
2011-12-29 20:31 PST, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review
Part 2: Add wrapper for for gfxContext v5 (69.00 KB, patch)
2011-12-29 20:53 PST, Bas Schouten (:bas.schouten)
jmuizelaar: review-
Details | Diff | Splinter Review
Part 2: Add wrapper for for gfxContext v6 (69.74 KB, patch)
2012-01-04 15:03 PST, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review
Part 1.5: Allow multiplying a Size with a Matrix. (915 bytes, patch)
2012-01-04 15:03 PST, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review
Part 5: Adjust gfxWindowsNativeDrawing to be compatible with the Azure Wrapper. V2 (3.35 KB, patch)
2012-01-04 19:00 PST, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review

Description Bas Schouten (:bas.schouten) 2011-12-15 07:40:25 PST
In order to facilitate a migration from Thebes to Azure we need to initially create a wrapper for Thebes around Azure. That way we can then gradually migrate code to use Azure.
Comment 1 Bas Schouten (:bas.schouten) 2011-12-15 08:15:58 PST
Created attachment 581975 [details] [diff] [review]
Part 1: Add gfx2DGlue code to Thebes for Azure wrapper.
Comment 2 Bas Schouten (:bas.schouten) 2011-12-15 08:17:09 PST
Created attachment 581977 [details] [diff] [review]
Part 2: Add wrapper for for gfxContext

Asking for feedback here. I still need to clean up some small bits before I can request review. But an examination at this point should make the review process very quick.
Comment 3 Bas Schouten (:bas.schouten) 2011-12-15 09:02:46 PST
Created attachment 581999 [details] [diff] [review]
Part 3: Adjust font code to be compatible with Azure.
Comment 4 Bas Schouten (:bas.schouten) 2011-12-15 09:07:29 PST
Created attachment 582002 [details] [diff] [review]
Part 4: Adjust gfxUtils to be compatible with the Azure Wrapper.
Comment 5 Bas Schouten (:bas.schouten) 2011-12-15 09:09:11 PST
Created attachment 582003 [details] [diff] [review]
Part 5: Adjust gfxWindowsNativeDrawing to be compatible with the Azure Wrapper.
Comment 6 Bas Schouten (:bas.schouten) 2011-12-15 09:10:31 PST
Created attachment 582004 [details] [diff] [review]
Part 6: Adjust nsRenderingContext to be compatible with the Azure Wrapper.
Comment 7 Joe Drew (not getting mail) 2011-12-15 11:16:53 PST
Comment on attachment 581975 [details] [diff] [review]
Part 1: Add gfx2DGlue code to Thebes for Azure wrapper.

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

::: gfx/thebes/gfx2DGlue.h
@@ +1,1 @@
>  

Add the license header

@@ +42,5 @@
> +inline Filter ToFilter(gfxPattern::GraphicsFilter aFilter)
> +{
> +  switch (aFilter) {
> +  case gfxPattern::FILTER_NEAREST:
> +    return FILTER_POINT;

This is going to add warnings - may as well make the default return in a default: block to silence them. Do this throughout.
Comment 8 Joe Drew (not getting mail) 2011-12-15 12:18:44 PST
Comment on attachment 581977 [details] [diff] [review]
Part 2: Add wrapper for for gfxContext

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

Looks reasonable at a brief glance. I didn't review in-depth though.

::: gfx/thebes/gfxContext.cpp
@@ +559,3 @@
>      cairo_translate(mCairo, pt.x, pt.y);
> +  } else {
> +    NS_ASSERTION(!mPathBuilder, "Cannot change transformation during an active path.");

Should we be more forceful about this, and use NS_ABORT_IF_FALSE?

@@ +1472,5 @@
>  void
>  gfxContext::PushGroupAndCopyBackground(gfxASurface::gfxContentType content)
>  {
> +  if (mCairo) {
> +    if (false && content == gfxASurface::CONTENT_COLOR_ALPHA &&

better switch that false

::: gfx/thebes/gfxContext.h
@@ +50,5 @@
>  #include "gfxPath.h"
>  #include "nsISupportsImpl.h"
>  
> +#include "mozilla/gfx/2D.h"
> +#include <vector>

Remind me - it was <algorithm> we had problems with (wrt std::min/std::max), right?

@@ +691,5 @@
>      void SetFlag(PRInt32 aFlag) { mFlags |= aFlag; }
>      void ClearFlag(PRInt32 aFlag) { mFlags &= ~aFlag; }
>      PRInt32 GetFlags() const { return mFlags; }
>  
> +    bool IsAzure() const { return !mCairo; }

Maybe we can remove the word "Azure" from this in general? Could be DrawTargetState etc. And IsCairo() instead of IsAzure()?

@@ +733,5 @@
> +    mozilla::gfx::Color color;
> +    nsRefPtr<gfxPattern> pattern;
> +    mozilla::RefPtr<mozilla::gfx::SourceSurface> sourceSurface;
> +    mozilla::gfx::Matrix surfTransform;
> +    mozilla::gfx::Matrix mTransform;

You should either have mFoo or just foo - not both!!

@@ +737,5 @@
> +    mozilla::gfx::Matrix mTransform;
> +    struct pushedClip {
> +      mozilla::RefPtr<mozilla::gfx::Path> path;
> +      mozilla::gfx::Rect rect;
> +      mozilla::gfx::Matrix transform;

Does it make sense to just add accessors to the pushed clips on the DrawTarget?

@@ +751,5 @@
> +  };
> +
> +  void EnsurePath();
> +  void EnsureUSPath();
> +  void EnsurePathBuilder();

You should probably document all of these

::: gfx/thebes/gfxPattern.h
@@ +133,5 @@
> +      mozilla::AlignedStorage2<mozilla::gfx::RadialGradientPattern> mRadialGradientPattern;
> +      mozilla::AlignedStorage2<mozilla::gfx::SurfacePattern> mSurfacePattern;
> +    };
> +
> +    mozilla::gfx::Pattern *mAzurePattern;

mGfxPattern maybe?
Comment 9 Joe Drew (not getting mail) 2011-12-15 14:22:55 PST
Comment on attachment 582003 [details] [diff] [review]
Part 5: Adjust gfxWindowsNativeDrawing to be compatible with the Azure Wrapper.

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

If my reading is correct, it looks like this will just silently do nothing with a gfxContext wrapping a DrawTarget. Or am I wrong?
Comment 10 Joe Drew (not getting mail) 2011-12-15 14:23:25 PST
Comment on attachment 582002 [details] [diff] [review]
Part 4: Adjust gfxUtils to be compatible with the Azure Wrapper.

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

Too hacky, even with an XXX comment.
Comment 11 Joe Drew (not getting mail) 2011-12-15 14:34:42 PST
Comment on attachment 581999 [details] [diff] [review]
Part 3: Adjust font code to be compatible with Azure.

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

::: gfx/thebes/gfxFont.cpp
@@ +1162,5 @@
> +        return &mGlyphBuffer[mNumGlyphs++];
> +    }
> +
> +    void Flush(DrawTarget *aDT, Pattern &aPattern, ScaledFont *aFont,
> +               bool aDrawToPath, bool aReverse, bool aFinish = false) {

Put { on its own line.

@@ +1173,5 @@
> +            for (PRUint32 i = 0; i < mNumGlyphs/2; ++i) {
> +                Glyph tmp = mGlyphBuffer[i];
> +                mGlyphBuffer[i] = mGlyphBuffer[mNumGlyphs - 1 - i];
> +                mGlyphBuffer[mNumGlyphs - 1 - i] = tmp;
> +            }

You could use std::reverse here, or even std::swap.

@@ +1391,1 @@
>          return;

Add an assertion here, or at least a TODO.

@@ +1414,5 @@
> +
> +          mat._11 = mat._22 = 1.0;
> +          mat._21 /= mAdjustedSize;
> +
> +          dt->SetTransform(mat * oldMat);

This is begging for some comments.
Comment 12 Bas Schouten (:bas.schouten) 2011-12-15 16:45:22 PST
(In reply to Joe Drew (:JOEDREW!) (Away Dec 17–Jan 2) from comment #9)
> Comment on attachment 582003 [details] [diff] [review]
> Part 5: Adjust gfxWindowsNativeDrawing to be compatible with the Azure
> Wrapper.
> 
> Review of attachment 582003 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If my reading is correct, it looks like this will just silently do nothing
> with a gfxContext wrapping a DrawTarget. Or am I wrong?

No, it will just go through the path that it always uses with D2D, i.e. it'll draw the native theme to a temp surface and then blend it to the context, rather than getting the DC from the context and drawing to that directly.
Comment 13 Bas Schouten (:bas.schouten) 2011-12-15 16:48:29 PST
(In reply to Joe Drew (:JOEDREW!) (Away Dec 17–Jan 2) from comment #11)
> Comment on attachment 581999 [details] [diff] [review]
> Part 3: Adjust font code to be compatible with Azure.
> 
> Review of attachment 581999 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxFont.cpp
> @@ +1391,1 @@
> >          return;
> 
> Add an assertion here, or at least a TODO.

Agreed on all points, fwiw, this should never happen, as far as I've determined only Canvas used this. And in this case we should always be on an Azure canvas.
Comment 14 Bas Schouten (:bas.schouten) 2011-12-27 15:28:40 PST
Created attachment 584499 [details] [diff] [review]
Part 2: Add wrapper for for gfxContext v2

Adjusted to take into account most of Joe's feedback comments. As discussed this does not make the Azure-Thebes wrapper work perfectly, but issues are marked with comments and this will allow testing across platforms.
Comment 15 Bas Schouten (:bas.schouten) 2011-12-27 15:30:57 PST
Comment on attachment 582003 [details] [diff] [review]
Part 5: Adjust gfxWindowsNativeDrawing to be compatible with the Azure Wrapper.

Re-requesting review as per bug comments.
Comment 16 Bas Schouten (:bas.schouten) 2011-12-27 18:09:40 PST
Created attachment 584514 [details] [diff] [review]
Part 2: Add wrapper for for gfxContext v3

Properly qreffed this time.
Comment 17 Bas Schouten (:bas.schouten) 2011-12-29 17:26:12 PST
Created attachment 584889 [details] [diff] [review]
Part 4: Adjust gfxUtils to be compatible with the Azure Wrapper. v2

Less hacky version as requested.
Comment 18 Bas Schouten (:bas.schouten) 2011-12-29 17:52:33 PST
Created attachment 584891 [details] [diff] [review]
Part 7: Adjust border rendering code to be compatible with the Azure Wrapper.
Comment 19 Bas Schouten (:bas.schouten) 2011-12-29 18:06:26 PST
Created attachment 584893 [details] [diff] [review]
Part 8: Adjust SVG code to be compatible with the Azure Wrapper.
Comment 20 Bas Schouten (:bas.schouten) 2011-12-29 18:08:38 PST
Created attachment 584894 [details] [diff] [review]
Part 8: Adjust SVG code to be compatible with the Azure Wrapper.

Corrected !IsAzure() -> IsCairo()
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-29 18:31:38 PST
Comment on attachment 584514 [details] [diff] [review]
Part 2: Add wrapper for for gfxContext v3

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

::: gfx/thebes/gfxContext.h
@@ +734,5 @@
> +    nsRefPtr<gfxPattern> pattern;
> +    mozilla::RefPtr<mozilla::gfx::SourceSurface> sourceSurface;
> +    mozilla::gfx::Matrix surfTransform;
> +    mozilla::gfx::Matrix transform;
> +    struct pushedClip {

PushedClip

@@ +775,5 @@
> +  nsRefPtr<gfxASurface> mSurface;
> +  PRInt32 mFlags;
> +
> +  mozilla::RefPtr<mozilla::gfx::DrawTarget> mDT;
> +  mozilla::RefPtr<mozilla::gfx::DrawTarget> mOriginalDT;

A bunch of typedefs to pull mozilla:: and mozilla::gfx:: types into gfxContext would help readability a lot here.
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-29 18:32:19 PST
Comment on attachment 584894 [details] [diff] [review]
Part 8: Adjust SVG code to be compatible with the Azure Wrapper.

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

It's not clear what gfxContext-Azure doesn't support that make you need to do this. That needs to be documented somewhere, probably in one of the earlier patches.

::: layout/svg/base/src/nsSVGUtils.cpp
@@ +1224,5 @@
> +    aContext->SetSource(aSurface);
> +    aContext->Paint(aOpacity);
> +    aContext->Restore();
> +  } else {
> +    DrawTarget *dt = aContext->GetDT();

Please call that method GetDrawTarget?

@@ +1238,5 @@
> +
> +    dt->FillRect(Rect(-pt.x, -pt.y, size.width, size.height),
> +                 SurfacePattern(surf, EXTEND_CLAMP,
> +                                Matrix(1.0f, 0, 0, 1.0f, -pt.x, -pt.y)),
> +                 DrawOptions(aOpacity));

Wouldn't DrawSurface be more efficient here?
Comment 23 Bas Schouten (:bas.schouten) 2011-12-29 18:52:47 PST
Created attachment 584903 [details] [diff] [review]
Part 2: Add wrapper for for gfxContext v4

Updated to avoid some needless compiler warnings.
Comment 24 Bas Schouten (:bas.schouten) 2011-12-29 20:15:14 PST
Created attachment 584911 [details] [diff] [review]
Part 9: Adjust gfxDrawable code to be compatible with the Azure Wrapper.
Comment 25 Bas Schouten (:bas.schouten) 2011-12-29 20:15:58 PST
Created attachment 584912 [details] [diff] [review]
Part 10: Adjust BasicLayers code to be compatible with the Azure Wrapper.
Comment 26 Bas Schouten (:bas.schouten) 2011-12-29 20:31:24 PST
Created attachment 584914 [details] [diff] [review]
Part 11: Allow using Azure for content drawing with D3D10 layers.
Comment 27 Bas Schouten (:bas.schouten) 2011-12-29 20:53:06 PST
Created attachment 584917 [details] [diff] [review]
Part 2: Add wrapper for for gfxContext v5

Adjusted for roc's comments.
Comment 28 Jeff Muizelaar [:jrmuizel] 2012-01-03 16:34:30 PST
Comment on attachment 584917 [details] [diff] [review]
Part 2: Add wrapper for for gfxContext v5

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

::: gfx/thebes/gfxContext.cpp
@@ +58,5 @@
>  
> +using namespace mozilla;
> +using namespace mozilla::gfx;
> +
> +class GeneralPattern

It's not obvious from the class name what this is for. A comment would be nice.

@@ +270,4 @@
>      nsRefPtr<gfxPath> path = new gfxPath(cairo_copy_path(mCairo));
>      return path.forget();
> +  } else {
> +    // XXX - This is not yet supported for Azure.

MOZ_ASSERT(0)

@@ +556,5 @@
> +
> +    Rect rect(0, 0, size.width, size.height);
> +    rect.Intersect(Rect(0, 0, surf->GetSize().width, surf->GetSize().height));
> +
> +    // XXX - Should fix pixel snapping.

What will make sure we fix this before turning this on?

@@ +570,3 @@
>      cairo_translate(mCairo, pt.x, pt.y);
> +  } else {
> +    NS_ASSERTION(!mPathBuilder, "Cannot change transformation during an active path.");

I'd use MOZ_ASSERT instead of NS_ASSERTION

@@ +697,5 @@
> +
> +    gfxSize newSize = size;
> +    newSize.width = newSize.width * matrix._11 + newSize.height * matrix._12;
> +    newSize.height = newSize.width * matrix._21 + newSize.height * matrix._22;
> +    return newSize;

It's probably worth adding a helper for this to azure, as the existing users are going to need something similar.

@@ +1261,5 @@
> +  } else {
> +    // XXX - We default to true here for now, this means any user will have
> +    // to assume any rectangle is within the clip and will not optimize it
> +    // away.
> +    return true;

This could probably use an implementation.

::: gfx/thebes/gfxContext.h
@@ +926,5 @@
>          if (aDisable) {
>              mSurface = aContext->CurrentSurface();
> +            if (!mSurface) {
> +              return;
> +            }

Don't we need this still?

::: gfx/thebes/gfxPattern.cpp
@@ +142,5 @@
>      cairo_matrix_t mat;
>      cairo_pattern_get_matrix(mPattern, &mat);
>      return gfxMatrix(*reinterpret_cast<gfxMatrix*>(&mat));
> +  } else {
> +    return GFXMatrix(mTransform);

I really don't like these GFXMatrix style functions that do a conversion. Using the same name as the destination type with the case changed is weird.

@@ +155,5 @@
> +      SurfacePattern(mSourceSurface, EXTEND_CLAMP, mTransform);
> +    return mGfxPattern;
> +  }
> +
> +  GraphicsExtend extend = (GraphicsExtend)cairo_pattern_get_extend(mPattern);

Can we just use cairo_extend_t instead of GraphicsExtend here?

@@ +160,5 @@
> +
> +  switch (cairo_pattern_get_type(mPattern)) {
> +  case CAIRO_PATTERN_TYPE_SURFACE:
> +    {
> +      GraphicsFilter filter = (GraphicsFilter)cairo_pattern_get_filter(mPattern);

Same here.

@@ +262,5 @@
> +        return mGfxPattern;
> +      }
> +      break;
> +    }
> +  }

I would like it better if this switch called individual functions to convert each kind of pattern instead of having them all in one big function.

@@ +310,5 @@
> +
> +bool
> +gfxPattern::IsOpaque()
> +{
> +  if (!mPattern) {

It would be good to be consistent in the use of if (mPattern) vs. if (!mPattern). i.e. change this function to be if (mPattern). The consistency helps with readability.

@@ +397,4 @@
>      return (GraphicsPatternType) cairo_pattern_get_type(mPattern);
> +  } else {
> +    // We should never be trying to get the type off an Azure gfx Pattern.
> +    NS_ERROR("Attempt to get type of an Azure gfxPattern!");

I would use something stronger like MOZ_ASSERT(0); here and elsewhere. Overall, there are also probably other places that could use a MOZ_ASSERT(0) on the conditions you don't expect.

@@ +409,4 @@
>      return cairo_pattern_status(mPattern);
> +  } else {
> +    // An Azure pattern as this point is never in error status.
> +    return 0;

return CAIRO_STATUS_SUCCESS;

::: gfx/thebes/gfxPattern.h
@@ +128,5 @@
>  protected:
>      cairo_pattern_t *mPattern;
> +
> +    union {
> +      mozilla::AlignedStorage2<mozilla::gfx::ColorPattern> mColorPattern;

Why do you need to use AlignedStorage2 here? is the natural alignment of ColorPattern etc. insufficient?
Comment 29 Jeff Muizelaar [:jrmuizel] 2012-01-03 16:37:33 PST
Comment on attachment 584914 [details] [diff] [review]
Part 11: Allow using Azure for content drawing with D3D10 layers.

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

::: gfx/layers/d3d10/ThebesLayerD3D10.cpp
@@ +363,5 @@
> +    ctx->Translate(-gfxPoint(aOffset.x, aOffset.y));
> +    gfxUtils::PathFromRegion(ctx, aRegion);
> +    ctx->SetColor(aColor);
> +    ctx->Fill();
> +  }

Why don't we need this?
Comment 30 Jeff Muizelaar [:jrmuizel] 2012-01-03 16:44:02 PST
Comment on attachment 582003 [details] [diff] [review]
Part 5: Adjust gfxWindowsNativeDrawing to be compatible with the Azure Wrapper.

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

::: gfx/thebes/gfxWindowsNativeDrawing.cpp
@@ +74,5 @@
> +        if (mContext->GetCairo()) {
> +          surf = mContext->CurrentSurface(&mDeviceOffset.x, &mDeviceOffset.y);
> +        }
> +
> +        if (surf && surf->CairoStatus())

surf is used uninitialized here.

@@ +217,5 @@
> +    nsRefPtr<gfxASurface> surf;
> +    
> +    if (mContext->GetCairo()) {
> +      surf = mContext->CurrentSurface(&mDeviceOffset.x, &mDeviceOffset.y);
> +    }

I'd rather this be written something like:
 if (!mContext->GetCairo()) {
  // azure
  return true;
 }

Also what are we doing in the situation where we have a cairo backed Azure context?
Comment 31 Jeff Muizelaar [:jrmuizel] 2012-01-03 16:45:16 PST
Comment on attachment 584889 [details] [diff] [review]
Part 4: Adjust gfxUtils to be compatible with the Azure Wrapper. v2

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

::: gfx/thebes/gfxUtils.cpp
@@ +305,2 @@
>              return;
>  

What about on Azure-cairo?
Comment 32 Bas Schouten (:bas.schouten) 2012-01-04 08:20:29 PST
(In reply to Jeff Muizelaar [:jrmuizel] from comment #31)
> Comment on attachment 584889 [details] [diff] [review]
> Part 4: Adjust gfxUtils to be compatible with the Azure Wrapper. v2
> 
> Review of attachment 584889 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxUtils.cpp
> @@ +305,2 @@
> >              return;
> >  
> 
> What about on Azure-cairo?

As soon as it can exists fully I can tell how it behaves and if it needs a special work-around :). We can detect the Azure-backend then and deal with it.
Comment 33 Bas Schouten (:bas.schouten) 2012-01-04 08:21:37 PST
(In reply to Jeff Muizelaar [:jrmuizel] from comment #30)
> Comment on attachment 582003 [details] [diff] [review]
> Part 5: Adjust gfxWindowsNativeDrawing to be compatible with the Azure
> Wrapper.
> 
> Review of attachment 582003 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxWindowsNativeDrawing.cpp
> @@ +74,5 @@
> > +        if (mContext->GetCairo()) {
> > +          surf = mContext->CurrentSurface(&mDeviceOffset.x, &mDeviceOffset.y);
> > +        }
> > +
> > +        if (surf && surf->CairoStatus())
> 
> surf is used uninitialized here.
> 
> @@ +217,5 @@
> > +    nsRefPtr<gfxASurface> surf;
> > +    
> > +    if (mContext->GetCairo()) {
> > +      surf = mContext->CurrentSurface(&mDeviceOffset.x, &mDeviceOffset.y);
> > +    }
> 
> I'd rather this be written something like:
>  if (!mContext->GetCairo()) {
>   // azure
>   return true;
>  }
> 
> Also what are we doing in the situation where we have a cairo backed Azure
> context?

This is really just a performance improvement to draw to the DC directly. Presumably we'll do GDI drawing through Skia and might be able to get a DC out from there, let's deal with that then.
Comment 34 Bas Schouten (:bas.schouten) 2012-01-04 08:25:55 PST
(In reply to Jeff Muizelaar [:jrmuizel] from comment #28)
> Comment on attachment 584917 [details] [diff] [review]
> Part 2: Add wrapper for for gfxContext v5
> 
> Review of attachment 584917 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxPattern.h
> @@ +128,5 @@
> >  protected:
> >      cairo_pattern_t *mPattern;
> > +
> > +    union {
> > +      mozilla::AlignedStorage2<mozilla::gfx::ColorPattern> mColorPattern;
> 
> Why do you need to use AlignedStorage2 here? is the natural alignment of
> ColorPattern etc. insufficient?

This is to create an area of memory large enough to contain any of the supported pattern types, without running any of their constructors. (Running multiple patterns with their constructors in the same area of memory would never really give desired behavior, both perf and correctness wise)
Comment 35 Bas Schouten (:bas.schouten) 2012-01-04 08:27:33 PST
(In reply to Jeff Muizelaar [:jrmuizel] from comment #29)
> Comment on attachment 584914 [details] [diff] [review]
> Part 11: Allow using Azure for content drawing with D3D10 layers.
> 
> Review of attachment 584914 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/d3d10/ThebesLayerD3D10.cpp
> @@ +363,5 @@
> > +    ctx->Translate(-gfxPoint(aOffset.x, aOffset.y));
> > +    gfxUtils::PathFromRegion(ctx, aRegion);
> > +    ctx->SetColor(aColor);
> > +    ctx->Fill();
> > +  }
> 
> Why don't we need this?

This is only used for ComponentAlpha drawing to fill the white/black background for alpha-recovery. We don't support component alpha right now with Azure, so we don't need this.
Comment 36 Jeff Muizelaar [:jrmuizel] 2012-01-04 13:28:40 PST
(In reply to Bas Schouten (:bas) from comment #34)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #28)
> > Why do you need to use AlignedStorage2 here? is the natural alignment of
> > ColorPattern etc. insufficient?
> 
> This is to create an area of memory large enough to contain any of the
> supported pattern types, without running any of their constructors. (Running
> multiple patterns with their constructors in the same area of memory would
> never really give desired behavior, both perf and correctness wise)

That makes sense. I was confused by the 2 in the name AlignedStorage2.
Comment 37 Bas Schouten (:bas.schouten) 2012-01-04 15:03:04 PST
Created attachment 585909 [details] [diff] [review]
Part 2: Add wrapper for for gfxContext v6

Updated as per review comments.
Comment 38 Bas Schouten (:bas.schouten) 2012-01-04 15:03:54 PST
Created attachment 585910 [details] [diff] [review]
Part 1.5: Allow multiplying a Size with a Matrix.

Required to implement Part 2 review comments.
Comment 39 Bas Schouten (:bas.schouten) 2012-01-04 19:00:49 PST
Created attachment 585965 [details] [diff] [review]
Part 5: Adjust gfxWindowsNativeDrawing to be compatible with the Azure Wrapper. V2

Adjusted for review comments.
Comment 40 Jeff Muizelaar [:jrmuizel] 2012-01-04 19:02:52 PST
Comment on attachment 585965 [details] [diff] [review]
Part 5: Adjust gfxWindowsNativeDrawing to be compatible with the Azure Wrapper. V2

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

::: gfx/thebes/gfxWindowsNativeDrawing.cpp
@@ +221,5 @@
>      nsRefPtr<gfxASurface> surf = mContext->CurrentSurface(&mDeviceOffset.x, &mDeviceOffset.y);
>      if (!surf || surf->CairoStatus())
>          return false;
>      if (surf->GetType() != gfxASurface::SurfaceTypeWin32 &&
> +       surf->GetType() != gfxASurface::SurfaceTypeWin32Printing) {

accidental indentation change here?
Comment 42 Jonathan Kew (:jfkthame) 2012-01-05 02:00:24 PST
Comment on attachment 581999 [details] [diff] [review]
Part 3: Adjust font code to be compatible with Azure.

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

A couple of drive-by nits......

::: gfx/thebes/gfxFont.cpp
@@ +1255,5 @@
> +    gfxRGBA color;
> +    ColorPattern colPat(Color(0, 0, 0, 0));
> +
> +    if (!aContext->IsAzure()) {
> +      bool success = SetupCairoFont(aContext);

In gfx, the convention is 4-space indentation.

@@ +1257,5 @@
> +
> +    if (!aContext->IsAzure()) {
> +      bool success = SetupCairoFont(aContext);
> +      if (NS_UNLIKELY(!success))
> +          return;

And if()-bodies are supposed to be braced, even when it's a single line.

@@ +1385,5 @@
> +
> +      // draw any remaining glyphs
> +      glyphs.Flush(cr, aDrawToPath, isRTL, true);
> +
> +    } else {

There are a number of 2-space indentations within this block.

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