Closed Bug 1385929 Opened 3 years ago Closed 3 years ago

Find a way to prevent duplicate gfxContext::Save/Restore

Categories

(Core :: Web Painting, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: u459114, Assigned: u459114)

References

Details

(Whiteboard: [qf])

Attachments

(2 files, 1 obsolete file)

In bug 1385861, I notice again duplicate gfxContext::Save/Restore.

We probabaly should find a mechanism to prevent it. For example, showing a warning massage(via NS_ASSERTION) if the state of a context is not changed between to gfxContext::Save calls.
Attached patch WIP (obsolete) — Splinter Review
Several test cases crashed by applying this patch
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a14988676a61d13f17691ebdc03c14e79623541&selectedJob=119924163
gfxContext::Save in gfxContext::PushGroupForBlendBack gfxContext::PushGroupAndCopyBackground
gfxContext::Restore in gfxContext::PopGroupAndBlend()

Actually, gfxContext::PushGroupForBlendBack/PushGroupAndCopyBackground do not change AzureState state at all.

Take gfxContext::PushGroupForBlendBack as an example
void
gfxContext::PushGroupForBlendBack(gfxContentType content, Float aOpacity, SourceSurface* aMask, const Matrix& aMaskTransform)
{
  mDT->PushLayer(content == gfxContentType::COLOR, aOpacity, aMask, aMaskTransform);
}

Since the context of mDT is not part of AzureState, this function does not change the content of gfxContext::CurrentState()
Attachment #8892387 - Attachment is obsolete: true
Priority: -- → P2
Whiteboard: [qf]
Attachment #8892440 - Flags: review?(matt.woodrow)
Attachment #8892441 - Flags: review?(matt.woodrow)
Comment on attachment 8892440 [details]
Bug 1385929 - Part 1. Check whether the content of the persisted state change.

https://reviewboard.mozilla.org/r/163392/#review170946

::: gfx/thebes/gfxContext.cpp:143
(Diff revision 6)
>  void
>  gfxContext::Restore()
>  {
> +#ifdef DEBUG
> +  // gfxContext::Restore is used to restore AzureState. We need to restore it
> +  // only if it was altered. The following APIs do change the context of

do change the content?

::: gfx/thebes/gfxContext.cpp:156
(Diff revision 6)
> +  //    only thing that you altered in the target context.
> +  // 3. Function of setup transform matrix, such as Multiply() and
> +  //    SetMatrix(). Using gfxContextMatrixAutoSaveRestore is more recommended
> +  //    if transform data is the only thing that you are going to alter.
> +  //
> +  // You will hit the assertion message bellow if there is no above functions

below

::: gfx/thebes/gfxContext.cpp:1038
(Diff revision 6)
>   * the state and might be false for the restored AzureState.
>   */
>  void
>  gfxContext::ChangeTransform(const Matrix &aNewMatrix, bool aUpdatePatternTransform)
>  {
> +  CURRENTSTATE_CHANGED(aUpdatePatternTransform)

Can we just move this down into the if() statement below where we actually change the state?

Seems like we could get rid of the parameter to CURRENTSTATE_CHANGED then too.
Attachment #8892440 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8892441 [details]
Bug 1385929 - Part 2. Remove unecessary gfxContext::Save/Restore found by Part 1.

https://reviewboard.mozilla.org/r/163394/#review170948
Attachment #8892441 - Flags: review?(matt.woodrow) → review+
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/027153981c66
Part 1. Check whether the content of the persisted state change. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/fd50ab554397
Part 2. Remove unecessary gfxContext::Save/Restore found by Part 1. r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/027153981c66
https://hg.mozilla.org/mozilla-central/rev/fd50ab554397
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.