Update BoxDecoration and RenderParagraph to use sky.Color instead of int.

Update BoxDecoration and RenderParagraph to use sky.Color instead of int.

Also add operator==, hashCode, toString, and some basic Color constants to
Color.

R=abarth@chromium.org, ianh@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/9b2e3a2a7839fb9a206ffa71e84f3c5c9f0f4c1e

[Matt Perry, 2015-06-03 18:35:33]

mpcomplete@chromium.org changed reviewers:
+ abarth@chromium.org

[kulakowski, 2015-06-03 19:29:51]

kulakowski@chromium.org changed reviewers:
+ kulakowski@chromium.org

[kulakowski, 2015-06-03 19:29:52]

https://codereview.chromium.org/1162023004/diff/20001/sky/engine/core/paintin...
File sky/engine/core/painting/Color.dart (right):

https://codereview.chromium.org/1162023004/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/Color.dart:31: String toString() => "Color($_value)";
"Color(0x${_value.toRadixString(16)})"

might be nicer. Though this will print e.g. 0xff and not 0x000000ff. There is
https://github.com/dart-lang/intl is you want the latter.

[abarth-chromium, 2015-06-03 19:31:18]

abarth@chromium.org changed reviewers:
- kulakowski@chromium.org

[abarth-chromium, 2015-06-03 19:31:19]

LGTM but Ianh as a comment for you

https://codereview.chromium.org/1162023004/diff/20001/sky/examples/fn2/contai...
File sky/examples/fn2/container.dart (right):

https://codereview.chromium.org/1162023004/diff/20001/sky/examples/fn2/contai...
sky/examples/fn2/container.dart:21: decoration: new
BoxDecoration(backgroundColor: const sky.Color(0xFFFFFF00)),
Should we do all the CSS colors?

[eseidel, 2015-06-03 19:32:20]

eseidel@chromium.org changed reviewers:
+ eseidel@chromium.org

[eseidel, 2015-06-03 19:32:20]

https://codereview.chromium.org/1162023004/diff/20001/sky/engine/core/paintin...
File sky/engine/core/painting/Color.dart (right):

https://codereview.chromium.org/1162023004/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/Color.dart:11: static const Color black = const
Color(0xFF000000);
I wouldn't build these into colors, but rather on top of them.  Have a separate
named_colors.dart if you like.  Or just use the themes/colors.dart (which exists
already) for now.

[Hixie, 2015-06-03 19:35:10]

ianh@google.com changed reviewers:
+ ianh@google.com
- eseidel@chromium.org

[Hixie, 2015-06-03 19:35:11]

https://codereview.chromium.org/1162023004/diff/20001/sky/engine/core/paintin...
File sky/engine/core/painting/Color.dart (right):

https://codereview.chromium.org/1162023004/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/Color.dart:15: static const Color blue = const
Color(0xFF0000FF);
I don't think we should name colours.
It's ok if the framework has palettes, but the core platform shouldn't deal in
names. Once we go down that path, there's no end to it.

https://codereview.chromium.org/1162023004/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/Color.dart:27: }
bool operator == (other) => other is Color && _value == other._value;

https://codereview.chromium.org/1162023004/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/Color.dart:30: }
int get hashCode => _value.hashCode;

https://codereview.chromium.org/1162023004/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/Color.dart:31: String toString() => "Color($_value)";
String toString() => "Color(0x${_value.toRadixString(16).padLeft(8, '0')})";

(and then replace the equivalent code in tests/resources/display_list.dart)

[Hixie, 2015-06-03 20:12:59]

https://codereview.chromium.org/1162023004/diff/20001/sky/tests/raw/render_fl...
File sky/tests/raw/render_flex.dart (right):

https://codereview.chromium.org/1162023004/diff/20001/sky/tests/raw/render_fl...
sky/tests/raw/render_flex.dart:68: renderBlock.add(new RenderSolidColor(const
sky.Color(0x7700FFFF), desiredSize: new sky.Size(50.0, 100.0)));
This is an example of why I don't think we should have named colours. Not all
the colours will ever be named, so you always end up with code that's half-named
half-not, which is ugly.

[Matt Perry, 2015-06-03 20:17:01]

PTAL

https://codereview.chromium.org/1162023004/diff/20001/sky/engine/core/paintin...
File sky/engine/core/painting/Color.dart (right):

