Teach gfx::Screen about color profiles

Teach gfx::Screen about color profiles

Stub in an api for getting the screen color profile associated
with a native view. Add TODO's for implementation.

BUG=368694

[Noel Gordon, 2014-04-30 16:32:09]

ui/views/widget/desktop_aura/desktop_screen_x11.h just has to be different from
every other port Screen implementation, dunno why.

[Ken Russell (switch to Gerrit), 2014-04-30 17:43:17]

LGTM overall.

https://codereview.chromium.org/263643004/diff/80001/ui/gfx/screen.h
File ui/gfx/screen.h (right):

https://codereview.chromium.org/263643004/diff/80001/ui/gfx/screen.h#newcode76
ui/gfx/screen.h:76: // Returns the color profile of the display nearest the
window.
Could you add a little more documentation here? What exactly does the color
profile contain upon return? An ICC color profile? Something different?

[Noel Gordon, 2014-04-30 17:51:29]

https://codereview.chromium.org/263643004/diff/80001/ui/gfx/screen.h
File ui/gfx/screen.h (right):

https://codereview.chromium.org/263643004/diff/80001/ui/gfx/screen.h#newcode76
ui/gfx/screen.h:76: // Returns the color profile of the display nearest the
window.
Yeah good point. Would this work for you? 

// Returns the ICC color profile data of the display nearest the window.

[Ken Russell (switch to Gerrit), 2014-04-30 17:52:02]

https://codereview.chromium.org/263643004/diff/80001/ui/gfx/screen.h
File ui/gfx/screen.h (right):

https://codereview.chromium.org/263643004/diff/80001/ui/gfx/screen.h#newcode76
ui/gfx/screen.h:76: // Returns the color profile of the display nearest the
window.
On 2014/04/30 17:51:29, Noel Gordon (Google) wrote:
> Yeah good point. Would this work for you? 
> 
> // Returns the ICC color profile data of the display nearest the window.

Sounds good.

[oshima, 2014-04-30 18:18:53]

you also have to update ui/views/widget/desktop_aura/desktop_screen_win, i
think.

https://codereview.chromium.org/263643004/diff/80001/ui/gfx/screen.h
File ui/gfx/screen.h (right):

https://codereview.chromium.org/263643004/diff/80001/ui/gfx/screen.h#newcode78
ui/gfx/screen.h:78: NativeView view, std::vector<char>* color_profile) const =
0;
Can you define a class (or struct) for color profile (in ui/display),
and document the format there? Do you think it's overkill?

Also, is it difficult to use display (or display_id) as a parameter instead of
native view?

[Noel Gordon, 2014-05-01 05:24:54]

https://codereview.chromium.org/263643004/diff/80001/ui/gfx/screen.h
File ui/gfx/screen.h (right):

https://codereview.chromium.org/263643004/diff/80001/ui/gfx/screen.h#newcode76
ui/gfx/screen.h:76: // Returns the color profile of the display nearest the
window.
On 2014/04/30 17:52:02, Ken Russell wrote:
> On 2014/04/30 17:51:29, Noel Gordon (Google) wrote:
> > Yeah good point. Would this work for you? 
> > 
> > // Returns the ICC color profile data of the display nearest the window.
> 
> Sounds good.

Done.

[Noel Gordon, 2014-05-01 05:52:21]

https://codereview.chromium.org/263643004/diff/80001/ui/gfx/screen.h
File ui/gfx/screen.h (right):

https://codereview.chromium.org/263643004/diff/80001/ui/gfx/screen.h#newcode78
ui/gfx/screen.h:78: NativeView view, std::vector<char>* color_profile) const =
0;
On 2014/04/30 18:18:53, oshima wrote:
> Can you define a class (or struct) for color profile (in ui/display),
> and document the format there? Do you think it's overkill?

Yes I think that'd be overkill.  At this level of the system, we just return the
ICC data. The higher level systems that use this data, know how to validate and
process it.  The structure of ICC data is covered by well-know and widely-used
international standard published by the ICC www.color.org, and is of
interest only to color management system software. gfx:Screen is not a color
management system, nor does it need to be.

[Noel Gordon, 2014-05-01 06:07:28]

Filed crbug.com/368995 abou the linux_chromeos-asan compile brokenness.

[Noel Gordon, 2014-05-01 07:33:24]

https://codereview.chromium.org/263643004/diff/80001/ui/gfx/screen.h
File ui/gfx/screen.h (right):

https://codereview.chromium.org/263643004/diff/80001/ui/gfx/screen.h#newcode78
ui/gfx/screen.h:78: NativeView view, std::vector<char>* color_profile) const =
0;
On 2014/04/30 18:18:53, oshima wrote:

