Closed Bug 1085529 Opened 11 years ago Closed 11 years ago

Port nsCSSRendering::PaintDecorationLine() to Moz2D

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(2 files, 2 obsolete files)

part 2 - Port nsCSSRendering::PaintDecorationLine() to Moz2D
25.69 KB, patch
mattwoodrow
: review+
jwatt
: checkin+
Details | Diff | Splinter Review
part 1 - Add an AutoPushPopClips helper to Moz2D
1.17 KB, patch
mattwoodrow
: review+
jwatt
: checkin+
Details | Diff | Splinter Review
No description provided.
Attachment #8508115 - Flags: review?(matt.woodrow)
Calling the local variable "aDrawTarget" is a bit of an abuse of our naming convention, but when we eventually come switch all nsRenderingContext* arguments to DrawTarget args it will make that work a lot easier to write and to review.
Comment on attachment 8508115 [details] [diff] [review] part 1 - Add an AutoPopClip helper to Moz2D Review of attachment 8508115 [details] [diff] [review]: ----------------------------------------------------------------- Can we make PushClipRect a function on this class, and only PopClip if it gets called? That way you can use this in cases where clipping isn't unconditional.
(In reply to Jonathan Watt [:jwatt] from comment #3) > Calling the local variable "aDrawTarget" is a bit of an abuse of our naming > convention, but when we eventually come switch all nsRenderingContext* > arguments to DrawTarget args it will make that work a lot easier to write > and to review. Where is this? I don't see it!
Comment on attachment 8508116 [details] [diff] [review] part 2 - Port nsCSSRendering::PaintDecorationLine() to Moz2D Review of attachment 8508116 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, assuming you like my suggestion for part 1. aDrawTarget.PushClipRect(rect); autoPopClip.EnsureInit(&aDrawTarget); Would become: autoRestoreClip.ClipToRect(rect); or similar.
Attachment #8508116 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #4) > Can we make PushClipRect a function on this class, and only PopClip if it > gets called? That way you can use this in cases where clipping isn't > unconditional. The combination of the default ctor and the EnsureEnit() function was intended to address this case. However, this class doesn't really meet the needs I'm seeing in later Moz2D patches I've started work on. Specifically I more want to be able to push multiple clips and have them all auto pop'ed.
Attachment #8508495 - Flags: review?(matt.woodrow)
Attachment #8508115 - Attachment is obsolete: true
Attachment #8508115 - Flags: review?(matt.woodrow)
The one thing I don't like about this is that it kinda hides a little that the clip is being applied to the DrawTarget. I think I might rename it to AutoDrawTargetClipper and call instances autoDrawTargetClipper. Then at least "DrawTarget" is in the name and the code reads: autoDrawTargetClipper.PushClip(path); autoDrawTargetClipper.PushClipRect(rect); which makes what's going on a bit clearer.
(In reply to Matt Woodrow (:mattwoodrow) from comment #5) > Where is this? I don't see it! Sorry, that comment belonged on bug 1085533.
Comment on attachment 8508495 [details] [diff] [review] part 1 - Add an AutoPushPopClips helper to Moz2D Review of attachment 8508495 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/Helpers.h @@ +44,5 @@ > RefPtr<DrawTarget> mDrawTarget; > Matrix mOldTransform; > }; > > +class AutoPushPopClips I'd prefer just AutoPopClips, or AutoRestoreClips. We're not really doing any sort of 'auto' pushing. It also matches the convention (AutoRestoreTransform) better.
Attachment #8508495 - Flags: review?(matt.woodrow) → review+
See comment 9. I'm using AutoDrawTargetClipper now. If we're going to call this AutoPopClips then I want to implement it differently. I'd rather add a GetClipStackLength() method to DrawTarget and in the AutoPopClips ctor record the current length. In the dtor we'd then pop clips until we're back to that recorded length. In fact I like this behavior since it allows us to continue to call PushClip/PushClipRect on the DT which makes the code clearer.
Like so.
Attachment #8508619 - Flags: review?(matt.woodrow)
Although maybe GetClipCount would be a better name?
Comment on attachment 8508619 [details] [diff] [review] part 1 - Add an AutoPopClips helper to Moz2D Review of attachment 8508619 [details] [diff] [review]: ----------------------------------------------------------------- This is indeed nicer and safer. I'm going to have to defer to Bas for Moz2D API changes though. Implementation looks good though, so if you can get an informal r+ on the API change then I can r+ the rest.
Attachment #8508619 - Flags: review?(matt.woodrow) → review?(bas)
Depends on: 1087224
Comment on attachment 8508619 [details] [diff] [review] part 1 - Add an AutoPopClips helper to Moz2D Bas preferred the other r+'ed RAII patch.
Attachment #8508619 - Attachment is obsolete: true
Attachment #8508619 - Flags: review?(bas)
Comment on attachment 8508495 [details] [diff] [review] part 1 - Add an AutoPushPopClips helper to Moz2D https://hg.mozilla.org/integration/mozilla-inbound/rev/3fa71a8dc29c
Attachment #8508495 - Flags: checkin+
Comment on attachment 8508116 [details] [diff] [review] part 2 - Port nsCSSRendering::PaintDecorationLine() to Moz2D https://hg.mozilla.org/integration/mozilla-inbound/rev/758840ac2f47
Attachment #8508116 - Flags: checkin+
Now that I've fixed bug 1087224 I re-pushed, this time with fuzzing for the failing test on Windows 8: https://hg.mozilla.org/integration/mozilla-inbound/rev/79b64654f809 https://hg.mozilla.org/integration/mozilla-inbound/rev/7a8948248cbd
https://hg.mozilla.org/mozilla-central/rev/79b64654f809 https://hg.mozilla.org/mozilla-central/rev/7a8948248cbd
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: