Use int rather than size_t to pass DIP amounts to vector icon functions.

Use int rather than size_t to pass DIP amounts to vector icon functions.

These are basically signed integral values rather than byte lengths in memory.
Clues that back this up:
* The vector icon code cast the values back to int to negate them.
* Nearly all callers actually supplied ints anyway (but by default our compilers
  don't warn on this)
* The rest of views uses ints for px/DIP values

This also makes an SkScalar->integral conversion that was happening implicitly
explicit, using the same conversion the parsing code uses.

BUG=none
TEST=none
TBR=sky

Committed: https://crrev.com/21e243fe392999bd8a060469f68fdf45955d21b6
Cr-Commit-Position: refs/heads/master@{#413846}

[Peter Kasting, 2016-08-18 22:17:55]

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

[commit-bot: I haz the power, 2016-08-18 22:18:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Kasting, 2016-08-18 22:18:42]

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

[Peter Kasting, 2016-08-18 22:42:42]

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

[commit-bot: I haz the power, 2016-08-18 22:43:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2016-08-18 23:25:37]

https://codereview.chromium.org/2255403002/diff/20001/ui/gfx/paint_vector_ico...
File ui/gfx/paint_vector_icon.cc (left):

https://codereview.chromium.org/2255403002/diff/20001/ui/gfx/paint_vector_ico...
ui/gfx/paint_vector_icon.cc:298:
canvas->Translate(gfx::Vector2d(-static_cast<int>(canvas_size), 0));
I don't think this cast is an indication that a negative canvas_size would make
sense. It's an indication that we want to move to the left by the (positive)
canvas_size.

[Peter Kasting, 2016-08-18 23:35:13]

https://codereview.chromium.org/2255403002/diff/20001/ui/gfx/paint_vector_ico...
File ui/gfx/paint_vector_icon.cc (left):

https://codereview.chromium.org/2255403002/diff/20001/ui/gfx/paint_vector_ico...
ui/gfx/paint_vector_icon.cc:298:
canvas->Translate(gfx::Vector2d(-static_cast<int>(canvas_size), 0));
On 2016/08/18 23:25:37, Evan Stade wrote:
> I don't think this cast is an indication that a negative canvas_size would
make
> sense. It's an indication that we want to move to the left by the (positive)
> canvas_size.

Well, a negative canvas size doesn't make sense in the same way a gfx::Size()
with negative width/height doesn't make sense.  Note that that takes ints as
well.  This is one of those cases where I think the style guide's "don't use an
unsigned type just to document that a value cannot be negative" is in play.

The units involved here are DIP, and while a negative size may not be sensible,
a negative quantity of DIPs is certainly a meaningful thing (here, it means
"move left").  I think the choice of the rest of the codebase to use ints for
DIP and px values is the right one.

[commit-bot: I haz the power, 2016-08-19 00:47:09]

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

[commit-bot: I haz the power, 2016-08-19 00:47:10]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-08-19 04:21:49]

https://codereview.chromium.org/2255403002/diff/20001/ui/gfx/paint_vector_ico...
File ui/gfx/paint_vector_icon.cc (left):

https://codereview.chromium.org/2255403002/diff/20001/ui/gfx/paint_vector_ico...
ui/gfx/paint_vector_icon.cc:298:
canvas->Translate(gfx::Vector2d(-static_cast<int>(canvas_size), 0));
On 2016/08/18 23:35:13, Peter Kasting wrote:
> On 2016/08/18 23:25:37, Evan Stade wrote:
> > I don't think this cast is an indication that a negative canvas_size would
> make
> > sense. It's an indication that we want to move to the left by the (positive)
> > canvas_size.
> 
> Well, a negative canvas size doesn't make sense in the same way a gfx::Size()
> with negative width/height doesn't make sense.  Note that that takes ints as
> well.  This is one of those cases where I think the style guide's "don't use
an
> unsigned type just to document that a value cannot be negative" is in play.

I didn't know the style guide said that and I still don't know why it says that
(why would we prefer a runtime assertion over a compile time one?)

> 
> The units involved here are DIP, and while a negative size may not be
sensible,
> a negative quantity of DIPs is certainly a meaningful thing (here, it means
> "move left").  I think the choice of the rest of the codebase to use ints for
> DIP and px values is the right one.

yea, you're hitting on something that I think is an important distinction. A
count of pixels is different from a vector measured in pixels.

My reading of the style guide doesn't match yours. It says "When appropriate,
you are welcome to use standard types like size_t". When do you think is more
appropriate than when you're describing a quantity that cannot be negative? It
has size in the name so I imagine this kind of thing (canvas_size) must be what
it's intended for. The line about not using unsigned types for non-negative
numbers is in the context of the discussion of unsigned integers like uint32_t.

[Peter Kasting, 2016-08-19 05:38:24]

On 2016/08/19 04:21:49, Evan Stade wrote:
> > The units involved here are DIP, and while a negative size may not be
> sensible,
> > a negative quantity of DIPs is certainly a meaningful thing (here, it means
> > "move left").  I think the choice of the rest of the codebase to use ints
for
> > DIP and px values is the right one.
> 
> yea, you're hitting on something that I think is an important distinction. A
> count of pixels is different from a vector measured in pixels.
> 
> My reading of the style guide doesn't match yours. It says "When appropriate,
> you are welcome to use standard types like size_t". When do you think is more
> appropriate than when you're describing a quantity that cannot be negative? It
> has size in the name so I imagine this kind of thing (canvas_size) must be
what
> it's intended for.

size_t is specifically for byte sizes of memory allocations, e.g.
std::string::length(), and the type is sized to match the pointer size on the
machine (so, on a 64-bit machine, size_t is 64 bits).  It's not just "anything
that is a non-negative size of something".  It has a fairly specific meaning,
much like ptrdiff_t.

There's a long history on the Google style guide directive here -- it's one of
the longest-standing and most-discussed bullets.  A lot of Googlers actually err
on the opposite side and disallow any use of size_t for any reason, but the
directive was intended to carve out an exception for use of size_t when doing
things like what the STL uses it for (lengths of memory-allocated objects) and
interacting directly with the STL and other such libraries.  The hope was to
avoid type-casting in such cases, as well as avoiding potential value truncation
problems, e.g. with large allocated object lengths passed to code that tried to
store them in an int.

The general directive against using an unsigned type as a way of documenting
that values don't go negative was supposed to apply to any unsigned type
(unsigned int, uint32_t, size_t, etc.).  I don't really love the style arbiters'
ruling here; the guide cites a bit of the potential issues for e.g.
non-terminating loops, but I kinda disagree with their take that those problems
outweigh the gains in this case.  Still, that's what the guide says.

If you still don't think this change is appropriate, you're welcome to ask the
opinions of other Chromium style arbiters here, but I would be stunned if they
weren't even more reticent to use size_t than I am -- I generally have to fight
to allow its use :).

[Evan Stade, 2016-08-22 20:38:07]

On 2016/08/19 05:38:24, Peter Kasting wrote:
> On 2016/08/19 04:21:49, Evan Stade wrote:
> > > The units involved here are DIP, and while a negative size may not be
> > sensible,
> > > a negative quantity of DIPs is certainly a meaningful thing (here, it
means
> > > "move left").  I think the choice of the rest of the codebase to use ints
> for
> > > DIP and px values is the right one.
> > 
> > yea, you're hitting on something that I think is an important distinction. A
> > count of pixels is different from a vector measured in pixels.
> > 
> > My reading of the style guide doesn't match yours. It says "When
appropriate,
> > you are welcome to use standard types like size_t". When do you think is
more
> > appropriate than when you're describing a quantity that cannot be negative?
It
> > has size in the name so I imagine this kind of thing (canvas_size) must be
> what
> > it's intended for.
> 
> size_t is specifically for byte sizes of memory allocations, e.g.
> std::string::length(), and the type is sized to match the pointer size on the
> machine (so, on a 64-bit machine, size_t is 64 bits).  It's not just "anything
> that is a non-negative size of something".  It has a fairly specific meaning,
> much like ptrdiff_t.
> 
> There's a long history on the Google style guide directive here -- it's one of
> the longest-standing and most-discussed bullets.  A lot of Googlers actually
err
> on the opposite side and disallow any use of size_t for any reason, but the
> directive was intended to carve out an exception for use of size_t when doing
> things like what the STL uses it for (lengths of memory-allocated objects) and
> interacting directly with the STL and other such libraries.  The hope was to
> avoid type-casting in such cases, as well as avoiding potential value
truncation
> problems, e.g. with large allocated object lengths passed to code that tried
to
> store them in an int.
> 
> The general directive against using an unsigned type as a way of documenting
> that values don't go negative was supposed to apply to any unsigned type
> (unsigned int, uint32_t, size_t, etc.).  I don't really love the style
arbiters'
> ruling here; the guide cites a bit of the potential issues for e.g.
> non-terminating loops, but I kinda disagree with their take that those
problems
> outweigh the gains in this case.  Still, that's what the guide says.
> 
> If you still don't think this change is appropriate, you're welcome to ask the
> opinions of other Chromium style arbiters here, but I would be stunned if they
> weren't even more reticent to use size_t than I am -- I generally have to
fight
> to allow its use :).

It's unfortunate that the style guide isn't at least more specific about what
constitutes an appropriate use of size_t, leaving that bit up for
interpretation. It may be as you say, but they should make that explicit if so.
If you want to start a thread about it on chromium-dev and there's a consensus,
and we codify that in the chrome-specific style guide using specific language
like what you've written above, then I'm all for this change. The fact that this
passed reviewer muster might indicate that not everyone has the same opinion.

[Peter Kasting, 2016-08-22 21:54:16]

On 2016/08/22 20:38:07, Evan Stade wrote:
> On 2016/08/19 05:38:24, Peter Kasting wrote:
> > On 2016/08/19 04:21:49, Evan Stade wrote:
> > > > The units involved here are DIP, and while a negative size may not be
> > > sensible,
> > > > a negative quantity of DIPs is certainly a meaningful thing (here, it
> means
> > > > "move left").  I think the choice of the rest of the codebase to use
ints
> > for
> > > > DIP and px values is the right one.
> > > 
> > > yea, you're hitting on something that I think is an important distinction.
A
> > > count of pixels is different from a vector measured in pixels.
> > > 
> > > My reading of the style guide doesn't match yours. It says "When
> appropriate,
> > > you are welcome to use standard types like size_t". When do you think is
> more
> > > appropriate than when you're describing a quantity that cannot be
negative?
> It
> > > has size in the name so I imagine this kind of thing (canvas_size) must be
> > what
> > > it's intended for.
> > 
> > size_t is specifically for byte sizes of memory allocations, e.g.
> > std::string::length(), and the type is sized to match the pointer size on
the
> > machine (so, on a 64-bit machine, size_t is 64 bits).  It's not just
"anything
> > that is a non-negative size of something".  It has a fairly specific
meaning,
> > much like ptrdiff_t.
> > 
> > There's a long history on the Google style guide directive here -- it's one
of
> > the longest-standing and most-discussed bullets.  A lot of Googlers actually
> err
> > on the opposite side and disallow any use of size_t for any reason, but the
> > directive was intended to carve out an exception for use of size_t when
doing
> > things like what the STL uses it for (lengths of memory-allocated objects)
and
> > interacting directly with the STL and other such libraries.  The hope was to
> > avoid type-casting in such cases, as well as avoiding potential value
> truncation
> > problems, e.g. with large allocated object lengths passed to code that tried
> to
> > store them in an int.
> > 
> > The general directive against using an unsigned type as a way of documenting
> > that values don't go negative was supposed to apply to any unsigned type
> > (unsigned int, uint32_t, size_t, etc.).  I don't really love the style
> arbiters'
> > ruling here; the guide cites a bit of the potential issues for e.g.
> > non-terminating loops, but I kinda disagree with their take that those
> problems
> > outweigh the gains in this case.  Still, that's what the guide says.
> > 
> > If you still don't think this change is appropriate, you're welcome to ask
the
> > opinions of other Chromium style arbiters here, but I would be stunned if
they
> > weren't even more reticent to use size_t than I am -- I generally have to
> fight
> > to allow its use :).
> 
> It's unfortunate that the style guide isn't at least more specific about what
> constitutes an appropriate use of size_t, leaving that bit up for
> interpretation. It may be as you say, but they should make that explicit if
so.
> If you want to start a thread about it on chromium-dev and there's a
consensus,
> and we codify that in the chrome-specific style guide using specific language
> like what you've written above, then I'm all for this change. The fact that
this
> passed reviewer muster might indicate that not everyone has the same opinion.

