Have one ICCProfile per WebScreenInfo in Blink.

Have one ICCProfile per WebScreenInfo in Blink.

This will allow us to keep track of the color profile per screen instead
of per window. That will allow Blink to use and expose different
information when the screen is updated or when a window changes screen.

BUG=685456
Review-Url: https://codereview.chromium.org/2649023004
Cr-Commit-Position: refs/heads/master@{#446827}
Committed: https://chromium.googlesource.com/chromium/src/+/afca06dd04d0a4f67ec35edfdb3c4cbd6f112363

[mlamouri (slow - plz ping), 2017-01-24 06:23:38]

mlamouri@chromium.org changed reviewers:
+ ccameron@chromium.org

[mlamouri (slow - plz ping), 2017-01-24 06:23:39]

ccameron@, can you give some early feedback on this? I'm mostly worried about
missing some use case because ScreenInfo has a ICCProfile information and there
is one passed at the window creation. My guess, without doing too much digging
is that this is an oversight. Do you confirm?

Furthermore, I did not update ColorBehavior which that CL breaks because its
global variable isn't updated. Ideally, we would need ColorBehavior to use the
ICCProfile from the Screen instead of a global variable. Does that sound
reasonable?

[ccameron, 2017-01-24 06:41:54]

On 2017/01/24 06:23:39, mlamouri wrote:
> ccameron@, can you give some early feedback on this? I'm mostly worried about
> missing some use case because ScreenInfo has a ICCProfile information and
there
> is one passed at the window creation. My guess, without doing too much digging
> is that this is an oversight. Do you confirm?

Unfortunately it's not an oversight ... Chromium added this global variable in
the distant past, and keeping it working while trying to move away from it is a
massive pain. Ideally we would have never added this global variable behavior,
but what's done is done.

We're working carefully to dig ourselves out of this hole now, without breaking
anything in the mean time.

> Furthermore, I did not update ColorBehavior which that CL breaks because its
> global variable isn't updated. Ideally, we would need ColorBehavior to use the
> ICCProfile from the Screen instead of a global variable. Does that sound
> reasonable?

The bug for fixing multi-monitor is crbug.com/679080.

I phrased it in terms of GraphicsLayer because GraphicsLayer is the only layer
type that actually needs to be aware of the screen that it's on. Part of the
plumbing of this may go through WebScreenInfo (it should more-or-less follow
device scale factor).

The hard part of the problem isn't tracking what monitor is current, but rather
correctly rendering content given a particular ColorBehavior.

I'd love to have more people helping out with the Blink side of this problem --
want to VC in my morning (PST)? I'll check if you're available on chat when I
get up.

[mlamouri (slow - plz ping), 2017-01-26 06:17:11]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 06:17:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mlamouri (slow - plz ping), 2017-01-26 06:18:13]

Description was changed from

==========
Have one ICCProfile per Screen in Blink.

This will allow us to keep track of the color profile per screen instead
of per window. That will allow Blink to use and expose different
information when the screen is updated or when a window changes screen.

This is still WIP. The ColorBehavior class wasn't updated because it is
using a global profile.

BUG=<TODO>
==========

to

==========
Have one ICCProfile per Screen in Blink.

This will allow us to keep track of the color profile per screen instead
of per window. That will allow Blink to use and expose different
information when the screen is updated or when a window changes screen.

BUG=<TODO>
==========

[commit-bot: I haz the power, 2017-01-26 07:27:04]

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

[commit-bot: I haz the power, 2017-01-26 07:27:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mlamouri (slow - plz ping), 2017-01-27 01:49:28]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 01:51:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mlamouri (slow - plz ping), 2017-01-27 01:52:19]

Description was changed from

==========
Have one ICCProfile per Screen in Blink.

This will allow us to keep track of the color profile per screen instead
of per window. That will allow Blink to use and expose different
information when the screen is updated or when a window changes screen.

BUG=<TODO>
==========

to

==========
Have one ICCProfile per WebScreenInfo in Blink.

This will allow us to keep track of the color profile per screen instead
of per window. That will allow Blink to use and expose different
information when the screen is updated or when a window changes screen.

BUG=685456
==========

[mlamouri (slow - plz ping), 2017-01-27 01:53:24]

