ui/gfx: Use separate components for color_space and switches

ui/gfx: Use separate components for color_space and switches

The need for switches to be separate is a temporary consequence of
many workarounds for the non-color-correct-rendering path spread
throughout the code. Once these workarounds are removed, the
switches component can be pulled back into color_space and sources.

R=thakis
TBR=avi for content/
BUG=723608
Review-Url: https://codereview.chromium.org/2915673003
Cr-Commit-Position: refs/heads/master@{#480175}
Committed: https://chromium.googlesource.com/chromium/src/+/fd83a51df6653957aa077d1f1083bfe4118920c4

[ccameron, 2017-05-30 23:26:08]

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

[commit-bot: I haz the power, 2017-05-30 23:26:48]

Dry run: CQ is trying da patch.

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

[ccameron, 2017-05-30 23:31:50]

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

[commit-bot: I haz the power, 2017-05-30 23:32:40]

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-05-30 23:38:10]

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

[commit-bot: I haz the power, 2017-05-30 23:38:11]

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...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ccameron, 2017-05-30 23:39:55]

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

[commit-bot: I haz the power, 2017-05-30 23:40:55]

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-05-30 23:48:21]

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

[commit-bot: I haz the power, 2017-05-30 23:48:22]

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

[ccameron, 2017-05-31 00:01:08]

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

[commit-bot: I haz the power, 2017-05-31 00:01:54]

Dry run: CQ is trying da patch.

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

[ccameron, 2017-05-31 00:45:06]

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

[commit-bot: I haz the power, 2017-05-31 00:45:19]

Dry run: CQ is trying da patch.

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

[ccameron, 2017-05-31 00:56:22]

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

[ccameron, 2017-05-31 00:56:23]

ptal -- lmk if this fixes the issues with my previous patch

[commit-bot: I haz the power, 2017-05-31 01:57:01]

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

[commit-bot: I haz the power, 2017-05-31 01:57:02]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[ccameron, 2017-05-31 02:36:53]

On 2017/05/31 00:56:23, ccameron wrote:
> ptal -- lmk if this fixes the issues with my previous patch

Oops, I didn't take into account the switches dependency here. I'll let you know
when I fix this up.

[ccameron, 2017-06-01 00:06:50]

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

[commit-bot: I haz the power, 2017-06-01 00:07:39]

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-01 00:51:24]

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

[commit-bot: I haz the power, 2017-06-01 00:51:25]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[ccameron, 2017-06-01 04:14:19]

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

[commit-bot: I haz the power, 2017-06-01 04:14:28]

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-01 05:49:41]

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

[commit-bot: I haz the power, 2017-06-01 05:49:41]

Dry run: This issue passed the CQ dry run.

[ccameron, 2017-06-01 14:47:09]

this should be working now -- ptal

[Nico, 2017-06-01 14:52:14]

Hm, making switches its own component seems a bit awkward. Maybe you could move
the color space switches into the colorspace component instead? Then the color
space component could export the color-space related switches, instead of
basically having a grab bag of switches in a component and then all the gfx
component depending on that.

https://codereview.chromium.org/2915673003/diff/140001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/2915673003/diff/140001/ui/gfx/BUILD.gn#newcod...
ui/gfx/BUILD.gn:405: "skia_color_space_util.h",
add new export.h file here

https://codereview.chromium.org/2915673003/diff/140001/ui/gfx/BUILD.gn#newcod...
ui/gfx/BUILD.gn:417: defines = [ "GFX_COLOR_IMPLEMENTATION" ]
optional nit: GFX_COLOR_SPACE_IMPLEMENTATION would be more self-consistent

https://codereview.chromium.org/2915673003/diff/140001/ui/gfx/BUILD.gn#newcod...
ui/gfx/BUILD.gn:559: component("switches") {
Please call this gfx_switches, else this will result in libswitches.so.

https://codereview.chromium.org/2915673003/diff/140001/ui/gfx/gfx_color_export.h
File ui/gfx/gfx_color_export.h (right):

https://codereview.chromium.org/2915673003/diff/140001/ui/gfx/gfx_color_expor...
ui/gfx/gfx_color_export.h:12: #define GFX_COLOR_EXPORT __declspec(dllexport)
As mentioned in gn file, GFX_COLOR_SPACE(S?)_EXPORT (and rename the file)

[ccameron, 2017-06-14 21:20:59]

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

[commit-bot: I haz the power, 2017-06-14 21:21:31]

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-14 21:26:06]

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

[commit-bot: I haz the power, 2017-06-14 21:26:07]

Dry run: 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...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[ccameron, 2017-06-15 06:10:20]

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

[commit-bot: I haz the power, 2017-06-15 06:10:31]

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-15 06:15:11]

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

[commit-bot: I haz the power, 2017-06-15 06:15:12]

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

[ccameron, 2017-06-15 06:21:36]

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

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

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-15 06:27:23]

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

