Closed Bug 892966 Opened 7 years ago Closed 6 years ago

Fix bugs with Quartz azure content

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Details

Attachments

(7 files, 3 obsolete files)

664 bytes, patch
jrmuizel
: review+
Details | Diff | Splinter Review
3.54 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
2.06 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
926 bytes, patch
jrmuizel
: review+
Details | Diff | Splinter Review
11.30 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
6.54 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
3.82 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
No description provided.
Attachment #774627 - Flags: review?(jmuizelaar)
Attachment #774628 - Flags: review?(jmuizelaar)
Const cast is pretty awful, but imglib seems to only call Flush and not MarkDirty for quartz surfaces.
Attachment #774629 - Flags: review?(jmuizelaar)
Attachment #774630 - Flags: review?(jmuizelaar)
This should pass tests now.
Attachment #774631 - Flags: review?(jmuizelaar)
Comment on attachment 774626 [details] [diff] [review]
Take a reference to the CGContext when create a DT wrapping it

Yes please.
Attachment #774626 - Flags: review?(jmuizelaar) → review+
Attachment #774631 - Flags: review?(jmuizelaar) → review+
This fixes at least 3 reftest failures.
Attachment #774634 - Flags: review?(jmuizelaar)
Attachment #774630 - Flags: review?(jmuizelaar) → review+
Comment on attachment 774634 [details] [diff] [review]
Respect the filter property of SurfacePattern

Fixed by bug 892968
Attachment #774634 - Attachment is obsolete: true
Attachment #774634 - Flags: review?(jmuizelaar)
Comment on attachment 774628 [details] [diff] [review]
Make gfxQuartzNativeDrawing support DrawTargetDual surfaces

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

::: gfx/thebes/gfxQuartzNativeDrawing.cpp
@@ +48,5 @@
> +                            NSToIntFloor(mNativeRect.height * mBackingScale));
> +        CGColorSpaceRef colorSpace = CGColorSpaceCreateDeviceRGB();
> +        mCGContext = CGBitmapContextCreate(nullptr, backingSize.width, backingSize.height,
> +                                           8, backingSize.width * 4, colorSpace,
> +                                           kCGBitmapByteOrder32Host | kCGImageAlphaPremultipliedFirst);

Instead of doing this we should just create a new DrawTargetCG. We can change the borrow api to have a separate Init function instead of using the constructor. That gets rid of the null check and we can reuse the the same BorrowedCG for both cases.
Attachment #774628 - Flags: review?(jmuizelaar) → review-
Comment on attachment 774627 [details] [diff] [review]
Make gfxQuartzNativeDrawing support Moz2D surface

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

This should be joined with the DrawTargetDual stuff.

::: gfx/2d/DrawTargetCG.cpp
@@ +1181,5 @@
>  BorrowedCGContext::BorrowCGContextFromDrawTarget(DrawTarget *aDT)
>  {
> +  if (!aDT) {
> +    return nullptr;
> +  }

How about moving this check to the constructor and explaining it a bit there.
Attachment #774627 - Flags: review?(jmuizelaar) → review-
Attachment #774627 - Attachment is obsolete: true
Attachment #774628 - Attachment is obsolete: true
Attachment #774694 - Flags: review?(jmuizelaar)
Attachment #774629 - Flags: review?(jmuizelaar)
Comment on attachment 774694 [details] [diff] [review]
Make gfxQuartzNativeDrawing support Moz2D surfaces

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

::: gfx/2d/2D.h
@@ +1016,5 @@
>  public:
> +  BorrowedCGContext()
> +    : cg(nullptr)
> +    , mDT(nullptr)
> +  { }

How would feel about having two versions of the constructor. One that takes a DrawTarget and one that doesn't. This will make the code in nsChildView.mm a bit more concise.

@@ +1018,5 @@
> +    : cg(nullptr)
> +    , mDT(nullptr)
> +  { }
> +
> +  CGContextRef Init(DrawTarget *aDT)

Maybe add a comment like "Init is separated from construction because we may not know the DrawTarget we're going to use at construction time if the BorrowedCGContext is a member of another class".

::: gfx/thebes/gfxQuartzNativeDrawing.cpp
@@ +37,5 @@
>  {
>      NS_ASSERTION(!mQuartzSurface, "BeginNativeDrawing called when drawing already in progress");
>  
> +    if (!mContext->IsCairo()) {
> +      mDrawTarget = mContext->GetDrawTarget();

I don't think you need to set mDrawTarget when we're not creating a new one.

i.e. if (mContext->GetDrawTarget()->IsDualDrawTarget()) should work fine.
Attachment #774694 - Flags: review?(jmuizelaar) → review+
Comment on attachment 774745 [details] [diff] [review]
Add CreateSourceSurfaceFromNativeSurface for CG

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

::: gfx/2d/SourceSurfaceCG.cpp
@@ +254,5 @@
> +{
> +  MOZ_ASSERT(cg);
> +  MOZ_ASSERT(GetContextType(cg) == CG_CONTEXT_TYPE_BITMAP);
> +
> +  mCg = CGContextRetain(cg);

Add to the member declaration about how it is now a strong reference to a CGContext.
Attachment #774745 - Flags: review?(jmuizelaar) → review+
Attachment #774865 - Flags: review?(jmuizelaar)
Comment on attachment 774865 [details] [diff] [review]
Setup dashing correctly with StrokeRect

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

::: gfx/2d/DrawTargetCG.h
@@ +68,5 @@
>  }
>  
> +static inline void
> +SetStrokeOptions(CGContextRef cg, const StrokeOptions &aStrokeOptions)
> +{

Let's just have SetStrokeOptions and use that everywhere.
Attachment #774865 - Flags: review?(jmuizelaar) → review+
Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/67eb0198434e as being the first most likely thing in the wide range when we weren't building on Mac at all to have caused https://tbpl.mozilla.org/php/getParsedLog.php?id=25242777&tree=Mozilla-Inbound once we started building again.
Comment on attachment 774629 [details] [diff] [review]
Reset our cached SourceSurface when MarkDirty/Flush is called on the gfxASurface

Turns out we do need this.

I guess Quartz has internal caches of CGImages (the downscaled ones maybe?) that don't get updated when we modify the bits.
Attachment #774629 - Flags: review?(jmuizelaar)
Attachment #774629 - Flags: review?(jmuizelaar) → review+
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.