> Also, is it difficult to use display (or display_id) as a parameter instead of
> native view?

Good question.

Yeap, difficult. gfx::Display is no good because of the propensity of Screen to
use the default screen information when there is no window/view, when asking the
screen for the display nearest window, as but one example. That behavior leads
to bugs like http://crbug.com/357443 https://crbug.com/342672 and I need much
better control than that.

I believe I discussed the with you 6 weeks ago on http://crbug.com/357443 when I
asked about the implementation of display_id() on mac-osx.  I'd be fine with an
ephemeral display_id() btw, but no answers from the mac folks on that bug.  Six
weeks might be considered sufficient time to provide an answer.  I'm a patient
man, but only so much, and I decided to work-around the entire issue of
display_id() by redesigning to _not rely on it at all_. 

I no longer care about display_id() on mac or elsewhere. I can make use of it
internally on the windows port, but not as part of the api.

Also, using NativeView in the api works with the call sites in RenderViewHost
and RenderWidgetHost.  They call GetDisplayNearestWindow(NativeView) (and tickle
bugs 357443 and 342672 in the process) when they read Blink::WebScreenInfo that
needs to be sent to Blink renderers.  I get the screen color profile via
GetDisplayColorProfile(NativeView) nearby, so using the NativeView in the API is
a much better fit all round.

You will see in the prelude (implementations of this api for mac and win) that
most sane operating systems provide an API to get the the color profile of the
associated screen _given the NativeView (window)_.  The chrome-os and linux crew
could learn from that.  I'm not going to implement this api for them though --
they seem to be undecided in this area based on the bugs I filed on them.  I'll
leave a TODO(port) hook, but those OSes need to interwork with us here, not the
other way around.

Hope that answers your question.

[Noel Gordon, 2014-05-01 07:47:30]

On 2014/04/30 18:18:53, oshima wrote:
> you also have to update ui/views/widget/desktop_aura/desktop_screen_win, i
> think.

class VIEWS_EXPORT DesktopScreenWin : public gfx::ScreenWin { .. } so I think
the screen_win.cc/.h in this CL provide the implementation.

[oshima, 2014-05-01 19:12:01]

On 2014/05/01 07:33:24, Noel Gordon (Google) wrote:
> https://codereview.chromium.org/263643004/diff/80001/ui/gfx/screen.h
> File ui/gfx/screen.h (right):
> 
> https://codereview.chromium.org/263643004/diff/80001/ui/gfx/screen.h#newcode78
> ui/gfx/screen.h:78: NativeView view, std::vector<char>* color_profile) const =
> 0;
> On 2014/04/30 18:18:53, oshima wrote:
> 
> > Also, is it difficult to use display (or display_id) as a parameter instead
of
> > native view?
> 
> Good question.
> 
> Yeap, difficult. gfx::Display is no good because of the propensity of Screen
to
> use the default screen information when there is no window/view, when asking
the
> screen for the display nearest window, as but one example. That behavior leads
> to bugs like http://crbug.com/357443 https://crbug.com/342672 and I need much
> better control than that.
> 
> I believe I discussed the with you 6 weeks ago on http://crbug.com/357443 when
I
> asked about the implementation of display_id() on mac-osx.  I'd be fine with
an
> ephemeral display_id() btw, but no answers from the mac folks on that bug. 
Six
> weeks might be considered sufficient time to provide an answer.  I'm a patient
> man, but only so much, and I decided to work-around the entire issue of
> display_id() by redesigning to _not rely on it at all_. 
>
> I no longer care about display_id() on mac or elsewhere. I can make use of it
> internally on the windows port, but not as part of the api.
> 
> Also, using NativeView in the api works with the call sites in RenderViewHost
> and RenderWidgetHost.  They call GetDisplayNearestWindow(NativeView) (and
tickle
> bugs 357443 and 342672 in the process) when they read Blink::WebScreenInfo
that
> needs to be sent to Blink renderers.  I get the screen color profile via
> GetDisplayColorProfile(NativeView) nearby, so using the NativeView in the API
is
> a much better fit all round.

I replied on 357443. Maybe I misunderstood something but using NativeView
doesn't seem
to solve the issue there because if native view is NULL, then you basically have
no
way to get this information correctly. You can query the display without native
view using
the bounds and that will give you the correct info even without native view.

I like API to be canonical and symmetric. Given that color profile belongs to
the display,
not a native view, I still prefer to use display/display Id. If there is
technical
difficulty, I'm open to other options of course.