I have tried to get the Google style arbiters to clarify the language of the
Google style guide before, but failed.  I don't know why they're opposed to
simply writing something clear; I think that since the language here is so
longstanding, they'd rather not touch what's "working" (even though IMO it is
not "working").

I don't think we should be adding language in the Chromium style guide that's
not an exception to the Google style guide, but merely an attempt to clarify
what it supposedly says.  However, perhaps others disagree.  What I'll do here
is start a thread with the Chromium C++ arbiters first, and see what they think
is the right plan.

[Evan Stade, 2016-08-23 01:26:18]

On 2016/08/22 21:54:16, Peter Kasting wrote:
> On 2016/08/22 20:38:07, Evan Stade wrote:
> > On 2016/08/19 05:38:24, Peter Kasting wrote:
> > > On 2016/08/19 04:21:49, Evan Stade wrote:
> > > > > The units involved here are DIP, and while a negative size may not be
> > > > sensible,
> > > > > a negative quantity of DIPs is certainly a meaningful thing (here, it
> > means
> > > > > "move left").  I think the choice of the rest of the codebase to use
> ints
> > > for
> > > > > DIP and px values is the right one.
> > > > 
> > > > yea, you're hitting on something that I think is an important
distinction.
> A
> > > > count of pixels is different from a vector measured in pixels.
> > > > 
> > > > My reading of the style guide doesn't match yours. It says "When
> > appropriate,
> > > > you are welcome to use standard types like size_t". When do you think is
> > more
> > > > appropriate than when you're describing a quantity that cannot be
> negative?
> > It
> > > > has size in the name so I imagine this kind of thing (canvas_size) must
be
> > > what
> > > > it's intended for.
> > > 
> > > size_t is specifically for byte sizes of memory allocations, e.g.
> > > std::string::length(), and the type is sized to match the pointer size on
> the
> > > machine (so, on a 64-bit machine, size_t is 64 bits).  It's not just
> "anything
> > > that is a non-negative size of something".  It has a fairly specific
> meaning,
> > > much like ptrdiff_t.
> > > 
> > > There's a long history on the Google style guide directive here -- it's
one
> of
> > > the longest-standing and most-discussed bullets.  A lot of Googlers
actually
> > err
> > > on the opposite side and disallow any use of size_t for any reason, but
the
> > > directive was intended to carve out an exception for use of size_t when
> doing
> > > things like what the STL uses it for (lengths of memory-allocated objects)
> and
> > > interacting directly with the STL and other such libraries.  The hope was
to
> > > avoid type-casting in such cases, as well as avoiding potential value
> > truncation
> > > problems, e.g. with large allocated object lengths passed to code that
tried
> > to
> > > store them in an int.
> > > 
> > > The general directive against using an unsigned type as a way of
documenting
> > > that values don't go negative was supposed to apply to any unsigned type
> > > (unsigned int, uint32_t, size_t, etc.).  I don't really love the style
> > arbiters'
> > > ruling here; the guide cites a bit of the potential issues for e.g.
> > > non-terminating loops, but I kinda disagree with their take that those
> > problems
> > > outweigh the gains in this case.  Still, that's what the guide says.
> > > 
> > > If you still don't think this change is appropriate, you're welcome to ask
> the
> > > opinions of other Chromium style arbiters here, but I would be stunned if
> they
> > > weren't even more reticent to use size_t than I am -- I generally have to
> > fight
> > > to allow its use :).
> > 
> > It's unfortunate that the style guide isn't at least more specific about
what
> > constitutes an appropriate use of size_t, leaving that bit up for
> > interpretation. It may be as you say, but they should make that explicit if
> so.
> > If you want to start a thread about it on chromium-dev and there's a
> consensus,
> > and we codify that in the chrome-specific style guide using specific
language
> > like what you've written above, then I'm all for this change. The fact that
> this
> > passed reviewer muster might indicate that not everyone has the same
opinion.
> 
> I have tried to get the Google style arbiters to clarify the language of the
> Google style guide before, but failed.  I don't know why they're opposed to
> simply writing something clear; I think that since the language here is so
> longstanding, they'd rather not touch what's "working" (even though IMO it is
> not "working").
> 
> I don't think we should be adding language in the Chromium style guide that's
> not an exception to the Google style guide, but merely an attempt to clarify
> what it supposedly says.  However, perhaps others disagree.  What I'll do here
> is start a thread with the Chromium C++ arbiters first, and see what they
think
> is the right plan.