mlamouri@chromium.org changed reviewers:
+ kinuko@chromium.org

[mlamouri (slow - plz ping), 2017-01-27 01:53:25]

ccameron@, kinuko@, PTAL

[commit-bot: I haz the power, 2017-01-27 03:35:52]

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

[commit-bot: I haz the power, 2017-01-27 03:35:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kinuko, 2017-01-27 03:46:38]

lgtm

[mlamouri (slow - plz ping), 2017-01-27 04:28:26]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 04:29:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 05:04:53]

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

[commit-bot: I haz the power, 2017-01-27 05:04:54]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2017-01-27 18:52:11]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 18:52:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ccameron, 2017-01-27 18:55:20]

I think this lgtm

We're currently batting problems where trybots and continuous builds are
producing different results in this area, so beware that this may hit that.

[commit-bot: I haz the power, 2017-01-27 18:58:01]

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

[commit-bot: I haz the power, 2017-01-27 18:58:02]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2017-01-27 20:03:27]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2017-01-27 20:04:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 20:09:55]

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

[commit-bot: I haz the power, 2017-01-27 20:10:01]

Failed to apply patch for content/renderer/render_view_impl.h:
While running git apply --index -p1;
  error: patch failed: content/renderer/render_view_impl.h:92
  error: content/renderer/render_view_impl.h: patch does not apply

Patch:       content/renderer/render_view_impl.h
Index: content/renderer/render_view_impl.h
diff --git a/content/renderer/render_view_impl.h
b/content/renderer/render_view_impl.h
index
e379b7e4ff1c056451b15ca432463371809f6e59..fe84a64243dad51b3d0a3fd5ea0792a3a37dc752
100644
--- a/content/renderer/render_view_impl.h
+++ b/content/renderer/render_view_impl.h
@@ -92,6 +92,10 @@ class WebHitTestResult;
 #endif
 }  // namespace blink
 
+namespace gfx {
+class ICCProfile;
+}
+
 namespace content {
 
 class HistoryController;
@@ -243,6 +247,9 @@ class CONTENT_EXPORT RenderViewImpl
   // Change the device scale factor and force the compositor to resize.
   void SetDeviceScaleFactorForTesting(float factor);
 
+  // Change the device ICC color profile while running a layout test.
+  void SetDeviceColorProfileForTesting(const gfx::ICCProfile& icc_profile);
+
   // Used to force the size of a window when running layout tests.
   void ForceResizeForTesting(const gfx::Size& new_size);
 
@@ -492,8 +499,6 @@ class CONTENT_EXPORT RenderViewImpl
 
   // RenderWidgetOwnerDelegate implementation
----------------------------------
 
-  void RenderWidgetDidSetColorProfile(
-      const gfx::ICCProfile& color_profile) override;
   void RenderWidgetFocusChangeComplete() override;
   bool DoesRenderWidgetHaveTouchEventHandlersAt(
       const gfx::Point& point) const override;

[mlamouri (slow - plz ping), 2017-01-27 21:55:52]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2017-01-27 21:55:52]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/2649023004/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-27 21:56:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 23:35:22]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485554152215950,
"parent_rev": "45d7c03cf6c50bf18b9d518b42b76b3b47308682", "commit_rev":
"afca06dd04d0a4f67ec35edfdb3c4cbd6f112363"}

[commit-bot: I haz the power, 2017-01-27 23:36:03]

Description was changed from

==========
Have one ICCProfile per WebScreenInfo in Blink.

This will allow us to keep track of the color profile per screen instead
of per window. That will allow Blink to use and expose different
information when the screen is updated or when a window changes screen.

BUG=685456
==========

to

==========
Have one ICCProfile per WebScreenInfo in Blink.

This will allow us to keep track of the color profile per screen instead
of per window. That will allow Blink to use and expose different
information when the screen is updated or when a window changes screen.

BUG=685456

Review-Url: https://codereview.chromium.org/2649023004
Cr-Commit-Position: refs/heads/master@{#446827}
Committed:
https://chromium.googlesource.com/chromium/src/+/afca06dd04d0a4f67ec35edfdb3c...
==========

[commit-bot: I haz the power, 2017-01-27 23:36:04]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/afca06dd04d0a4f67ec35edfdb3c...