Convert gfxPattern to Moz2D

RESOLVED FIXED in mozilla35

Status

()

Core
Graphics: Layers
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

35 Branch
mozilla35
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
gfxPattern uses cairo patterns internally, but we only ever use them with Moz2D.
(Assignee)

Comment 1

4 years ago
Created attachment 8474357 [details] [diff] [review]
Part 1: Remove dead code
Attachment #8474357 - Flags: review?(bas)
(Assignee)

Comment 2

4 years ago
Created attachment 8474358 [details] [diff] [review]
Part 2: Remove callers of DrawTarget::Mask

This was dead code, just not all that obvious.
Attachment #8474358 - Flags: review?(bas)
(Assignee)

Comment 3

4 years ago
Created attachment 8474359 [details] [diff] [review]
Part 3: Remove DrawTarget::Mask

I'm not sure if we actually want to do this or not. We no longer have any callers in mozilla-central, but it might be worth keeping the functionality in Moz2D.
Attachment #8474359 - Flags: review?(bas)
(Assignee)

Comment 4

4 years ago
Created attachment 8474360 [details] [diff] [review]
Part 3 (was part 4): Convert gfxPattern to Moz2D
Attachment #8474360 - Flags: review?(bas)
Attachment #8474357 - Flags: review?(bas) → review+
Attachment #8474358 - Flags: review?(jwatt)
Attachment #8474358 - Flags: review?(bas)
Attachment #8474358 - Flags: review+
Comment on attachment 8474359 [details] [diff] [review]
Part 3: Remove DrawTarget::Mask

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

I feel we shouldn't do this, this functionality could very well be useful in the future.
Attachment #8474359 - Flags: review?(bas) → review-
Comment on attachment 8474360 [details] [diff] [review]
Part 3 (was part 4): Convert gfxPattern to Moz2D

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

Awesome work!

::: gfx/thebes/gfxPattern.cpp
@@ +64,5 @@
>  void
>  gfxPattern::AddColorStop(gfxFloat offset, const gfxRGBA& c)
>  {
> +  if (mGfxPattern->GetType() != PatternType::LINEAR_GRADIENT &&
> +      mGfxPattern->GetType() != PatternType::RADIAL_GRADIENT) {

nit: little warning here maybe?

@@ +72,5 @@
> +  mStops = nullptr;
> +  gfxRGBA color = c;
> +  if (gfxPlatform::GetCMSMode() == eCMSMode_All) {
> +    qcms_transform *transform = gfxPlatform::GetCMSRGBTransform();
> +    if (transform)

nit { }

@@ +125,5 @@
>  Pattern*
>  gfxPattern::GetPattern(DrawTarget *aTarget, Matrix *aPatternTransform)
>  {
> +  if (!mStops &&
> +      !mStopsList.IsEmpty()) {

nit: MOZ_ASSERT(mGfxPattern->GetType() etc.

@@ +190,5 @@
>  
>  void
>  gfxPattern::SetFilter(GraphicsFilter filter)
>  {
> +  if (mGfxPattern->GetType() != PatternType::SURFACE) {

nit: Might want to add a little warning here to remind someone they're doing something silly.

::: gfx/thebes/gfxPattern.h
@@ +123,5 @@
>  
>      mozilla::RefPtr<mozilla::gfx::SourceSurface> mSourceSurface;
>      mozilla::gfx::Matrix mTransform;
>      mozilla::RefPtr<mozilla::gfx::GradientStops> mStops;
> +    nsTArray<mozilla::gfx::GradientStop> mStopsList;

I prefer std::vector, but it seems we can never figure out which one we prefer so I'll leave it up to you :).
Attachment #8474360 - Flags: review?(bas) → review+
(Assignee)

Comment 7

4 years ago
(In reply to Bas Schouten (:bas.schouten) from comment #5)
> Comment on attachment 8474359 [details] [diff] [review]
> Part 3: Remove DrawTarget::Mask
> 
> Review of attachment 8474359 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I feel we shouldn't do this, this functionality could very well be useful in
> the future.

I figured you might say that :) No real harm in keeping except that it's probably pretty buggy (in some backends).

Updated

4 years ago
Attachment #8474358 - Flags: review?(jwatt) → review+
Happy days. :)
(Assignee)

Comment 9

4 years ago
Created attachment 8482462 [details] [diff] [review]
Part 4 (was part 5): Stop using the gfxASurface ctor for gfxPattern in gfxWindowsNativeDrawing

Missed this ctor.

I also had to fuzz some tests to get it all to pass. Going through a cairo_pattern_t reduced all values to cairo_fixed_t precision which hid some bugs where the tests weren't actually the same.
Attachment #8482462 - Flags: review?(bas)
Comment on attachment 8482462 [details] [diff] [review]
Part 4 (was part 5): Stop using the gfxASurface ctor for gfxPattern in gfxWindowsNativeDrawing

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

::: gfx/thebes/gfxWindowsNativeDrawing.cpp
@@ +268,5 @@
>          if (!gfxAlphaRecovery::RecoverAlpha(black, white)) {
>              NS_ERROR("Alpha recovery failure");
>              return;
>          }
> +        RefPtr<SourceSurface> source =

Any chance you could just convert this function to Moz2d?
(Assignee)

Comment 11

4 years ago
Not easily! That code relies heavily on gfxWindowsSurface to create DIBs and get the DC out. We don't have equivalent code for Moz2D yet. I'd rather not block this bug on doing that.
Attachment #8482462 - Flags: review?(bas) → review+

Updated

4 years ago
Attachment #8474359 - Attachment is obsolete: true

Updated

4 years ago
Attachment #8474360 - Attachment description: Part 4: Convert gfxPattern to Moz2D → Part 3 (was part 4): Convert gfxPattern to Moz2D

Updated

4 years ago
Attachment #8482462 - Attachment description: Part 5: Stop using the gfxASurface ctor for gfxPattern in gfxWindowsNativeDrawing → Part 4 (was part 5): Stop using the gfxASurface ctor for gfxPattern in gfxWindowsNativeDrawing
I pushed Matt's patches (they required a fair bit of unbitrotting as well as changes to includes to get unified builds working):

https://hg.mozilla.org/integration/mozilla-inbound/rev/55948a19e455
https://hg.mozilla.org/integration/mozilla-inbound/rev/771d12cf1f21
https://hg.mozilla.org/integration/mozilla-inbound/rev/de43b84e2afc
https://hg.mozilla.org/integration/mozilla-inbound/rev/936fe5626189

I also had to mark four reftests as passing now and change the fuzz for others:

https://hg.mozilla.org/integration/mozilla-inbound/rev/826f233564b4
OS: Mac OS X → All
Hardware: x86 → All
Version: 29 Branch → 35 Branch
You need to log in before you can comment on or make changes to this bug.