Issue 6871051: Make SkClipStack::Iter use SkClipStack::Element.

Issue 6871051: Make SkClipStack::Iter use SkClipStack::Element. (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
11 years, 6 months ago by bsalomon

Modified:
11 years, 6 months ago

Reviewers:
robertphillips

CC:
skia-review_googlegroups.com

Base URL:
http://skia.googlecode.com/svn/trunk/

Visibility:
Public.

More Reviews

Description

Make SkClipStack::Iter use SkClipStack::Element. R=robertphillips@google.com Committed: https://code.google.com/p/skia/source/detail?r=6661

Patch Set 2 : #

Total comments: 12

Created: 11 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+369 lines, -408 lines)			Patch
M	include/core/SkClipStack.h	View	8 chunks	+80 lines, -51 lines	2 comments	Download
M	src/core/SkCanvas.cpp	View	2 chunks	+28 lines, -17 lines	4 comments	Download
M	src/core/SkClipStack.cpp	View	2 chunks	+5 lines, -99 lines	0 comments	Download
M	src/core/SkTLList.h	View	1 chunk	+8 lines, -2 lines	0 comments	Download
M	src/gpu/GrClipMaskManager.h	View	3 chunks	+5 lines, -3 lines	2 comments	Download
M	src/gpu/GrClipMaskManager.cpp	View	36 chunks	+156 lines, -164 lines	0 comments	Download
M	src/pdf/SkPDFDevice.cpp	View	4 chunks	+24 lines, -23 lines	0 comments	Download
M	tests/ClipStackTest.cpp	View	10 chunks	+63 lines, -49 lines	4 comments	Download

Messages

Total messages: 3

Expand All Messages | Collapse All Messages

robertphillips

LGTM + some questions/suggestions https://codereview.appspot.com/6871051/diff/2001/include/core/SkClipStack.h File include/core/SkClipStack.h (right): https://codereview.appspot.com/6871051/diff/2001/include/core/SkClipStack.h#newcode231 include/core/SkClipStack.h:231: inline void checkEmpty() const; delete ...

11 years, 6 months ago (2012-12-03 23:03:24 UTC) #2

bsalomon

11 years, 6 months ago (2012-12-04 14:02:11 UTC) #3

https://codereview.appspot.com/6871051/diff/2001/include/core/SkClipStack.h
File include/core/SkClipStack.h (right):

https://codereview.appspot.com/6871051/diff/2001/include/core/SkClipStack.h#n...
include/core/SkClipStack.h:231: inline void checkEmpty() const;
On 2012/12/03 23:03:24, robertphillips wrote:
> delete these?

whoops.. forgot to save again before uploading... done.

https://codereview.appspot.com/6871051/diff/2001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.appspot.com/6871051/diff/2001/src/core/SkCanvas.cpp#newcod...
src/core/SkCanvas.cpp:1208: element->getRect().round(&ir);
On 2012/12/03 23:03:24, robertphillips wrote:
> What happened to the tmpClip.op(ir, clip->fOp)?

yikes, restored.

https://codereview.appspot.com/6871051/diff/2001/src/core/SkCanvas.cpp#newcod...
src/core/SkCanvas.cpp:1227: 
On 2012/12/03 23:03:24, robertphillips wrote:
> static const?

Done.

https://codereview.appspot.com/6871051/diff/2001/src/gpu/GrClipMaskManager.h
File src/gpu/GrClipMaskManager.h (right):

https://codereview.appspot.com/6871051/diff/2001/src/gpu/GrClipMaskManager.h#...
src/gpu/GrClipMaskManager.h:159: 
On 2012/12/03 23:03:24, robertphillips wrote:
> Move SkTLList.h to top?

Done.

https://codereview.appspot.com/6871051/diff/2001/tests/ClipStackTest.cpp
File tests/ClipStackTest.cpp (right):

https://codereview.appspot.com/6871051/diff/2001/tests/ClipStackTest.cpp#newc...
tests/ClipStackTest.cpp:536: break;
On 2012/12/03 23:03:24, robertphillips wrote:
> failure case?

Yeah the reducer shouldn't ever produce an explicit empty. Added SkDEBUGFAIL

https://codereview.appspot.com/6871051/diff/2001/tests/ClipStackTest.cpp#newc...
tests/ClipStackTest.cpp:558: return;
On 2012/12/03 23:03:24, robertphillips wrote:
> failure case?
There are some optimizations yet to be added to the reducer. When we play back
the element list into a new stack the stack may notice that the clip is empty
and synthesize an empty element. The same can happen when creating the randomly
generated stack, which is also played back into a region.

Expand All Messages | Collapse All Messages