Refactor some vector icon parsing code for reuse

Refactor some vector icon parsing code for reuse.

The goal of this refactor is to create a shared class for
iterating over the elements in a vector icon. This allows
us to get data out of a vector icon without running it through
PaintPath.

BUG=718549
Review-Url: https://codereview.chromium.org/2861203002
Cr-Commit-Position: refs/heads/master@{#470629}
Committed: https://chromium.googlesource.com/chromium/src/+/731e5cc35e6abe694a9d25d0b2345da435a43bfe

[Evan Stade, 2017-05-05 22:56:04]

Description was changed from

==========
wip - refactor some vector icon parsing code for reuse

BUG=
==========

to

==========
Refactor some vector icon parsing code for reuse.

The goal of this refactor is to create a shared class for
iterating over the elements in a vector icon. This allows
us to get data out of a vector icon without running it through
PaintPath.

BUG=718549
==========

[Evan Stade, 2017-05-05 22:56:04]

estade@chromium.org changed reviewers:
+ tdanderson@chromium.org

[Evan Stade, 2017-05-05 22:58:02]

Split off from the greater project so it's easier to digest.

Here's how this will be useful

https://codereview.chromium.org/2860163002/diff/20001/ui/gfx/paint_vector_ico...

(GetDurationOfAnimation needs it)

[Evan Stade, 2017-05-05 22:58:08]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 22:59:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 23:31:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 23:31:38]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Evan Stade, 2017-05-08 16:49:18]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 16:49:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 17:08:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 17:08:26]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tdanderson, 2017-05-08 20:13:20]

LG, I have left some comments below.

https://codereview.chromium.org/2861203002/diff/30001/ui/gfx/paint_vector_ico...
File ui/gfx/paint_vector_icon.cc (right):

https://codereview.chromium.org/2861203002/diff/30001/ui/gfx/paint_vector_ico...
ui/gfx/paint_vector_icon.cc:5: #include "ui/gfx/paint_vector_icon.h"
general comment for follow-up CL(s): I think the addition of unit testing to the
vector icon code would be a good idea. Have you given any thought to that?

https://codereview.chromium.org/2861203002/diff/30001/ui/gfx/paint_vector_ico...
ui/gfx/paint_vector_icon.cc:28: // Helper that simplifies iterating over a
sequence of PathElements.
optional: "...of the form 'command, (argument,)*'" or similar?

https://codereview.chromium.org/2861203002/diff/30001/ui/gfx/paint_vector_ico...
ui/gfx/paint_vector_icon.cc:36: if (command_index_ < 0)
From a usage point of view I would expect that: 

PathParser parser(path_elements);
const CommandType command_type = parser.CurrentCommand();

would set |command_type| as the first thing in |path_elements|. It seems a bit
strange that I need to call Advance() once before being able to read the first
element.

