Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2161633002:
Add Overstroke gm (Closed)
|
Created:
4 years, 5 months ago by Harry Stern Modified:
4 years, 4 months ago Reviewers:
reed1 CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionAdd Overstroke gm
Already exists as a sampleapp, but sampleapps don't get tested
automatically
BUG=skia:5405, 5406, chrome:589769
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2161633002
Committed: https://skia.googlesource.com/skia/+/02aea1c92c4606c76c17dfba8829638fda84d2d8
Patch Set 1 #Patch Set 2 : Add Overstroke gm #Patch Set 3 : Completely reorganized code, added cubics and ovals with overstroke #
Total comments: 9
Patch Set 4 : Add comment explaining purpose of gm, inline points #Patch Set 5 : Hopefully fix build errors on windows #Messages
Total messages: 22 (15 generated)
Description was changed from ========== Add Overstroke gm Already exists as a sampleapp, but sampleapps don't get tested automatically BUG=skia: ========== to ========== Add Overstroke gm Already exists as a sampleapp, but sampleapps don't get tested automatically BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2161633002 ==========
hstern@google.com changed reviewers: + reed@google.com
https://codereview.chromium.org/2161633002/diff/40001/gm/OverStroke.cpp File gm/OverStroke.cpp (right): https://codereview.chromium.org/2161633002/diff/40001/gm/OverStroke.cpp#newco... gm/OverStroke.cpp:24: SkPoint p1 = SkPoint::Make(0, 0); nit: I think it hurts readability a little to name these points and then use them once, rather than just inline the values... e.g. path.moveTo(0, 0); path.lineTo(100, 0); ... https://codereview.chromium.org/2161633002/diff/40001/gm/OverStroke.cpp#newco... gm/OverStroke.cpp:37: SkPoint p1 = SkPoint::Make(0, 0); ditto https://codereview.chromium.org/2161633002/diff/40001/gm/OverStroke.cpp#newco... gm/OverStroke.cpp:50: SkRect oval = SkRect::MakeWH(100, 50); nit: use SkRect::MakeXYWH(0, -25, 100, 50); https://codereview.chromium.org/2161633002/diff/40001/gm/OverStroke.cpp#newco... gm/OverStroke.cpp:198: DEF_SIMPLE_GM(OverStroke, canvas, 500, 500) { // overstroke means ... and that's why I want to exercise it
https://codereview.chromium.org/2161633002/diff/40001/gm/OverStroke.cpp File gm/OverStroke.cpp (right): https://codereview.chromium.org/2161633002/diff/40001/gm/OverStroke.cpp#newco... gm/OverStroke.cpp:24: SkPoint p1 = SkPoint::Make(0, 0); On 2016/08/02 15:21:31, reed1 wrote: > nit: I think it hurts readability a little to name these points and then use > them once, rather than just inline the values... > > e.g. > path.moveTo(0, 0); > path.lineTo(100, 0); > ... I actually think the exact opposite. seeing a bunch of random values, especially for the cubicTo, just makes my brain freeze up. I'm going to try to indent/newline to make it clear which are the points and see if that helps. https://codereview.chromium.org/2161633002/diff/40001/gm/OverStroke.cpp#newco... gm/OverStroke.cpp:37: SkPoint p1 = SkPoint::Make(0, 0); On 2016/08/02 15:21:31, reed1 wrote: > ditto Done. https://codereview.chromium.org/2161633002/diff/40001/gm/OverStroke.cpp#newco... gm/OverStroke.cpp:50: SkRect oval = SkRect::MakeWH(100, 50); On 2016/08/02 15:21:31, reed1 wrote: > nit: use SkRect::MakeXYWH(0, -25, 100, 50); Done. https://codereview.chromium.org/2161633002/diff/40001/gm/OverStroke.cpp#newco... gm/OverStroke.cpp:54: path.arcTo(oval, 0, 359, true); There's no way to make a complete oval just with arcTo is there? If I do path.close() it looks complete, which I guess it good enough. https://codereview.chromium.org/2161633002/diff/40001/gm/OverStroke.cpp#newco... gm/OverStroke.cpp:198: DEF_SIMPLE_GM(OverStroke, canvas, 500, 500) { On 2016/08/02 15:21:31, reed1 wrote: > // overstroke means ... and that's why I want to exercise it Done, at top of file.
The CQ bit was checked by hstern@google.com to run a CQ dry run
The CQ bit was unchecked by hstern@google.com
Description was changed from ========== Add Overstroke gm Already exists as a sampleapp, but sampleapps don't get tested automatically BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2161633002 ========== to ========== Add Overstroke gm Already exists as a sampleapp, but sampleapps don't get tested automatically BUG=skia:5405,5406,chrome:589769 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2161633002 ==========
The CQ bit was checked by hstern@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by hstern@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hstern@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add Overstroke gm Already exists as a sampleapp, but sampleapps don't get tested automatically BUG=skia:5405,5406,chrome:589769 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2161633002 ========== to ========== Add Overstroke gm Already exists as a sampleapp, but sampleapps don't get tested automatically BUG=skia:5405,5406,chrome:589769 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2161633002 Committed: https://skia.googlesource.com/skia/+/02aea1c92c4606c76c17dfba8829638fda84d2d8 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/02aea1c92c4606c76c17dfba8829638fda84d2d8 |
