Closed Bug 1292192 Opened 5 years ago Closed 5 years ago

Simplify CanvasRenderingContext2D::SwitchRenderingMode


(Core :: Graphics: Layers, defect)

Not set



Tracking Status
firefox50 --- affected
firefox51 --- fixed


(Reporter: nical, Assigned: nical)




(1 file, 1 obsolete file)

SwitchRenderingMode is a bit complicated:
* It has a separate code path for when there is no BufferProvider, but I recently made it so we always have one if we have a target, so we don't need the two code paths.
* It also restores only the clips at the top of the sytle stack instead of restoring all clips, and in fact it does that after EnsureTarget which has already restored all clips, so it should not restore clips at all.
* It can return the old target without popping clips
* it use DrawSurface which is affected by the transform after having set the transform to restore the pixels, and should use CopySurface instead.

The list is already long enough for me to not be sure whether that has something to do with a canvas crash I am investigating.
Let's simplify and fix that.
Attached patch Simplify SwitchRenderingMode (obsolete) — Splinter Review
Attachment #8777859 - Flags: review?(gwright)
Comment on attachment 8777859 [details] [diff] [review]
Simplify SwitchRenderingMode

Review of attachment 8777859 [details] [diff] [review]:

Looks fine to me, but the code is still a little confusing. Correct me if I'm wrong, but having mTarget implies we have mBufferProvider, but not vice versa, right?

Because of the logic in IsTargetValid(), I think that means we can safely assume there's always mBufferProvider in this method, because if mTarget is nullptr we will hit the other half of the boolean checking for mBufferProvider, and if we have mTarget that implies we must have mBufferProvider. In that case, should we be asserting whether mBufferProvider exists, rather than simply checking if and returning false?

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +1341,3 @@
>    RefPtr<PersistentBufferProvider> oldBufferProvider = mBufferProvider;
> +
> +  // Return the old target to the buffer provider, if any.

I first read this comment as meaning that we might not have a buffer provider. If it means what I think it means, it'd probably be better worded as "If we have an old target, return it to the buffer provider".
Attachment #8777859 - Flags: review?(gwright) → review+
I hadn't noticed that EnsureTarget did not restore the clips and transform in all of its code paths. This patch makes sure that EnsureDrawTarget is responsible for doing so in all cases after borrowing the DT from the provider or after doing whichever skiagl dance.
The patch also addresses previous review comments.
Attachment #8777859 - Attachment is obsolete: true
Attachment #8778242 - Flags: review?(gwright)
Pushed by
Simplify CanvasRenderingContext2D::SwitchRenderingMode. r=gw280
This is in the middle of a series of patches that fix various issues both on central and inbound so I am marking it as affecting the two trains as reminder, even though it isn't filed against a specific issue.
(In reply to Nicolas Silva [:nical] from comment #5)
> both in central and inbound 

I meant central and aurora.
Attachment #8778242 - Flags: review?(gwright) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1294351
You need to log in before you can comment on or make changes to this bug.