Closed Bug 1054838 Opened 6 years ago Closed 5 years ago

Convert gfxPattern to Moz2D

Categories

(Core :: Graphics: Layers, defect)

35 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(4 files, 1 obsolete file)

gfxPattern uses cairo patterns internally, but we only ever use them with Moz2D.
Attachment #8474357 - Flags: review?(bas)
This was dead code, just not all that obvious.
Attachment #8474358 - Flags: review?(bas)
Attached patch Part 3: Remove DrawTarget::Mask (obsolete) — Splinter Review
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)
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+
(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).
Attachment #8474358 - Flags: review?(jwatt) → review+
Happy days. :)
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?
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+
Attachment #8474359 - Attachment is obsolete: true
Attachment #8474360 - Attachment description: Part 4: Convert gfxPattern to Moz2D → Part 3 (was part 4): Convert gfxPattern to Moz2D
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.