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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2236483004: Add SkDashMeasure.h (name up for bikeshedding) which takes a dashinfo (Closed)

Created:
4 years, 4 months ago by Harry Stern
Modified:
3 years, 11 months ago
Reviewers:
caryclark, reed1
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@initializer_constructor
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

Add SkDashMeasure.h (name up for bikeshedding) which takes a dashinfo and a path and produces a dashed path. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2236483004

Patch Set 1 #

Patch Set 2 : remove accidentally-included test gm #

Total comments: 22

Patch Set 3 : Fix formatting and incorrect index for conic endpoint #

Patch Set 4 : Add dashIsOn helper function #

Patch Set 5 : Move dashmeasure to experimental. does not build #

Patch Set 6 : Actually remove src/core/SkDashMeasure.h #

Unified diffs Side-by-side diffs Delta from patch set Stats (+108 lines, -0 lines) Patch
A experimental/curve_dash_measure/SkDashMeasure.h View 1 2 3 4 1 chunk +22 lines, -0 lines 0 comments Download
A experimental/curve_dash_measure/SkDashMeasure.cpp View 1 2 3 4 1 chunk +86 lines, -0 lines 0 comments Download

Messages

Total messages: 8 (3 generated)
Harry Stern
4 years, 4 months ago (2016-08-10 22:01:37 UTC) #3
Harry Stern
On 2016/08/10 22:01:37, Harry Stern wrote: I made the function static inline because I expect ...
4 years, 4 months ago (2016-08-10 22:02:42 UTC) #4
caryclark
On 2016/08/10 22:02:42, Harry Stern wrote: > On 2016/08/10 22:01:37, Harry Stern wrote: > > ...
4 years, 4 months ago (2016-08-11 12:25:07 UTC) #5
caryclark
https://codereview.chromium.org/2236483004/diff/20001/src/core/SkDashMeasure.h File src/core/SkDashMeasure.h (right): https://codereview.chromium.org/2236483004/diff/20001/src/core/SkDashMeasure.h#newcode19 src/core/SkDashMeasure.h:19: SkScalar interval_accum = 0.0f; remove this https://codereview.chromium.org/2236483004/diff/20001/src/core/SkDashMeasure.h#newcode22 src/core/SkDashMeasure.h:22: interval_accum ...
4 years, 4 months ago (2016-08-11 12:35:13 UTC) #6
Harry Stern
4 years, 4 months ago (2016-08-11 15:21:09 UTC) #7 
https://codereview.chromium.org/2236483004/diff/20001/src/core/SkDashMeasure.h
File src/core/SkDashMeasure.h (right):

https://codereview.chromium.org/2236483004/diff/20001/src/core/SkDashMeasure....
src/core/SkDashMeasure.h:19: SkScalar interval_accum = 0.0f;
On 2016/08/11 12:35:12, caryclark wrote:
> remove this

Done.

https://codereview.chromium.org/2236483004/diff/20001/src/core/SkDashMeasure....
src/core/SkDashMeasure.h:22: interval_accum = intervals[interval_index];
On 2016/08/11 12:35:12, caryclark wrote:
> SkScalar internal_accum ...

Done.

https://codereview.chromium.org/2236483004/diff/20001/src/core/SkDashMeasure....
src/core/SkDashMeasure.h:32: case (SkPath::kMove_Verb):
On 2016/08/11 12:35:12, caryclark wrote:
> no need for parens

Done.

https://codereview.chromium.org/2236483004/diff/20001/src/core/SkDashMeasure....
src/core/SkDashMeasure.h:45: end = pts[3];
On 2016/08/11 12:35:12, caryclark wrote:
> pts[2]

Done.

https://codereview.chromium.org/2236483004/diff/20001/src/core/SkDashMeasure....
src/core/SkDashMeasure.h:65: SkScalar prevT = 0.0f;
On 2016/08/11 12:35:12, caryclark wrote:
> 0 is fine here

Done.

https://codereview.chromium.org/2236483004/diff/20001/src/core/SkDashMeasure....
src/core/SkDashMeasure.h:72: if (!(interval_index%2)) {
On 2016/08/11 12:35:12, caryclark wrote:
> spaces on both sides of %

Done. I'm actually going to put this in a tiny helper function since it's not
clear just from reading it what/why I'm doing.

https://codereview.chromium.org/2236483004/diff/20001/src/core/SkDashMeasure....
src/core/SkDashMeasure.h:76: dst->moveTo(pos);
On 2016/08/11 12:35:12, caryclark wrote:
> put else on same line as close brace

Done, but I have written my elses on separate lines pretty much everywhere else.
Should I go through SkCurveMeasure and open a CL to fix those?

https://codereview.chromium.org/2236483004/diff/20001/src/core/SkDashMeasure....
src/core/SkDashMeasure.h:81: interval_index =
(interval_index+1)%dashInfo.fCount;
On 2016/08/11 12:35:12, caryclark wrote:
> spaces on both sides of %

Done.

https://codereview.chromium.org/2236483004/diff/20001/src/core/SkDashMeasure....
src/core/SkDashMeasure.h:83: if (!(interval_index%2)) {
On 2016/08/11 12:35:12, caryclark wrote:
> spaces on either side of %

Done.

https://codereview.chromium.org/2236483004/diff/20001/src/core/SkDashMeasure....
src/core/SkDashMeasure.h:84: SkPathMeasure_segTo(pts, segtype, prevT, 1.0f,
dst);
On 2016/08/11 12:35:12, caryclark wrote:
> 1 is fine here

Done.

https://codereview.chromium.org/2236483004/diff/20001/src/core/SkDashMeasure....
src/core/SkDashMeasure.h:86: else {
On 2016/08/11 12:35:12, caryclark wrote:
> put else on same line as close brace

Done.

Powered by Google App Engine
This is Rietveld 408576698