Hookup CoreGraphics Azure content

RESOLVED FIXED in mozilla17

Status

()

RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: nical)

Tracking

unspecified
mozilla17
x86
macOS
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
Posted patch WIP patchSplinter Review
Here's a current work in progress patch. It is nearly usable, but I did run into a hang/loop in the region code when opening the awesome bar.
(Assignee)

Comment 1

7 years ago
I extracted the part related to creating a DrawTarget with pre-allocated data from the WIP patch.
In DrawTargetCG two of the Init() functions were nearly identical, I made it so that one uses the other to avoid code duplication.

In the existing code there is a "// xxx handle format" comment and then we use 32bit BGRA always rather than using the parameter aFormat. Is there a reason we don't use aFormat?
Attachment #645340 - Flags: review?(jmuizelaar)
(Assignee)

Updated

7 years ago
Blocks: 776957
(Reporter)

Comment 2

7 years ago
Comment on attachment 645340 [details] [diff] [review]
Support CreateDrawTargetForData with Azure's CoreGraphics backend.

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

::: gfx/2d/DrawTargetCG.cpp
@@ +821,5 @@
> +  //XXX: we'd be better off reusing the Colorspace across draw targets
> +  mColorSpace = CGColorSpaceCreateDeviceRGB();
> +
> +  mSize = aSize;
> +  mData = NULL;

Add a comment about how mData = NULL means we don't own the data.

@@ +913,5 @@
>    // XXX: currently we allocate ourselves so that we can easily return a gfxImageSurface
>    // we might not need to later if once we don't need to support gfxImageSurface
> +  // XXX: currently Init implicitly clears, that can often be a waste of time
> +  int stride = aSize.width*4;
> +  void *data = calloc(aSize.height * stride, 1);

I wonder if this would be cleaner if we just only allocated if the data parameter was NULL. Then we could avoid duplicating the tests at the top.

@@ +918,2 @@
>  
> +  bool status = Init((unsigned char*)data, aSize, stride, aFormat);

Use a c++ style cast
Attachment #645340 - Flags: review?(jmuizelaar) → review-
(Assignee)

Comment 3

7 years ago
Attachment #645340 - Attachment is obsolete: true
Attachment #645823 - Flags: review?(jmuizelaar)
(Reporter)

Comment 4

7 years ago
Comment on attachment 645823 [details] [diff] [review]
Support CreateDrawTargetForData with Azure's CoreGraphics backend.

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

::: gfx/2d/DrawTargetCG.cpp
@@ +821,5 @@
> +  //XXX: we'd be better off reusing the Colorspace across draw targets
> +  mColorSpace = CGColorSpaceCreateDeviceRGB();
> +
> +  if (aData == NULL) {
> +    mData = calloc(aSize.height * aStride, 1);

You dropped the comment about implicitly clearing with calloc(). You should add it back.

@@ +824,5 @@
> +  if (aData == NULL) {
> +    mData = calloc(aSize.height * aStride, 1);
> +    aData = static_cast<unsigned char*>(mData);  
> +  } else {
> +    mData = NULL;

It's probably worth adding a comment about how mData == NULL means we don't own the data.
Attachment #645823 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 5

7 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> Comment on attachment 645823 [details] [diff] [review]
> Support CreateDrawTargetForData with Azure's CoreGraphics backend.
> 
> Review of attachment 645823 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/DrawTargetCG.cpp
> @@ +821,5 @@
> > +  //XXX: we'd be better off reusing the Colorspace across draw targets
> > +  mColorSpace = CGColorSpaceCreateDeviceRGB();
> > +
> > +  if (aData == NULL) {
> > +    mData = calloc(aSize.height * aStride, 1);
> 
> You dropped the comment about implicitly clearing with calloc(). You should
> add it back.

oops
 
> @@ +824,5 @@
> > +  if (aData == NULL) {
> > +    mData = calloc(aSize.height * aStride, 1);
> > +    aData = static_cast<unsigned char*>(mData);  
> > +  } else {
> > +    mData = NULL;
> 
> It's probably worth adding a comment about how mData == NULL means we don't
> own the data.

It is, and I explained it in a comment in the header above the declaration of mData. I can also comment here if you want.
(Assignee)

Comment 6

7 years ago
Attachment #645823 - Attachment is obsolete: true
Attachment #646216 - Flags: review?(jmuizelaar)
(Reporter)

Updated

7 years ago
Attachment #646216 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 7

7 years ago
Comment on attachment 646216 [details] [diff] [review]
Support CreateDrawTargetForData with Azure's CoreGraphics backend.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad87f9ac3e5d
Attachment #646216 - Flags: checkin+
(Assignee)

Updated

7 years ago
Whiteboard: [leave open]
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Assignee: nobody → nical.bugzilla
Whiteboard: [leave open]
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.