Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(225)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 13957009: First pass at Comment API (Closed)

Created:
7 years, 8 months ago by robertphillips
Modified:
7 years, 6 months ago
CC:
skia-review_googlegroups.com
Visibility:
Public.

Description

This version differs from the initial proposal in two main ways: 1) I used the "comment" naming convention rather than the "annotation" naming convention to avoid confusion with the other annotations 2) I removed the groupID that was returned from beginGroup and passed into endGroup. It didn't seem to buy us anything and was making it a bit painful to record and replay pictures. Let the discussion begin ...

Patch Set 1 #

Total comments: 1

Patch Set 2 : moved to beginCommentGroup/endCommentGroup #

Total comments: 4

Patch Set 3 : Addressed code review issues #

Unified diffs Side-by-side diffs Delta from patch set Stats (+235 lines, -9 lines) Patch
M debugger/QT/SkDebuggerGUI.cpp View 1 2 1 chunk +4 lines, -2 lines 0 comments Download
M debugger/SkDebugCanvas.h View 1 2 1 chunk +6 lines, -0 lines 0 comments Download
M debugger/SkDebugCanvas.cpp View 1 2 1 chunk +12 lines, -0 lines 0 comments Download
M debugger/SkDrawCommand.h View 1 2 2 chunks +37 lines, -0 lines 0 comments Download
M debugger/SkDrawCommand.cpp View 1 2 3 chunks +28 lines, -0 lines 0 comments Download
M gm/rects.cpp View 1 2 2 chunks +6 lines, -0 lines 0 comments Download
M include/core/SkCanvas.h View 1 2 2 chunks +39 lines, -1 line 0 comments Download
M include/utils/SkDumpCanvas.h View 1 2 2 chunks +8 lines, -1 line 0 comments Download
M include/utils/SkProxyCanvas.h View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M src/core/SkCanvas.cpp View 1 2 1 chunk +0 lines, -4 lines 0 comments Download
M src/core/SkPictureFlat.h View 1 2 1 chunk +4 lines, -1 line 0 comments Download
M src/core/SkPicturePlayback.cpp View 1 2 1 chunk +12 lines, -0 lines 0 comments Download
M src/core/SkPictureRecord.h View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M src/core/SkPictureRecord.cpp View 1 2 2 chunks +30 lines, -0 lines 0 comments Download
M src/pipe/SkGPipeWrite.cpp View 1 2 2 chunks +15 lines, -0 lines 0 comments Download
M src/utils/SkDumpCanvas.cpp View 1 2 1 chunk +12 lines, -0 lines 0 comments Download
M src/utils/SkProxyCanvas.cpp View 1 2 1 chunk +12 lines, -0 lines 0 comments Download
M tests/CanvasTest.cpp View 1 2 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
robertphillips
7 years, 8 months ago (2013-04-22 18:49:10 UTC) #1
robertphillips
Additionally, the "rects" gm has been modified to demonstrate the API's usage
7 years, 8 months ago (2013-04-22 20:11:00 UTC) #2
reed1
beginGoup(name) addComment(foo, bar) endGroup() Are groups and comments related? Should this be named beginCommentGroup()? Grouping ...
7 years, 8 months ago (2013-04-22 20:39:51 UTC) #3
f(malita)
On 2013/04/22 20:39:51, reed1 wrote: > Are groups and comments related? Should this be named ...
7 years, 8 months ago (2013-04-22 20:51:08 UTC) #4
reed1
Why not just use convention? addComment("begin-group:", "imgID"); ... addComment("end-group", "imgID"); ... 1. Does skia know ...
7 years, 8 months ago (2013-04-22 21:18:17 UTC) #5
f(malita)
On 2013/04/22 21:18:17, reed1 wrote: > Why not just use convention? > > addComment("begin-group:", "imgID"); ...
7 years, 8 months ago (2013-04-22 21:50:05 UTC) #6
djsollen
Just my 2 cents, but the use case that I always imagined grouping being really ...
7 years, 8 months ago (2013-04-24 13:00:24 UTC) #7
djsollen
https://codereview.chromium.org/13957009/diff/1/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/13957009/diff/1/include/core/SkCanvas.h#newcode856 include/core/SkCanvas.h:856: virtual void endGroup(); Perhaps beginCommentGroup and endCommentGroup would be ...
7 years, 8 months ago (2013-04-24 13:01:54 UTC) #8
robertphillips
This patch renames beginGroup/endGroup to beginCommentGroup/endCommentGroup so we can see how that looks. Right now ...
7 years, 8 months ago (2013-04-24 17:50:01 UTC) #9
reed1
Perhaps we try just addComment() and experiment with manual level specifying (e.g. some textual prefixing) ...
7 years, 8 months ago (2013-04-24 19:05:02 UTC) #10
robertphillips
Abandoning in favor of SkAnnotation-based approach.
7 years, 8 months ago (2013-04-24 20:05:12 UTC) #11
robertphillips
And ... back to life. Please take a look and see if this is still/again ...
7 years, 7 months ago (2013-05-24 19:24:23 UTC) #12
reed1
canvas api seems fine for starters.
7 years, 7 months ago (2013-05-24 19:25:40 UTC) #13
f(malita)
On 2013/05/24 19:25:40, reed1 wrote: > canvas api seems fine for starters. As a consumer ...
7 years, 7 months ago (2013-05-26 16:43:14 UTC) #14
djsollen
https://codereview.chromium.org/13957009/diff/11001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/13957009/diff/11001/include/core/SkCanvas.h#newcode1157 include/core/SkCanvas.h:1157: void init(SkCanvas* canvas, const char* description) { why have ...
7 years, 6 months ago (2013-05-28 14:19:11 UTC) #15
robertphillips
https://codereview.chromium.org/13957009/diff/11001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/13957009/diff/11001/include/core/SkCanvas.h#newcode1157 include/core/SkCanvas.h:1157: void init(SkCanvas* canvas, const char* description) { On 2013/05/28 ...
7 years, 6 months ago (2013-05-28 15:45:30 UTC) #16
reed1
lgtm
7 years, 6 months ago (2013-05-29 12:59:46 UTC) #17
robertphillips
7 years, 6 months ago (2013-05-29 13:24:38 UTC) #18
Message was sent while issue was closed. 
committed as r9310

Powered by Google App Engine
This is Rietveld 408576698