Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
549213004:
[Mac] Implement a new WebSandboxSupport method to get the display's color space. (Closed)
|
Created:
6 years, 3 months ago by Robert Sesek Modified:
6 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, bgraur Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[Mac] Implement a new WebSandboxSupport method to get the display's color space.
This new interface is defined at https://codereview.chromium.org/554033002/.
BUG=397642, 306348
R=avi@chromium.org
Committed: https://crrev.com/b6ae4d0ecc983a71a32ab1480a70e065c718c195
Cr-Commit-Position: refs/heads/master@{#293852}
Patch Set 1 #
Total comments: 6
Messages
Total messages: 22 (2 generated)
This code LGTM though: "This new interface is defined at https://codereview.chromium.org/551983002/." That CL doesn't exist.
On 2014/09/08 20:07:48, Avi wrote: > This code LGTM though: > > "This new interface is defined at https://codereview.chromium.org/551983002/.%22 > > That CL doesn't exist. Thanks for catching that. Rietveld ate the first CL, re-uploaded at https://codereview.chromium.org/554033002/.
The CQ bit was checked by rsesek@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsesek@chromium.org/549213004/1
Message was sent while issue was closed.
Committed patchset #1 (id:1) as e52f81edbd89a35cb107e2ab66336d4cb0777ef5
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/b6ae4d0ecc983a71a32ab1480a70e065c718c195 Cr-Commit-Position: refs/heads/master@{#293852}
Message was sent while issue was closed.
noel@chromium.org changed reviewers: + noel@chromium.org
That CL mentioned in the description does not exist. Could we fix the reference in the description in codereview.com? https://codereview.chromium.org/549213004/diff/1/content/ppapi_plugin/ppapi_w... File content/ppapi_plugin/ppapi_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/549213004/diff/1/content/ppapi_plugin/ppapi_w... content/ppapi_plugin/ppapi_webkitplatformsupport_impl.cc:102: return base::mac::GetSystemColorSpace(); Why does ppapi need color profiles? That'd allow user fingerprinting via user-private information - their color profile.
https://codereview.chromium.org/549213004/diff/1/content/renderer/renderer_we... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/549213004/diff/1/content/renderer/renderer_we... content/renderer/renderer_webkitplatformsupport_impl.cc:580: return base::mac::GetSystemColorSpace(); This could be called on an arbitrary thread, such as an impl-side-painting thread for example. Are we thread-safe here? Also, what color profile does this return (aka for which screen)? (Sorry to ask these question, but the the change description doesn't tell me why we're making this change). https://codereview.chromium.org/549213004/diff/1/content/renderer/renderer_we... content/renderer/renderer_webkitplatformsupport_impl.cc:844: #if defined(OS_WIN) Wonder what's going on here. See the comment at line 854. Doesn't gfx::ColorProfile profile provide what you want?
https://codereview.chromium.org/549213004/diff/1/content/renderer/renderer_we... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/549213004/diff/1/content/renderer/renderer_we... content/renderer/renderer_webkitplatformsupport_impl.cc:580: return base::mac::GetSystemColorSpace(); On 2014/09/10 07:35:37, Noel Gordon wrote: > Are we thread-safe here? And it seems we're not; the other code review https://codereview.chromium.org/554033002 appears to be crashing image decoder unit tests, which is new.
On 2014/09/10 07:22:36, Noel Gordon wrote: > That CL mentioned in the description does not exist. Could we fix the reference > in the description in codereview.com? Seems that's fixed, but I think we should revert this change.
On 2014/09/10 14:02:12, Noel Gordon wrote: > On 2014/09/10 07:22:36, Noel Gordon wrote: > > That CL mentioned in the description does not exist. Could we fix the > reference > > in the description in codereview.com? > > Seems that's fixed, but I think we should revert this change. If you think it's that broken, the emails you should be sending are "hey you broke things so I reverted you" :) Please never be afraid to revert something that's not working.
https://codereview.chromium.org/549213004/diff/1/content/renderer/renderer_we... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/549213004/diff/1/content/renderer/renderer_we... content/renderer/renderer_webkitplatformsupport_impl.cc:580: return base::mac::GetSystemColorSpace(); On 2014/09/10 07:52:50, Noel Gordon wrote: > On 2014/09/10 07:35:37, Noel Gordon wrote: > > Are we thread-safe here? > > And it seems we're not; the other code review > > https://codereview.chromium.org/554033002 > > appears to be crashing image decoder unit tests, which is new. That has nothing to do with thread safety. There's no sandboxSupport in webkit tests. Yes, this is thread safe. It's also identical to what gfx::ColorProfile's ctor does. https://codereview.chromium.org/549213004/diff/1/content/renderer/renderer_we... content/renderer/renderer_webkitplatformsupport_impl.cc:844: #if defined(OS_WIN) On 2014/09/10 07:35:37, Noel Gordon wrote: > Wonder what's going on here. See the comment at line 854. Doesn't > gfx::ColorProfile profile provide what you want? Sorry, I didn't see this method. I'll see if this can be used instead.
On 2014/09/10 14:13:38, Avi wrote: > On 2014/09/10 14:02:12, Noel Gordon wrote: > > > Seems that's fixed, but I think we should revert this change. > > If you think it's that broken, the emails you should be sending are "hey you > broke things so I reverted you" :) Please never be afraid to revert something > that's not working. No worries. I didn't say it was broken:) I said we should revert. This to avoid the privacy issue with ppapi for one, and try to do this all another way.
On 2014/09/10 14:34:12, rsesek wrote: > https://codereview.chromium.org/549213004/diff/1/content/renderer/renderer_we... > File content/renderer/renderer_webkitplatformsupport_impl.cc (right): > > https://codereview.chromium.org/549213004/diff/1/content/renderer/renderer_we... > content/renderer/renderer_webkitplatformsupport_impl.cc:580: return > base::mac::GetSystemColorSpace(); > On 2014/09/10 07:52:50, Noel Gordon wrote: > > On 2014/09/10 07:35:37, Noel Gordon wrote: > > > Are we thread-safe here? > > > > And it seems we're not; the other code review > > > > https://codereview.chromium.org/554033002 > > > > appears to be crashing image decoder unit tests, which is new. > > That has nothing to do with thread safety. There's no sandboxSupport in webkit > tests. Yes, this is thread safe. It's also identical to what gfx::ColorProfile's > ctor does. ok. > https://codereview.chromium.org/549213004/diff/1/content/renderer/renderer_we... > content/renderer/renderer_webkitplatformsupport_impl.cc:844: #if defined(OS_WIN) > On 2014/09/10 07:35:37, Noel Gordon wrote: > > Wonder what's going on here. See the comment at line 854. Doesn't > > gfx::ColorProfile profile provide what you want? > > Sorry, I didn't see this method. I'll see if this can be used instead. yes, this method provides "the other way" I mentioned.
On 2014/09/10 14:38:29, Noel Gordon wrote: > On 2014/09/10 14:13:38, Avi wrote: > > On 2014/09/10 14:02:12, Noel Gordon wrote: > > > > > Seems that's fixed, but I think we should revert this change. > > > > If you think it's that broken, the emails you should be sending are "hey you > > broke things so I reverted you" :) Please never be afraid to revert something > > that's not working. > > No worries. I didn't say it was broken:) I said we should revert. This to avoid > the privacy issue with ppapi for one, and try to do this all another way. Yes, I'll revert this if screenColorProfile() works.
On 2014/09/10 14:44:55, rsesek wrote: > On 2014/09/10 14:38:29, Noel Gordon wrote: > > On 2014/09/10 14:13:38, Avi wrote: > > > On 2014/09/10 14:02:12, Noel Gordon wrote: > > > > > > > Seems that's fixed, but I think we should revert this change. > > > > > > If you think it's that broken, the emails you should be sending are "hey you > > > broke things so I reverted you" :) Please never be afraid to revert > something > > > that's not working. > > > > No worries. I didn't say it was broken:) I said we should revert. This to > avoid > > the privacy issue with ppapi for one, and try to do this all another way. > > Yes, I'll revert this if screenColorProfile() works. I think we can make it work, let's continue over on the other review.
+bogdan We'll probably revert this patch, so this is just for interest. Just wanted to check your team's work w.r.t the "ppapi and color profiles situation" is still ok.
On 2014/09/10 14:44:55, rsesek wrote: > > Yes, I'll revert this if screenColorProfile() works. Seems it did work https://codereview.chromium.org/554033002 ...
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/563943003/ by noel@chromium.org. The reason for reverting is: Not needed, https://codereview.chromium.org/554033002 instead.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