https://codereview.chromium.org/1162023004/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/Color.dart:11: static const Color black = const
Color(0xFF000000);
On 2015/06/03 19:32:20, eseidel wrote:
> I wouldn't build these into colors, but rather on top of them.  Have a
separate
> named_colors.dart if you like.  Or just use the themes/colors.dart (which
exists
> already) for now.

Done.

https://codereview.chromium.org/1162023004/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/Color.dart:15: static const Color blue = const
Color(0xFF0000FF);
On 2015/06/03 19:35:10, Hixie wrote:
> I don't think we should name colours.
> It's ok if the framework has palettes, but the core platform shouldn't deal in
> names. Once we go down that path, there's no end to it.

Done.

https://codereview.chromium.org/1162023004/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/Color.dart:27: }
On 2015/06/03 19:35:11, Hixie wrote:
> bool operator == (other) => other is Color && _value == other._value;

Done.

https://codereview.chromium.org/1162023004/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/Color.dart:30: }
On 2015/06/03 19:35:11, Hixie wrote:
> int get hashCode => _value.hashCode;

Done.

https://codereview.chromium.org/1162023004/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/Color.dart:31: String toString() => "Color($_value)";
On 2015/06/03 19:35:11, Hixie wrote:
> String toString() => "Color(0x${_value.toRadixString(16).padLeft(8, '0')})";
> 
> (and then replace the equivalent code in tests/resources/display_list.dart)

Awesome, thanks. Done.

Are you OK with the Paint.toString displaying Paint(Color(0x00F00BA4)) ?

[Hixie, 2015-06-03 20:24:54]

> Are you OK with the Paint.toString displaying Paint(Color(0x00F00BA4)) ?

Yeah that's totally fine. In fact, that'd be great. That way we can move even
more of this out of display-list.dart and it makes print debugging a lot easier.
All our internal classes should have useful toString()s.

Don't forget to update the test expectation files if you change the format.

[Hixie, 2015-06-03 20:25:07]

https://codereview.chromium.org/1162023004/diff/40001/sky/engine/core/paintin...
File sky/engine/core/painting/Color.dart (right):

https://codereview.chromium.org/1162023004/diff/40001/sky/engine/core/paintin...
sky/engine/core/painting/Color.dart:18: bool operator ==(other) => other is
Color && _value == other._value;
I recommend a space between the == and the argument, since otherwise it looks
like the operator is "==("

[Matt Perry, 2015-06-03 20:37:32]

Updated the test expectations.

https://codereview.chromium.org/1162023004/diff/40001/sky/engine/core/paintin...
File sky/engine/core/painting/Color.dart (right):

https://codereview.chromium.org/1162023004/diff/40001/sky/engine/core/paintin...
sky/engine/core/painting/Color.dart:18: bool operator ==(other) => other is
Color && _value == other._value;
On 2015/06/03 20:25:07, Hixie wrote:
> I recommend a space between the == and the argument, since otherwise it looks
> like the operator is "==("

No space is the preferred style according to
https://www.dartlang.org/articles/style-guide/#whitespace and all other
instances in our codebase (git grep operator -- '*.dart'). Plus, four-eyed
frowny operator is best operator.

[Hixie, 2015-06-03 20:52:57]

On 2015/06/03 at 20:37:32, mpcomplete wrote:
> Updated the test expectations.
> 
>
https://codereview.chromium.org/1162023004/diff/40001/sky/engine/core/paintin...
> File sky/engine/core/painting/Color.dart (right):
> 
>
https://codereview.chromium.org/1162023004/diff/40001/sky/engine/core/paintin...
> sky/engine/core/painting/Color.dart:18: bool operator ==(other) => other is
Color && _value == other._value;
> On 2015/06/03 20:25:07, Hixie wrote:
> > I recommend a space between the == and the argument, since otherwise it
looks
> > like the operator is "==("
> 
> No space is the preferred style according to
https://www.dartlang.org/articles/style-guide/#whitespace and all other
instances in our codebase (git grep operator -- '*.dart'). Plus, four-eyed
frowny operator is best operator.

I disagree with that style guide. But I guess that's not your problem. :-)

[Hixie, 2015-06-03 20:53:54]

lgtm

[Matt Perry, 2015-06-03 20:57:09]

Committed patchset #4 (id:60001) manually as
9b2e3a2a7839fb9a206ffa71e84f3c5c9f0f4c1e (presubmit successful).