> You will see in the prelude (implementations of this api for mac and win) that
> most sane operating systems provide an API to get the the color profile of the
> associated screen _given the NativeView (window)_.  The chrome-os and linux
crew
> could learn from that.  I'm not going to implement this api for them though --
> they seem to be undecided in this area based on the bugs I filed on them. 
I'll
> leave a TODO(port) hook, but those OSes need to interwork with us here, not
the
> other way around.

Is that only API they provide? What if I jsut want to get a color profile for
given dislpay?
Do I have to create a native view?

> 
> Hope that answers your question.

[Noel Gordon, 2014-05-05 15:10:19]

On 2014/05/01 19:12:01, oshima wrote:

> I replied on 357443.

Thank you.

> Maybe I misunderstood something but using NativeView
> doesn't seem to solve the issue there because if native view is NULL,
> then you basically have no way to get this information correctly.
> You can query the display without native view using the bounds and that
> will give you the correct info even without native view.

Yeah, currently the wrong NativeView is being sent on mac
when grabbing screen info, that's all. Chrome background
tabs do have a NativeView, but

  [NativeView window];

is NULL since the tab is in background (which is a mac-only
design problem, Aura chrome tabs always have a window). So
when reading screen info in gfx, we find that

  [[NativeView window] screen];

ends up NULL too, and some of the gfx::Screen api return the
default screen info in that case. Need to avoid that to fix
bug 357443 and to support mac-multi-monitor color.


> I like API to be canonical and symmetric. Given that color profile belongs to
> the display, not a native view,

I'm not sure "displays own color profiles" is a given. On
mac you can call

  [[NativeView window] colorSpace];

to get the profile, without reference to a display (screen).
I don't really see a need to make color profiles belong to
anyone, certainly not a NativeView or display, as yet.


> I still prefer to use display/display Id. If
> there is technical difficulty, I'm open to other options of course.

Yeah one thing to consider is how a NativeView window maps
to a display - it's an OS-specific rule, and typically it's
the display the window overlaps the most. So you need the
NativeView's _window_ to resolve its OS display, via OS
rules, I believe.

I think the trick is to make chrome-mac send the right NSView
(aka NativeView) when calling both GetDisplayNearestWindow
and GetDisplayColorProfile.  Seemed symmetric to me.


> > You will see in the prelude (implementations of this api for mac and win)
that
> > most sane operating systems provide an API to get the the color profile of
the
> > associated screen _given the NativeView (window)_.
> 
> Is that only API they provide?
> What if I just want to get a color profile for given display?

Sure you can enumerate the attached displays, via the OS, and
extract the color profile of a display, or from a given one.


> Do I have to create a native view?

Nope, but if you want to work out which display a native view
is on, you need the native view :)

You could use its bounds, but the OS works that out given the
native view.  Also, some of our native views don't have bounds,
background pages for example, and those might resolve to the
primary screen.  I'd prefer an api that fails in that case,
which is what I wrote.

[Noel Gordon, 2014-05-07 06:21:22]

+sky

[Noel Gordon, 2014-05-07 06:42:56]

Built chrome-os and chrome-linux locally. Seems
ui/views/widget/desktop_aura/desktop_screen_x11.h is used by both , and would be
the convenient and common integration point for color profiles access for these
linux variants of chrome. +erg in case I got that wrong.

[oshima, 2014-05-07 13:44:16]

On 2014/05/05 15:10:19, Noel Gordon (Google) wrote:
> On 2014/05/01 19:12:01, oshima wrote:
> 
> > I replied on 357443.
> 
> Thank you.
> 
> > Maybe I misunderstood something but using NativeView
> > doesn't seem to solve the issue there because if native view is NULL,
> > then you basically have no way to get this information correctly.
> > You can query the display without native view using the bounds and that
> > will give you the correct info even without native view.
> 
> Yeah, currently the wrong NativeView is being sent on mac
> when grabbing screen info, that's all. Chrome background
> tabs do have a NativeView, but
> 
>   [NativeView window];
> 
> is NULL since the tab is in background (which is a mac-only
> design problem, Aura chrome tabs always have a window). So
> when reading screen info in gfx, we find that
> 
>   [[NativeView window] screen];
> 
> ends up NULL too, and some of the gfx::Screen api return the
> default screen info in that case. Need to avoid that to fix
> bug 357443 and to support mac-multi-monitor color.
> 
> 
> > I like API to be canonical and symmetric. Given that color profile belongs
to
> > the display, not a native view,
> 
> I'm not sure "displays own color profiles" is a given. On
> mac you can call
> 
>   [[NativeView window] colorSpace];
> 
> to get the profile, without reference to a display (screen).
> I don't really see a need to make color profiles belong to
> anyone, certainly not a NativeView or display, as yet.

