Add RenderView / Widget setDeviceColorProfile interface to Page

Add RenderView / Widget setDeviceColorProfile interface to Page

Add setDeviceColorProfile() to allow a render widget to set the
screen color profile of its blink::Page.

Also add a resetDeviceColorProfile() method used to restore the
blink::Page to default color profile state.

Implementation of each in Page are FIXME.

BUG=368663
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181899

[eseidel, 2014-04-30 21:17:46]

Page is generally per-process.  Are you sure you don't want per-frame?

[Noel Gordon, 2014-05-05 05:20:11]

Yep. I'm following everywhere Blink::WebScreenInfo get IPC-sent from the browser
to their associated blink renderer (RenderView/RenderWidget), and that seems to
be to Page only at the moment.

If the frames that compose a Page get moved out of process in future, then I
guess the supporting code  on the browser process side will have to deal with
getting screen info sent to such frames.

[eseidel, 2014-05-07 17:14:59]

You really need a web/ reviewer here.  I technically can review web/ changes,
but I'm not very good at it.

https://codereview.chromium.org/261633002/diff/20001/public/web/WebView.h
File public/web/WebView.h (right):

https://codereview.chromium.org/261633002/diff/20001/public/web/WebView.h#new...
public/web/WebView.h:303: virtual void deviceColorProfileChanged(const char*
profile, size_t) = 0;
I would expect we'd use a WebData or some other more sophisticated container
type than a byte-stream?

[abarth-chromium, 2014-05-07 17:31:55]

This seems more like a property of WebScreenInfo rather than WebView.

If the color profile changes dynamically, I would have expected an interface in
public/platform that Blink can use to install a listener.  That approach isn't
coupled to WebView at all.

[abarth-chromium, 2014-05-07 17:33:59]

Take a look at WebScreenOrientationListener as an example of subscribing to
changes to some property of the screen.  That's used for a DOM interface, but it
might give you some idea about how to structure the notification aspect.

Also, should we create a WebColorProfile type to abstract the profile itself?

[eseidel, 2014-05-29 00:26:22]

Looks like this CL is waiting for an update (no rush, just going through my CL
inbox).

[Noel Gordon, 2014-09-05 16:59:45]

On 2014/05/29 00:26:22, eseidel wrote:
> Looks like this CL is waiting for an update (no rush, just going through my CL
> inbox).

Took the time to jump over this, and tour further into blink renderers and
paint-time color correction,, skia and impl-side-paint, skia filters, YUV image
decoding, and eventually to the GPU, to see what changes these systems need to
handle color profile updates and co-ordinate painting during same to avoid a
flash incorrect color on the screen.

[Noel Gordon, 2014-09-05 17:24:11]

On 2014/05/07 17:14:59, eseidel wrote:
> You really need a web/ reviewer here.  I technically can review web/ changes,
> but I'm not very good at it.

Sure you are ;) we are passing the profile data through like just we do the hdpi
scale factor change.  They are similar
in that page state is updated and a repaint is needed.

> https://codereview.chromium.org/261633002/diff/20001/public/web/WebView.h
> File public/web/WebView.h (right):
> 
>
https://codereview.chromium.org/261633002/diff/20001/public/web/WebView.h#new...
> public/web/WebView.h:303: virtual void deviceColorProfileChanged(const char*
> profile, size_t) = 0;
> I would expect we'd use a WebData or some other more sophisticated container
> type than a byte-stream?

Would sophisticated container require a data copy ( was trying to avoid that).

[Noel Gordon, 2014-09-05 17:29:55]

On 2014/05/07 17:31:55, abarth wrote:
> This seems more like a property of WebScreenInfo rather than WebView.

Had it in WebScreenInfo once upon a time, caused all sorts of color flashing
issues, so I decoupled it.

> If the color profile changes dynamically, I would have expected an interface
in
> public/platform that Blink can use to install a listener.  That approach isn't
> coupled to WebView at all.

