Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(18)

Issue 2898583002: color: Fix color not being reset between layout tests (Closed)

Created:
1 year, 5 months ago by ccameron
Modified:
1 year, 4 months ago
CC:
blink-reviews, cc-bugs_chromium.org, chromium-reviews, darin-cc_chromium.org, einbinder+watch-test-runner_chromium.org, jam, jochen+watch_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-test-runner_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

color: Fix color not being reset between layout tests Layout tests run WebTestDelegate::SetDeviceColorProfile("reset") after every test, which resets the ICC profile for rasterization to an empty profile. This is not the right thing to do with color correct rendering, because it will make LayerTreeHostImpl::GetRasterColorSpace return an invalid color space, which is used as a signal that color correct rendering is disabled. Fix this in two ways. First, make WebTestDelegate::SetDeviceColorProfile("reset") reset the ICC profile to be whatever was specified at the command line. Second, make LayerTreeHostImpl::GetRasterColorSpace never return an invalid color space when color correct rendering is enabled. Third, fix a bug in RenderWidget where ICCProfile::GetColorSpace is used instead of ICCProfile::GetParametricColorSpace. BUG=724714 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2898583002 Cr-Commit-Position: refs/heads/master@{#473829} Committed: https://chromium.googlesource.com/chromium/src/+/01912990baad76adb75b28ccc11c74691c9b34d3

Patch Set 1 #

Patch Set 2 : Remove extra changes #

Total comments: 4

Patch Set 3 : Review feedbakc #

Unified diffs Side-by-side diffs Delta from patch set Stats (+43 lines, -5 lines) Patch
M cc/trees/layer_tree_host_impl.cc View 1 2 1 chunk +16 lines, -4 lines 0 comments Download
M cc/trees/layer_tree_host_impl_unittest.cc View 1 chunk +13 lines, -0 lines 0 comments Download
M content/renderer/render_widget.h View 1 chunk +2 lines, -0 lines 0 comments Download
M content/renderer/render_widget.cc View 1 chunk +2 lines, -1 line 0 comments Download
M content/test/layouttest_support.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M ui/gfx/icc_profile.h View 1 chunk +1 line, -0 lines 0 comments Download
M ui/gfx/test/icc_profiles.h View 1 chunk +3 lines, -0 lines 0 comments Download
M ui/gfx/test/icc_profiles.cc View 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 18 (10 generated)
ccameron
ptal -- enne@ for content, avi@ for OWNERship
1 year, 5 months ago (2017-05-20 02:24:20 UTC) #4
enne (OOO)
https://codereview.chromium.org/2898583002/diff/20001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2898583002/diff/20001/cc/trees/layer_tree_host_impl.cc#newcode1359 cc/trees/layer_tree_host_impl.cc:1359: result = sync_tree()->raster_color_space(); I realize you're not changing this ...
1 year, 5 months ago (2017-05-20 03:32:21 UTC) #6
Avi (use Gerrit)
LGTM stamp for when enne is happy
1 year, 5 months ago (2017-05-20 23:35:36 UTC) #9
ccameron
https://codereview.chromium.org/2898583002/diff/20001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2898583002/diff/20001/cc/trees/layer_tree_host_impl.cc#newcode1359 cc/trees/layer_tree_host_impl.cc:1359: result = sync_tree()->raster_color_space(); On 2017/05/20 03:32:21, enne wrote: > ...
1 year, 4 months ago (2017-05-22 23:27:08 UTC) #10
enne (OOO)
https://codereview.chromium.org/2898583002/diff/20001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2898583002/diff/20001/cc/trees/layer_tree_host_impl.cc#newcode1359 cc/trees/layer_tree_host_impl.cc:1359: result = sync_tree()->raster_color_space(); On 2017/05/22 at 23:27:08, ccameron wrote: ...
1 year, 4 months ago (2017-05-23 00:13:44 UTC) #11
ccameron
Thanks! https://codereview.chromium.org/2898583002/diff/20001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2898583002/diff/20001/cc/trees/layer_tree_host_impl.cc#newcode1359 cc/trees/layer_tree_host_impl.cc:1359: result = sync_tree()->raster_color_space(); On 2017/05/23 00:13:43, enne wrote: ...
1 year, 4 months ago (2017-05-23 06:28:40 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2898583002/40001
1 year, 4 months ago (2017-05-23 06:29:24 UTC) #15
commit-bot: I haz the power
1 year, 4 months ago (2017-05-23 07:34:20 UTC) #18
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/01912990baad76adb75b28ccc11c...

Powered by Google App Engine
This is Rietveld 408576698