Closed Bug 1042259 Opened 6 years ago Closed 6 years ago

Cleanup color space usage in DrawTargetCG

Categories

(Core :: Graphics, defect)

26 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: milan, Assigned: milan)

Details

Attachments

(1 file, 1 obsolete file)

Clean up a few "to do" type comments in DrawTargetCG.
* use vector to pass in stops, like DrawTargetSkia does (sorting before hand)
* color spaces are created/destroyed too often
Assignee: nobody → milan
Comment on attachment 8460437 [details] [diff] [review]
Stops in a vector like Skia and reuse color spaces. r=mstange

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

I like the cleanup! Looks good except for the style issues and for the sorting change which I don't understand.

::: gfx/2d/DrawTargetCG.cpp
@@ +139,5 @@
>    }
>  }
>  
>  
> +  DrawTargetCG::DrawTargetCG() : mColorSpace(nullptr), mCg(nullptr), mSnapshot(nullptr)

Common style for this is:
DrawTargetCG::DrawTargetCG()
  : mColorSpace(nullptr)
  , mCg(nullptr)
  , mSnapshot(nullptr)

Actually, initializing mSnapshot to nullptr isn't necessary, so you may want to drop that.

@@ +381,5 @@
>  {
>    public:
>    MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(GradientStopsCG)
> +
> +  /// The stops must be sorted non-decreasing by the caller

Why?

@@ +393,3 @@
>      mExtend = aExtendMode;
>      if (aExtendMode == ExtendMode::CLAMP) {
> +      uint32_t numStops = aStops.size();

size_t

@@ +415,2 @@
>      } else {
>        mGradient = nullptr;

You can remove this line because you're already handling it in the initialization list.

@@ +436,5 @@
>  TemporaryRef<GradientStops>
>  DrawTargetCG::CreateGradientStops(GradientStop *aStops, uint32_t aNumStops,
>                                    ExtendMode aExtendMode) const
>  {
> +  std::vector<GradientStop> stops;

Instead of the loop you should be able to do just
std::vector<GradientStop> stops(aStops, aStops + aNumStops);

@@ +441,5 @@
> +  stops.reserve(aNumStops);
> +  for (uint32_t i = 0; i < aNumStops; i++) {
> +    stops.push_back( aStops[i] );
> +  }
> +  std::stable_sort(stops.begin(), stops.end());

I don't see any sorting in the old code. Is it necessary?

@@ +571,5 @@
>    return gradient;
>  }
>  
>  static void
> +DrawLinearRepeatingGradient( CGColorSpaceRef aColorSpace, CGContextRef cg,

no space after ( please

@@ +610,5 @@
>    return hypot(a.x-b.x, a.y-b.y);
>  }
>  
>  static void
> +DrawRadialRepeatingGradient( CGColorSpaceRef aColorSpace, CGContextRef cg,

no space after ( please
(In reply to Markus Stange [:mstange] from comment #2)
> Comment on attachment 8460437 [details] [diff] [review]
> Stops in a vector like Skia and reuse color spaces. r=mstange
> 
> Review of attachment 8460437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like the cleanup! Looks good except for the style issues and for the
> sorting change which I don't understand.

I'll take care of the style changes, and you're right, the sorting is a part of another changeset, it isn't necessary with this one; I'll take it out.
Addressing the style issues and removing sorting; it isn't needed at this time.
Attachment #8460437 - Attachment is obsolete: true
Attachment #8460437 - Flags: review?(mstange)
Attachment #8460989 - Flags: review?(mstange)
Attachment #8460989 - Flags: review?(mstange) → review+
https://hg.mozilla.org/mozilla-central/rev/2ccd1be83593
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.