NSScreen seems to have colorSpace, and you can get NSScreen from NSView,
so it looks to me that color profile does belong to the display/screen.

If you really think color profile doesn't belong to a display, then why do you
want this
API to be in gfx::Screen? gfx::Screen is to get information about the
screen/display after all.

> 
> 
> > I still prefer to use display/display Id. If
> > there is technical difficulty, I'm open to other options of course.
> 
> Yeah one thing to consider is how a NativeView window maps
> to a display - it's an OS-specific rule, and typically it's
> the display the window overlaps the most. So you need the
> NativeView's _window_ to resolve its OS display, via OS
> rules, I believe.
> 
> I think the trick is to make chrome-mac send the right NSView
> (aka NativeView) when calling both GetDisplayNearestWindow
> and GetDisplayColorProfile.  Seemed symmetric to me.
> 
> > > You will see in the prelude (implementations of this api for mac and win)
> that
> > > most sane operating systems provide an API to get the the color profile of
> the
> > > associated screen _given the NativeView (window)_.
> > 
> > Is that only API they provide?
> > What if I just want to get a color profile for given display?
> 
> Sure you can enumerate the attached displays, via the OS, and
> extract the color profile of a display, or from a given one.
> 
> 
> > Do I have to create a native view?
> 
> Nope, but if you want to work out which display a native view
> is on, you need the native view :)
> 
> You could use its bounds, but the OS works that out given the
> native view.  Also, some of our native views don't have bounds,
> background pages for example, and those might resolve to the
> primary screen.  I'd prefer an api that fails in that case,
> which is what I wrote.

There is no reason to use bounds if you have a native view, right?

[Noel Gordon, 2014-05-07 15:07:24]

On 2014/05/07 13:44:16, oshima wrote:

> > > Do I have to create a native view?
> > 
> > Nope, but if you want to work out which display a native view
> > is on, you need the native view :)
> > 
> > You could use its bounds, but the OS works that out given the
> > native view,  Also, some of our native views don't have bounds,
> > ...
> 
> There is no reason to use bounds if you have a native view, right?

Correct. I need a native view.

[Noel Gordon, 2014-05-07 15:36:52]

On 2014/05/07 13:44:16, oshima wrote:

> > to get the profile, without reference to a display (screen).
> > I don't really see a need to make color profiles belong to
> > anyone, certainly not a NativeView or display, as yet.
> 
> NSScreen seems to have colorSpace

Yes, and we need to extract it.

> and you can get NSScreen from NSView,

NSView doesn't have a 'screen' protocol though, according to
the Apple developer docs.  It's window does, and the way the
the screen color space extraction can be done is

  [[[NativeView window] screen] colorSpace];

based on my reading of the Apple documentation.

> so it looks to me that color profile does belong to the display/screen.

I think might just be misunderstanding what you mean by
"belong" ...

> If you really think color profile doesn't belong to a display, then why do you
> want this
> API to be in gfx::Screen? gfx::Screen is to get information about the
> screen/display after all.

... and I'd tend to instead say that the color profile is a
"property" of the screen, for discussion's sake.

I'm fine with a gfx::Screen api that extracts that property
from a native view.

[Noel Gordon, 2014-05-07 15:41:55]

> from a native view.
I meant, given a native view, sorry.

[sky, 2014-05-07 16:38:34]

https://codereview.chromium.org/263643004/diff/110001/ui/gfx/screen.h
File ui/gfx/screen.h (right):

https://codereview.chromium.org/263643004/diff/110001/ui/gfx/screen.h#newcode78
ui/gfx/screen.h:78: NativeView view, std::vector<char>* color_profile) const =
0;
I agree with Oshima on the wierdness of this API. What is NativeView isn't on
this screen at all?

What if view is NULL?

Also, when you wrap one param per line (see style guide).

[Noel Gordon, 2014-05-09 13:59:38]

On 2014/05/07 16:38:34, sky wrote:
> What if view is NULL?

Can a NULL view be painted (color profiles apply at paint-time)?

A NULL view has no DPI, size, color profile etc, and the API returns false for a
NULL view to let the caller know.

[Noel Gordon, 2014-05-09 14:13:08]

On 2014/05/07 16:38:34, sky wrote:
> https://codereview.chromium.org/263643004/diff/110001/ui/gfx/screen.h
> File ui/gfx/screen.h (right):
> 
>
https://codereview.chromium.org/263643004/diff/110001/ui/gfx/screen.h#newcode78
> ui/gfx/screen.h:78: NativeView view, std::vector<char>* color_profile) const =
> 0;
> I agree with Oshima on the wierdness of this API.

