We should set the colorspace in the PDF plugin.
<rdar://problem/40030981>
Created attachment 339869 [details] Patch
Comment on attachment 339869 [details] Patch r=me.
Comment on attachment 339869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339869&action=review > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:637 > + if ([m_pdfLayerController.get() respondsToSelector:@selector(setDeviceColorSpace:)]) No need for the .get()s
Comment on attachment 339869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339869&action=review > Source/WebKit/WebProcess/Plugins/PDF/PDFLayerControllerSPI.h:170 > +- (void) setDeviceColorSpace:(CGColorSpaceRef)colorSpace; No space after the ) >> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:637 >> + if ([m_pdfLayerController.get() respondsToSelector:@selector(setDeviceColorSpace:)]) > > No need for the .get()s No need for the .get()s > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:638 > + [m_pdfLayerController.get() setDeviceColorSpace:screenColorSpace()]; What does screenColorSpace do? It seems like it will usually be wrong, and we should be getting it from the page/view, not the screen (because there are multiple screens).
Comment on attachment 339869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339869&action=review >> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:638 >> + [m_pdfLayerController.get() setDeviceColorSpace:screenColorSpace()]; > > What does screenColorSpace do? It seems like it will usually be wrong, and we should be getting it from the page/view, not the screen (because there are multiple screens). You could call screenColorSpace(this) maybe? The PDFPlugin's Widget will be tied to the correct screen, then 'screenColorSpace' can give you the right answer, tied to the display the PDF is being shown on.
Created attachment 339889 [details] Patch
(In reply to Brent Fulgham from comment #6) > Comment on attachment 339869 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=339869&action=review > > >> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:638 > >> + [m_pdfLayerController.get() setDeviceColorSpace:screenColorSpace()]; > > > > What does screenColorSpace do? It seems like it will usually be wrong, and we should be getting it from the page/view, not the screen (because there are multiple screens). > > You could call screenColorSpace(this) maybe? The PDFPlugin's Widget will be > tied to the correct screen, then 'screenColorSpace' can give you the right > answer, tied to the display the PDF is being shown on. I used the PDF frame's view in the latest patch. Thanks for reviewing, all!
Comment on attachment 339889 [details] Patch How does this update if the screen colorspace changes?
(In reply to Simon Fraser (smfr) from comment #9) > Comment on attachment 339889 [details] > Patch > > How does this update if the screen colorspace changes? That is a good point ... The current patch does not handle this, I believe. Thanks for reviewing!
Comment on attachment 339889 [details] Patch Clearing flags on attachment: 339889 Committed r231535: <https://trac.webkit.org/changeset/231535>
All reviewed patches have been landed. Closing bug.