ok thanks, given the language in our own style guide, lgtm

[Peter Kasting, 2016-08-23 01:33:37]

On 2016/08/23 01:26:18, Evan Stade wrote:
> ok thanks, given the language in our own style guide, lgtm

Without your pushback I wouldn't have rediscovered the language in our own style
guide, so no harm done!

[Peter Kasting, 2016-08-23 01:33:54]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2016-08-23 01:34:44]

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, 2016-08-23 01:37:27]

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

[commit-bot: I haz the power, 2016-08-23 01:37:28]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Peter Kasting, 2016-08-23 20:11:54]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2016-08-23 20:11:54]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2255403002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-23 20:12:20]

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, 2016-08-23 20:21:39]

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

[commit-bot: I haz the power, 2016-08-23 20:21:41]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Peter Kasting, 2016-08-23 20:25:25]

Description was changed from

==========
Use int rather than size_t to pass DIP amounts to vector icon functions.

These are basically signed integral values rather than byte lengths in memory.
Clues that back this up:
* The vector icon code cast the values back to int to negate them.
* Nearly all callers actually supplied ints anyway (but by default our compilers
  don't warn on this)
* The rest of views uses ints for px/DIP values

This also makes an SkScalar->integral conversion that was happening implicitly
explicit, using the same conversion the parsing code uses.

BUG=none
TEST=none
==========

to

==========
Use int rather than size_t to pass DIP amounts to vector icon functions.

These are basically signed integral values rather than byte lengths in memory.
Clues that back this up:
* The vector icon code cast the values back to int to negate them.
* Nearly all callers actually supplied ints anyway (but by default our compilers
  don't warn on this)
* The rest of views uses ints for px/DIP values

This also makes an SkScalar->integral conversion that was happening implicitly
explicit, using the same conversion the parsing code uses.

BUG=none
TEST=none
TBR=sky
==========

[Peter Kasting, 2016-08-23 20:25:25]

pkasting@chromium.org changed reviewers:
+ sky@chromium.org

[Peter Kasting, 2016-08-23 20:25:50]

TBRing to sky for mechanical change in ui/views/examples/.

[Peter Kasting, 2016-08-23 20:25:59]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2016-08-23 20:26:44]

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, 2016-08-23 21:37:15]

Description was changed from

==========
Use int rather than size_t to pass DIP amounts to vector icon functions.

