Open
Bug 1026777
Opened 10 years ago
Updated 2 years ago
Port DisplayItemClip to Moz2D
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
ASSIGNED
People
(Reporter: mstange, Assigned: mstange)
Details
Attachments
(1 file, 3 obsolete files)
18.18 KB,
patch
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8441707 -
Attachment is obsolete: true
Attachment #8442054 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 2•10 years ago
|
||
forgot to remove the now unneeded snapping changes
Attachment #8442054 -
Attachment is obsolete: true
Attachment #8442054 -
Flags: review?(matt.woodrow)
Attachment #8442057 -
Flags: review?(matt.woodrow)
Comment 3•10 years ago
|
||
Comment on attachment 8442057 [details] [diff] [review] v2 Review of attachment 8442057 [details] [diff] [review]: ----------------------------------------------------------------- This seems like it might confuse gfxContext. In BasicLayers at least we use GetClipExtents, which is determined based off of the clip state tracked by gfxContext. If we've manually added some extra clips to the DT, then these won't be included. ::: layout/base/DisplayItemClip.cpp @@ +428,5 @@ > } > return str; > } > #endif > + Trailing whitespace @@ +443,5 @@ > + > +void > +DisplayItemClipSetter::ApplyClipFor(const DisplayItemClip& aClip, const nsRect& aClippedBounds) > +{ > + const DisplayItemClip& clip = SimplifiedClip(aClip, aClippedBounds); Why is this a reference? @@ +448,5 @@ > + if (mPushedClipCount > 0 && clip != mAppliedClip) { > + UnapplyClip(); > + } > + if (mPushedClipCount == 0 && clip.HasClip()) { > + mPushedClipCount = clip.ApplyTo(mDT, mPresContext, mCommonClipCount); Whitespace
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #3) > Comment on attachment 8442057 [details] [diff] [review] > v2 > > Review of attachment 8442057 [details] [diff] [review]: > ----------------------------------------------------------------- > > This seems like it might confuse gfxContext. In BasicLayers at least we use > GetClipExtents, which is determined based off of the clip state tracked by > gfxContext. If we've manually added some extra clips to the DT, then these > won't be included. True. What should we do about that? Should we just wait for the new PushLayer API (or whatever Moz2D changes we make so that BasicLayers can use it) before landing this? > @@ +443,5 @@ > > + > > +void > > +DisplayItemClipSetter::ApplyClipFor(const DisplayItemClip& aClip, const nsRect& aClippedBounds) > > +{ > > + const DisplayItemClip& clip = SimplifiedClip(aClip, aClippedBounds); > > Why is this a reference? Because I thought that might avoid one more copy, but I think not using a reference here would achieve the same thing (through RVO). I can test whether it makes a difference. Note using a reference here is still safe because const references to temporaries keep them alive.
Comment 5•10 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #4) > True. What should we do about that? Should we just wait for the new > PushLayer API (or whatever Moz2D changes we make so that BasicLayers can use > it) before landing this? We use GetClipExtents for computing the draw region to pass to the thebes layer callback, and for computing the size of 3d transformed layers, neither of these are likely to be fixed by Moz2D API changes. I think we'll just have to pass some information about the clip state into BasicLayerManager to solve this. So we really just need to write the patch converting the rest of BasicLayers to Moz2D. > > > @@ +443,5 @@ > > > + > > > +void > > > +DisplayItemClipSetter::ApplyClipFor(const DisplayItemClip& aClip, const nsRect& aClippedBounds) > > > +{ > > > + const DisplayItemClip& clip = SimplifiedClip(aClip, aClippedBounds); > > > > Why is this a reference? > > Because I thought that might avoid one more copy, but I think not using a > reference here would achieve the same thing (through RVO). I can test > whether it makes a difference. Note using a reference here is still safe > because const references to temporaries keep them alive. Ok, didn't know that :)
Updated•10 years ago
|
Attachment #8442057 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8442057 -
Attachment is obsolete: true
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•