color: Use ICCProfile from display::Display instead of FromBestMonitor

color: Use ICCProfile from display::Display instead of FromBestMonitor

Remove aura::WindowTreeHost::GetICCProfileForCurrentDisplay, since
this can be inferred from display::Screen::GetDisplayNearestWindow.

This sets us up to notify the renderer and window of their new color
space when moved across displays.

Still rely on ICCProfile::FromBestMonitor for now, since removing that
will require attention to detail on all platforms.

R=hubbe
TBR=skyostil (headless/)
BUG=735613
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2950183002
Cr-Commit-Position: refs/heads/master@{#482056}
Committed: https://chromium.googlesource.com/chromium/src/+/0269cae2f81d9920c4371e9fab0af5b569ddbe91

[ccameron, 2017-06-21 21:25:29]

Description was changed from

==========
color: Use ICCProfile from display::Display instead of FromBestMonitor

This sets us up to notify the renderer and window of their new color
space when moved across displays.

BUG=735613
==========

to

==========
color: Use ICCProfile from display::Display instead of FromBestMonitor

Remove aura::WindowTreeHost::GetICCProfileForCurrentDisplay, since
this can be inferred from display::Screen::GetDisplayNearestWindow.

This sets us up to notify the renderer and window of their new color
space when moved across displays.

Still rely on ICCProfile::FromBestMonitor for now, since removing that
will require attention to detail on all platforms.

BUG=735613
==========

[ccameron, 2017-06-21 21:27:30]

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

[hubbe, 2017-06-21 21:49:14]

lgtm

https://codereview.chromium.org/2950183002/diff/1/ui/aura/window_tree_host.cc
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/2950183002/diff/1/ui/aura/window_tree_host.cc...
ui/aura/window_tree_host.cc:307:
compositor_->SetScaleAndSize(ui::GetScaleFactorForNativeView(window()),
What does this change do?

[ccameron, 2017-06-21 23:21:49]

Thanks!

https://codereview.chromium.org/2950183002/diff/1/ui/aura/window_tree_host.cc
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/2950183002/diff/1/ui/aura/window_tree_host.cc...
ui/aura/window_tree_host.cc:307:
compositor_->SetScaleAndSize(ui::GetScaleFactorForNativeView(window()),
On 2017/06/21 21:49:13, hubbe wrote:
> What does this change do?

Oh, I just inlined the above GetDeviceScaleFactorFromDisplay function here.

The ui::GetScaleFactorForNativeView function really shouldn't exist ... I was
thinking to remove it entirely, but that became too big of an incidental change.

[ccameron, 2017-06-21 23:23:24]

Description was changed from

==========
color: Use ICCProfile from display::Display instead of FromBestMonitor

Remove aura::WindowTreeHost::GetICCProfileForCurrentDisplay, since
this can be inferred from display::Screen::GetDisplayNearestWindow.

This sets us up to notify the renderer and window of their new color
space when moved across displays.

Still rely on ICCProfile::FromBestMonitor for now, since removing that
will require attention to detail on all platforms.

BUG=735613
==========

to

==========
color: Use ICCProfile from display::Display instead of FromBestMonitor

Remove aura::WindowTreeHost::GetICCProfileForCurrentDisplay, since
this can be inferred from display::Screen::GetDisplayNearestWindow.

This sets us up to notify the renderer and window of their new color
space when moved across displays.

Still rely on ICCProfile::FromBestMonitor for now, since removing that
will require attention to detail on all platforms.

R=hubbe
TBR=skyostil (headless/)
BUG=735613
==========

[ccameron, 2017-06-21 23:23:30]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2017-06-21 23:23:54]

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-06-21 23:40:44]

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

[commit-bot: I haz the power, 2017-06-21 23:40:45]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[ccameron, 2017-06-22 00:20:28]

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

[commit-bot: I haz the power, 2017-06-22 00:20:41]

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-06-22 01:11:26]

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

[commit-bot: I haz the power, 2017-06-22 01:11:27]

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_...)

[ccameron, 2017-06-22 05:11:45]

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

[commit-bot: I haz the power, 2017-06-22 05:12:03]

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-06-22 05:49:25]

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

[commit-bot: I haz the power, 2017-06-22 05:49:25]

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

[ccameron, 2017-06-22 07:04:43]

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

[commit-bot: I haz the power, 2017-06-22 07:04:57]

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-06-22 07:23:25]

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

[commit-bot: I haz the power, 2017-06-22 07:23:26]

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

[ccameron, 2017-06-22 07:47:12]

Description was changed from

==========
color: Use ICCProfile from display::Display instead of FromBestMonitor

Remove aura::WindowTreeHost::GetICCProfileForCurrentDisplay, since
this can be inferred from display::Screen::GetDisplayNearestWindow.

This sets us up to notify the renderer and window of their new color
space when moved across displays.

Still rely on ICCProfile::FromBestMonitor for now, since removing that
will require attention to detail on all platforms.

R=hubbe
TBR=skyostil (headless/)
BUG=735613
==========

to

==========
color: Use ICCProfile from display::Display instead of FromBestMonitor

Remove aura::WindowTreeHost::GetICCProfileForCurrentDisplay, since
this can be inferred from display::Screen::GetDisplayNearestWindow.

This sets us up to notify the renderer and window of their new color
space when moved across displays.

Still rely on ICCProfile::FromBestMonitor for now, since removing that
will require attention to detail on all platforms.

R=hubbe
TBR=skyostil (headless/)
BUG=735613
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ccameron, 2017-06-22 07:47:16]

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