Oshima did not use the word "weird". Indeed, I have found his review
comments/question very helpful.

> What is NativeView isn't on this screen at all?

If a view is born off-screen, user can't see it regardless of whether you
attempt to paint it.  Move it on-screen though, and a NotifyScreenInfoChanged()
occurs, which reads/applies the color profile of the screen the view moved onto
during a WasResized() IPC to the renderer.

An on-screen view has the color profile of it's screen. Move it off-screen and
it retains that color profile. If you move it back on-screen, a
NotifyScreenInfoChanged() occurs ...

> Also, when you wrap one param per line (see style guide).

presubmit.py seems to allow it.

[sky, 2014-05-09 15:10:58]

On Fri, May 9, 2014 at 6:59 AM,  <noel@chromium.org> wrote:
> On 2014/05/07 16:38:34, sky wrote:
>>
>> What if view is NULL?
>
>
> Can a NULL view be painted (color profiles apply at paint-time)?

The API doesn't imply anything about paint time. My question is what
happens if passed a NULL view? That needs to be documented.

  -Scott

>
> A NULL view has no DPI, size, color profile etc, and the API returns false
> for a
> NULL view to let the caller know.
>
>
> https://codereview.chromium.org/263643004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-05-09 15:11:16]

On Fri, May 9, 2014 at 7:13 AM,  <noel@chromium.org> wrote:
> On 2014/05/07 16:38:34, sky wrote:
>>
>> https://codereview.chromium.org/263643004/diff/110001/ui/gfx/screen.h
>> File ui/gfx/screen.h (right):
>
>
>
>
https://codereview.chromium.org/263643004/diff/110001/ui/gfx/screen.h#newcode78
>>
>> ui/gfx/screen.h:78: NativeView view, std::vector<char>* color_profile)
>> const =
>> 0;
>> I agree with Oshima on the wierdness of this API.
>
>
> Oshima did not use the word "weird". Indeed, I have found his review
> comments/question very helpful.
>
>
>> What is NativeView isn't on this screen at all?
>
>
> If a view is born off-screen, user can't see it regardless of whether you
> attempt to paint it.  Move it on-screen though, and a
> NotifyScreenInfoChanged()
> occurs, which reads/applies the color profile of the screen the view moved
> onto
> during a WasResized() IPC to the renderer.
>
> An on-screen view has the color profile of it's screen. Move it off-screen
> and
> it retains that color profile. If you move it back on-screen, a
> NotifyScreenInfoChanged() occurs ...
>
>
>> Also, when you wrap one param per line (see style guide).
>
>
> presubmit.py seems to allow it.

presubmit.py does not catch everything.

  -Scott
>
>
> https://codereview.chromium.org/263643004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[oshima, 2014-05-09 17:06:06]

In case you missed this comment:

> If you really think color profile doesn't belong to a 
> display, then why do you want this API to be in gfx::Screen?
> gfx::Screen is to get information about the screen/display after all.

[Noel Gordon, 2014-05-12 14:35:32]

Thanks - thought I responded to it in #18 :)

[sky, 2014-05-12 16:20:37]

On 2014/05/07 15:36:52, Noel Gordon (Google) wrote:
> On 2014/05/07 13:44:16, oshima wrote:
> 
> > > to get the profile, without reference to a display (screen).
> > > I don't really see a need to make color profiles belong to
> > > anyone, certainly not a NativeView or display, as yet.
> > 
> > NSScreen seems to have colorSpace
> 
> Yes, and we need to extract it.
> 
> > and you can get NSScreen from NSView,
> 
> NSView doesn't have a 'screen' protocol though, according to
> the Apple developer docs.  It's window does, and the way the
> the screen color space extraction can be done is
> 
>   [[[NativeView window] screen] colorSpace];
> 
> based on my reading of the Apple documentation.

An API like this implies the screen has one and only one colorSpace. This
matches my conceptual model too. What I don't understand then is why you want
the screen API to also take a NativeView?

> 
> > so it looks to me that color profile does belong to the display/screen.
> 
> I think might just be misunderstanding what you mean by
> "belong" ...
> 
> > If you really think color profile doesn't belong to a display, then why do
you
> > want this
> > API to be in gfx::Screen? gfx::Screen is to get information about the
> > screen/display after all.
> 
> ... and I'd tend to instead say that the color profile is a
> "property" of the screen, for discussion's sake.
> 
> I'm fine with a gfx::Screen api that extracts that property
> from a native view.

