code review 13239051: image: add RGB and RGB48

image: add RGB and RGB48

[chai2010, 2013-09-12 13:01:41]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
http://code.google.com/p/go/

[r, 2013-09-12 22:12:02]

This will have to wait until after the 1.2 release is out the door.
Please send the CL again then.

[nigeltao, 2013-09-13 01:17:28]

Rob is right that we're in the Go 1.2 feature freeze, but for the record, here's
some comments.

As I also mentioned on golang-nuts, I don't see the need for the RGB48 types.

https://codereview.appspot.com/13239051/diff/4001/src/pkg/image/color/color.go
File src/pkg/image/color/color.go (right):

https://codereview.appspot.com/13239051/diff/4001/src/pkg/image/color/color.g...
src/pkg/image/color/color.go:18: // RGB represents a traditional 24-bit color,
The comments should mention that it's a fully opaque color.

https://codereview.appspot.com/13239051/diff/4001/src/pkg/image/color/color.g...
src/pkg/image/color/color.go:31: return
Returning zero alpha is wrong. It should be 0xFFFF.

https://codereview.appspot.com/13239051/diff/4001/src/pkg/image/color/color.g...
src/pkg/image/color/color.go:41: return uint32(c.R), uint32(c.G), uint32(c.B), 0
Ditto.

[chai2010, 2013-12-10 00:36:30]

ping

https://codereview.appspot.com/13239051/diff/4001/src/pkg/image/color/color.go
File src/pkg/image/color/color.go (right):

https://codereview.appspot.com/13239051/diff/4001/src/pkg/image/color/color.g...
src/pkg/image/color/color.go:18: // RGB represents a traditional 24-bit color,
On 2013/09/13 01:17:28, nigeltao wrote:
> The comments should mention that it's a fully opaque color.

Done.

https://codereview.appspot.com/13239051/diff/4001/src/pkg/image/color/color.g...
src/pkg/image/color/color.go:31: return
On 2013/09/13 01:17:28, nigeltao wrote:
> Returning zero alpha is wrong. It should be 0xFFFF.

Done.

https://codereview.appspot.com/13239051/diff/4001/src/pkg/image/color/color.g...
src/pkg/image/color/color.go:41: return uint32(c.R), uint32(c.G), uint32(c.B), 0
On 2013/09/13 01:17:28, nigeltao wrote:
> Ditto.

Done.

[nigeltao, 2013-12-11 00:06:18]

https://codereview.appspot.com/13239051/diff/4001/src/pkg/image/color/color.go
File src/pkg/image/color/color.go (right):

https://codereview.appspot.com/13239051/diff/4001/src/pkg/image/color/color.g...
src/pkg/image/color/color.go:18: // RGB represents a traditional 24-bit color,
On 2013/12/10 00:36:30, chai2010 wrote:
> On 2013/09/13 01:17:28, nigeltao wrote:
> > The comments should mention that it's a fully opaque color.
> 
> Done.

Not done?

[chai2010, 2013-12-11 01:04:40]

PTAL

https://codereview.appspot.com/13239051/diff/4001/src/pkg/image/color/color.go
File src/pkg/image/color/color.go (right):

https://codereview.appspot.com/13239051/diff/4001/src/pkg/image/color/color.g...
src/pkg/image/color/color.go:18: // RGB represents a traditional 24-bit color,
On 2013/12/11 00:06:18, nigeltao wrote:
> On 2013/12/10 00:36:30, chai2010 wrote:
> > On 2013/09/13 01:17:28, nigeltao wrote:
> > > The comments should mention that it's a fully opaque color.
> > 
> > Done.
> 
> Not done?

Sorry, i made a mistake with Patch Set 5.
Now i fix the comments with Patch Set 4.

[nigeltao, 2013-12-11 11:40:01]

LGTM.

[nigeltao, 2013-12-11 11:40:30]

Rob, what do you think?

[r, 2013-12-12 19:24:12]

LGTM

[r, 2013-12-12 19:24:35]

*** Submitted as https://code.google.com/p/go/source/detail?r=63d1713372d1 ***

image: add RGB and RGB48

R=golang-dev, r, nigeltao
CC=golang-dev
https://codereview.appspot.com/13239051

Committer: Rob Pike <r@golang.org>