[commit-bot: I haz the power, 2017-06-15 06:27:24]

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

[ccameron, 2017-06-15 06:32:02]

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

[commit-bot: I haz the power, 2017-06-15 06:32:14]

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-15 06:48:48]

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

[commit-bot: I haz the power, 2017-06-15 06:48:57]

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-15 07:01:23]

On 2017/06/01 14:52:14, Nico wrote:
> Hm, making switches its own component seems a bit awkward. Maybe you could
move
> the color space switches into the colorspace component instead? Then the color
> space component could export the color-space related switches, instead of
> basically having a grab bag of switches in a component and then all the gfx
> component depending on that.

So, I tried disentangling this in a number of ways, and they all ended up
falling apart because of temporary ways that these things are pulled together.

For instance
- memory_buffer_sources depends on color space because of a workaround that can
be removed once we always enable color correct rendering
- icc_profile_x11 depends on switches.h (not even the color ones) because of a
workaround for headless mode that will go away once we no longer have a concept
of "best" monitor, but rather have a more precise monitor to send data from
("best" is currently used for the pre-color-correct rendering model).

So, I've gone back to the component("switches") model, with the plan to split
switches and re-merge parts back to color_space and parts back to gfx once we
have remove the color-correct-rendering flag (once the feature launches).

[commit-bot: I haz the power, 2017-06-15 08:25:00]

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

[commit-bot: I haz the power, 2017-06-15 08:25:01]

Dry run: This issue passed the CQ dry run.

[ccameron, 2017-06-15 19:51:33]

Patchset #9 (id:160001) has been deleted

[ccameron, 2017-06-15 19:51:52]

Patchset #13 (id:260001) has been deleted

[ccameron, 2017-06-15 19:52:04]

Patchset #12 (id:240001) has been deleted

[ccameron, 2017-06-15 19:52:12]

Patchset #11 (id:220001) has been deleted

[ccameron, 2017-06-15 19:52:22]

Patchset #10 (id:200001) has been deleted

[ccameron, 2017-06-15 19:52:30]

Patchset #9 (id:180001) has been deleted

[ccameron, 2017-06-15 20:13:13]

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

[commit-bot: I haz the power, 2017-06-15 20:13:47]

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-15 21:30:07]

Description was changed from

==========
ui/gfx: Use separate export for components

BUG=723608
==========

to

==========
ui/gfx: Use separate components for color_space and switches

The need for switches to be separate is a temporary consequence of
many workarounds for the non-color-correct-rendering path spread
throughout the code. Once these workarounds are removed, the
switches component can be pulled back into color_space and sources.

BUG=723608
==========

[ccameron, 2017-06-15 21:30:46]

ptal -- I ended up keeping the switches as a separate grab-bag of switches from
throughout ui/gfx, but added plans for how-and-when to separate it.

https://codereview.chromium.org/2915673003/diff/280001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/2915673003/diff/280001/ui/gfx/BUILD.gn#newcod...
ui/gfx/BUILD.gn:558: # once color correct rendering is enabled.
This exists for two reasons
1. The "enable color correction" switch needs to be accessed in all sorts of
strange places because of interactions with IOSurface color spaces that crept in
to macOS 10.12. Once this flag is "on by default", those workarounds will be
removed.
2. Monitor information is queried using the "ICCProfile::FromBestMonitor"
function, which queries the "headless" attribute. Once the color correct
rendering flag is "on by default", we will be able to remove the
"ICCProfile::FromBestMonitor" function (since it currently provides the color
for image decode in Blink, which will go away).

Given this tangle, I think that making switches be a separate component for now
makes the most sense.

After color correct rendering can't be turned off, color_space_switches will be
moved into the color_space target (which can't be done because of skia-exclusion
by nacl right now), and switches can be moved back to being a source_set.

[commit-bot: I haz the power, 2017-06-15 21:49:01]

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

[commit-bot: I haz the power, 2017-06-15 21:49:02]

Dry run: This issue passed the CQ dry run.

[ccameron, 2017-06-16 19:09:23]

ping

[Nico, 2017-06-16 19:24:09]

Oh, sorry. lgtm with component("switches") -> component("gfx_switches"). I'm
looking forward to this being cleaned up again :-)

