Bug 91300 – Remove Widget from screenColorProfile

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Payne]

Remove Widget from screenColorProfile

[Tony Payne]

Created attachment 152379 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

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.

[Tony Payne]

Created attachment 152381 [details]
Patch

[Tony Payne]

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.

[Adam Barth]

Comment on attachment 152381 [details]
Patch

Ok.

[Adam Barth]

Thanks for the patch.

[WebKit Review Bot]

Comment on attachment 152381 [details]
Patch

Clearing flags on attachment: 152381

Committed r122655: <http://trac.webkit.org/changeset/122655>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

(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 :/