Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(3292)

Issue 6354105: Getting tests ready for enforcing balanced save/restore calls in SkPicture (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 5 months ago by junov1
Modified:
12 years, 5 months ago
Reviewers:
reed1
CC:
skia-review_googlegroups.com
Base URL:
http://skia.googlecode.com/svn/trunk/
Visibility:
Public.

Patch Set 1 #

Total comments: 2

Patch Set 2 : putting save/restore in SkPicture begin/endRecording #

Total comments: 2

Patch Set 3 : moving bracketting save/restore to SkPictureRecord #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+128 lines, -131 lines) Patch
M src/core/SkCanvas.cpp View 1 1 chunk +0 lines, -2 lines 0 comments Download
M src/core/SkPicture.cpp View 1 2 1 chunk +0 lines, -1 line 1 comment Download
M src/core/SkPicturePlayback.cpp View 3 chunks +3 lines, -3 lines 0 comments Download
M src/core/SkPictureRecord.h View 1 2 2 chunks +3 lines, -0 lines 0 comments Download
M src/core/SkPictureRecord.cpp View 1 2 5 chunks +24 lines, -6 lines 0 comments Download
M tests/CanvasTest.cpp View 1 2 9 chunks +98 lines, -119 lines 0 comments Download

Messages

Total messages: 11
junov1
PTAL http://codereview.appspot.com/6354105/diff/1/src/core/SkPicturePlayback.cpp File src/core/SkPicturePlayback.cpp (right): http://codereview.appspot.com/6354105/diff/1/src/core/SkPicturePlayback.cpp#newcode550 src/core/SkPicturePlayback.cpp:550: offsetToRestore >= fReader.offset()); truns out that it is ...
12 years, 5 months ago (2012-07-13 16:01:51 UTC) #1
reed1
Should we be able to change SkCanvas::drawPicture() to *not* have to add save/restore after this ...
12 years, 5 months ago (2012-07-13 18:12:59 UTC) #2
junov1
I like the idea of hanging on to the extra save/restore in SkCanvas::drawPicture because it ...
12 years, 5 months ago (2012-07-13 18:18:39 UTC) #3
reed1
On 2012/07/13 18:18:39, junov1 wrote: > I like the idea of hanging on to the ...
12 years, 5 months ago (2012-07-13 18:20:40 UTC) #4
junov1
come to think of it. putting it in begin/endRecording is nice because it also covers ...
12 years, 5 months ago (2012-07-13 18:24:34 UTC) #5
reed1
On 2012/07/13 18:24:34, junov1 wrote: > come to think of it. putting it in begin/endRecording ...
12 years, 5 months ago (2012-07-13 18:25:29 UTC) #6
junov1
Ok, patch set 2 haz the save/restore in the new location. All tests pass.
12 years, 5 months ago (2012-07-13 18:46:02 UTC) #7
reed1
http://codereview.appspot.com/6354105/diff/5002/include/core/SkPicture.h File include/core/SkPicture.h (left): http://codereview.appspot.com/6354105/diff/5002/include/core/SkPicture.h#oldcode135 include/core/SkPicture.h:135: SkPicturePlayback* fPlayback; Is it clearer to have this field ...
12 years, 5 months ago (2012-07-13 18:49:00 UTC) #8
junov1
> Is it clearer to have this field here, or in SkPictureRecord? I moved it ...
12 years, 5 months ago (2012-07-13 21:18:01 UTC) #9
reed1
lgtm w/ tiny nit about reverting a (basically) unchanged file. http://codereview.appspot.com/6354105/diff/10001/src/core/SkPicture.cpp File src/core/SkPicture.cpp (left): http://codereview.appspot.com/6354105/diff/10001/src/core/SkPicture.cpp#oldcode162 ...
12 years, 5 months ago (2012-07-16 13:54:40 UTC) #10
junov1
12 years, 5 months ago (2012-07-16 19:10:38 UTC) #11
Fixed with r4618
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b