https://codereview.chromium.org/2915673003/diff/140001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/2915673003/diff/140001/ui/gfx/BUILD.gn#newcod...
ui/gfx/BUILD.gn:559: component("switches") {
On 2017/06/01 14:52:14, Nico wrote:
> Please call this gfx_switches, else this will result in libswitches.so.

I think this wasn't done.

https://codereview.chromium.org/2915673003/diff/280001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/2915673003/diff/280001/ui/gfx/BUILD.gn#newcod...
ui/gfx/BUILD.gn:558: # once color correct rendering is enabled.
On 2017/06/15 21:30:46, ccameron wrote:
> This exists for two reasons
> 1. The "enable color correction" switch needs to be accessed in all sorts of
> strange places because of interactions with IOSurface color spaces that crept
in
> to macOS 10.12. Once this flag is "on by default", those workarounds will be
> removed.
> 2. Monitor information is queried using the "ICCProfile::FromBestMonitor"
> function, which queries the "headless" attribute. Once the color correct
> rendering flag is "on by default", we will be able to remove the
> "ICCProfile::FromBestMonitor" function (since it currently provides the color
> for image decode in Blink, which will go away).
> 
> Given this tangle, I think that making switches be a separate component for
now
> makes the most sense.
> 
> After color correct rendering can't be turned off, color_space_switches will
be
> moved into the color_space target (which can't be done because of
skia-exclusion
> by nacl right now), and switches can be moved back to being a source_set.

Can you add some of this to the comment?

[ccameron, 2017-06-16 19:42:55]

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

[commit-bot: I haz the power, 2017-06-16 19:43:24]

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-16 19:49:27]

Thanks!

https://codereview.chromium.org/2915673003/diff/140001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/2915673003/diff/140001/ui/gfx/BUILD.gn#newcod...
ui/gfx/BUILD.gn:559: component("switches") {
On 2017/06/16 19:24:09, Nico wrote:
> On 2017/06/01 14:52:14, Nico wrote:
> > Please call this gfx_switches, else this will result in libswitches.so.
> 
> I think this wasn't done.

Oops. Fixed.

https://codereview.chromium.org/2915673003/diff/280001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/2915673003/diff/280001/ui/gfx/BUILD.gn#newcod...
ui/gfx/BUILD.gn:558: # once color correct rendering is enabled.
On 2017/06/16 19:24:09, Nico wrote:
> On 2017/06/15 21:30:46, ccameron wrote:
> > This exists for two reasons
> > 1. The "enable color correction" switch needs to be accessed in all sorts of
> > strange places because of interactions with IOSurface color spaces that
crept
> in
> > to macOS 10.12. Once this flag is "on by default", those workarounds will be
> > removed.
> > 2. Monitor information is queried using the "ICCProfile::FromBestMonitor"
> > function, which queries the "headless" attribute. Once the color correct
> > rendering flag is "on by default", we will be able to remove the
> > "ICCProfile::FromBestMonitor" function (since it currently provides the
color
> > for image decode in Blink, which will go away).
> > 
> > Given this tangle, I think that making switches be a separate component for
> now
> > makes the most sense.
> > 
> > After color correct rendering can't be turned off, color_space_switches will
> be
> > moved into the color_space target (which can't be done because of
> skia-exclusion
> > by nacl right now), and switches can be moved back to being a source_set.
> 
> Can you add some of this to the comment?

Done.

[ccameron, 2017-06-16 19:49:47]

Description was changed from

==========
ui/gfx: Use separate components for color_space and switches

The need for switches to be separate is a temporary consequence of
many workarounds for the non-color-correct-rendering path spread
throughout the code. Once these workarounds are removed, the
switches component can be pulled back into color_space and sources.

BUG=723608
==========

to

==========
ui/gfx: Use separate components for color_space and switches

The need for switches to be separate is a temporary consequence of
many workarounds for the non-color-correct-rendering path spread
throughout the code. Once these workarounds are removed, the
switches component can be pulled back into color_space and sources.

R=thakis
TBR=avi for content/
BUG=723608
==========

[ccameron, 2017-06-16 20:19:53]

The CQ bit was unchecked by ccameron@chromium.org

[ccameron, 2017-06-16 20:20:06]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-06-16 20:20:07]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2915673003/#ps300001
(title: "Review feedback")

[commit-bot: I haz the power, 2017-06-16 20:20:31]

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-16 21:26:15]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1497644406173860,
"parent_rev": "0c2987eee004d427ace7113e539b93f5a1946979", "commit_rev":
"fd83a51df6653957aa077d1f1083bfe4118920c4"}

[commit-bot: I haz the power, 2017-06-16 21:26:31]

Description was changed from

==========
ui/gfx: Use separate components for color_space and switches

The need for switches to be separate is a temporary consequence of
many workarounds for the non-color-correct-rendering path spread
throughout the code. Once these workarounds are removed, the
switches component can be pulled back into color_space and sources.

R=thakis
TBR=avi for content/
BUG=723608
==========

to

==========
ui/gfx: Use separate components for color_space and switches

The need for switches to be separate is a temporary consequence of
many workarounds for the non-color-correct-rendering path spread
throughout the code. Once these workarounds are removed, the
switches component can be pulled back into color_space and sources.

R=thakis
TBR=avi for content/
BUG=723608

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

[commit-bot: I haz the power, 2017-06-16 21:26:34]

Committed patchset #10 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/fd83a51df6653957aa077d1f1083...