[oshima, 2014-05-12 18:53:42]

On 2014/05/12 16:20:37, sky wrote:
> On 2014/05/07 15:36:52, Noel Gordon (Google) wrote:
> > On 2014/05/07 13:44:16, oshima wrote:
> > 
> > > > to get the profile, without reference to a display (screen).
> > > > I don't really see a need to make color profiles belong to
> > > > anyone, certainly not a NativeView or display, as yet.
> > > 
> > > NSScreen seems to have colorSpace
> > 
> > Yes, and we need to extract it.
> > 
> > > and you can get NSScreen from NSView,
> > 
> > NSView doesn't have a 'screen' protocol though, according to
> > the Apple developer docs.  It's window does, and the way the
> > the screen color space extraction can be done is
> > 
> >   [[[NativeView window] screen] colorSpace];
> > 
> > based on my reading of the Apple documentation.
> 
> An API like this implies the screen has one and only one colorSpace. This
> matches my conceptual model too. What I don't understand then is why you want
> the screen API to also take a NativeView?

Exactly.

And if you agree that the color space is a property of the screen (I assume you
meant NSScreen, which corresponds to gfx::Display in chrome),
API should be either

gfx::Display::GetColorSpace

or

gfx:Screen::GetColorSpace(gfx::Display)


> 
> > 
> > > so it looks to me that color profile does belong to the display/screen.
> > 
> > I think might just be misunderstanding what you mean by
> > "belong" ...
> > 
> > > If you really think color profile doesn't belong to a display, then why do
> you
> > > want this
> > > API to be in gfx::Screen? gfx::Screen is to get information about the
> > > screen/display after all.
> > 
> > ... and I'd tend to instead say that the color profile is a
> > "property" of the screen, for discussion's sake.
> > 
> > I'm fine with a gfx::Screen api that extracts that property
> > from a native view.

[Ken Russell (switch to Gerrit), 2014-05-13 03:28:12]

On 2014/05/12 18:53:42, oshima wrote:
> 
> Exactly.
> 
> And if you agree that the color space is a property of the screen (I assume
you
> meant NSScreen, which corresponds to gfx::Display in chrome),
> API should be either
> 
> gfx::Display::GetColorSpace
> 
> or
> 
> gfx:Screen::GetColorSpace(gfx::Display)

noel@ pointed out to me in an offline discussion that in crbug.com/352884 there
was an attempt to make gfx::Screen more self-contained on Mac OS. It looks like
that work's stalled.

After thought, I agree that the best direction for this work would be to
associate the color profile with the Display. However, phrasing the API that way
and avoiding the use of NativeView in this CL will make fixing crbug.com/352884
a prerequisite. That seems a lot to ask given that others have tried and failed
to fix that other bug, that Noel is doing this work alone, and is essentially
trying to land an already-working first sketch of display color profile support
behind a flag so others can contribute.

oshima, sky, can you please suggest a path forward for this work that you would
support? What about landing this work in its current form and requiring that the
underlying Screen APIs be restructured before it's enabled by default?

[oshima, 2014-05-13 05:30:45]

On 2014/05/13 03:28:12, Ken Russell wrote:
> On 2014/05/12 18:53:42, oshima wrote:
> > 
> > Exactly.
> > 
> > And if you agree that the color space is a property of the screen (I assume
> you
> > meant NSScreen, which corresponds to gfx::Display in chrome),
> > API should be either
> > 
> > gfx::Display::GetColorSpace
> > 
> > or
> > 
> > gfx:Screen::GetColorSpace(gfx::Display)
> 
> noel@ pointed out to me in an offline discussion that in crbug.com/352884
there
> was an attempt to make gfx::Screen more self-contained on Mac OS. It looks
like
> that work's stalled.
> 
> After thought, I agree that the best direction for this work would be to
> associate the color profile with the Display. However, phrasing the API that
way
> and avoiding the use of NativeView in this CL will make fixing
crbug.com/352884
> a prerequisite. That seems a lot to ask given that others have tried and
failed
> to fix that other bug, that Noel is doing this work alone, and is essentially
> trying to land an already-working first sketch of display color profile
support
> behind a flag so others can contribute.
> 
> oshima, sky, can you please suggest a path forward for this work that you
would
> support? What about landing this work in its current form and requiring that
the
> underlying Screen APIs be restructured before it's enabled by default?

