ui/gfx/color_profile: implement GetDisplayColorProfile on mac

ui/gfx/color_profile: implement GetDisplayColorProfile on mac

Like https://codereview.chromium.org/312723003 for win32, extract
the display color profile associated with a screen display device
on mac given rectangular bounds in screen coordinates.

Use gfx::ScreenRectFromNSRect to compute the screen that overlaps
the bounds most to match the gfx::Screen concept of overlap.

Add unit tests for on-screen, partially-on-screen, off-screen and
empty bounds.

Note an sRGB color profile is a common case, especially on win32,
so GetDisplayColorProfile returns true and an empty color profile
in that case. Add a header file comment about that.

TEST=gfx_unittests --gtest_filter="ColorProfileTest*Bounds"
BUG=368694
Committed: https://crrev.com/ac030e207c4d6609e5275fe24e0a65c23a3f74c0
Cr-Commit-Position: refs/heads/master@{#299253}

[Noel Gordon, 2014-10-03 18:45:56]

Patchset #7 (id:120001) has been deleted

[Noel Gordon, 2014-10-06 07:13:46]

noel@chromium.org changed reviewers:
+ danakj@google.com, rsesek@chromium.org

[Noel Gordon, 2014-10-06 07:16:05]

noel@chromium.org changed reviewers:
+ danakj@chromium.org
- danakj@google.com

[Robert Sesek, 2014-10-06 17:56:00]

https://codereview.chromium.org/612613002/diff/160001/ui/gfx/color_profile_ma...
File ui/gfx/color_profile_mac.mm (right):

https://codereview.chromium.org/612613002/diff/160001/ui/gfx/color_profile_ma...
ui/gfx/color_profile_mac.mm:46: return true;
Why do this and not assign the profile data?

https://codereview.chromium.org/612613002/diff/160001/ui/gfx/color_profile_ma...
ui/gfx/color_profile_mac.mm:48: if (const char* data = (char *)[profile_data
bytes])
C-style casts are banned.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Casting

https://codereview.chromium.org/612613002/diff/160001/ui/gfx/color_profile_ma...
ui/gfx/color_profile_mac.mm:51: if (gfx::InvalidColorProfileLength(length))
Why not check the length before assigning into the vector?

[Robert Sesek, 2014-10-06 17:56:22]

Also: the CL title has the directories in the wrong order (gfx/ui vs ui/gfx).

[Noel Gordon, 2014-10-07 01:42:03]

On 2014/10/06 17:56:22, Robert Sesek wrote:
> Also: the CL title has the directories in the wrong order (gfx/ui vs ui/gfx).

Ah yeah, fixed.

[Noel Gordon, 2014-10-07 01:57:43]

https://codereview.chromium.org/612613002/diff/160001/ui/gfx/color_profile_ma...
File ui/gfx/color_profile_mac.mm (right):

https://codereview.chromium.org/612613002/diff/160001/ui/gfx/color_profile_ma...
ui/gfx/color_profile_mac.mm:46: return true;
On 2014/10/06 17:56:00, Robert Sesek wrote:
> Why do this and not assign the profile data?

The sRGB profile is common, and renderers can interpolate one without assistance
from the browser. To save sending sRGB profiles to all renderers, it is
communicated to them with an empty color profile, see:

https://code.google.com/p/chromium/codesearch#chromium/src/content/common/vie...

https://codereview.chromium.org/612613002/diff/160001/ui/gfx/color_profile_ma...
ui/gfx/color_profile_mac.mm:48: if (const char* data = (char *)[profile_data
bytes])
On 2014/10/06 17:56:00, Robert Sesek wrote:
> C-style casts are banned.
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Casting

Done.

https://codereview.chromium.org/612613002/diff/160001/ui/gfx/color_profile_ma...
ui/gfx/color_profile_mac.mm:51: if (gfx::InvalidColorProfileLength(length))
On 2014/10/06 17:56:00, Robert Sesek wrote:
> Why not check the length before assigning into the vector?

Done.

[Robert Sesek, 2014-10-07 14:20:10]

https://codereview.chromium.org/612613002/diff/160001/ui/gfx/color_profile_ma...
File ui/gfx/color_profile_mac.mm (right):

https://codereview.chromium.org/612613002/diff/160001/ui/gfx/color_profile_ma...
ui/gfx/color_profile_mac.mm:46: return true;
On 2014/10/07 01:57:43, Noel Gordon wrote:
> On 2014/10/06 17:56:00, Robert Sesek wrote:
> > Why do this and not assign the profile data?
> 
> The sRGB profile is common, and renderers can interpolate one without
assistance
> from the browser. To save sending sRGB profiles to all renderers, it is
> communicated to them with an empty color profile, see:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/vie...

That's kind of an odd thing to bake into this API, but it looks like it's what
the Windows implementation does too. This should definitely be documented in the
header.

https://codereview.chromium.org/612613002/diff/180001/ui/gfx/color_profile_ma...
File ui/gfx/color_profile_mac.mm (right):

https://codereview.chromium.org/612613002/diff/180001/ui/gfx/color_profile_ma...
ui/gfx/color_profile_mac.mm:13: NSScreen* GetNSScreenFromBounds(const gfx::Rect&
bounds) {
I just noticed that this code is very similar to that in screen_mac.mm:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/screen_mac....

Could you use Screen::GetNativeScreen()->GetDisplayMatching() here instead?

https://codereview.chromium.org/612613002/diff/180001/ui/gfx/color_profile_ma...
ui/gfx/color_profile_mac.mm:14: NSScreen* screen = NULL;
nit: NULL -> nil

https://codereview.chromium.org/612613002/diff/180001/ui/gfx/color_profile_ma...
ui/gfx/color_profile_mac.mm:48: const char* data = static_cast<const char
*>([profile_data bytes]);
nti: no space before * in the cast parameter

[Noel Gordon, 2014-10-07 23:34:27]

https://codereview.chromium.org/612613002/diff/180001/ui/gfx/color_profile_ma...
File ui/gfx/color_profile_mac.mm (right):

https://codereview.chromium.org/612613002/diff/180001/ui/gfx/color_profile_ma...
ui/gfx/color_profile_mac.mm:14: NSScreen* screen = NULL;
On 2014/10/07 14:20:10, Robert Sesek wrote:
> nit: NULL -> nil

Done.

https://codereview.chromium.org/612613002/diff/180001/ui/gfx/color_profile_ma...
ui/gfx/color_profile_mac.mm:48: const char* data = static_cast<const char
*>([profile_data bytes]);
On 2014/10/07 14:20:10, Robert Sesek wrote:
> nti: no space before * in the cast parameter

Done.

[Noel Gordon, 2014-10-07 23:43:17]

On 2014/10/07 14:20:10, Robert Sesek wrote:
>
https://codereview.chromium.org/612613002/diff/160001/ui/gfx/color_profile_ma...
> File ui/gfx/color_profile_mac.mm (right):
> 
>
https://codereview.chromium.org/612613002/diff/160001/ui/gfx/color_profile_ma...
> ui/gfx/color_profile_mac.mm:46: return true;
> On 2014/10/07 01:57:43, Noel Gordon wrote:
> > On 2014/10/06 17:56:00, Robert Sesek wrote:
> > > Why do this and not assign the profile data?
> > 
> > The sRGB profile is common, and renderers can interpolate one without
> assistance
> > from the browser. To save sending sRGB profiles to all renderers, it is
> > communicated to them with an empty color profile, see:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/vie...
> 
> That's kind of an odd thing to bake into this API, but it looks like it's what
> the Windows implementation does too. 

Yes we need match windows here, which is why wrote the code the way I did before
you asked me to change it.  Anyhow, so long as it logically matches windows, I
am happy.  And it not odd to need to reliably and easily detect the sRGB color
profile in a renderer in cases like CSS color correction and untagged images for
example, where the color-correction step (sRGB -> sRGB) can be avoided.

> This should definitely be documented in the header.

Added a comment.

[Noel Gordon, 2014-10-07 23:54:09]

On 2014/10/07 14:20:10, Robert Sesek wrote:
 
>
https://codereview.chromium.org/612613002/diff/180001/ui/gfx/color_profile_ma...
> File ui/gfx/color_profile_mac.mm (right):
> 
>
https://codereview.chromium.org/612613002/diff/180001/ui/gfx/color_profile_ma...
> ui/gfx/color_profile_mac.mm:13: NSScreen* GetNSScreenFromBounds(const
gfx::Rect&
> bounds) {
> I just noticed that this code is very similar to that in screen_mac.mm:

Yeah, I noticed that once upon a time too,
https://codereview.chromium.org/263643004

>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/screen_mac....
> 
> Could you use Screen::GetNativeScreen()->GetDisplayMatching() here instead?

but it was recommended that color profiles be separate existence from
gfx::Display / gfx::Screen, with implementations in color_profile_<port> files,
so that's what I have done.

[Noel Gordon, 2014-10-07 23:57:25]

Ahem ... "be separate existence from"  ->  "have a separate existence from"

[Robert Sesek, 2014-10-08 01:33:17]

LGTM % comment about the #include (below)

On 2014/10/07 23:54:09, Noel Gordon wrote:
> Yeah, I noticed that once upon a time too,
> https://codereview.chromium.org/263643004
> 
> but it was recommended that color profiles be separate existence from
> gfx::Display / gfx::Screen, with implementations in color_profile_<port>
files,
> so that's what I have done.

Thanks for the links -- this has quite some history!

On 2014/10/07 23:43:17, Noel Gordon wrote:
> On 2014/10/07 14:20:10, Robert Sesek wrote:
> > That's kind of an odd thing to bake into this API, but it looks like it's
what
> > the Windows implementation does too. 
> 
> Yes we need match windows here, which is why wrote the code the way I did
before
> you asked me to change it.  Anyhow, so long as it logically matches windows, I
> am happy.  And it not odd to need to reliably and easily detect the sRGB color
> profile in a renderer in cases like CSS color correction and untagged images
for
> example, where the color-correction step (sRGB -> sRGB) can be avoided.

I think it's fine to optimize for that case, but it seemed odd without any
reference to why the implementation would do it. Thanks for adding the comment.

https://codereview.chromium.org/612613002/diff/200001/ui/gfx/color_profile_ma...
File ui/gfx/color_profile_mac.mm (right):

https://codereview.chromium.org/612613002/diff/200001/ui/gfx/color_profile_ma...
ui/gfx/color_profile_mac.mm:8: #include "base/mac/sdk_forward_declarations.h"
Do you need this #include?

[Noel Gordon, 2014-10-08 01:58:58]

https://codereview.chromium.org/612613002/diff/200001/ui/gfx/color_profile_ma...
File ui/gfx/color_profile_mac.mm (right):

https://codereview.chromium.org/612613002/diff/200001/ui/gfx/color_profile_ma...
ui/gfx/color_profile_mac.mm:8: #include "base/mac/sdk_forward_declarations.h"
On 2014/10/08 01:33:17, Robert Sesek wrote:
> Do you need this #include?

Yes, code won't compile otherwise.

[Robert Sesek, 2014-10-08 02:03:14]

https://codereview.chromium.org/612613002/diff/200001/ui/gfx/color_profile_ma...
File ui/gfx/color_profile_mac.mm (right):

https://codereview.chromium.org/612613002/diff/200001/ui/gfx/color_profile_ma...
ui/gfx/color_profile_mac.mm:8: #include "base/mac/sdk_forward_declarations.h"
On 2014/10/08 01:58:58, Noel Gordon wrote:
> On 2014/10/08 01:33:17, Robert Sesek wrote:
> > Do you need this #include?
> 
> Yes, code won't compile otherwise.

What's the error? I don't see you using any SDK forward declarations here. Do
you need to #import <Cocoa/Cocoa.h> ?

[Noel Gordon, 2014-10-08 02:14:37]

On 2014/10/08 02:03:14, Robert Sesek wrote:
>
https://codereview.chromium.org/612613002/diff/200001/ui/gfx/color_profile_ma...
> File ui/gfx/color_profile_mac.mm (right):
> 
>
https://codereview.chromium.org/612613002/diff/200001/ui/gfx/color_profile_ma...
> ui/gfx/color_profile_mac.mm:8: #include "base/mac/sdk_forward_declarations.h"
> On 2014/10/08 01:58:58, Noel Gordon wrote:
> > On 2014/10/08 01:33:17, Robert Sesek wrote:
> > > Do you need this #include?
> > 
> > Yes, code won't compile otherwise.
> 
> What's the error? I don't see you using any SDK forward declarations here. Do
> you need to #import <Cocoa/Cocoa.h> ?

../../ui/gfx/color_profile_mac.mm:13:1: error: unknown type name 'NSScreen'
NSScreen* GetNSScreenFromBounds(const gfx::Rect& bounds) {
^
../../ui/gfx/color_profile_mac.mm:14:3: error: unknown type name 'NSScreen'
  NSScreen* screen = nil;
  ^
../../ui/gfx/color_profile_mac.mm:17:8: error: unknown type name 'NSScreen'
  for (NSScreen* monitor in [NSScreen screens]) {
       ^
../../ui/gfx/color_profile_mac.mm:17:30: error: use of undeclared identifier
'NSScreen'
  for (NSScreen* monitor in [NSScreen screens]) {
                             ^
../../ui/gfx/color_profile_mac.mm:38:3: error: unknown type name 'NSScreen'
  NSScreen* screen = GetNSScreenFromBounds(bounds);
  ^
../../ui/gfx/color_profile_mac.mm:41:3: error: unknown type name 'NSColorSpace';
did you mean 'CGColorSpace'?
  NSColorSpace* color_space = [screen colorSpace];
  ^~~~~~~~~~~~
  CGColorSpace
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.8.sdk/System/Library/Frameworks/CoreGraphics.framework/Headers/CGColorSpace.h:8:16:
note: 'CGColorSpace' declared here
typedef struct CGColorSpace *CGColorSpaceRef;
               ^
../../ui/gfx/color_profile_mac.mm:45:29: error: use of undeclared identifier
'NSColorSpace'
  if ([color_space isEqual:[NSColorSpace sRGBColorSpace]])
                            ^
../../ui/gfx/color_profile_mac.mm:47:27: error: receiver type 'CGColorSpace *'
is not 'id' or interface pointer, consider casting it to 'id'
[-Werror,-Wreceiver-expr]
  NSData* profile_data = [color_space ICCProfileData];
                          ^~~~~~~~~~~
../../ui/gfx/color_profile_mac.mm:47:39: error: instance method
'-ICCProfileData' not found (return type defaults to 'id')
[-Werror,-Wobjc-method-access]
  NSData* profile_data = [color_space ICCProfileData];
                                      ^~~~~~~~~~~~~~
9 errors generated.
[3/9] OBJCXX obj/ui/gfx/gfx_unittests.color_profile

[Robert Sesek, 2014-10-08 02:16:11]

You don't need sdk_forward_declarations.h -- that's only used for targeting
future OS features. Just import Cocoa.h.

[Noel Gordon, 2014-10-08 02:18:51]

> you need to #import <Cocoa/Cocoa.h> ?

If I do import that file, and ditch #include
"base/mac/sdk_forward_declarations.h", compiles ok for me.

[Noel Gordon, 2014-10-08 02:24:39]

On 2014/10/08 02:18:51, Noel Gordon wrote:
> > you need to #import <Cocoa/Cocoa.h> ?
> 
> If I do import that file, and ditch #include
> "base/mac/sdk_forward_declarations.h", compiles ok for me.

Right, new patch uploaded for that, PTAL.

[Robert Sesek, 2014-10-08 02:31:07]

LGTM

[Noel Gordon, 2014-10-08 02:56:57]

On 2014/10/08 01:33:17, Robert Sesek wrote:
> LGTM % comment about the #include (below)
> 
> On 2014/10/07 23:54:09, Noel Gordon wrote:
> > Yeah, I noticed that once upon a time too,
> > https://codereview.chromium.org/263643004
> > 
> > but it was recommended that color profiles be separate existence from
> > gfx::Display / gfx::Screen, with implementations in color_profile_<port>
> files,
> > so that's what I have done.
> 
> Thanks for the links -- this has quite some history!

I'll say.  [cocoa_view_ window] foo bar: not gonna work for a background tab in
mac chrome.  Using window on the API was there to force that issue.

> On 2014/10/07 23:43:17, Noel Gordon wrote:
> > On 2014/10/07 14:20:10, Robert Sesek wrote:
> > > That's kind of an odd thing to bake into this API, but it looks like it's
> what
> > > the Windows implementation does too. 
> > 
> > Yes we need match windows here, which is why wrote the code the way I did
> before
> > you asked me to change it.  Anyhow, so long as it logically matches windows,
I
> > am happy.  And it not odd to need to reliably and easily detect the sRGB
color
> > profile in a renderer in cases like CSS color correction and untagged images
> for
> > example, where the color-correction step (sRGB -> sRGB) can be avoided.
> 
> I think it's fine to optimize for that case, but it seemed odd without any
> reference to why the implementation would do it.

There are more cases btw in Blink, Skia, the Chrome Compositor, and the GPU. 
Best I don't try describe them all :)

> Thanks for adding the comment.

Right.

[Noel Gordon, 2014-10-08 04:33:04]

On 2014/10/08 02:16:11, Robert Sesek wrote:
> You don't need sdk_forward_declarations.h -- that's only used for targeting
> future OS features. Just import Cocoa.h.

I've learnt just enough cocoa / objc to get this job done. I don't usually
program in cocoa, so appreciate the tip. Worked fine.

[Noel Gordon, 2014-10-08 04:39:36]

+danakj for OWNERS approval

[Noel Gordon, 2014-10-09 23:01:44]

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

[Noel Gordon, 2014-10-09 23:02:17]

On 2014/10/08 04:39:36, Noel Gordon wrote:
> +danakj for OWNERS approval

or maybe +sky

[sky, 2014-10-10 02:36:10]

LGTM assuming assign does what you want.

https://codereview.chromium.org/612613002/diff/240001/ui/gfx/color_profile_ma...
File ui/gfx/color_profile_mac.mm (right):

https://codereview.chromium.org/612613002/diff/240001/ui/gfx/color_profile_ma...
ui/gfx/color_profile_mac.mm:52: profile->assign(data, data + length);
I'm curious, what does this get mapped to? Does data and data + length get
wrapped into iterators?

[Noel Gordon, 2014-10-12 01:31:00]

https://codereview.chromium.org/612613002/diff/240001/ui/gfx/color_profile_ma...
File ui/gfx/color_profile_mac.mm (right):

https://codereview.chromium.org/612613002/diff/240001/ui/gfx/color_profile_ma...
ui/gfx/color_profile_mac.mm:52: profile->assign(data, data + length);
On 2014/10/10 02:36:10, sky wrote:
> I'm curious, what does this get mapped to? Does data and data + length get
> wrapped into iterators?

I believe that plain-old-data pointers satisfy the requirements of being an
InputIterator, that this overload of assign() needs

  vector::assign(InputIterator a, InputIterator b)

https://gist.github.com/anonymous/5ebe2c9110f51ba11715

[Noel Gordon, 2014-10-12 01:32:15]

The CQ bit was checked by noel@chromium.org

[commit-bot: I haz the power, 2014-10-12 01:33:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/612613002/240001

[commit-bot: I haz the power, 2014-10-12 02:03:40]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2014-10-12 02:04:08]

Patchset 12 (id:??) landed as
https://crrev.com/ac030e207c4d6609e5275fe24e0a65c23a3f74c0
Cr-Commit-Position: refs/heads/master@{#299253}