ui/gfx/color_profile: implement GetDisplayColorProfile on win

ui/gfx/color_profile: implement GetDisplayColorProfile on win

Add a global device color profile cache and use it to extract the
color profile associated with screen display devices.

TEST=ui_unittests.exe --gtest_filter="ColorProfileTest*"
BUG=368694
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276589

[sky, 2014-06-04 15:29:23]

Is it possible to write test coverage?

https://codereview.chromium.org/312723003/diff/1/ui/gfx/color_profile_win.cc
File ui/gfx/color_profile_win.cc (right):

https://codereview.chromium.org/312723003/diff/1/ui/gfx/color_profile_win.cc#...
ui/gfx/color_profile_win.cc:67: DCHECK(profile->empty());
Can you add a DCHECK that this isn't on the UI thread?

https://codereview.chromium.org/312723003/diff/1/ui/gfx/color_profile_win.cc#...
ui/gfx/color_profile_win.cc:80: HDC hdc = ::CreateDC(monitor.szDevice, NULL,
NULL, NULL);
Use the scoper in scoped_hdc.h.

https://codereview.chromium.org/312723003/diff/1/ui/gfx/color_profile_win.cc#...
ui/gfx/color_profile_win.cc:90: std::string file;
nit: file_data or maybe profile_data.

https://codereview.chromium.org/312723003/diff/1/ui/gfx/color_profile_win.cc#...
ui/gfx/color_profile_win.cc:92: profile->assign(file.data(), file.data() +
file.size());
*profile = file;

https://codereview.chromium.org/312723003/diff/1/ui/gfx/color_profile_win.cc#...
ui/gfx/color_profile_win.cc:94: if (gfx::InvalidColorProfileLength(length))
Do this before you set profile (if on 91).

[Noel Gordon, 2014-06-05 03:55:15]

On 2014/06/04 15:29:23, sky wrote:
> Is it possible to write test coverage?

@ken Do I have it right: some of our bots run headless?  I could write a
unittest to see if I could call GetDisplayColorProfile() without exploding, but
headless bots would produce different results.

[Noel Gordon, 2014-06-05 04:20:03]

https://codereview.chromium.org/312723003/diff/1/ui/gfx/color_profile_win.cc
File ui/gfx/color_profile_win.cc (right):

https://codereview.chromium.org/312723003/diff/1/ui/gfx/color_profile_win.cc#...
ui/gfx/color_profile_win.cc:67: DCHECK(profile->empty());
On 2014/06/04 15:29:24, sky wrote:
> Can you add a DCHECK that this isn't on the UI thread?

Plan is to do that at the call-site in render_widget_host ...

  DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
  DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO));

https://codereview.chromium.org/312723003/diff/1/ui/gfx/color_profile_win.cc#...
ui/gfx/color_profile_win.cc:80: HDC hdc = ::CreateDC(monitor.szDevice, NULL,
NULL, NULL);
On 2014/06/04 15:29:24, sky wrote:
> Use the scoper in scoped_hdc.h.

It requires a HWND and works for a GetDC/ReleaseDC pair only. For a monitor you
need CreateDC, and that needs to be paired with a DeleteDC call.  Educating
scoped_hdc.h about CreateDC / DeleteDC might be an interesting subject for
another patch.

https://codereview.chromium.org/312723003/diff/1/ui/gfx/color_profile_win.cc#...
ui/gfx/color_profile_win.cc:90: std::string file;
On 2014/06/04 15:29:24, sky wrote:
> nit: file_data or maybe profile_data.

I called it data.

https://codereview.chromium.org/312723003/diff/1/ui/gfx/color_profile_win.cc#...
ui/gfx/color_profile_win.cc:92: profile->assign(file.data(), file.data() +
file.size());
On 2014/06/04 15:29:24, sky wrote:
> *profile = file;

Need the vector->assign() here.  STL is lame when it comes to assigning
stl::strings to stl::vectors with a binary = op ... 

c:\src\chrome\src\ui\gfx\color_profile_win.cc(92) : error C2679: binary '=' : no
operator found which takes a right-hand operand of type 'std::string' (or there
is no acceptable conversion)
        c:\src\depot_tools\win_toolchain\vs2013_files\vc\include\vector(948):
could be 'std::vector<char,std::allocator<char>>
&std::vector<char,std::allocator<char>>::operator =(const
std::vector<char,std::allocator<char>> &)'
        c:\src\depot_tools\win_toolchain\vs2013_files\vc\include\vector(926): or
      'std::vector<char,std::allocator<char>>
