Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2307533003:
Add support for CUBIC_TO_S for rendering vector icons (Closed)
|
Created:
4 years, 3 months ago by tdanderson Modified:
4 years, 3 months ago Reviewers:
Evan Stade CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for CUBIC_TO_S for rendering vector icons
Add support for the vector icon drawing command
CUBIC_TO_S, which will correspond to the SVG
path command 'S'. This new command computes
its implicit first control point by using
information about the current point and the
second control point on the previous command.
BUG=643265
TEST=manual
Committed: https://crrev.com/9bcbbd3b5ffe60e191a92ff7cabde7cf3aac4502
Cr-Commit-Position: refs/heads/master@{#416996}
Patch Set 1 #
Total comments: 12
Patch Set 2 : comments addressed #
Total comments: 1
Messages
Total messages: 21 (11 generated)
The CQ bit was checked by tdanderson@chromium.org 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...
tdanderson@chromium.org changed reviewers: + estade@chromium.org
Evan, please take a look. I will modify Skiafy as follow-up.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:227: SkScalar x = path_elements[++i].arg; why the name change? https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:244: case CUBIC_TO_S: { we don't want/need an R_ version? https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:256: delta_x = path_elements[i - 2].arg - path_elements[i - 4].arg; this makes me a little uncomfortable. Can we use SkPath::getPoint to get the last couple points instead of just getLastPt?
https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:227: SkScalar x = path_elements[++i].arg; On 2016/09/01 20:21:22, Evan Stade wrote: > why the name change? For naming consistency with the SVG spec. https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:244: case CUBIC_TO_S: { On 2016/09/01 20:21:22, Evan Stade wrote: > we don't want/need an R_ version? That case is handled in the skiafy script. https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:256: delta_x = path_elements[i - 2].arg - path_elements[i - 4].arg; On 2016/09/01 20:21:22, Evan Stade wrote: > this makes me a little uncomfortable. Can we use SkPath::getPoint to get the > last couple points instead of just getLastPt? What worries you about this? Even if getPoint() is used, we'd still need two array-accesses for the second control point on the previous command; as I understand it the control points are not considered points on the path.
https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:227: SkScalar x = path_elements[++i].arg; On 2016/09/01 20:45:00, tdanderson wrote: > On 2016/09/01 20:21:22, Evan Stade wrote: > > why the name change? > > For naming consistency with the SVG spec. that was not my goal in choosing these variable names and I am not sure it's worth attempting to maintain that kind of consistency https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:244: case CUBIC_TO_S: { On 2016/09/01 20:45:00, tdanderson wrote: > On 2016/09/01 20:21:22, Evan Stade wrote: > > we don't want/need an R_ version? > > That case is handled in the skiafy script. ok. Could you name it SHORTHAND_CUBIC_TO or something more obvious than S? https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:256: delta_x = path_elements[i - 2].arg - path_elements[i - 4].arg; On 2016/09/01 20:45:00, tdanderson wrote: > On 2016/09/01 20:21:22, Evan Stade wrote: > > this makes me a little uncomfortable. Can we use SkPath::getPoint to get the > > last couple points instead of just getLastPt? > > What worries you about this? I dunno, I just dislike negative signs in an array access... > Even if getPoint() is used, we'd still need two > array-accesses for the second control point on the previous command; as I > understand it the control points are not considered points on the path. they aren't? That's surprising. What do the points on the path look like? This makes it look like all the control points you pass are added to the path: https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkPath.cpp?rcl...
The CQ bit was checked by tdanderson@chromium.org 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...
Evan, please take another look. https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:227: SkScalar x = path_elements[++i].arg; On 2016/09/01 20:53:22, Evan Stade (ooo friday) wrote: > On 2016/09/01 20:45:00, tdanderson wrote: > > On 2016/09/01 20:21:22, Evan Stade wrote: > > > why the name change? > > > > For naming consistency with the SVG spec. > > that was not my goal in choosing these variable names and I am not sure it's > worth attempting to maintain that kind of consistency OK, I'll change them back. https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:244: case CUBIC_TO_S: { On 2016/09/01 20:53:22, Evan Stade (ooo friday) wrote: > On 2016/09/01 20:45:00, tdanderson wrote: > > On 2016/09/01 20:21:22, Evan Stade wrote: > > > we don't want/need an R_ version? > > > > That case is handled in the skiafy script. > > ok. Could you name it SHORTHAND_CUBIC_TO or something more obvious than S? Done. https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:256: delta_x = path_elements[i - 2].arg - path_elements[i - 4].arg; On 2016/09/01 20:53:22, Evan Stade (ooo friday) wrote: > On 2016/09/01 20:45:00, tdanderson wrote: > > On 2016/09/01 20:21:22, Evan Stade wrote: > > > this makes me a little uncomfortable. Can we use SkPath::getPoint to get the > > > last couple points instead of just getLastPt? > > > > What worries you about this? > > I dunno, I just dislike negative signs in an array access... > > > Even if getPoint() is used, we'd still need two > > array-accesses for the second control point on the previous command; as I > > understand it the control points are not considered points on the path. > > they aren't? That's surprising. What do the points on the path look like? This > makes it look like all the control points you pass are added to the path: > https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkPath.cpp?rcl... Sorry, it looks like I was mistaken. Changed in the next patch set.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2016/09/02 16:40:49, tdanderson wrote: > Evan, please take another look. > > https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc > File ui/gfx/paint_vector_icon.cc (right): > > https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc... > ui/gfx/paint_vector_icon.cc:227: SkScalar x = path_elements[++i].arg; > On 2016/09/01 20:53:22, Evan Stade (ooo friday) wrote: > > On 2016/09/01 20:45:00, tdanderson wrote: > > > On 2016/09/01 20:21:22, Evan Stade wrote: > > > > why the name change? > > > > > > For naming consistency with the SVG spec. > > > > that was not my goal in choosing these variable names and I am not sure it's > > worth attempting to maintain that kind of consistency > > OK, I'll change them back. > > https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc... > ui/gfx/paint_vector_icon.cc:244: case CUBIC_TO_S: { > On 2016/09/01 20:53:22, Evan Stade (ooo friday) wrote: > > On 2016/09/01 20:45:00, tdanderson wrote: > > > On 2016/09/01 20:21:22, Evan Stade wrote: > > > > we don't want/need an R_ version? > > > > > > That case is handled in the skiafy script. > > > > ok. Could you name it SHORTHAND_CUBIC_TO or something more obvious than S? > > Done. > > https://codereview.chromium.org/2307533003/diff/1/ui/gfx/paint_vector_icon.cc... > ui/gfx/paint_vector_icon.cc:256: delta_x = path_elements[i - 2].arg - > path_elements[i - 4].arg; > On 2016/09/01 20:53:22, Evan Stade (ooo friday) wrote: > > On 2016/09/01 20:45:00, tdanderson wrote: > > > On 2016/09/01 20:21:22, Evan Stade wrote: > > > > this makes me a little uncomfortable. Can we use SkPath::getPoint to get > the > > > > last couple points instead of just getLastPt? > > > > > > What worries you about this? > > > > I dunno, I just dislike negative signs in an array access... > > > > > Even if getPoint() is used, we'd still need two > > > array-accesses for the second control point on the previous command; as I > > > understand it the control points are not considered points on the path. > > > > they aren't? That's surprising. What do the points on the path look like? This > > makes it look like all the control points you pass are added to the path: > > > https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkPath.cpp?rcl... > > Sorry, it looks like I was mistaken. Changed in the next patch set. Evan, friendly ping on this CL.
lgtm https://codereview.chromium.org/2307533003/diff/20001/ui/gfx/paint_vector_ico... File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/2307533003/diff/20001/ui/gfx/paint_vector_ico... ui/gfx/paint_vector_icon.cc:255: if (previous_command_type == CUBIC_TO || it seems there's no easy way to get the last verb out of the SkPath, which is too bad, because if (path.getLastVerb() == SkPath::kCubic_Verb) would be slick.
The CQ bit was checked by tdanderson@chromium.org
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.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add support for CUBIC_TO_S for rendering vector icons Add support for the vector icon drawing command CUBIC_TO_S, which will correspond to the SVG path command 'S'. This new command computes its implicit first control point by using information about the current point and the second control point on the previous command. BUG=643265 TEST=manual ========== to ========== Add support for CUBIC_TO_S for rendering vector icons Add support for the vector icon drawing command CUBIC_TO_S, which will correspond to the SVG path command 'S'. This new command computes its implicit first control point by using information about the current point and the second control point on the previous command. BUG=643265 TEST=manual Committed: https://crrev.com/9bcbbd3b5ffe60e191a92ff7cabde7cf3aac4502 Cr-Commit-Position: refs/heads/master@{#416996} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9bcbbd3b5ffe60e191a92ff7cabde7cf3aac4502 Cr-Commit-Position: refs/heads/master@{#416996} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