The hdpi change handling adjacent doesn't use listeners, so I followed that
since I have a similar job to do.  hdpi and color profile changes are interwined
by the WasResized IPC coming from the browser, and I followed the cow path.

[Noel Gordon, 2014-09-05 18:16:22]

On 2014/05/07 17:33:59, abarth wrote:
> Take a look at WebScreenOrientationListener as an example of subscribing to
> changes to some property of the screen.  That's used for a DOM interface, but
it
> might give you some idea about how to structure the notification aspect.

A color profile is a variable of state in platform/graphics. It might be for the
screen or the printer, and the paint layers refer to the current color profile
during painting.  On update, the state of platform/graphics must first be
changed, then the page has to be repainted. I wonder if something like
WebScreenOrientationListener a bit much for this page repaint job.

> Also, should we create a WebColorProfile type to abstract the profile itself?

If the color profile leaves blink (not sure if that's what you meant), it'll
likely be internal to a WebFilter.

[Noel Gordon, 2014-09-11 11:41:47]

+ jochen since we this work is per crbug.com/369787 Thoughts?

[Noel Gordon, 2014-09-11 11:42:28]

noel@chromium.org changed reviewers:
+ jochen@chromium.org
- abarth@chromium.org, eseidel@chromium.org

[jochen (gone - plz use gerrit), 2014-09-11 14:03:55]

I agree that putting the call next to the device scale factor makes sense. I
wonder what happens if you drag a tab that shares a process with another tab to
a different monitor? But I guess whatever happens is also reflected in the scale
factor.

Anyway, maybe using WebData is indeed a good idea. It will create a copy, yes,
but for the string, you'll also need to copy the data out of the ipc message,
no?

[Noel Gordon, 2014-09-11 17:24:44]

On 2014/09/11 14:03:55, jochen wrote:
> I agree that putting the call next to the device scale factor makes sense. I
> wonder what happens if you drag a tab that shares a process with another tab
to
> a different monitor? But I guess whatever happens is also reflected in the
scale
> factor.

Correct. Two RenderWidgets can be in the same process say, but displayed on
different monitors. Each RenderWidget has it's own scale factor.

> Anyway, maybe using WebData is indeed a good idea. It will create a copy, yes,
> but for the string, you'll also need to copy the data out of the ipc message,
> no?

Yes, the IPC copies directly into an stl::string. Maybe a VebVector instead of
WebData. I'm adding a resetColorProfile() while I'm at it ...

[Noel Gordon, 2014-09-12 04:38:09]

On 2014/09/11 14:03:55, jochen wrote:
> I wonder what happens if you drag a tab that shares a process with another tab
to
> a different monitor? But I guess whatever happens is also reflected in the
scale
> factor.

And that's in theory, btw. In practice, how well do dpi changes on multiple
monitors work in blink?

https://code.google.com/p/chromium/issues/detail?id=338130#c20
https://code.google.com/p/chromium/issues/detail?id=338130#c23

Been banging on that bug to get an assist, and banged on it again today.  Note
that dpi and color profile changes are related in that they go through the same
renderer_host / renderer IPC control code paths, so any dynamic dpi change bugs
suggest that dynamic color profile changes might have a similar or related bug
maifestation.

> I agree that putting the call next to the device scale factor makes sense.

Indeed, see above.

[Noel Gordon, 2014-09-12 04:45:51]

On 2014/09/12 04:38:09, Noel Gordon wrote:

> renderer_host / renderer IPC control code paths, so any dynamic dpi change
bugs
> suggest that dynamic color profile changes might have a similar or related bug
> maifestation.

/ manifestation even.

[jochen (gone - plz use gerrit), 2014-09-12 11:48:41]

ok, lgtm

[Noel Gordon, 2014-09-12 13:13:30]

The CQ bit was checked by noel@chromium.org

[commit-bot: I haz the power, 2014-09-12 13:13:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/261633002/60001

[commit-bot: I haz the power, 2014-09-12 13:18:30]

Committed patchset #4 (id:60001) as 181899