&std::vector<char,std::allocator<char>>::operator
=(std::initializer_list<_Elem>)'
        with
        [
            _Elem=char
        ]
        c:\src\depot_tools\win_toolchain\vs2013_files\vc\include\vector(822): or
      'std::vector<char,std::allocator<char>>
&std::vector<char,std::allocator<char>>::operator
=(std::vector<char,std::allocator<char>> &&)'
        while trying to match the argument list
'(std::vector<char,std::allocator<char>>, std::string)'
ninja: build stopped: subcommand failed.

:(

https://codereview.chromium.org/312723003/diff/1/ui/gfx/color_profile_win.cc#...
ui/gfx/color_profile_win.cc:94: if (gfx::InvalidColorProfileLength(length))
On 2014/06/04 15:29:24, sky wrote:
> Do this before you set profile (if on 91).

Think we test for valid profile length sometime after the profile is assigned
to, and then clear it if it is invalid.  The profile is always clear before
assignment due to line @67.

[Noel Gordon, 2014-06-05 04:21:11]

On 2014/06/05 04:20:03, Noel Gordon (Google) wrote:
Ahem, "Think we need to test for valid profile ..."

[Ken Russell (switch to Gerrit), 2014-06-05 17:46:47]

On 2014/06/05 03:55:15, Noel Gordon (Google) wrote:
> On 2014/06/04 15:29:23, sky wrote:
> > Is it possible to write test coverage?
> 
> @ken Do I have it right: some of our bots run headless?  I could write a
> unittest to see if I could call GetDisplayColorProfile() without exploding,
but
> headless bots would produce different results.

The Win, Mac and Linux bots on the Chromium waterfall all run in VMs as far as I
know. They have desktops, but they're virtual. I think the call to
GetDisplayColorProfile should still work, though.

If it turns out you need machines with a real display, we could add a test that
is run on the chromium.gpu waterfall. See
http://www.chromium.org/developers/testing/gpu-recipe . Would be glad to work
with you on putting such a test on these bots.

[Noel Gordon, 2014-06-06 13:11:20]

On 2014/06/05 17:46:47, Ken Russell wrote:

> The Win, Mac and Linux bots on the Chromium waterfall all run in VMs as far as
I
> know. They have desktops, but they're virtual. I think the call to
> GetDisplayColorProfile should still work, though.

Right, added basic test to see if GetDisplayColorProfile() calls basically work.
 
> If it turns out you need machines with a real display, we could add a test
that
> is run on the chromium.gpu waterfall. See
> http://www.chromium.org/developers/testing/gpu-recipe . Would be glad to work
> with you on putting such a test on these bots.

Right, and noted (gpu-recipe for end-to-end testing of color profiles).

[Noel Gordon, 2014-06-10 14:41:11]

Anything more I need do here?

[sky, 2014-06-10 15:58:59]

https://codereview.chromium.org/312723003/diff/100001/ui/gfx/color_profile_wi...
File ui/gfx/color_profile_win_unittest.cc (right):

https://codereview.chromium.org/312723003/diff/100001/ui/gfx/color_profile_wi...
ui/gfx/color_profile_win_unittest.cc:17: const gfx::Rect in_screen_bounds(10,
10, 100, 100);
How do you know any of the bounds you're using in these tests correlate to the
screen in anyway?

[Noel Gordon, 2014-06-11 02:35:13]

The win32 primary screen/desktop origin is (0,0).

[sky, 2014-06-11 15:47:21]

LGTM

[Noel Gordon, 2014-06-11 23:17:10]

The CQ bit was checked by noel@chromium.org

[commit-bot: I haz the power, 2014-06-11 23:18:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noel@chromium.org/312723003/100001

[commit-bot: I haz the power, 2014-06-12 00:34:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-12 00:37:27]

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

[commit-bot: I haz the power, 2014-06-12 00:37:29]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Noel Gordon, 2014-06-12 03:12:29]

The CQ bit was checked by noel@chromium.org

[commit-bot: I haz the power, 2014-06-12 03:15:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noel@chromium.org/312723003/100001

[commit-bot: I haz the power, 2014-06-12 08:55:05]

Change committed as 276589

[Noel Gordon, 2014-06-12 10:10:06]

¯\(ツ)/¯