[Azure] Create Thebes Wrapper around Azure

RESOLVED FIXED in mozilla12

Status

()

defect
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: bas.schouten, Assigned: bas.schouten)

Tracking

unspecified
mozilla12
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(12 attachments, 8 obsolete attachments)

5.13 KB, patch
joe
: review+
Details | Diff | Splinter Review
22.15 KB, patch
joe
: review+
Details | Diff | Splinter Review
831 bytes, patch
joe
: review+
Details | Diff | Splinter Review
2.61 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
1.74 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.45 KB, patch
Details | Diff | Splinter Review
1.23 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
5.65 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.62 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
69.74 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
915 bytes, patch
jrmuizel
: review+
Details | Diff | Splinter Review
3.35 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
Assignee

Description

8 years ago
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.
Assignee

Comment 2

8 years ago
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.
Attachment #581977 - Flags: feedback?(joe)
Assignee

Updated

8 years ago
Attachment #581975 - Attachment description: Add gfx2DGlue code to Thebes for Azure wrapper. → Part 1: Add gfx2DGlue code to Thebes for Azure wrapper.
Assignee

Updated

8 years ago
Attachment #582003 - Flags: review? → review?(joe)
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.
Attachment #581975 - Flags: review?(joe) → review+
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?
Attachment #581977 - Flags: feedback?(joe) → feedback+
Attachment #582004 - Flags: review?(joe) → review+
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?
Attachment #582003 - Flags: review?(joe) → review-
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.
Attachment #582002 - Flags: review?(joe) → review-
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.
Attachment #581999 - Flags: review?(joe) → review+
Assignee

Comment 12

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

Comment 13

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

Comment 14

8 years ago
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.
Attachment #581977 - Attachment is obsolete: true
Attachment #584499 - Flags: review?(jmuizelaar)
Assignee

Comment 15

8 years ago
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.
Attachment #582003 - Flags: review- → review?(jmuizelaar)
Assignee

Comment 16

8 years ago
Properly qreffed this time.
Attachment #584499 - Attachment is obsolete: true
Attachment #584499 - Flags: review?(jmuizelaar)
Attachment #584514 - Flags: review?(jmuizelaar)
Assignee

Comment 17

8 years ago
Less hacky version as requested.
Attachment #582002 - Attachment is obsolete: true
Attachment #584889 - Flags: review?(jmuizelaar)
Assignee

Comment 20

8 years ago
Corrected !IsAzure() -> IsCairo()
Attachment #584893 - Attachment is obsolete: true
Attachment #584893 - Flags: review?(roc)
Attachment #584894 - Flags: review?(roc)
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 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?
Assignee

Comment 23

8 years ago
Updated to avoid some needless compiler warnings.
Attachment #584514 - Attachment is obsolete: true
Attachment #584514 - Flags: review?(jmuizelaar)
Attachment #584903 - Flags: review?(jmuizelaar)
Assignee

Comment 27

8 years ago
Adjusted for roc's comments.
Attachment #584903 - Attachment is obsolete: true
Attachment #584903 - Flags: review?(jmuizelaar)
Attachment #584917 - Flags: review?(jmuizelaar)
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?
Attachment #584917 - Flags: review?(jmuizelaar) → review-
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?
Attachment #584914 - Flags: review?(jmuizelaar) → review+
Attachment #584911 - Flags: review?(jmuizelaar) → review+
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?
Attachment #582003 - Flags: review?(jmuizelaar) → review-
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?
Attachment #584889 - Flags: review?(jmuizelaar) → review+
Assignee

Comment 32

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

Comment 33

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

Comment 34

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

Comment 35

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

Comment 37

8 years ago
Updated as per review comments.
Attachment #584917 - Attachment is obsolete: true
Attachment #585909 - Flags: review?(jmuizelaar)
Assignee

Comment 38

8 years ago
Required to implement Part 2 review comments.
Attachment #585910 - Flags: review?(jmuizelaar)
Attachment #585910 - Flags: review?(jmuizelaar) → review+
Attachment #585909 - Flags: review?(jmuizelaar) → review+
Assignee

Comment 39

8 years ago
Adjusted for review comments.
Attachment #582003 - Attachment is obsolete: true
Attachment #585965 - Flags: review?(jmuizelaar)
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?
Attachment #585965 - Flags: review?(jmuizelaar) → review+
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.
Depends on: 715763
Assignee

Updated

8 years ago
No longer depends on: 715763
Assignee

Updated

8 years ago
No longer depends on: 715658

Updated

7 years ago
Depends on: 780018
Depends on: 840869

Updated

6 years ago
Depends on: 854296

Updated

4 years ago
Depends on: 1131264
Assignee

Updated

4 years ago
Attachment #584894 - Flags: review?(roc)
You need to log in before you can comment on or make changes to this bug.