https://codereview.chromium.org/2861203002/diff/30001/ui/gfx/paint_vector_ico...
ui/gfx/paint_vector_icon.cc:47: SkScalar GetArgument(int index) const {
Consider GetArgumentOfCurrentCommand() for increased clarity here.

https://codereview.chromium.org/2861203002/diff/30001/ui/gfx/paint_vector_ico...
ui/gfx/paint_vector_icon.cc:88: case NEW_PATH:
nit: move the 0-argument block above the 1-argument block so the blocks are in
order

[Evan Stade, 2017-05-08 22:13:38]

https://codereview.chromium.org/2861203002/diff/30001/ui/gfx/paint_vector_ico...
File ui/gfx/paint_vector_icon.cc (right):

https://codereview.chromium.org/2861203002/diff/30001/ui/gfx/paint_vector_ico...
ui/gfx/paint_vector_icon.cc:5: #include "ui/gfx/paint_vector_icon.h"
On 2017/05/08 20:13:20, tdanderson wrote:
> general comment for follow-up CL(s): I think the addition of unit testing to
the
> vector icon code would be a good idea. Have you given any thought to that?

Did you have something in mind that you think is important to test?

I like to write tests for things that have a high chance of regressing and/or
not being noticed when regressing. I don't think vector icons have either of
these attributes. Have you seen many, if any, regressions in the routines in
this file?

The other benefit of unit tests is that they provide greater assurance that your
changes still prove functional when you're refactoring or otherwise overhauling
existing code. But they create the burden of updating them when you're making
tweaks to the code, which is often more work than changing the code itself. I
don't think it's worthwhile to write a comprehensive test suite that will
probably just get in the way for a refactoring that's that the current date
purely hypothetical.

I also buy into the notion that tests, especially poorly written ones, create a
false sense of security which, overall, is harmful. Thus if there is not a clear
and present need for tests I err on the side of not writing them.

Also many mistakes made during development are caught by the DCHECKs sprinkled
throughout this file, so our implicit test coverage (chances of turning some bot
red due to an error) is not zero just because explicit test coverage is zero.

https://codereview.chromium.org/2861203002/diff/30001/ui/gfx/paint_vector_ico...
ui/gfx/paint_vector_icon.cc:28: // Helper that simplifies iterating over a
sequence of PathElements.
On 2017/05/08 20:13:20, tdanderson wrote:
> optional: "...of the form 'command, (argument,)*'" or similar?

shrug, I can't think of any extra wording that adds much clarity.

https://codereview.chromium.org/2861203002/diff/30001/ui/gfx/paint_vector_ico...
ui/gfx/paint_vector_icon.cc:36: if (command_index_ < 0)
On 2017/05/08 20:13:20, tdanderson wrote:
> From a usage point of view I would expect that: 
> 
> PathParser parser(path_elements);
> const CommandType command_type = parser.CurrentCommand();
> 
> would set |command_type| as the first thing in |path_elements|.

simplified this and the loop below. Once again proving that while loops are just
poorly-written for loops.

https://codereview.chromium.org/2861203002/diff/30001/ui/gfx/paint_vector_ico...
ui/gfx/paint_vector_icon.cc:47: SkScalar GetArgument(int index) const {
On 2017/05/08 20:13:20, tdanderson wrote:
> Consider GetArgumentOfCurrentCommand() for increased clarity here.

Acknowledged.

https://codereview.chromium.org/2861203002/diff/30001/ui/gfx/paint_vector_ico...
ui/gfx/paint_vector_icon.cc:88: case NEW_PATH:
On 2017/05/08 20:13:20, tdanderson wrote:
> nit: move the 0-argument block above the 1-argument block so the blocks are in
> order

I like this here because it's what I consider a default case, and default
usually comes at the end (of course we don't have an actual |default| for the
normal reason). Also this matches the order on my keyboard number row. Why do
you argue it should be at the top?

[Evan Stade, 2017-05-08 22:13:41]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 22:14:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 22:23:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 22:23:01]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Evan Stade, 2017-05-08 22:55:50]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 23:07:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 01:04:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 01:04:48]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tdanderson, 2017-05-09 16:42:49]

ps5 lgtm

https://codereview.chromium.org/2861203002/diff/30001/ui/gfx/paint_vector_ico...
File ui/gfx/paint_vector_icon.cc (right):

