Closed
Bug 1210560
Opened 9 years ago
Closed 9 years ago
Convert PushGroup/PopGroup calls to specialized versions as much as possible
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: bas.schouten, Assigned: bas.schouten)
References
Details
Attachments
(8 files, 12 obsolete files)
4.54 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
9.37 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
20.62 KB,
patch
|
jwatt
:
review+
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
6.04 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
16.80 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
5.36 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
4.79 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
7.18 KB,
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
Eventually we want to convert most instances of PushGroup/PopGroup to a PushGroup/PopGroup API in Moz2D which will not feature the ability to pop to a separate pattern or surface. Users which do not need other functionality can already be converted to a similar API on gfxContext.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8668638 -
Flags: review?(jmuizelaar)
Updated•9 years ago
|
Attachment #8668638 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8671548 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 3•9 years ago
|
||
This is more involved SVG changes, would like review from Jwatt as well.
Attachment #8671549 -
Flags: review?(jwatt)
Attachment #8671549 -
Flags: review?(jmuizelaar)
Comment 4•9 years ago
|
||
Comment on attachment 8671548 [details] [diff] [review]
Part 2: Convert some simple users to PushGroupForBlendBack.
Review of attachment 8671548 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/basic/BasicLayerManager.cpp
@@ +108,5 @@
> + // If the layer is opaque in its visible region we can push a gfxContentType::COLOR
> + // group. We need to make sure that only pixels inside the layer's visible
> + // region are copied back to the destination. Remember if we've already
> + // clipped precisely to the visible region.
> + *aNeedsClipToVisibleRegion = !didCompleteClip || aRegion.GetNumRects() > 1;
Seems like this comment and the *aNeedsClipToVisibleRegion could be lifted out.
::: layout/svg/nsSVGPatternFrame.cpp
@@ +416,5 @@
>
> patternWithChildren->mSource = nullptr;
>
> if (aGraphicOpacity != 1.0f) {
> + gfx->PopGroupAndBlend();
The opacity is ignored here.
Attachment #8671548 -
Flags: review?(jmuizelaar) → review-
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8671548 -
Attachment is obsolete: true
Attachment #8671565 -
Flags: review?(jmuizelaar)
Updated•9 years ago
|
Attachment #8671565 -
Flags: review?(jmuizelaar) → review-
Assignee | ||
Comment 7•9 years ago
|
||
Fix a transform mistake and a use-after-free.
Attachment #8671549 -
Attachment is obsolete: true
Attachment #8671549 -
Flags: review?(jwatt)
Attachment #8671549 -
Flags: review?(jmuizelaar)
Attachment #8671722 -
Flags: review?(jwatt)
Attachment #8671722 -
Flags: review?(jmuizelaar)
Updated•9 years ago
|
Attachment #8671565 -
Flags: review- → review+
Comment 8•9 years ago
|
||
Comment on attachment 8671722 [details] [diff] [review]
Part 3: Convert more complex SVG usecases to use PushGroupForBlendBack and temporary surfaces. v2
Review of attachment 8671722 [details] [diff] [review]:
-----------------------------------------------------------------
Why doesn't this code PushClip for clipping to the ClipPath?
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> Comment on attachment 8671722 [details] [diff] [review]
> Part 3: Convert more complex SVG usecases to use PushGroupForBlendBack and
> temporary surfaces. v2
>
> Review of attachment 8671722 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Why doesn't this code PushClip for clipping to the ClipPath?
Trivial clips already work through applying it to the context. For other cases I just tried to stick as close to what's currently happening as possible.
Comment 10•9 years ago
|
||
I think I sort of found out my answer. As I understand it, we need to be able to construct the clip path using an arbitrary combination of unions and intersections of paths. e.g. (A and B) or C
Flags: needinfo?(jmuizelaar)
Comment 11•9 years ago
|
||
Comment on attachment 8671722 [details] [diff] [review]
Part 3: Convert more complex SVG usecases to use PushGroupForBlendBack and temporary surfaces. v2
Review of attachment 8671722 [details] [diff] [review]:
-----------------------------------------------------------------
I've only quickly looked this over. jwatt should do a more thorough review.
::: layout/svg/nsSVGUtils.cpp
@@ +711,5 @@
> }
>
> + if (aFrame->StyleDisplay()->mMixBlendMode != NS_STYLE_BLEND_NORMAL) {
> + RefPtr<DrawTarget> targetDT = target->GetDrawTarget();
> + target = &aContext;
This looks like a dead store.
Attachment #8671722 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 12•9 years ago
|
||
Updated a mistake in transforms.
Attachment #8671722 -
Attachment is obsolete: true
Attachment #8671722 -
Flags: review?(jwatt)
Attachment #8674267 -
Flags: review?(jwatt)
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8674267 [details] [diff] [review]
Part 3: Convert more complex SVG usecases to use PushGroupForBlendBack and temporary surfaces. v3
Carrying r+ from jrmuizel.
Attachment #8674267 -
Flags: review+
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8675435 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 15•9 years ago
|
||
Fixed some bugs with relation to context state management.
Attachment #8675435 -
Attachment is obsolete: true
Attachment #8675435 -
Flags: review?(jmuizelaar)
Attachment #8676302 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8676302 -
Attachment is obsolete: true
Attachment #8676302 -
Flags: review?(jmuizelaar)
Attachment #8677425 -
Flags: review?(jmuizelaar)
Comment 17•9 years ago
|
||
Comment on attachment 8677425 [details] [diff] [review]
Convert BasicLayers usecases to PushGroupForBlendback and temporary surfaces. v3
Review of attachment 8677425 [details] [diff] [review]:
-----------------------------------------------------------------
This adds a big hunk code to BasicLayerManager which isn't that appealing to me. Is it basically just implementing PushGroup inline?
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #17)
> Comment on attachment 8677425 [details] [diff] [review]
> Convert BasicLayers usecases to PushGroupForBlendback and temporary
> surfaces. v3
>
> Review of attachment 8677425 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> This adds a big hunk code to BasicLayerManager which isn't that appealing to
> me. Is it basically just implementing PushGroup inline?
Somewhat, it's a little more efficient as it avoids some dances around pattern management and state changes and such. I'm actually happy about that because it makes the cost paid by the intermediate surface creation explicitly visible.
Assignee | ||
Comment 19•9 years ago
|
||
This code doesn't ever get hit using our tests, nor do I see any way it ever would get hit. Let's just kill it rather than bother converting it.
Attachment #8678827 -
Flags: review?(roc)
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8678832 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•9 years ago
|
Attachment #8678832 -
Attachment is obsolete: true
Attachment #8678832 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 22•9 years ago
|
||
My qref skills are unimpressive.
Attachment #8678851 -
Attachment is obsolete: true
Attachment #8678851 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 23•9 years ago
|
||
Attachment #8678853 -
Attachment is obsolete: true
Attachment #8678856 -
Flags: review?(jmuizelaar)
Comment 24•9 years ago
|
||
Comment on attachment 8677425 [details] [diff] [review]
Convert BasicLayers usecases to PushGroupForBlendback and temporary surfaces. v3
Review of attachment 8677425 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/basic/BasicLayerManager.cpp
@@ +184,5 @@
> + dataSurf->Unmap();
> + dataSurf = src->GetDataSurface();
> + dataSurf->Map(DataSourceSurface::MapType::READ, &map);
> + dataSurf->Unmap();
> + }
What is the code above here for?
@@ +188,5 @@
> + }
> +
> + dt->Mask(SurfacePattern(src, ExtendMode::CLAMP, Matrix::Translation(group.mGroupOffset.x, group.mGroupOffset.y)),
> + SurfacePattern(group.mMaskSurface, ExtendMode::CLAMP, group.mMaskTransform),
> + DrawOptions(group.mOpacity, group.mOperator));
Mask is not supported with DrawTargetCG. Please use MaskSurface.
Attachment #8677425 -
Flags: review?(jmuizelaar) → review-
Comment on attachment 8678827 [details] [diff] [review]
Part 4: Remove unused code to support non-operator OVER. r=roc
Review of attachment 8678827 [details] [diff] [review]:
-----------------------------------------------------------------
Can you explain why this is unused? Seems to me it's still necessary if some code (an addon maybe) does a canvas.drawWindow while the globalCompositeOperation is not OVER.
Attachment #8678827 -
Flags: review?(roc)
Assignee | ||
Comment 26•9 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25)
> Comment on attachment 8678827 [details] [diff] [review]
> Part 4: Remove unused code to support non-operator OVER. r=roc
>
> Review of attachment 8678827 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Can you explain why this is unused? Seems to me it's still necessary if some
> code (an addon maybe) does a canvas.drawWindow while the
> globalCompositeOperation is not OVER.
Well, if that addon exists that would be wonderful, and I'd love to hear about it, but since none of our tests hit it, and drawWindow is not in the Canvas spec and simply a facility we're providing to people, it seems like a better idea to break this, and only add code to deal with this if it turns out there's actually anyone using it.
Assignee | ||
Comment 27•9 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #24)
> Comment on attachment 8677425 [details] [diff] [review]
> Convert BasicLayers usecases to PushGroupForBlendback and temporary
> surfaces. v3
>
> Review of attachment 8677425 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: gfx/layers/basic/BasicLayerManager.cpp
> @@ +184,5 @@
> > + dataSurf->Unmap();
> > + dataSurf = src->GetDataSurface();
> > + dataSurf->Map(DataSourceSurface::MapType::READ, &map);
> > + dataSurf->Unmap();
> > + }
>
> What is the code above here for?
Just debugging stuff that slipped in. It's gone.
> @@ +188,5 @@
> > + }
> > +
> > + dt->Mask(SurfacePattern(src, ExtendMode::CLAMP, Matrix::Translation(group.mGroupOffset.x, group.mGroupOffset.y)),
> > + SurfacePattern(group.mMaskSurface, ExtendMode::CLAMP, group.mMaskTransform),
> > + DrawOptions(group.mOpacity, group.mOperator));
>
> Mask is not supported with DrawTargetCG. Please use MaskSurface.
Will do, I'd have to check the mask transform is a translation only, but I don't think that's ever not the case.
Assignee | ||
Comment 28•9 years ago
|
||
Attachment #8677425 -
Attachment is obsolete: true
Attachment #8679957 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 29•9 years ago
|
||
(In reply to Bas Schouten (:bas.schouten) from comment #26)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25)
> > Comment on attachment 8678827 [details] [diff] [review]
> > Part 4: Remove unused code to support non-operator OVER. r=roc
> >
> > Review of attachment 8678827 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > Can you explain why this is unused? Seems to me it's still necessary if some
> > code (an addon maybe) does a canvas.drawWindow while the
> > globalCompositeOperation is not OVER.
>
> Well, if that addon exists that would be wonderful, and I'd love to hear
> about it, but since none of our tests hit it, and drawWindow is not in the
> Canvas spec and simply a facility we're providing to people, it seems like a
> better idea to break this, and only add code to deal with this if it turns
> out there's actually anyone using it.
Could you be persuaded to take this approach, and if not could you point me at some application of this so I can make sure I test it when converting this code?
Flags: needinfo?(roc)
I can't point to any application using drawWindow with non-OVER, but it doesn't seem right to me to just break it. I imagine it'd be pretty easy to keep non-OVER operators working with drawWindow. Is it not just a few lines of code, perhaps with a temporary surface?
Flags: needinfo?(roc)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #24)
> > + dt->Mask(SurfacePattern(src, ExtendMode::CLAMP, Matrix::Translation(group.mGroupOffset.x, group.mGroupOffset.y)),
> > + SurfacePattern(group.mMaskSurface, ExtendMode::CLAMP, group.mMaskTransform),
> > + DrawOptions(group.mOpacity, group.mOperator));
>
> Mask is not supported with DrawTargetCG. Please use MaskSurface.
Then shouldn't we either remove Mask from DrawTarget or implement it for DrawTargetCG? It seems bad to have DrawTarget methods that one should never call.
Flags: needinfo?(jmuizelaar)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
> I can't point to any application using drawWindow with non-OVER, but it
> doesn't seem right to me to just break it.
What I'm trying to get at is that it seems wrong to have combinations of features that just don't work because we don't think that particular combination is important. That makes our APIs weird and unpredictable. I understand that drawWindow isn't Web-exposed, but it has still been widely used in addons.
Assignee | ||
Comment 33•9 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
> > I can't point to any application using drawWindow with non-OVER, but it
> > doesn't seem right to me to just break it.
>
> What I'm trying to get at is that it seems wrong to have combinations of
> features that just don't work because we don't think that particular
> combination is important. That makes our APIs weird and unpredictable. I
> understand that drawWindow isn't Web-exposed, but it has still been widely
> used in addons.
If we want this combination to work. We need to have a reftest for it somehow, as it stands, we don't even know if it works as expected right now! (I mean, I assume it does, but the current code isn't being tested anywhere) And yes, adding it is just a couple of lines of code to create a temporary surface, and that's fine, but I feel having code specifically to support an untested, never been used functionality around, is unwise.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #24)
> > > + dt->Mask(SurfacePattern(src, ExtendMode::CLAMP, Matrix::Translation(group.mGroupOffset.x, group.mGroupOffset.y)),
> > > + SurfacePattern(group.mMaskSurface, ExtendMode::CLAMP, group.mMaskTransform),
> > > + DrawOptions(group.mOpacity, group.mOperator));
> >
> > Mask is not supported with DrawTargetCG. Please use MaskSurface.
>
> Then shouldn't we either remove Mask from DrawTarget or implement it for
> DrawTargetCG? It seems bad to have DrawTarget methods that one should never
> call.
I agree, I -really- don't want to kill mask though since I think it's a useful function (as this case proved) in general to have. But apparently according to Jeff it would take some effort to implement in DrawTargetCG.
Assignee | ||
Comment 34•9 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
> > I can't point to any application using drawWindow with non-OVER, but it
> > doesn't seem right to me to just break it.
>
> What I'm trying to get at is that it seems wrong to have combinations of
> features that just don't work because we don't think that particular
> combination is important. That makes our APIs weird and unpredictable. I
> understand that drawWindow isn't Web-exposed, but it has still been widely
> used in addons.
Also, I don't know.. do for example Canvas shadows work with DrawWindow right now? I somehow doubt it, or at least, maybe or maybe not as expected etc. So I'm not sure in the case of DrawWindow we can say this is a valid concern. Also, if we -do- decide to support this, I'd like to move the code to CanvasRenderingContext2D::DrawWindow so we make it explicit what this code is designed to support.
Flags: needinfo?(roc)
Assignee | ||
Comment 35•9 years ago
|
||
I also can't find any codepaths using this. I don't think there's any way Canvas uses this code either. Is it okay if I replace this with an ASSERT?
Attachment #8679975 -
Flags: review?(roc)
Assignee | ||
Comment 36•9 years ago
|
||
Comment on attachment 8679975 [details] [diff] [review]
Part 6: Remove unused code to support non-operator OVER in nsCSSRendering. r=roc
Never mind. I've found where this is used.
Attachment #8679975 -
Flags: review?(roc)
CanvasRenderingContext2D::DrawWindow already has code to draw to a temporary surface. We draw directly under the following condition:
// Rendering directly is faster and can be done if mTarget supports Azure
// and does not need alpha blending.
if (gfxPlatform::GetPlatform()->SupportsAzureContentForDrawTarget(mTarget) &&
GlobalAlpha() == 1.0f)
The temp-surface code always draws with OVER, which is already a bug when you have GlobalAlpha() < 1.0 and non-OVER composition op. But anyway, I suggest you go ahead and remove the non-OVER path from RenderDocument, asserting that the current operator is OVER. And just add an operator check to the above condition that assertion can't be hit. And if you're feeling really good, pass the correct op in CanvasRenderingContext2D::DrawWindow's call to DrawSurface. Shadows still wouldn't work but otherwise at least we're going forwards not backwards and it's a trivial patch.
(In reply to Bas Schouten (:bas.schouten) from comment #33)
> I agree, I -really- don't want to kill mask though since I think it's a
> useful function (as this case proved) in general to have. But apparently
> according to Jeff it would take some effort to implement in DrawTargetCG.
We should just remove it until we can make it usable. The cost of someone accidentally using it outweighs the cost of maybe having to reland it.
Flags: needinfo?(roc)
Assignee | ||
Comment 38•9 years ago
|
||
Attachment #8679975 -
Attachment is obsolete: true
Attachment #8680294 -
Flags: review?(roc)
Assignee | ||
Comment 39•9 years ago
|
||
qref to remove silly debugging hunk.
Attachment #8680296 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8680294 -
Attachment is obsolete: true
Attachment #8680294 -
Flags: review?(roc)
Assignee | ||
Comment 40•9 years ago
|
||
Attachment #8678827 -
Attachment is obsolete: true
Attachment #8680308 -
Flags: review?(roc)
Comment on attachment 8680296 [details] [diff] [review]
Part 6: Convert code to support non-operator OVER in nsCSSRendering to moz2d. v2
Review of attachment 8680296 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/base/nsCSSRendering.cpp
@@ -5053,5 @@
> return DrawResult::TEMPORARY_ERROR;
> }
>
> - gfxContext* ctx = aRenderingContext.ThebesContext();
> - CompositionOp op = ctx->CurrentOp();
Assert that the CurrentOp is OVER here.
::: layout/base/nsLayoutUtils.cpp
@@ +6254,5 @@
> + IntRect tmpDTRect;
> +
> + if (destCtx->CurrentOp() != CompositionOp::OP_OVER) {
> + Rect imageRect = ToRect(params.imageSpaceToDeviceSpace.TransformBounds(params.region.Rect()));
> + imageRect.ToIntRect(&tmpDTRect);
Just casting here is not good. RoundOut before calling ToIntRect.
Attachment #8680296 -
Flags: review?(roc) → review+
Comment on attachment 8680308 [details] [diff] [review]
Part 4: Remove code to support non-operator OVER in nsRenderDocument and move to CanvasRenderingContext2D::DrawWindow. r=roc
Review of attachment 8680308 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks!!
Attachment #8680308 -
Flags: review?(roc) → review+
Assignee | ||
Comment 43•9 years ago
|
||
Comment on attachment 8680296 [details] [diff] [review]
Part 6: Convert code to support non-operator OVER in nsCSSRendering to moz2d. v2
Review of attachment 8680296 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/base/nsCSSRendering.cpp
@@ -5053,5 @@
> return DrawResult::TEMPORARY_ERROR;
> }
>
> - gfxContext* ctx = aRenderingContext.ThebesContext();
> - CompositionOp op = ctx->CurrentOp();
We don't need it to be.. right? I made nsLayoutUtils::DrawImage deal with !OVER in the rest of the patch.
::: layout/base/nsLayoutUtils.cpp
@@ +6254,5 @@
> + IntRect tmpDTRect;
> +
> + if (destCtx->CurrentOp() != CompositionOp::OP_OVER) {
> + Rect imageRect = ToRect(params.imageSpaceToDeviceSpace.TransformBounds(params.region.Rect()));
> + imageRect.ToIntRect(&tmpDTRect);
You're absolutely right.
Updated•9 years ago
|
Attachment #8678856 -
Flags: review?(jmuizelaar) → review+
Updated•9 years ago
|
Attachment #8679957 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8678856 -
Attachment description: Part 7: Remove unused PushGroup/PopGroup/PopGroupToSource functions v4 → Part 8: Remove unused PushGroup/PopGroup/PopGroupToSource functions v4
Assignee | ||
Comment 44•9 years ago
|
||
Attachment #8683678 -
Flags: review?(karlt)
Comment 45•9 years ago
|
||
Comment on attachment 8674267 [details] [diff] [review]
Part 3: Convert more complex SVG usecases to use PushGroupForBlendBack and temporary surfaces. v3
Review of attachment 8674267 [details] [diff] [review]:
-----------------------------------------------------------------
nsSVGClipPathFrame::ApplyClipOrPaintClipMask really needs to die, but rather than supply you with a bunch of detailed review comments on how I'd like that done I'll just write a follow-up patch. Thanks for getting things this far.
::: layout/svg/nsSVGClipPathFrame.cpp
@@ +175,5 @@
> + gfxContextMatrixAutoSaveRestore autoRestoreMatrix(&aReferenceContext);
> +
> + aReferenceContext.SetMatrix(gfxMatrix());
> + gfxRect rect = aReferenceContext.GetClipExtents();
> + ToRect(rect).ToIntRect(&intRect);
We should probably use |intRect = RoundedOut(ToRect(rect));| here.
Attachment #8674267 -
Flags: review?(jwatt) → review+
Comment 46•9 years ago
|
||
Comment on attachment 8683678 [details] [diff] [review]
Part 7: Convert GTK widget code to use Moz2D instead of PushGroup/PopGroup
Review of attachment 8683678 [details] [diff] [review]:
-----------------------------------------------------------------
::: widget/gtk/nsWindow.cpp
@@ +2221,5 @@
> // call UpdateTranslucentWindowAlpha once. After we have dropped
> // support for non-Thebes graphics, UpdateTranslucentWindowAlpha will be
> // our private interface so we can rework things to avoid this.
> boundsRect = region.GetBounds();
> + dt->PushClipRect(Rect(boundsRect.x, boundsRect.y,
Pushing clips to the DrawDarget, wrapping the DrawTarget in a gfxContext and then passing the gfxContext off to some arbitrary code is currently prone to restoring the wrong clipping, no?
The gfxContext ctor doesn't save the clipping stack off the DrawTarget, so if the code that is passed the gfxContext pushes clipping state then pops it, the DrawTarget will have all clipping cleared.
Comment 47•9 years ago
|
||
Ah, no I'm mis-remembering - clip handling is safe because gfxContext::Restore() only pops as many clips as were pushed. It's transforms on DrawTarget that aren't maintained when they're wrapped in a gfxContext (per query to Bas, because adjusting for device transforms would be iffy). So there's no problem here.
Comment 48•9 years ago
|
||
Comment on attachment 8683678 [details] [diff] [review]
Part 7: Convert GTK widget code to use Moz2D instead of PushGroup/PopGroup
Taking review as discussed with Karl and Bas.
Attachment #8683678 -
Flags: review?(karlt) → review+
Comment 49•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fc53a141139
https://hg.mozilla.org/integration/mozilla-inbound/rev/11ce065429a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/5515fd47eb5a
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ccdc96a5559
https://hg.mozilla.org/integration/mozilla-inbound/rev/2afe2414f3fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc6e7bbaf2ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/291eaaa12013
https://hg.mozilla.org/integration/mozilla-inbound/rev/af5c9cf6898b
Comment 50•9 years ago
|
||
Comment 51•9 years ago
|
||
Comment 52•9 years ago
|
||
Comment 53•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8fc53a141139
https://hg.mozilla.org/mozilla-central/rev/11ce065429a5
https://hg.mozilla.org/mozilla-central/rev/5515fd47eb5a
https://hg.mozilla.org/mozilla-central/rev/2ccdc96a5559
https://hg.mozilla.org/mozilla-central/rev/2afe2414f3fc
https://hg.mozilla.org/mozilla-central/rev/fc6e7bbaf2ad
https://hg.mozilla.org/mozilla-central/rev/291eaaa12013
https://hg.mozilla.org/mozilla-central/rev/af5c9cf6898b
https://hg.mozilla.org/mozilla-central/rev/265f46e1ee7b
https://hg.mozilla.org/mozilla-central/rev/3f97905378fd
https://hg.mozilla.org/mozilla-central/rev/1c7753515d42
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Flags: needinfo?(jmuizelaar)
You need to log in
before you can comment on or make changes to this bug.
Description
•