Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10448110:
Adds monitor ICC profile support for win and mac (Closed)
|
Created:
8 years, 6 months ago by tpayne Modified:
7 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, jochen+watch-content_chromium.org, noel.gordon_gmail.com, sky, vandebo (ex-Chrome) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdds monitor ICC profile support for win and mac
BUG=143
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147361
Patch Set 1 #Patch Set 2 : Add missing files, fix file locations for qcms.gyp #Patch Set 3 : Fix windows compile #Patch Set 4 : Fix Windows compile #
Total comments: 1
Patch Set 5 : Rebased, work in progress #Patch Set 6 : Switched to using WebVector #Patch Set 7 : Updated API #Patch Set 8 : Cleanup #Patch Set 9 : Added HWND and type to API #Patch Set 10 : Fixed exit time destructors #Patch Set 11 : Fix win/mac compile errors #Patch Set 12 : Fix Windows lak #
Total comments: 16
Patch Set 13 : Rebased #Patch Set 14 : Addressed comments #Patch Set 15 : Missed one hwnd reference #Patch Set 16 : Missed one hwnd reference #
Total comments: 10
Patch Set 17 : Addressed comments #Patch Set 18 : Using file_util #Patch Set 19 : Trying again... #
Total comments: 5
Patch Set 20 : Added comment #
Total comments: 6
Patch Set 21 : Addressed Comments #
Total comments: 2
Patch Set 22 : Renamed reply_msg to rofile #
Total comments: 3
Patch Set 23 : Fix reading length from small string #
Total comments: 33
Patch Set 24 : Addressed feedback #
Total comments: 10
Patch Set 25 : Addressed Comments #Patch Set 26 : Removed parent_window from API, removed unused headers #Patch Set 27 : One more unused header #
Total comments: 6
Patch Set 28 : Addressed comments. Don't double-correct when --enable-monitor-profile present. #Patch Set 29 : Moved check for enable-monitor-profile to content #Patch Set 30 : Cleanup bad merge #
Total comments: 9
Patch Set 31 : Addressed sky's comments #Patch Set 32 : Fix win compile #Patch Set 33 : Fix android build #
Total comments: 1
Messages
Total messages: 60 (0 generated)
http://codereview.chromium.org/10448110/diff/5001/content/browser/renderer_ho... File content/browser/renderer_host/render_message_filter.cc (right): http://codereview.chromium.org/10448110/diff/5001/content/browser/renderer_ho... content/browser/renderer_host/render_message_filter.cc:871: void RenderMessageFilter::OnAsyncOpenFile(const IPC::Message& msg, here's an example of using the file thread
Ready for review
http://codereview.chromium.org/10448110/diff/21004/content/browser/renderer_h... File content/browser/renderer_host/render_message_filter.cc (right): http://codereview.chromium.org/10448110/diff/21004/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.cc:724: gfx::NativeViewId parent_hwnd, hwnd... ew. Please call it something platform-agnostic. http://codereview.chromium.org/10448110/diff/21004/content/browser/renderer_h... File content/browser/renderer_host/render_message_filter.h (right): http://codereview.chromium.org/10448110/diff/21004/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.h:85: virtual void OverrideThreadForMessage( this needs to go next to other OVERRIDEs from the same super class/interface. http://codereview.chromium.org/10448110/diff/21004/content/common/view_messag... File content/common/view_messages.h (right): http://codereview.chromium.org/10448110/diff/21004/content/common/view_messag... content/common/view_messages.h:754: IPC_SYNC_MESSAGE_CONTROL2_1(ViewHostMsg_GetMonitorColorProfile, I don't know how kosher it is to have a sync IPC message that blocks on a file read. Doesn't that mean that a slow disk would delay a page load? http://codereview.chromium.org/10448110/diff/21004/ui/gfx/color_profile.h File ui/gfx/color_profile.h (right): http://codereview.chromium.org/10448110/diff/21004/ui/gfx/color_profile.h#new... ui/gfx/color_profile.h:21: protected: no need http://codereview.chromium.org/10448110/diff/21004/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/21004/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:37: BOOL res = GetICMProfile(screen_dc, &path_len, reinterpret_cast<LPWSTR>(&path)); 80 http://codereview.chromium.org/10448110/diff/21004/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:57: } no curlies
http://codereview.chromium.org/10448110/diff/21004/content/common/view_messag... File content/common/view_messages.h (right): http://codereview.chromium.org/10448110/diff/21004/content/common/view_messag... content/common/view_messages.h:754: IPC_SYNC_MESSAGE_CONTROL2_1(ViewHostMsg_GetMonitorColorProfile, On 2012/06/12 00:48:24, Evan Stade wrote: > I don't know how kosher it is to have a sync IPC message that blocks on a file > read. Doesn't that mean that a slow disk would delay a page load? Adam, Tony, can one of you guys comment on whether this is a problem? My original plan was to load the file before renderer startup and provide the data in the renderer config struct. Because not all images require color correction, Tony suggested lazy-loading the profile.
> Adam, Tony, can one of you guys comment on whether this is a problem? You should avoid sync IPCs from the render to the browser. Blocking the entire render on talking to the disk is a bad idea. > My original plan was to load the file before renderer startup and provide the > data in the renderer config struct. Because not all images require color > correction, Tony suggested lazy-loading the profile. You certainly should not add a disk read that block startup. What we do commonly is kick off an async read some amount of time after startup and then push the information to renderers when that read completes. That means the browser will not have the optimal color profile for a moment when starting up, but hopefully that's better than slowing down startup for everyone.
On 2012/06/14 22:27:27, tpayne wrote: > http://codereview.chromium.org/10448110/diff/21004/content/common/view_messag... > File content/common/view_messages.h (right): > > http://codereview.chromium.org/10448110/diff/21004/content/common/view_messag... > content/common/view_messages.h:754: > IPC_SYNC_MESSAGE_CONTROL2_1(ViewHostMsg_GetMonitorColorProfile, > On 2012/06/12 00:48:24, Evan Stade wrote: > > I don't know how kosher it is to have a sync IPC message that blocks on a file > > read. Doesn't that mean that a slow disk would delay a page load? > > Adam, Tony, can one of you guys comment on whether this is a problem? > > My original plan was to load the file before renderer startup and provide the > data in the renderer config struct. Because not all images require color > correction, Tony suggested lazy-loading the profile. Slowing down renderer startup is worse since most renderers won't need to pay this cost. It's only the tab that hangs. We already do this in other cases like fetching cookies or large blobs of data from the clipboard.
On 2012/06/14 22:37:54, abarth wrote: > > Adam, Tony, can one of you guys comment on whether this is a problem? > > You should avoid sync IPCs from the render to the browser. Blocking the entire > render on talking to the disk is a bad idea. > > > My original plan was to load the file before renderer startup and provide the > > data in the renderer config struct. Because not all images require color > > correction, Tony suggested lazy-loading the profile. > > You certainly should not add a disk read that block startup. What we do > commonly is kick off an async read some amount of time after startup and then > push the information to renderers when that read completes. That means the > browser will not have the optimal color profile for a moment when starting up, > but hopefully that's better than slowing down startup for everyone. We could also kick off an async read some amount of time after startup and just hold it in memory in the browser process. When the renderer process needs the info, we return the data from memory. If we get a request before it's loaded, the images will not have an optimal color profile. I'm not sure this is much better than just reading from disk the first time and keeping it in memory. Odds are if you're not using the data it'll get swapped to disk and it'll be just as slow.
http://codereview.chromium.org/10448110/diff/21004/content/browser/renderer_h... File content/browser/renderer_host/render_message_filter.cc (right): http://codereview.chromium.org/10448110/diff/21004/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.cc:728: gfx::ColorProfile::GetColorProfile(parent_hwnd, &profile); We probably want to cache this in the browser process so you don't have to re-read the file for every renderer process.
http://codereview.chromium.org/10448110/diff/21004/content/browser/renderer_h... File content/browser/renderer_host/render_message_filter.cc (right): http://codereview.chromium.org/10448110/diff/21004/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.cc:728: gfx::ColorProfile::GetColorProfile(parent_hwnd, &profile); On 2012/06/14 22:56:36, tony wrote: > We probably want to cache this in the browser process so you don't have to > re-read the file for every renderer process. I do, one level down (in gfx::ColorProfile::GetColorProfile
http://codereview.chromium.org/10448110/diff/21004/ui/gfx/color_profile.cc File ui/gfx/color_profile.cc (right): http://codereview.chromium.org/10448110/diff/21004/ui/gfx/color_profile.cc#ne... ui/gfx/color_profile.cc:13: *new std::map<gfx::NativeViewId, std::vector<char> >(); Nit: Can you use CR_DEFINE_STATIC_LOCAL?
http://codereview.chromium.org/10448110/diff/21004/ui/gfx/color_profile.cc File ui/gfx/color_profile.cc (right): http://codereview.chromium.org/10448110/diff/21004/ui/gfx/color_profile.cc#ne... ui/gfx/color_profile.cc:13: *new std::map<gfx::NativeViewId, std::vector<char> >(); On 2012/06/14 23:18:10, tony wrote: > Nit: Can you use CR_DEFINE_STATIC_LOCAL? I couldn't get that to parse correctly because of the comma in the template type params.
http://codereview.chromium.org/10448110/diff/21004/content/browser/renderer_h... File content/browser/renderer_host/render_message_filter.cc (right): http://codereview.chromium.org/10448110/diff/21004/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.cc:724: gfx::NativeViewId parent_hwnd, On 2012/06/12 00:48:24, Evan Stade wrote: > hwnd... ew. Please call it something platform-agnostic. Done. http://codereview.chromium.org/10448110/diff/21004/content/browser/renderer_h... File content/browser/renderer_host/render_message_filter.h (right): http://codereview.chromium.org/10448110/diff/21004/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.h:85: virtual void OverrideThreadForMessage( On 2012/06/12 00:48:24, Evan Stade wrote: > this needs to go next to other OVERRIDEs from the same super class/interface. Done. http://codereview.chromium.org/10448110/diff/21004/ui/gfx/color_profile.h File ui/gfx/color_profile.h (right): http://codereview.chromium.org/10448110/diff/21004/ui/gfx/color_profile.h#new... ui/gfx/color_profile.h:21: protected: On 2012/06/12 00:48:24, Evan Stade wrote: > no need Done. http://codereview.chromium.org/10448110/diff/21004/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/21004/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:37: BOOL res = GetICMProfile(screen_dc, &path_len, reinterpret_cast<LPWSTR>(&path)); On 2012/06/12 00:48:24, Evan Stade wrote: > 80 Done. http://codereview.chromium.org/10448110/diff/21004/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:57: } On 2012/06/12 00:48:24, Evan Stade wrote: > no curlies Done.
LGTM. Some questions/comments below. I don't need to see this again before it lands. Thanks! http://codereview.chromium.org/10448110/diff/46001/content/renderer/renderer_... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10448110/diff/46001/content/renderer/renderer_... content/renderer/renderer_webkitplatformsupport_impl.cc:669: *toProfile = profile; Is this information cached in WebCore? I seem to remember that we only read this information when we start up the image decoders. When you land this patch, you should watch the perf bots to see if this causes a startup time regression. http://codereview.chromium.org/10448110/diff/46001/ui/gfx/color_profile.cc File ui/gfx/color_profile.cc (right): http://codereview.chromium.org/10448110/diff/46001/ui/gfx/color_profile.cc#ne... ui/gfx/color_profile.cc:11: // Force data to leak so exit time destructors are not called. It's interesting that this code is called on different threads in different ports. I wonder if that's worth noting somehow. One way to do that is with a comment. Another option is an ASSERT that shows that locks in the thread on the first call. I mention that mostly because this code isn't threadsafe. http://codereview.chromium.org/10448110/diff/46001/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/46001/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:31: std::vector<char>* profile) { Should we ASSERT that we're on the FILE thread here? Maybe we do that automatically already.
http://codereview.chromium.org/10448110/diff/46001/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/46001/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:31: std::vector<char>* profile) { On 2012/06/19 20:36:50, abarth wrote: > Should we ASSERT that we're on the FILE thread here? Maybe we do that > automatically already. yea, I think it happens automatically with browser thread restrictions stuff (and the code at this level doesn't know about the different browser threads so it wouldn't be possible to check here)
http://codereview.chromium.org/10448110/diff/46001/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/46001/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:31: std::vector<char>* profile) { On 2012/06/19 21:08:56, Evan Stade wrote: > On 2012/06/19 20:36:50, abarth wrote: > > Should we ASSERT that we're on the FILE thread here? Maybe we do that > > automatically already. > > yea, I think it happens automatically with browser thread restrictions stuff > (and the code at this level doesn't know about the different browser threads so > it wouldn't be possible to check here) nix this comment ^ use file_util::ReadFileToString?
I'll make the change to use file_util soon. http://codereview.chromium.org/10448110/diff/46001/content/renderer/renderer_... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10448110/diff/46001/content/renderer/renderer_... content/renderer/renderer_webkitplatformsupport_impl.cc:669: *toProfile = profile; On 2012/06/19 20:36:50, abarth wrote: > Is this information cached in WebCore? I seem to remember that we only read > this information when we start up the image decoders. Yes, it is cached in WebCore. We only read it the first time we decode an image with an embedded ICC profile. > When you land this patch, you should watch the perf bots to see if this causes a > startup time regression. http://codereview.chromium.org/10448110/diff/46001/ui/gfx/color_profile.cc File ui/gfx/color_profile.cc (right): http://codereview.chromium.org/10448110/diff/46001/ui/gfx/color_profile.cc#ne... ui/gfx/color_profile.cc:11: // Force data to leak so exit time destructors are not called. On 2012/06/19 20:36:50, abarth wrote: > It's interesting that this code is called on different threads in different > ports. I wonder if that's worth noting somehow. One way to do that is with a > comment. Another option is an ASSERT that shows that locks in the thread on the > first call. I mention that mostly because this code isn't threadsafe. Done. http://codereview.chromium.org/10448110/diff/46001/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/46001/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:31: std::vector<char>* profile) { On 2012/06/19 20:36:50, abarth wrote: > Should we ASSERT that we're on the FILE thread here? Maybe we do that > automatically already. Done.
http://codereview.chromium.org/10448110/diff/46001/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/46001/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:31: std::vector<char>* profile) { On 2012/06/19 20:36:50, abarth wrote: > Should we ASSERT that we're on the FILE thread here? Maybe we do that > automatically already. file_util::ReadFileAsString already does this. http://codereview.chromium.org/10448110/diff/46001/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:31: std::vector<char>* profile) { On 2012/06/19 21:32:38, Evan Stade wrote: > On 2012/06/19 21:08:56, Evan Stade wrote: > > On 2012/06/19 20:36:50, abarth wrote: > > > Should we ASSERT that we're on the FILE thread here? Maybe we do that > > > automatically already. > > > > yea, I think it happens automatically with browser thread restrictions stuff > > (and the code at this level doesn't know about the different browser threads > so > > it wouldn't be possible to check here) > > nix this comment ^ > > use file_util::ReadFileToString? Actually done this time.
your latest patch upload failed (I can't view the diffs). Please try again.
On 2012/06/20 02:09:12, Evan Stade wrote: > your latest patch upload failed (I can't view the diffs). Please try again. Done.
lgtm http://codereview.chromium.org/10448110/diff/33021/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/33021/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:18: size_t ReadLengthFromString(const std::string& data) { comment this function http://codereview.chromium.org/10448110/diff/33021/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:45: if (profileData.size() > MAX_PROFILE_LENGTH) put this check with the rest of the length checks below?
http://codereview.chromium.org/10448110/diff/33021/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/33021/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:18: size_t ReadLengthFromString(const std::string& data) { On 2012/06/20 02:41:47, Evan Stade wrote: > comment this function Done. http://codereview.chromium.org/10448110/diff/33021/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:45: if (profileData.size() > MAX_PROFILE_LENGTH) On 2012/06/20 02:41:47, Evan Stade wrote: > put this check with the rest of the length checks below? I'd prefer to bail out as soon as possible. No need to read the length from the string in this case.
http://codereview.chromium.org/10448110/diff/33021/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/33021/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:45: if (profileData.size() > MAX_PROFILE_LENGTH) On 2012/06/20 02:46:54, tpayne wrote: > On 2012/06/20 02:41:47, Evan Stade wrote: > > put this check with the rest of the length checks below? > > I'd prefer to bail out as soon as possible. No need to read the length from the > string in this case. there's a slight difference: you're effectively making MAX_PROFILE_LENGTH 4 bytes less. But the reason I suggested it is because it's easier to see what's happening if you keep all length checks together.
http://codereview.chromium.org/10448110/diff/43003/content/browser/renderer_h... File content/browser/renderer_host/render_message_filter.cc (right): http://codereview.chromium.org/10448110/diff/43003/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.cc:394: IPC_MESSAGE_HANDLER_DELAY_REPLY(ViewHostMsg_GetMonitorColorProfile, no need to use IPC_MESSAGE_HANDLER_DELAY_REPLY, that's only for when you don't have the output parameters by the time that the dispatcher function returns (i.e. because you're waiting on an async operation) http://codereview.chromium.org/10448110/diff/43003/content/renderer/renderer_... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10448110/diff/43003/content/renderer/renderer_... content/renderer/renderer_webkitplatformsupport_impl.cc:663: WebVector<char>* toProfile) { nit: to_profile http://codereview.chromium.org/10448110/diff/43003/content/renderer/renderer_... File content/renderer/renderer_webkitplatformsupport_impl.h (right): http://codereview.chromium.org/10448110/diff/43003/content/renderer/renderer_... content/renderer/renderer_webkitplatformsupport_impl.h:68: WebKit::WebVector<char>*) OVERRIDE; nit: chrome style is to name parameters in the .h
http://codereview.chromium.org/10448110/diff/43003/content/browser/renderer_h... File content/browser/renderer_host/render_message_filter.cc (right): http://codereview.chromium.org/10448110/diff/43003/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.cc:394: IPC_MESSAGE_HANDLER_DELAY_REPLY(ViewHostMsg_GetMonitorColorProfile, On 2012/06/20 16:21:21, John Abd-El-Malek wrote: > no need to use IPC_MESSAGE_HANDLER_DELAY_REPLY, that's only for when you don't > have the output parameters by the time that the dispatcher function returns > (i.e. because you're waiting on an async operation) Done. http://codereview.chromium.org/10448110/diff/43003/content/renderer/renderer_... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10448110/diff/43003/content/renderer/renderer_... content/renderer/renderer_webkitplatformsupport_impl.cc:663: WebVector<char>* toProfile) { On 2012/06/20 16:21:21, John Abd-El-Malek wrote: > nit: to_profile Done. http://codereview.chromium.org/10448110/diff/43003/content/renderer/renderer_... File content/renderer/renderer_webkitplatformsupport_impl.h (right): http://codereview.chromium.org/10448110/diff/43003/content/renderer/renderer_... content/renderer/renderer_webkitplatformsupport_impl.h:68: WebKit::WebVector<char>*) OVERRIDE; On 2012/06/20 16:21:21, John Abd-El-Malek wrote: > nit: chrome style is to name parameters in the .h Done.
lgtm http://codereview.chromium.org/10448110/diff/49002/content/browser/renderer_h... File content/browser/renderer_host/render_message_filter.h (right): http://codereview.chromium.org/10448110/diff/49002/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.h:192: std::vector<char>* reply_msg); nit: reply_msg->profile
http://codereview.chromium.org/10448110/diff/49002/content/browser/renderer_h... File content/browser/renderer_host/render_message_filter.h (right): http://codereview.chromium.org/10448110/diff/49002/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.h:192: std::vector<char>* reply_msg); On 2012/06/20 17:34:01, John Abd-El-Malek wrote: > nit: reply_msg->profile Done.
http://codereview.chromium.org/10448110/diff/31048/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/31048/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:23: memcpy(&length, data.data(), sizeof(length)); this will be an error if data.empty()
http://codereview.chromium.org/10448110/diff/31048/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/31048/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:23: memcpy(&length, data.data(), sizeof(length)); On 2012/06/20 21:10:51, Evan Stade wrote: > this will be an error if data.empty() or if size < sizeof(uint32)
http://codereview.chromium.org/10448110/diff/31048/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/31048/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:23: memcpy(&length, data.data(), sizeof(length)); On 2012/06/20 21:11:30, Evan Stade wrote: > On 2012/06/20 21:10:51, Evan Stade wrote: > > this will be an error if data.empty() > > or if size < sizeof(uint32) Good catch. Done.
lgtm
Sky@ or ben@, I need someone from src/ui/OWNERS to review this CL as well.
http://codereview.chromium.org/10448110/diff/42005/content/browser/renderer_h... File content/browser/renderer_host/render_message_filter.cc (right): http://codereview.chromium.org/10448110/diff/42005/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.cc:410: // Windows monitor profile must be read from a file. Well the fact is really that "color profiles must be read from files on Windows", whether they be monitor profiles or not. http://codereview.chromium.org/10448110/diff/42005/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.cc:727: gfx::ColorProfile::GetColorProfile(parent_window, profile); |type| is silently dropped here. // TODO(you) pass all parameters through to gfx::ColorProfile. as a reminder? http://codereview.chromium.org/10448110/diff/42005/content/browser/renderer_h... File content/browser/renderer_host/render_message_filter.h (right): http://codereview.chromium.org/10448110/diff/42005/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.h:189: // Used to look up the monitor color profile for a window or a named profile. // Return the monitor color profile of a window or a named profile. nit: and regardless, the comment suggests to me that |name| would be a more accurate name for this variable than |type|, and yes, that would require changing type -> name in a few other places in this patch. http://codereview.chromium.org/10448110/diff/42005/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.h:190: void OnGetMonitorColorProfile(gfx::NativeViewId parent_window, OnGetMonitorColorProfile? We're somewhat fixated with monitor profiles here. How about OnGetSystemColorProfile? Adam mentioned this on the webkit bug and I don't think his comments were addressed. http://codereview.chromium.org/10448110/diff/42005/content/renderer/renderer_... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10448110/diff/42005/content/renderer/renderer_... content/renderer/renderer_webkitplatformsupport_impl.cc:666: std::string type_str(type.utf8()); nit: you could ditch this line ... http://codereview.chromium.org/10448110/diff/42005/content/renderer/renderer_... content/renderer/renderer_webkitplatformsupport_impl.cc:668: view_id, type_str, &profile)); ... and here just write view_id, std::string(type.utf8()), &profile)); http://codereview.chromium.org/10448110/diff/42005/third_party/qcms/qcms.gyp File third_party/qcms/qcms.gyp (right): http://codereview.chromium.org/10448110/diff/42005/third_party/qcms/qcms.gyp#... third_party/qcms/qcms.gyp:47: ['OS == "win"', { These gyp changes seems unrelated to the current patch. Would it make sense to cut it out and submit it separately? Though, I might be misunderstanding why it's included in this patch. http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile.cc File ui/gfx/color_profile.cc (right): http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile.cc#ne... ui/gfx/color_profile.cc:17: You could add a DCHECK(!parent_window) here, because it is always 0 currently. General design issues: let's just imagine you could get the actual HWND. Chrome creates a new HWND for each renderer and those HWND are unique. But this data map<> is keyed on HWND so it could grow quite large, no? But maybe |parent_window| is the browser HWND? (It's not clear to me from reading the code). Smaller data map ok, but still not ideal. Start chrome and tear off a tab, and use Visual Studio Spy++ to examine the relationships b/w browser HWND and their renderer HWND. Do that, and I think you'll resolve the questions you're yet to tackle: which HWND should it be, does it really matter, and why should WebCore care? Anyho, those are just design issues to think about. Currently |parent_window| is always 0. If we get to the point where it's not, then a map<HWND, std::vector> > won't really cut it. http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_linux.cc File ui/gfx/color_profile_linux.cc (right): http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_linux... ui/gfx/color_profile_linux.cc:12: } For reference http://mxr.mozilla.org/mozilla-aurora/source/gfx/thebes/gfxPlatformGtk.cpp#486 gfxPlatformGtk::GetPlatformCMSOutputProfile() shows hows to read the ICC ATOM. http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_mac.cc File ui/gfx/color_profile_mac.cc (right): http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_mac.c... ui/gfx/color_profile_mac.cc:16: size_t length = CFDataGetLength(icc_profile); In the windows implementation you check lengths, viz., MAX_PROFILE_LENGTH etc. Here you accept the system profile length. How come? Does CGColorSpaceCopyICCProfile vet the profile somehow? Is it trustworthy? http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:14: #define MAX_PROFILE_LENGTH (4 * 1000 * 1000) I don't see how this #define is particular to win32. How about we move this into "ui/gfx/color_profile.h". While you're are at, #define MIN_PROFILE_LENGTH (128) in the same file. http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:20: size_t ReadLengthFromString(const std::string& data) { Nice comment, but this function has really confused matters below in ReadProfile() and introduced bugs. It's not helping and I'd remove it. http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:51: return; I'd add an additional clause here: if (profileData.size() < MIN_PROFILE_LENGTH) return; http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:53: if (length == 0 || length + sizeof(uint32) > profileData.size()) So maybe length + sizeof(uint32) could be less than than profile Data.size() and you'd accept it. Reading the length off the profile header seems fraught and best left to the qcms color profile library in my opinion, and ... http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:55: const char* dataBuffer = profileData.data() + sizeof(uint32); ... as your comment @ line 18 said, the length is in the profile header. Here you skip dataBuffer past the length (4 bytes) and then write |length| bytes via profile->assign(). Yikes! If you don't provide the length within the returned color profile data, how would the qcms library read it? It sure wants to. The ICC profile header is 128 bytes, length included I believe. (Please go check). And I'd hope that either qcms_profile_from_memory() or qcms_profile_is_bogus() in WebCore fail with the data you are returning to it here. (Again, please check). Not sure how you tested this code.
Holding off on addressing the other comments until we get closure on the API, because I think it's causing most of the problems here. And... I'm still going to need src/ui owners approval. I'm out Mon-Wed next week and am aiming to wrap this up on Thu/Fri next week, while all of US is on vacation, so I'd appreciate if I could get the approval from sky@ or ben@ this week. http://codereview.chromium.org/10448110/diff/42005/content/browser/renderer_h... File content/browser/renderer_host/render_message_filter.cc (right): http://codereview.chromium.org/10448110/diff/42005/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.cc:727: gfx::ColorProfile::GetColorProfile(parent_window, profile); On 2012/06/21 16:58:33, noel chromium wrote: > |type| is silently dropped here. > > // TODO(you) pass all parameters through to gfx::ColorProfile. > > as a reminder? The more I think about this, the more I'm certain we've screwed up by trying to meld the monitor profile and named profile getters into one API. Things would be much cleaner and clearer if there was a separate API for getting a named profile. If you're okay with that, I'd like to update the WK change and then return to this CL.
> If you're okay with that, I'd like to update the WK change ... I'm fine with named profile on a separate api. And since webkit doesn't use them for now, the changes on the webkit-side should be simple.
http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile.h File ui/gfx/color_profile.h (right): http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile.h#new... ui/gfx/color_profile.h:18: class UI_EXPORT ColorProfile { Can these just be functions in the gfx namespace rather than in this ColorProfile class, which never seems to have an instance?
http://codereview.chromium.org/10448110/diff/42005/content/browser/renderer_h... File content/browser/renderer_host/render_message_filter.cc (right): http://codereview.chromium.org/10448110/diff/42005/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.cc:410: // Windows monitor profile must be read from a file. On 2012/06/21 16:58:33, noel chromium wrote: > Well the fact is really that "color profiles must be read from files on > Windows", whether they be monitor profiles or not. Now that this call is only for the monitor profile, the comment should be okay as is. http://codereview.chromium.org/10448110/diff/42005/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.cc:727: gfx::ColorProfile::GetColorProfile(parent_window, profile); On 2012/06/29 00:25:16, tpayne wrote: > On 2012/06/21 16:58:33, noel chromium wrote: > > |type| is silently dropped here. > > > > // TODO(you) pass all parameters through to gfx::ColorProfile. > > > > as a reminder? > > The more I think about this, the more I'm certain we've screwed up by trying to > meld the monitor profile and named profile getters into one API. Things would be > much cleaner and clearer if there was a separate API for getting a named > profile. > > If you're okay with that, I'd like to update the WK change and then return to > this CL. This is now done. I think this name should be good, now. http://codereview.chromium.org/10448110/diff/42005/content/browser/renderer_h... File content/browser/renderer_host/render_message_filter.h (right): http://codereview.chromium.org/10448110/diff/42005/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.h:189: // Used to look up the monitor color profile for a window or a named profile. On 2012/06/21 16:58:33, noel chromium wrote: > // Return the monitor color profile of a window or a named profile. > > nit: and regardless, the comment suggests to me that |name| would be a more > accurate name for this variable than |type|, and yes, that would require > changing type -> name in a few other places in this patch. N/A. http://codereview.chromium.org/10448110/diff/42005/content/browser/renderer_h... content/browser/renderer_host/render_message_filter.h:190: void OnGetMonitorColorProfile(gfx::NativeViewId parent_window, On 2012/06/21 16:58:33, noel chromium wrote: > OnGetMonitorColorProfile? We're somewhat fixated with monitor profiles here. > > How about OnGetSystemColorProfile? Adam mentioned this on the webkit bug and I > don't think his comments were addressed. N/A http://codereview.chromium.org/10448110/diff/42005/content/renderer/renderer_... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): http://codereview.chromium.org/10448110/diff/42005/content/renderer/renderer_... content/renderer/renderer_webkitplatformsupport_impl.cc:666: std::string type_str(type.utf8()); On 2012/06/21 16:58:33, noel chromium wrote: > nit: you could ditch this line ... N/A http://codereview.chromium.org/10448110/diff/42005/content/renderer/renderer_... content/renderer/renderer_webkitplatformsupport_impl.cc:668: view_id, type_str, &profile)); On 2012/06/21 16:58:33, noel chromium wrote: > ... and here just write > > view_id, std::string(type.utf8()), &profile)); N/A http://codereview.chromium.org/10448110/diff/42005/third_party/qcms/qcms.gyp File third_party/qcms/qcms.gyp (right): http://codereview.chromium.org/10448110/diff/42005/third_party/qcms/qcms.gyp#... third_party/qcms/qcms.gyp:47: ['OS == "win"', { On 2012/06/21 16:58:33, noel chromium wrote: > These gyp changes seems unrelated to the current patch. Would it make sense to > cut it out and submit it separately? Though, I might be misunderstanding why > it's included in this patch. Done. http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile.cc File ui/gfx/color_profile.cc (right): http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile.cc#ne... ui/gfx/color_profile.cc:17: On 2012/06/21 16:58:33, noel chromium wrote: > You could add a DCHECK(!parent_window) here, because it is always 0 currently. > > General design issues: let's just imagine you could get the actual HWND. Chrome > creates a new HWND for each renderer and those HWND are unique. But this data > map<> is keyed on HWND so it could grow quite large, no? > > But maybe |parent_window| is the browser HWND? (It's not clear to me from > reading the code). Smaller data map ok, but still not ideal. > > Start chrome and tear off a tab, and use Visual Studio Spy++ to examine the > relationships b/w browser HWND and their renderer HWND. Do that, and I think > you'll resolve the questions you're yet to tackle: which HWND should it be, does > it really matter, and why should WebCore care? > > Anyho, those are just design issues to think about. Currently |parent_window| > is always 0. If we get to the point where it's not, then a map<HWND, > std::vector> > won't really cut it. Done. http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile.h File ui/gfx/color_profile.h (right): http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile.h#new... ui/gfx/color_profile.h:18: class UI_EXPORT ColorProfile { On 2012/06/29 16:11:40, Ben Goodger (Google) wrote: > Can these just be functions in the gfx namespace rather than in this > ColorProfile class, which never seems to have an instance? Done. http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_linux.cc File ui/gfx/color_profile_linux.cc (right): http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_linux... ui/gfx/color_profile_linux.cc:12: } On 2012/06/21 16:58:33, noel chromium wrote: > For reference > http://mxr.mozilla.org/mozilla-aurora/source/gfx/thebes/gfxPlatformGtk.cpp#486 > > gfxPlatformGtk::GetPlatformCMSOutputProfile() shows hows to read the ICC ATOM. Thanks http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_mac.cc File ui/gfx/color_profile_mac.cc (right): http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_mac.c... ui/gfx/color_profile_mac.cc:16: size_t length = CFDataGetLength(icc_profile); On 2012/06/21 16:58:33, noel chromium wrote: > In the windows implementation you check lengths, viz., MAX_PROFILE_LENGTH etc. > Here you accept the system profile length. How come? Does > CGColorSpaceCopyICCProfile vet the profile somehow? Is it trustworthy? Done. http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:14: #define MAX_PROFILE_LENGTH (4 * 1000 * 1000) On 2012/06/21 16:58:33, noel chromium wrote: > I don't see how this #define is particular to win32. How about we move this into > "ui/gfx/color_profile.h". > > While you're are at, #define MIN_PROFILE_LENGTH (128) in the same file. Done. http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:20: size_t ReadLengthFromString(const std::string& data) { On 2012/06/21 16:58:33, noel chromium wrote: > Nice comment, but this function has really confused matters below in > ReadProfile() and introduced bugs. It's not helping and I'd remove it. Done. http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:51: return; On 2012/06/21 16:58:33, noel chromium wrote: > I'd add an additional clause here: > > if (profileData.size() < MIN_PROFILE_LENGTH) > return; Done. http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:53: if (length == 0 || length + sizeof(uint32) > profileData.size()) On 2012/06/21 16:58:33, noel chromium wrote: > So maybe length + sizeof(uint32) could be less than than profile Data.size() and > you'd accept it. Reading the length off the profile header seems fraught and > best left to the qcms color profile library in my opinion, and ... Done. http://codereview.chromium.org/10448110/diff/42005/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:55: const char* dataBuffer = profileData.data() + sizeof(uint32); On 2012/06/21 16:58:33, noel chromium wrote: > ... as your comment @ line 18 said, the length is in the profile header. Here > you skip dataBuffer past the length (4 bytes) and then write |length| bytes via > profile->assign(). Yikes! > > If you don't provide the length within the returned color profile data, how > would the qcms library read it? It sure wants to. The ICC profile header is > 128 bytes, length included I believe. (Please go check). > > And I'd hope that either qcms_profile_from_memory() or qcms_profile_is_bogus() > in WebCore fail with the data you are returning to it here. (Again, please > check). > Not sure how you tested this code. I haven't figured out a reliable way of testing, mostly because my win debugging fu is inadequate. I ran through the win_rel and win_layout_rel bots and I ran locally, but I was not able to verify if the profile was being used. Clearly, it wasn't so I need a better testing approach.
http://codereview.chromium.org/10448110/diff/66005/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/66005/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:11: #include "base/sys_byteorder.h" This include is not used anymore. http://codereview.chromium.org/10448110/diff/66005/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:20: WCHAR path[MAX_PATH]; MAX_PATH + 1 http://codereview.chromium.org/10448110/diff/66005/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:24: reinterpret_cast<LPWSTR>(&path)); This current code appears to spray the stack with rubbish data. You want to write data into |path| here, but you write into |&path| instead. Please test GetICMProfile(screen_dc, &path_len, path); with your debugger to verify this new code is returning the profile in the path WCHAR variable.
> This current code appears to spray the stack with rubbish data. You want to > write data into |path| here, but you write into |&path| instead. Oh I should note that &path[0] == path == &path for array types only, after C99. The stack is fine. I'd use the more idiomatic |path|, aka GetICMProfile(screen_dc, &path_len, path);
http://codereview.chromium.org/10448110/diff/66005/ui/gfx/color_profile.h File ui/gfx/color_profile.h (right): http://codereview.chromium.org/10448110/diff/66005/ui/gfx/color_profile.h#new... ui/gfx/color_profile.h:15: #define MIN_PROFILE_LENGTH 128 We don't use #defines in chrome code, instead use kOnstants. http://codereview.chromium.org/10448110/diff/66005/ui/gfx/color_profile.h#new... ui/gfx/color_profile.h:21: UI_EXPORT void GetColorProfile(gfx::NativeViewId, std::vector<char>* profile); you never seem to use parent_window here. what's it for? also, why gfx::NativeViewId vs. gfx::NativeView?
On 2012/07/03 14:40:30, noel chromium wrote: > Please test I was able to debug using WinDBG and confirm that the profile is being read from the correct file, is being returned to the renderer, is parsed successfully by qcms, passes the bogocity check and is used in creating the transform. As you mentioned, Noel, DRT is using the null implementation of screenColorProfile which is why we do not see a change in layout test results.
http://codereview.chromium.org/10448110/diff/66005/ui/gfx/color_profile.h File ui/gfx/color_profile.h (right): http://codereview.chromium.org/10448110/diff/66005/ui/gfx/color_profile.h#new... ui/gfx/color_profile.h:15: #define MIN_PROFILE_LENGTH 128 On 2012/07/09 15:56:46, Ben Goodger (Google) wrote: > We don't use #defines in chrome code, instead use kOnstants. Done. http://codereview.chromium.org/10448110/diff/66005/ui/gfx/color_profile.h#new... ui/gfx/color_profile.h:21: UI_EXPORT void GetColorProfile(gfx::NativeViewId, std::vector<char>* profile); On 2012/07/09 15:56:46, Ben Goodger (Google) wrote: > you never seem to use parent_window here. what's it for? It's for future use, to support multiple monitors. > also, why gfx::NativeViewId vs. gfx::NativeView? Well, I'm not sure. This is sent over IPC from the renderer. http://codereview.chromium.org/10448110/diff/66005/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/66005/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:11: #include "base/sys_byteorder.h" On 2012/07/03 14:40:30, noel chromium wrote: > This include is not used anymore. Done. http://codereview.chromium.org/10448110/diff/66005/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:20: WCHAR path[MAX_PATH]; On 2012/07/03 14:40:30, noel chromium wrote: > MAX_PATH + 1 Done. http://codereview.chromium.org/10448110/diff/66005/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:24: reinterpret_cast<LPWSTR>(&path)); On 2012/07/03 14:40:30, noel chromium wrote: > This current code appears to spray the stack with rubbish data. You want to > write data into |path| here, but you write into |&path| instead. > > Please test > > GetICMProfile(screen_dc, &path_len, path); > > with your debugger to verify this new code is returning the profile in the path > WCHAR variable. Done.
After discussing with Noel, we don't think this API will ever need a parent_window. The correct window of the renderer is the same window used by the browser. Either way, I think it's cleaner not to include it in the API until it is actually needed.
On 2012/07/13 22:24:23, tpayne wrote: > I was able to debug using WinDBG and confirm that the profile is being read from > the correct file, is being returned to the renderer, is parsed successfully by > qcms, passes the bogocity check and is used in creating the transform. OK good, thanks for checking. > As you mentioned, Noel, DRT is using the null implementation of > screenColorProfile which is why we do not see a change in layout test results. Good.
Some nits below. Also, please be aware of r18675 http://src.chromium.org/viewvc/chrome?view=rev&revision=18675 which is win32 only, but does work, and I think this change breaks it. Maybe check if the user is running with --enable-monitor-profile on win32, and return an empty profile if so. http://codereview.chromium.org/10448110/diff/87001/ui/gfx/color_profile.cc File ui/gfx/color_profile.cc (right): http://codereview.chromium.org/10448110/diff/87001/ui/gfx/color_profile.cc#ne... ui/gfx/color_profile.cc:5: #include "color_profile.h" #include "ui/gfx/color_profile.h" http://codereview.chromium.org/10448110/diff/87001/ui/gfx/color_profile.cc#ne... ui/gfx/color_profile.cc:8: #include "base/logging.h" You're not using logging.h http://codereview.chromium.org/10448110/diff/87001/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/87001/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:7: #include <stdio.h> I'd maybe remove these includes (lines 7 and 8).
On 2012/07/16 12:14:27, noel chromium wrote: > Some nits below. Also, please be aware of r18675 > > http://src.chromium.org/viewvc/chrome?view=rev&revision=18675 > > which is win32 only, but does work, and I think this change breaks it. Maybe > check if the user is running with --enable-monitor-profile on win32, and return > an empty profile if so. Done. I had tested --enable-monitor-profile but did not realize I had to close all windows between tests. Verified that my patch was double-correcting tagged images when --enable-monitor-profile was present.
http://codereview.chromium.org/10448110/diff/87001/ui/gfx/color_profile.cc File ui/gfx/color_profile.cc (right): http://codereview.chromium.org/10448110/diff/87001/ui/gfx/color_profile.cc#ne... ui/gfx/color_profile.cc:5: #include "color_profile.h" On 2012/07/16 12:14:27, noel chromium wrote: > #include "ui/gfx/color_profile.h" Done. http://codereview.chromium.org/10448110/diff/87001/ui/gfx/color_profile.cc#ne... ui/gfx/color_profile.cc:8: #include "base/logging.h" On 2012/07/16 12:14:27, noel chromium wrote: > You're not using logging.h Done. http://codereview.chromium.org/10448110/diff/87001/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/87001/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:7: #include <stdio.h> On 2012/07/16 12:14:27, noel chromium wrote: > I'd maybe remove these includes (lines 7 and 8). I think I'm no longer using <stdio.h>, but why remove <windows.h>, isn't it necessary for GetDC, GetICMProfile, etc?
On 2012/07/16 23:44:16, tpayne wrote: > http://codereview.chromium.org/10448110/diff/87001/ui/gfx/color_profile.cc > File ui/gfx/color_profile.cc (right): > > http://codereview.chromium.org/10448110/diff/87001/ui/gfx/color_profile.cc#ne... > ui/gfx/color_profile.cc:5: #include "color_profile.h" > On 2012/07/16 12:14:27, noel chromium wrote: > > #include "ui/gfx/color_profile.h" > > Done. > > http://codereview.chromium.org/10448110/diff/87001/ui/gfx/color_profile.cc#ne... > ui/gfx/color_profile.cc:8: #include "base/logging.h" > On 2012/07/16 12:14:27, noel chromium wrote: > > You're not using logging.h > > Done. > > http://codereview.chromium.org/10448110/diff/87001/ui/gfx/color_profile_win.cc > File ui/gfx/color_profile_win.cc (right): > > http://codereview.chromium.org/10448110/diff/87001/ui/gfx/color_profile_win.c... > ui/gfx/color_profile_win.cc:7: #include <stdio.h> > On 2012/07/16 12:14:27, noel chromium wrote: > > I'd maybe remove these includes (lines 7 and 8). > > I think I'm no longer using <stdio.h>, but why remove <windows.h>, isn't it > necessary for GetDC, GetICMProfile, etc? Ben, Noel, would you guys take another look, please?
On 2012/07/16 23:43:54, tpayne wrote: > Done. I had tested --enable-monitor-profile but did not realize I > had to close all windows between tests. Browsers often latch the users color profile, so you do need restart the browser between tests. > Verified that my patch was double-correcting tagged images when > --enable-monitor-profile was present. Double-correcting never looks good. I see the changes to control that now, so LGTM.
mostly nits http://codereview.chromium.org/10448110/diff/91002/ui/gfx/color_profile.cc File ui/gfx/color_profile.cc (right): http://codereview.chromium.org/10448110/diff/91002/ui/gfx/color_profile.cc#ne... ui/gfx/color_profile.cc:15: CR_DEFINE_STATIC_LOCAL(std::vector<char>, screen_profile, ()); I can't tell from your comment if this is called from multiple threads. If so, I don't think CR_DEFINE_STATIC_LOCAL is thread safe. If not, can you add DCHECKs that its called on the right thread? http://codereview.chromium.org/10448110/diff/91002/ui/gfx/color_profile.h File ui/gfx/color_profile.h (right): http://codereview.chromium.org/10448110/diff/91002/ui/gfx/color_profile.h#new... ui/gfx/color_profile.h:19: void ReadColorProfile(std::vector<char>* profile); Is there any reason to declare ReadColorProfile in this header? Seems internal to color_profile.cc. http://codereview.chromium.org/10448110/diff/91002/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/91002/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:11: remove newline. http://codereview.chromium.org/10448110/diff/91002/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:18: WCHAR path[MAX_PATH + 1]; MAX_PATH -> path_len
http://codereview.chromium.org/10448110/diff/91002/ui/gfx/color_profile.cc File ui/gfx/color_profile.cc (right): http://codereview.chromium.org/10448110/diff/91002/ui/gfx/color_profile.cc#ne... ui/gfx/color_profile.cc:15: CR_DEFINE_STATIC_LOCAL(std::vector<char>, screen_profile, ()); On 2012/07/18 14:36:18, sky wrote: > I can't tell from your comment if this is called from multiple threads. If so, I > don't think CR_DEFINE_STATIC_LOCAL is thread safe. If not, can you add DCHECKs > that its called on the right thread? It seems that src/ui is not allowed to depend on src/content. The IPC code upstream ensures we run on the FILE thread on Windows and the IO thread on all other platforms. http://codereview.chromium.org/10448110/diff/91002/ui/gfx/color_profile.h File ui/gfx/color_profile.h (right): http://codereview.chromium.org/10448110/diff/91002/ui/gfx/color_profile.h#new... ui/gfx/color_profile.h:19: void ReadColorProfile(std::vector<char>* profile); On 2012/07/18 14:36:18, sky wrote: > Is there any reason to declare ReadColorProfile in this header? Seems internal > to color_profile.cc. Done. http://codereview.chromium.org/10448110/diff/91002/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/91002/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:11: On 2012/07/18 14:36:18, sky wrote: > remove newline. Done. http://codereview.chromium.org/10448110/diff/91002/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:18: WCHAR path[MAX_PATH + 1]; On 2012/07/18 14:36:18, sky wrote: > MAX_PATH -> path_len Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tpayne@chromium.org/10448110/87004
Try job failure for 10448110-87004 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
http://codereview.chromium.org/10448110/diff/91002/ui/gfx/color_profile_win.cc File ui/gfx/color_profile_win.cc (right): http://codereview.chromium.org/10448110/diff/91002/ui/gfx/color_profile_win.c... ui/gfx/color_profile_win.cc:18: WCHAR path[MAX_PATH + 1]; On 2012/07/18 17:41:52, tpayne wrote: > On 2012/07/18 14:36:18, sky wrote: > > MAX_PATH -> path_len > > Done. Undone. Doesn't compile.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tpayne@chromium.org/10448110/100004
Try job failure for 10448110-100004 (retry) on android for steps "compile, build" (clobber build). It's a second try, previously, steps "compile, build" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tpayne@chromium.org/10448110/101003
On 2012/07/18 21:49:08, I haz the power (commit-bot) wrote: > Try job failure for 10448110-100004 (retry) on android for steps "compile, > build" (clobber build). > It's a second try, previously, steps "compile, build" failed. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu... Fixed android build by removing color_profile_linux.cc and providing an empty implementation for everything except OS_MAC and OS_WIN
Change committed as 147361
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/10448110/diff/101003/ui/gfx/color_prof... File ui/gfx/color_profile_mac.cc (right): https://chromiumcodereview.appspot.com/10448110/diff/101003/ui/gfx/color_prof... ui/gfx/color_profile_mac.cc:13: CFDataRef icc_profile(CGColorSpaceCopyICCProfile(monitor_color_space)); This is leaked, right? (Fix at https://codereview.chromium.org/12390058 , shout if you think that's wrong.) |