[commit-bot: I haz the power, 2017-06-22 07:47:29]

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-06-22 07:57:16]

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

[commit-bot: I haz the power, 2017-06-22 07:57:17]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ccameron, 2017-06-22 21:32:36]

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

[commit-bot: I haz the power, 2017-06-22 21:33:11]

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-06-22 22:59:26]

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

[commit-bot: I haz the power, 2017-06-22 22:59:27]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[ccameron, 2017-06-23 06:12:55]

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

[commit-bot: I haz the power, 2017-06-23 06:13:05]

Dry run: CQ is trying da patch.

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

[ccameron, 2017-06-23 06:17:37]

The CQ bit was unchecked by ccameron@chromium.org

[ccameron, 2017-06-23 06:17:38]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-06-23 06:17:38]

The patchset sent to the CQ was uploaded after l-g-t-m from hubbe@chromium.org
Link to the patchset: https://codereview.chromium.org/2950183002/#ps140001
(title: "Update expectations")

[commit-bot: I haz the power, 2017-06-23 06:17:46]

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-06-23 06:36:37]

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

[commit-bot: I haz the power, 2017-06-23 06:36:39]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[ccameron, 2017-06-23 16:01:47]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2017-06-23 16:02:03]

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-06-23 18:07:47]

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

[commit-bot: I haz the power, 2017-06-23 18:07:48]

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_...)

[ccameron, 2017-06-23 20:24:22]

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

[commit-bot: I haz the power, 2017-06-23 20:24:42]

Dry run: CQ is trying da patch.

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

[hubbe, 2017-06-23 21:31:52]

lgtm

https://codereview.chromium.org/2950183002/diff/140001/cc/surfaces/surface_ag...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2950183002/diff/140001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator.cc:941: blending_color_space_ =
blending_color_space.IsValid()
Can we just DCHECK() instead?

https://codereview.chromium.org/2950183002/diff/140001/content/browser/web_co...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/2950183002/diff/140001/content/browser/web_co...
content/browser/web_contents/web_contents_view_aura.cc:714:
display.color_space().GetICCProfile(&results->icc_profile);
seems this would be easier to read if said something like:
  results->icc_profile = display.color_space().GetICCProfile();

What do we do if/when the color space can't be represented as an ICC profile?

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

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

[commit-bot: I haz the power, 2017-06-23 22:27:29]

Dry run: This issue passed the CQ dry run.

[ccameron, 2017-06-23 22:30:52]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-06-23 22:31:00]

The CQ bit was unchecked by ccameron@chromium.org

[ccameron, 2017-06-23 22:44:10]

Good points -- both answers are "yes I want to but this patch is getting too
big"

https://codereview.chromium.org/2950183002/diff/140001/cc/surfaces/surface_ag...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2950183002/diff/140001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator.cc:941: blending_color_space_ =
blending_color_space.IsValid()
On 2017/06/23 21:31:52, hubbe wrote:
> Can we just DCHECK() instead?

There's the question: "Suppose we can't parse a display's ICC profile. At what
point do we compensate for that?".

For the moment, this is the choke point that all paths go through.

Once we get a bit more cleanup in place, I'd like to add DCHECKs here, and
remove the validation. But first we'll need to clean up the platform-specific
color profile code.

https://codereview.chromium.org/2950183002/diff/140001/content/browser/web_co...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/2950183002/diff/140001/content/browser/web_co...
content/browser/web_contents/web_contents_view_aura.cc:714:
display.color_space().GetICCProfile(&results->icc_profile);
On 2017/06/23 21:31:52, hubbe wrote:
> seems this would be easier to read if said something like:
>   results->icc_profile = display.color_space().GetICCProfile();
> 
> What do we do if/when the color space can't be represented as an ICC profile?

I didn't change this instance because it affects IPCs and starts getting into
Blink (so the scope grows). My next patch in this sequence changes ScreenInfo to
have a gfx::ColorSpace instead of ICCProfile.

[ccameron, 2017-06-23 22:44:34]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2017-06-23 22:45:01]

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-06-23 22:49:04]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1498257874773230,
"parent_rev": "d0d6b1ea36c414ad30a7ab0d4fb560ecaf9704e3", "commit_rev":
"0269cae2f81d9920c4371e9fab0af5b569ddbe91"}

[commit-bot: I haz the power, 2017-06-23 22:49:17]

Description was changed from

==========
color: Use ICCProfile from display::Display instead of FromBestMonitor

Remove aura::WindowTreeHost::GetICCProfileForCurrentDisplay, since
this can be inferred from display::Screen::GetDisplayNearestWindow.

This sets us up to notify the renderer and window of their new color
space when moved across displays.

Still rely on ICCProfile::FromBestMonitor for now, since removing that
will require attention to detail on all platforms.

R=hubbe
TBR=skyostil (headless/)
BUG=735613
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
color: Use ICCProfile from display::Display instead of FromBestMonitor

Remove aura::WindowTreeHost::GetICCProfileForCurrentDisplay, since
this can be inferred from display::Screen::GetDisplayNearestWindow.

This sets us up to notify the renderer and window of their new color
space when moved across displays.

Still rely on ICCProfile::FromBestMonitor for now, since removing that
will require attention to detail on all platforms.

R=hubbe
TBR=skyostil (headless/)
BUG=735613
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2950183002
Cr-Commit-Position: refs/heads/master@{#482056}
Committed:
https://chromium.googlesource.com/chromium/src/+/0269cae2f81d9920c4371e9fab0a...
==========

[commit-bot: I haz the power, 2017-06-23 22:49:18]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/0269cae2f81d9920c4371e9fab0a...