Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
570023002:
Move screenColorProfile from Platform to ImageDecoder (Closed)
|
Created:
6 years, 3 months ago by Noel Gordon Modified:
6 years, 3 months ago CC:
blink-reviews, dstockwell, mkwst+moarreviews_chromium.org, Robert Sesek, sugoi1 Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionMove screenColorProfile from Platform to ImageDecoder
Platform is the wrong place for screenColorprofile, where it was
the only method there that did not know its Widget. ImageDecoder
is the only caller currently, move it there.
No change in behavior, covered by existing layout tests.
BUG=369787
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181978
Patch Set 1 : #Patch Set 2 : RVO: make screenColorProfile return the ColorProfile. #
Total comments: 6
Messages
Total messages: 32 (9 generated)
Patchset #1 (id:1) has been deleted
noel@chromium.org changed reviewers: + dstockwell@chromium.org
https://crbug.com/397321 for the intermittent win_blink_rel accelerated compositing CHECK failures during trys.
Other earlier examples of the failure, from sept 12 http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/26866
Commit bot doesn't see the same failure (looks like the try is misconfigured).
mkwst@chromium.org changed reviewers: + mkwst@chromium.org
mkwst@chromium.org changed reviewers: + mkwst@chromium.org
Drive-by LGTM (you'll still need a platform/ OWNER to stamp the CL). https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... File Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:46: typedef Vector<char> ColorProfile; It looks this type is only used in the OutputDeviceProfile class, is that the case? If so, you might consider scoping the typedef to that class. https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:221: // FIXME: Add optional ICCv4 support and support for multiple monitors. Nit: Is there a bug filed for this FIXME? It'd be nice to link to it if there is one.
Drive-by LGTM (you'll still need a platform/ OWNER to stamp the CL). https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... File Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:46: typedef Vector<char> ColorProfile; It looks this type is only used in the OutputDeviceProfile class, is that the case? If so, you might consider scoping the typedef to that class. https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:221: // FIXME: Add optional ICCv4 support and support for multiple monitors. Nit: Is there a bug filed for this FIXME? It'd be nice to link to it if there is one.
https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... File Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:46: typedef Vector<char> ColorProfile; On 2014/09/15 07:12:16, Mike West wrote: > It looks this type is only used in the OutputDeviceProfile class, is that the > case? If so, you might consider scoping the typedef to that class. Yeah, it's needed in (JPEG|PNG|WEBP)ImageDecoder files also. https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:221: // FIXME: Add optional ICCv4 support and support for multiple monitors. https://crbug.com/353818 and I know the bug well. I might link to it in a future CL, or I might just remove this code once it's fixed.
On 2014/09/15 07:12:17, Mike West wrote: > Drive-by LGTM (you'll still need a platform/ OWNER to stamp the CL). I'd normally ping tkent, but he is overloaded atm.
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/570023002/40001
The CQ bit was unchecked by jochen@chromium.org
jochen@chromium.org changed reviewers: + jochen@chromium.org
please wait for am owner to approve the patch. a drive by review like Mike's will help to accelerate to process, but it's not OK to just skip it
Doug is an owner.
On 2014/09/15 at 08:13:46, noel wrote: > Doug is an owner. Sorry, I'm not a Source/platform OWNER, only Source/core.
On 2014/09/15 08:31:29, dstockwell wrote: > On 2014/09/15 at 08:13:46, noel wrote: > > Doug is an owner. > > Sorry, I'm not a Source/platform OWNER, only Source/core. Apologies, my misunderstanding.
noel@chromium.org changed reviewers: - dstockwell@chromium.org
Trying blink_win_try again, pinged the gardener about the blink_win_try issue https://crbug.com/397321
https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... File Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:226: colorProfile.append(profile.data(), profile.size()); Why not just use the WebVector<>? You now copy the data twice.
https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... File Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:226: colorProfile.append(profile.data(), profile.size()); On 2014/09/15 12:46:02, jochen wrote: > Why not just use the WebVector<>? You now copy the data twice. The data was copied twice before this change (refer to PlatformScreen.cpp), unless I'm misunderstadning you.
On 2014/09/15 at 12:55:53, noel wrote: > https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... > File Source/platform/image-decoders/ImageDecoder.h (right): > > https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... > Source/platform/image-decoders/ImageDecoder.h:226: colorProfile.append(profile.data(), profile.size()); > On 2014/09/15 12:46:02, jochen wrote: > > Why not just use the WebVector<>? You now copy the data twice. > > The data was copied twice before this change (refer to PlatformScreen.cpp), unless I'm misunderstadning you. yes, but why not fix that?
On 2014/09/15 12:57:41, jochen wrote: > On 2014/09/15 at 12:55:53, noel wrote: > > > https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... > > File Source/platform/image-decoders/ImageDecoder.h (right): > > > > > https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... > > Source/platform/image-decoders/ImageDecoder.h:226: > colorProfile.append(profile.data(), profile.size()); > > On 2014/09/15 12:46:02, jochen wrote: > > > Why not just use the WebVector<>? You now copy the data twice. > > > > The data was copied twice before this change (refer to PlatformScreen.cpp), > unless I'm misunderstadning you. > > yes, but why not fix that? Not really the subject of this patch, but this copy has not been seen as a problem before. Is it a problem? On recall on https://codereview.chromium.org/261633002/ I ended up having to copy this data ~4 times.
On 2014/09/15 at 13:20:46, noel wrote: > On 2014/09/15 12:57:41, jochen wrote: > > On 2014/09/15 at 12:55:53, noel wrote: > > > > > https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... > > > File Source/platform/image-decoders/ImageDecoder.h (right): > > > > > > > > https://codereview.chromium.org/570023002/diff/40001/Source/platform/image-de... > > > Source/platform/image-decoders/ImageDecoder.h:226: > > colorProfile.append(profile.data(), profile.size()); > > > On 2014/09/15 12:46:02, jochen wrote: > > > > Why not just use the WebVector<>? You now copy the data twice. > > > > > > The data was copied twice before this change (refer to PlatformScreen.cpp), > > unless I'm misunderstadning you. > > > > yes, but why not fix that? > > Not really the subject of this patch, but this copy has not been seen as a problem before. Is it a problem? On recall on https://codereview.chromium.org/261633002/ I ended up having to copy this data ~4 times. can you clean this up in a follow up patch? Just use a WebVector<> everywhere. this patch lgtm
On 2014/09/15 14:09:04, jochen wrote: > can you clean this up in a follow up patch? Just use a WebVector<> everywhere. Sure I can. The JPEGImageDecoder / PNGImageDecoder break if I typedef blink::WebVector<> ColorProfile. Should be easy to fix, provided use of WebVector<> deeper in platform/graphics is ok with everyone? > this patch lgtm Thanks.
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/570023002/40001
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as 181978
Message was sent while issue was closed.
On 2014/09/15 14:14:29, Noel Gordon wrote: > On 2014/09/15 14:09:04, jochen wrote: > > > can you clean this up in a follow up patch? Just use a WebVector<> everywhere. > > Sure I can. The JPEGImageDecoder / PNGImageDecoder break if I typedef > blink::WebVector<> ColorProfile. > > Should be easy to fix, provided use of WebVector<> deeper in platform/graphics > is ok with everyone? Filed https://crbug.com/414282 about that. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
