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

Issue 2146073003: Creating framework for drawShadowedPicture (Closed)

Created:
4 years, 5 months ago by vjiaoblack
Modified:
4 years, 5 months ago
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

Patch Set 1 #

Total comments: 2

Patch Set 2 : Fixed 'const sk_sp<SkLights>', also fixed some crumbs from merging #

Total comments: 6

Patch Set 3 : Edited code to meet requirements discussed in discussion #

Patch Set 4 : Added setLights and getLIghts calls #

Total comments: 10

Patch Set 5 : Made req. changes #

Patch Set 6 : Removed files under testing. #

Patch Set 7 : Missed error #

Total comments: 8

Patch Set 8 : Made req changes #

Total comments: 2

Patch Set 9 : Fixed comment. #

Total comments: 2

Patch Set 10 : Made a few more chanage #

Patch Set 11 : ugh #

Patch Set 12 : Made changes to better hide changes from public #

Total comments: 8

Patch Set 13 : Modified the defines #

Patch Set 14 : removed from gyp/core.gypi #

Patch Set 15 : Fixed case error #

Patch Set 16 : bugfix #

Patch Set 17 : Merged with master #

Patch Set 18 : removed redundant code #

Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -3 lines) Patch
M include/core/SkCanvas.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -1 line 0 comments Download
M src/core/SkPictureRecord.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +3 lines, -1 line 0 comments Download
M tools/debugger/SkDebugCanvas.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 68 (35 generated)
vjiaoblack
4 years, 5 months ago (2016-07-14 14:23:41 UTC) #3
reed1
https://codereview.chromium.org/2146073003/diff/1/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2146073003/diff/1/include/core/SkCanvas.h#newcode1059 include/core/SkCanvas.h:1059: void drawShadowedPicture(const sk_sp<SkPicture>& picture, const sk_sp<SkLights> lights) { api ...
4 years, 5 months ago (2016-07-14 14:25:13 UTC) #4
vjiaoblack
https://codereview.chromium.org/2146073003/diff/1/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2146073003/diff/1/include/core/SkCanvas.h#newcode1059 include/core/SkCanvas.h:1059: void drawShadowedPicture(const sk_sp<SkPicture>& picture, const sk_sp<SkLights> lights) { On ...
4 years, 5 months ago (2016-07-14 14:42:05 UTC) #6
jvanverth1
Some comments: https://codereview.chromium.org/2146073003/diff/20001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2146073003/diff/20001/include/core/SkCanvas.h#newcode1056 include/core/SkCanvas.h:1056: * However, this time, we will use ...
4 years, 5 months ago (2016-07-14 17:27:09 UTC) #7
vjiaoblack
https://codereview.chromium.org/2146073003/diff/20001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2146073003/diff/20001/include/core/SkCanvas.h#newcode1056 include/core/SkCanvas.h:1056: * However, this time, we will use the passed-in ...
4 years, 5 months ago (2016-07-14 20:38:18 UTC) #8
vjiaoblack
4 years, 5 months ago (2016-07-14 21:05:16 UTC) #9
jvanverth1
https://codereview.chromium.org/2146073003/diff/60001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2146073003/diff/60001/include/core/SkCanvas.h#newcode465 include/core/SkCanvas.h:465: void setLights(sk_sp<SkLights> lights); You need to hide this (and ...
4 years, 5 months ago (2016-07-15 19:16:30 UTC) #10
vjiaoblack
https://codereview.chromium.org/2146073003/diff/60001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2146073003/diff/60001/include/core/SkCanvas.h#newcode465 include/core/SkCanvas.h:465: void setLights(sk_sp<SkLights> lights); On 2016/07/15 19:16:30, jvanverth1 wrote: > ...
4 years, 5 months ago (2016-07-18 13:03:23 UTC) #11
vjiaoblack
Sorry. Missed error.
4 years, 5 months ago (2016-07-18 13:30:05 UTC) #12
jvanverth1
https://codereview.chromium.org/2146073003/diff/120001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2146073003/diff/120001/include/core/SkCanvas.h#newcode460 include/core/SkCanvas.h:460: void translateZ(SkScalar z); Still need to put this behind ...
4 years, 5 months ago (2016-07-18 15:02:26 UTC) #13
vjiaoblack
https://codereview.chromium.org/2146073003/diff/120001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2146073003/diff/120001/include/core/SkCanvas.h#newcode460 include/core/SkCanvas.h:460: void translateZ(SkScalar z); On 2016/07/18 15:02:26, jvanverth1 wrote: > ...
4 years, 5 months ago (2016-07-18 16:51:06 UTC) #14
jvanverth1
Final comment, otherwise this looks okay to me. Brian or Mike, do you have any ...
4 years, 5 months ago (2016-07-18 17:32:08 UTC) #15
vjiaoblack
https://codereview.chromium.org/2146073003/diff/140001/include/core/SkLights.h File include/core/SkLights.h (right): https://codereview.chromium.org/2146073003/diff/140001/include/core/SkLights.h#newcode59 include/core/SkLights.h:59: SkImage* fShadowMap; // if we use an sk_sp, this ...
4 years, 5 months ago (2016-07-18 17:56:21 UTC) #16
vjiaoblack
4 years, 5 months ago (2016-07-20 16:07:01 UTC) #17
bsalomon
Should we use a macro other than SK_RELEASE to guard this? https://codereview.chromium.org/2146073003/diff/160001/include/core/SkLights.h File include/core/SkLights.h (right): ...
4 years, 5 months ago (2016-07-20 16:25:54 UTC) #18
vjiaoblack
https://codereview.chromium.org/2146073003/diff/160001/include/core/SkLights.h File include/core/SkLights.h (right): https://codereview.chromium.org/2146073003/diff/160001/include/core/SkLights.h#newcode51 include/core/SkLights.h:51: fShadowMap = shadowMap; On 2016/07/20 16:25:54, bsalomon wrote: > ...
4 years, 5 months ago (2016-07-20 20:15:04 UTC) #19
bsalomon
lgtm, but I do think it'd be good to hide this behind a different macro ...
4 years, 5 months ago (2016-07-21 13:45:13 UTC) #20
jvanverth1
https://codereview.chromium.org/2146073003/diff/220001/src/core/SkPictureRecord.h File src/core/SkPictureRecord.h (right): https://codereview.chromium.org/2146073003/diff/220001/src/core/SkPictureRecord.h#newcode162 src/core/SkPictureRecord.h:162: void didTranslateZ(SkScalar) I think it would be cleaner-looking to ...
4 years, 5 months ago (2016-07-21 13:56:53 UTC) #21
vjiaoblack
https://codereview.chromium.org/2146073003/diff/220001/src/core/SkPictureRecord.h File src/core/SkPictureRecord.h (right): https://codereview.chromium.org/2146073003/diff/220001/src/core/SkPictureRecord.h#newcode162 src/core/SkPictureRecord.h:162: void didTranslateZ(SkScalar) On 2016/07/21 13:56:53, jvanverth1 wrote: > I ...
4 years, 5 months ago (2016-07-21 15:11:20 UTC) #22
vjiaoblack
4 years, 5 months ago (2016-07-21 15:19:50 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2146073003/260001
4 years, 5 months ago (2016-07-21 16:09:28 UTC) #34
commit-bot: I haz the power
Committed patchset #14 (id:260001) as https://skia.googlesource.com/skia/+/0ae097d116f4332be02a135ffc99c162473dee6a
4 years, 5 months ago (2016-07-21 16:10:29 UTC) #36
vjiaoblack
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/2167223002/ by vjiaoblack@google.com. ...
4 years, 5 months ago (2016-07-21 16:23:58 UTC) #38
vjiaoblack
4 years, 5 months ago (2016-07-21 16:54:38 UTC) #39
jvanverth1
lgtm
4 years, 5 months ago (2016-07-21 17:13:34 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2146073003/280001
4 years, 5 months ago (2016-07-21 17:24:56 UTC) #47
commit-bot: I haz the power
Committed patchset #15 (id:280001) as https://skia.googlesource.com/skia/+/95302da19d8b0a3bcd9d9be0e79f486760787f09
4 years, 5 months ago (2016-07-21 17:25:58 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2146073003/300001
4 years, 5 months ago (2016-07-21 21:29:55 UTC) #53
vjiaoblack
... found another bug! and fixed it.
4 years, 5 months ago (2016-07-21 21:30:12 UTC) #55
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2146073003/300001
4 years, 5 months ago (2016-07-21 21:30:34 UTC) #57
commit-bot: I haz the power
Try jobs failed on following builders: skia_presubmit-Trybot on master.client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/builds/11588)
4 years, 5 months ago (2016-07-21 21:31:56 UTC) #59
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2146073003/340001
4 years, 5 months ago (2016-07-22 17:03:22 UTC) #66
commit-bot: I haz the power
4 years, 5 months ago (2016-07-22 17:04:19 UTC) #68
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as
https://skia.googlesource.com/skia/+/5bfee98c8cf59db8d71aa6672088b107f0abf8c9

Powered by Google App Engine
This is Rietveld 408576698