Remove Widget from screenColorProfile
Created attachment 152379 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 152379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152379&action=review > Source/Platform/ChangeLog:8 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). You'll need to remove the line in order to land this patch. > Source/WebCore/ChangeLog:9 > + Chromium, the only platform implementing screenColorProfile, does not > + need the Widget, so removing for simplicity. Ok. Does that mean we've given up on multimon support? I guess we can add it back later when we want to fix the multimon bugs.
Created attachment 152381 [details] Patch
Comment on attachment 152379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152379&action=review >> Source/Platform/ChangeLog:8 >> + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). > > You'll need to remove the line in order to land this patch. Done. >> Source/WebCore/ChangeLog:9 >> + need the Widget, so removing for simplicity. > > Ok. Does that mean we've given up on multimon support? I guess we can add it back later when we want to fix the multimon bugs. No, but we don't think Widget is part of the solution, at least for Chrome. First, ImageDecoder doesn't have access to a widget. Second, at least in chrome, the browser already has a handle to the HWND. Third, the most likely solution to multiple monitors is likely to be GPU-based.
Comment on attachment 152381 [details] Patch Ok.
Thanks for the patch.
Comment on attachment 152381 [details] Patch Clearing flags on attachment: 152381 Committed r122655: <http://trac.webkit.org/changeset/122655>
All reviewed patches have been landed. Closing bug.
(In reply to comment #5) > > > > Ok. Does that mean we've given up on multimon support? I guess we can add it back later when we want to fix the multimon bugs. > > No, but we don't think Widget is part of the solution, at least for Chrome. First, ImageDecoder doesn't have access to a widget. Second, at least in chrome, the browser already has a handle to the HWND. Third, the most likely solution to multiple monitors is likely to be GPU-based. And from James comments https://bugs.webkit.org/show_bug.cgi?id=88565#c26 through #c29 re color correction on the mac port, in particular, https://bugs.webkit.org/show_bug.cgi?id=88565#c29 "... it sounds like they apply color correction at render time, not decode time. It seems that this is the only sane way to deal with multi-monitor or profile switching situations. Is there any reason we can't do the same - is the paint time cost too much?" I answered this question in https://bugs.webkit.org/show_bug.cgi?id=88565#c55 onwards. To make it fast on win32, I'll need GPU assist per http://codereview.chromium.org/10703062. But regardless, at render time I know where the page is relative multiple monitors. So I don't need the image decoders layers to tell me, or guess, their Widget. Of course the current approach (reading the primary monitor profile) does not attempt to solve the multiple monitor or profile switching cases. Consider a user who moves their browser window onto a second monitor with a different color profile than their primary. We render wrong because the images have been corrected for the primary monitor only :/