See my comment #2 in crbgu.com/352884. I said his solution is fine (esp the
issue on osx side prevent us from doing the right thing). The issue in #4 does
not seem to be relevant as this CL doesn't solve NULL window issue, and still
assumes that the correct NSView with window should be passed. (correct me if I'm
wrong).

For deeper issue with the ID on mac, it is my understanding that Mac may return
different ID for internal display when external display is connected. 
(http://stackoverflow.com/questions/2861871/how-to-resolve-cgdirectdisplayid-c...)
One solution is to assign fixed ID for internal display because it'll never
change, and according to
https://developer.apple.com/library/mac/documentation/graphicsimaging/Concept...,
you can get this info using CGDisplayIsBuiltin.

[Ken Russell (switch to Gerrit), 2014-05-14 00:58:17]

On 2014/05/13 05:30:45, oshima wrote:
> On 2014/05/13 03:28:12, Ken Russell wrote:
> > On 2014/05/12 18:53:42, oshima wrote:
> > > 
> > > Exactly.
> > > 
> > > And if you agree that the color space is a property of the screen (I
assume
> > you
> > > meant NSScreen, which corresponds to gfx::Display in chrome),
> > > API should be either
> > > 
> > > gfx::Display::GetColorSpace
> > > 
> > > or
> > > 
> > > gfx:Screen::GetColorSpace(gfx::Display)
> > 
> > noel@ pointed out to me in an offline discussion that in crbug.com/352884
> there
> > was an attempt to make gfx::Screen more self-contained on Mac OS. It looks
> like
> > that work's stalled.
> > 
> > After thought, I agree that the best direction for this work would be to
> > associate the color profile with the Display. However, phrasing the API that
> way
> > and avoiding the use of NativeView in this CL will make fixing
> crbug.com/352884
> > a prerequisite. That seems a lot to ask given that others have tried and
> failed
> > to fix that other bug, that Noel is doing this work alone, and is
essentially
> > trying to land an already-working first sketch of display color profile
> support
> > behind a flag so others can contribute.
> > 
> > oshima, sky, can you please suggest a path forward for this work that you
> would
> > support? What about landing this work in its current form and requiring that
> the
> > underlying Screen APIs be restructured before it's enabled by default?
> 
> See my comment #2 in crbgu.com/352884. I said his solution is fine (esp the
> issue on osx side prevent us from doing the right thing). The issue in #4 does
> not seem to be relevant as this CL doesn't solve NULL window issue, and still
> assumes that the correct NSView with window should be passed. (correct me if
I'm
> wrong).
> 
> For deeper issue with the ID on mac, it is my understanding that Mac may
return
> different ID for internal display when external display is connected. 
>
(http://stackoverflow.com/questions/2861871/how-to-resolve-cgdirectdisplayid-c...)
> One solution is to assign fixed ID for internal display because it'll never
> change, and according to
>
https://developer.apple.com/library/mac/documentation/graphicsimaging/Concept...,
> you can get this info using CGDisplayIsBuiltin.

oshima@ and I talked further offline and he said it would be fine for the time
being to add a utility method to fetch the color profile from a NativeView. He
simply wants to avoid adding something to Screen which seems like an undesirable
long-term direction. noel@, would that work? Perhaps in color_profile.h with
platform-specific implementations?

[oshima, 2014-05-14 01:08:06]

On 2014/05/14 00:58:17, Ken Russell wrote:
> On 2014/05/13 05:30:45, oshima wrote:
> > On 2014/05/13 03:28:12, Ken Russell wrote:
> > > On 2014/05/12 18:53:42, oshima wrote:
> > > > 
> > > > Exactly.
> > > > 
> > > > And if you agree that the color space is a property of the screen (I
> assume
> > > you
> > > > meant NSScreen, which corresponds to gfx::Display in chrome),
> > > > API should be either
> > > > 
> > > > gfx::Display::GetColorSpace
> > > > 
> > > > or
> > > > 
> > > > gfx:Screen::GetColorSpace(gfx::Display)
> > > 
> > > noel@ pointed out to me in an offline discussion that in crbug.com/352884
> > there
> > > was an attempt to make gfx::Screen more self-contained on Mac OS. It looks
> > like
> > > that work's stalled.
> > > 
> > > After thought, I agree that the best direction for this work would be to
> > > associate the color profile with the Display. However, phrasing the API
that
> > way
> > > and avoiding the use of NativeView in this CL will make fixing
> > crbug.com/352884
> > > a prerequisite. That seems a lot to ask given that others have tried and
> > failed
> > > to fix that other bug, that Noel is doing this work alone, and is
> essentially
> > > trying to land an already-working first sketch of display color profile
> > support
> > > behind a flag so others can contribute.
> > > 
> > > oshima, sky, can you please suggest a path forward for this work that you
> > would
> > > support? What about landing this work in its current form and requiring
that
> > the
> > > underlying Screen APIs be restructured before it's enabled by default?
> > 
> > See my comment #2 in crbgu.com/352884. I said his solution is fine (esp the
> > issue on osx side prevent us from doing the right thing). The issue in #4
does
> > not seem to be relevant as this CL doesn't solve NULL window issue, and
still
> > assumes that the correct NSView with window should be passed. (correct me if
> I'm
> > wrong).
> > 
> > For deeper issue with the ID on mac, it is my understanding that Mac may
> return
> > different ID for internal display when external display is connected. 
> >
>
(http://stackoverflow.com/questions/2861871/how-to-resolve-cgdirectdisplayid-c...)
> > One solution is to assign fixed ID for internal display because it'll never
> > change, and according to
> >
>
https://developer.apple.com/library/mac/documentation/graphicsimaging/Concept...,
> > you can get this info using CGDisplayIsBuiltin.
> 
> oshima@ and I talked further offline and he said it would be fine for the time
> being to add a utility method to fetch the color profile from a NativeView. He
> simply wants to avoid adding something to Screen which seems like an
undesirable
> long-term direction. noel@, would that work? Perhaps in color_profile.h with
> platform-specific implementations?

I just noticed that we already have ui/gfx/color_profile.h and ColorProfile
class there.
I think you can add the API to get color profile from native view there.

[Noel Gordon, 2014-05-14 03:26:56]

On 2014/05/14 00:58:17, Ken Russell wrote:
>
> oshima@ and I talked further offline and he said it would be fine for the time
> being to add a utility method to fetch the color profile from a NativeView.

Thanks for getting together, you guys saved me 24 hours.

> He simply wants to avoid adding something to Screen which seems like an
undesirable
> long-term direction. noel@, would that work? Perhaps in color_profile.h with
> platform-specific implementations?

Yeap, works for me. Let's do that.

[Noel Gordon, 2014-05-14 03:38:22]

On 2014/05/13 05:30:45, oshima wrote:
> On 2014/05/13 03:28:12, Ken Russell wrote:
> > On 2014/05/12 18:53:42, oshima wrote:
> > > 
> > > Exactly.
> > > 
> > > And if you agree that the color space is a property of the screen (I
assume
> > you
> > > meant NSScreen, which corresponds to gfx::Display in chrome),
> > > API should be either
> > > 
> > > gfx::Display::GetColorSpace
> > > 
> > > or
> > > 
> > > gfx:Screen::GetColorSpace(gfx::Display)
> > 
> > noel@ pointed out to me in an offline discussion that in crbug.com/352884
> there
> > was an attempt to make gfx::Screen more self-contained on Mac OS. It looks
> like
> > that work's stalled.
> > 
> > After thought, I agree that the best direction for this work would be to
> > associate the color profile with the Display. However, phrasing the API that
> way
> > and avoiding the use of NativeView in this CL will make fixing
> crbug.com/352884
> > a prerequisite. That seems a lot to ask given that others have tried and
> failed
> > to fix that other bug, that Noel is doing this work alone, and is
essentially
> > trying to land an already-working first sketch of display color profile
> support
> > behind a flag so others can contribute.
> > 
> > oshima, sky, can you please suggest a path forward for this work that you
> would
> > support? What about landing this work in its current form and requiring that
> the
> > underlying Screen APIs be restructured before it's enabled by default?
> 
> See my comment #2 in crbgu.com/352884. I said his solution is fine (esp the
> issue on osx side prevent us from doing the right thing). The issue in #4 does
> not seem to be relevant as this CL doesn't solve NULL window issue, and still
> assumes that the correct NSView with window should be passed. (correct me if
I'm
> wrong).

Correct, the issue in #4 is not something that gfx (or this patch) needs to
solve: it has to be solved above the gfx level.

> For deeper issue with the ID on mac, it is my understanding that Mac may
return
> different ID for internal display when external display is connected. 

and for extra spice, when the monitor disconnected and reconnected to the same
port.  I believe that the screen ID also changes when your monitor goes to sleep
and then later awoken.  mac screen ID's at OS-level seem to be ephemeral

>
(http://stackoverflow.com/questions/2861871/how-to-resolve-cgdirectdisplayid-c...)
> One solution is to assign fixed ID for internal display because it'll never
> change, and according to
>
https://developer.apple.com/library/mac/documentation/graphicsimaging/Concept...,
> you can get this info using CGDisplayIsBuiltin.