https://codereview.chromium.org/2861203002/diff/30001/ui/gfx/paint_vector_ico...
ui/gfx/paint_vector_icon.cc:5: #include "ui/gfx/paint_vector_icon.h"
On 2017/05/08 22:13:37, Evan Stade wrote:
> On 2017/05/08 20:13:20, tdanderson wrote:
> > general comment for follow-up CL(s): I think the addition of unit testing to
> the
> > vector icon code would be a good idea. Have you given any thought to that?
> 
> Did you have something in mind that you think is important to test?
> 
> I like to write tests for things that have a high chance of regressing and/or
> not being noticed when regressing. I don't think vector icons have either of
> these attributes. Have you seen many, if any, regressions in the routines in
> this file?
> 
> The other benefit of unit tests is that they provide greater assurance that
your
> changes still prove functional when you're refactoring or otherwise
overhauling
> existing code. But they create the burden of updating them when you're making
> tweaks to the code, which is often more work than changing the code itself. I
> don't think it's worthwhile to write a comprehensive test suite that will
> probably just get in the way for a refactoring that's that the current date
> purely hypothetical.
> 
> I also buy into the notion that tests, especially poorly written ones, create
a
> false sense of security which, overall, is harmful. Thus if there is not a
clear
> and present need for tests I err on the side of not writing them.
> 
> Also many mistakes made during development are caught by the DCHECKs sprinkled
> throughout this file, so our implicit test coverage (chances of turning some
bot
> red due to an error) is not zero just because explicit test coverage is zero.

Thank you for sharing this philosophy. I will take an AI to read over the vector
icon code in more detail with testing in mind, and will follow up on this with
you in a separate discussion.

https://codereview.chromium.org/2861203002/diff/30001/ui/gfx/paint_vector_ico...
ui/gfx/paint_vector_icon.cc:28: // Helper that simplifies iterating over a
sequence of PathElements.
On 2017/05/08 22:13:37, Evan Stade wrote:
> On 2017/05/08 20:13:20, tdanderson wrote:
> > optional: "...of the form 'command, (argument,)*'" or similar?
> 
> shrug, I can't think of any extra wording that adds much clarity.

Acknowledged.

https://codereview.chromium.org/2861203002/diff/30001/ui/gfx/paint_vector_ico...
ui/gfx/paint_vector_icon.cc:88: case NEW_PATH:
On 2017/05/08 22:13:37, Evan Stade wrote:
> On 2017/05/08 20:13:20, tdanderson wrote:
> > nit: move the 0-argument block above the 1-argument block so the blocks are
in
> > order
> 
> I like this here because it's what I consider a default case, and default
> usually comes at the end (of course we don't have an actual |default| for the
> normal reason).

> Also this matches the order on my keyboard number row.

I am not sure how that is related. If I had a switch for chars, I would not
order the cases as 'q', 'w', 'e', 'r', ... to match physical keyboard key
ordering.

> Why do you argue it should be at the top?

To me the most readable organization here is for the return values to be in
increasing order. But as you have explained above, you feel it is more readable
when 0 is treated as 'default'. Since neither way is *un*readable, leaving it
as-is works for me.

[Evan Stade, 2017-05-09 21:23:32]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2017-05-09 21:24:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 23:47:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 23:47:28]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Evan Stade, 2017-05-10 01:57:26]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2017-05-10 01:58:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 04:00:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 04:00:42]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[Evan Stade, 2017-05-10 16:31:49]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2017-05-10 16:33:28]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 17:23:28]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1494433909577790,
"parent_rev": "2362e7ebf867f622b28c9f02171dd6b6885ff589", "commit_rev":
"731e5cc35e6abe694a9d25d0b2345da435a43bfe"}

[commit-bot: I haz the power, 2017-05-10 17:23:41]

Description was changed from

==========
Refactor some vector icon parsing code for reuse.

The goal of this refactor is to create a shared class for
iterating over the elements in a vector icon. This allows
us to get data out of a vector icon without running it through
PaintPath.

BUG=718549
==========

to

==========
Refactor some vector icon parsing code for reuse.

The goal of this refactor is to create a shared class for
iterating over the elements in a vector icon. This allows
us to get data out of a vector icon without running it through
PaintPath.

BUG=718549

Review-Url: https://codereview.chromium.org/2861203002
Cr-Commit-Position: refs/heads/master@{#470629}
Committed:
https://chromium.googlesource.com/chromium/src/+/731e5cc35e6abe694a9d25d0b234...
==========

[commit-bot: I haz the power, 2017-05-10 17:23:42]

Committed patchset #5 (id:70001) as
https://chromium.googlesource.com/chromium/src/+/731e5cc35e6abe694a9d25d0b234...