These are basically signed integral values rather than byte lengths in memory.
Clues that back this up:
* The vector icon code cast the values back to int to negate them.
* Nearly all callers actually supplied ints anyway (but by default our compilers
  don't warn on this)
* The rest of views uses ints for px/DIP values

This also makes an SkScalar->integral conversion that was happening implicitly
explicit, using the same conversion the parsing code uses.

BUG=none
TEST=none
TBR=sky
==========

to

==========
Use int rather than size_t to pass DIP amounts to vector icon functions.

These are basically signed integral values rather than byte lengths in memory.
Clues that back this up:
* The vector icon code cast the values back to int to negate them.
* Nearly all callers actually supplied ints anyway (but by default our compilers
  don't warn on this)
* The rest of views uses ints for px/DIP values

This also makes an SkScalar->integral conversion that was happening implicitly
explicit, using the same conversion the parsing code uses.

BUG=none
TEST=none
TBR=sky
==========

[commit-bot: I haz the power, 2016-08-23 21:37:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-23 21:40:00]

Description was changed from

==========
Use int rather than size_t to pass DIP amounts to vector icon functions.

These are basically signed integral values rather than byte lengths in memory.
Clues that back this up:
* The vector icon code cast the values back to int to negate them.
* Nearly all callers actually supplied ints anyway (but by default our compilers
  don't warn on this)
* The rest of views uses ints for px/DIP values

This also makes an SkScalar->integral conversion that was happening implicitly
explicit, using the same conversion the parsing code uses.

BUG=none
TEST=none
TBR=sky
==========

to

==========
Use int rather than size_t to pass DIP amounts to vector icon functions.

These are basically signed integral values rather than byte lengths in memory.
Clues that back this up:
* The vector icon code cast the values back to int to negate them.
* Nearly all callers actually supplied ints anyway (but by default our compilers
  don't warn on this)
* The rest of views uses ints for px/DIP values

This also makes an SkScalar->integral conversion that was happening implicitly
explicit, using the same conversion the parsing code uses.

BUG=none
TEST=none
TBR=sky

Committed: https://crrev.com/21e243fe392999bd8a060469f68fdf45955d21b6
Cr-Commit-Position: refs/heads/master@{#413846}
==========

[commit-bot: I haz the power, 2016-08-23 21:40:01]

Patchset 3 (id:??) landed as
https://crrev.com/21e243fe392999bd8a060469f68fdf45955d21b6
Cr-Commit-Position: refs/heads/master@{#413846}

[sky, 2016-08-23 23:53:37]

Thanks for the interesting discussion! I'm wondering if there should be
DCHECK_GE(xxx, 0) in some of these places now?

https://codereview.chromium.org/2255403002/diff/40001/ui/views/examples/vecto...
File ui/views/examples/vector_example.cc (right):

https://codereview.chromium.org/2255403002/diff/40001/ui/views/examples/vecto...
ui/views/examples/vector_example.cc:100: if (base::StringToInt(new_contents,
&size_))
If negative is considered an error condition shouldn't this DCHECK (which is
fine, give this is an example), or show the error some way?

[Peter Kasting, 2016-08-24 02:42:00]

https://codereview.chromium.org/2255403002/diff/40001/ui/views/examples/vecto...
File ui/views/examples/vector_example.cc (right):

https://codereview.chromium.org/2255403002/diff/40001/ui/views/examples/vecto...
ui/views/examples/vector_example.cc:100: if (base::StringToInt(new_contents,
&size_))
On 2016/08/23 23:53:37, sky wrote:
> If negative is considered an error condition shouldn't this DCHECK (which is
> fine, give this is an example), or show the error some way?

It looks like gfx::Size() will silently clamp negatives to zero, so I think
right now you'll get the same effect as if you entered a zero.

I figured given that this was an example, this kind of error-checking/handling
didn't really matter, but this is, I think, a behavior change from before
(sorry, I should have noticed this).  I think previously, entering a negative
would return false, so we'd just clear the input field.

So the behavior-preserving change would be to add " || (size_ < 0)" to the
condition.

Should I make this change?  Do something else?

[sky, 2016-08-24 15:53:59]

This is an example, so not terribly important. I leave it to you.

On Tue, Aug 23, 2016 at 7:42 PM,  <pkasting@chromium.org> wrote:
>
>
https://codereview.chromium.org/2255403002/diff/40001/ui/views/examples/vecto...
> File ui/views/examples/vector_example.cc (right):
>
>
https://codereview.chromium.org/2255403002/diff/40001/ui/views/examples/vecto...
> ui/views/examples/vector_example.cc:100: if
> (base::StringToInt(new_contents, &size_))
> On 2016/08/23 23:53:37, sky wrote:
>> If negative is considered an error condition shouldn't this DCHECK
> (which is
>> fine, give this is an example), or show the error some way?
>
> It looks like gfx::Size() will silently clamp negatives to zero, so I
> think right now you'll get the same effect as if you entered a zero.
>
> I figured given that this was an example, this kind of
> error-checking/handling didn't really matter, but this is, I think, a
> behavior change from before (sorry, I should have noticed this). I
> think previously, entering a negative would return false, so we'd just
> clear the input field.
>
> So the behavior-preserving change would be to add " || (size_ < 0)" to
> the condition.
>
> Should I make this change? Do something else?
>
> https://codereview.chromium.org/2255403002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.