Bug 185445 – Set colorspace in the PDF plugin.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Per Arne Vollan]

We should set the colorspace in the PDF plugin.

[Per Arne Vollan]

<rdar://problem/40030981>

[Per Arne Vollan]

Created attachment 339869 [details]
Patch

[Brent Fulgham]

Comment on attachment 339869 [details]
Patch

r=me.

[Tim Horton]

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

[Tim Horton]

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).

[Brent Fulgham]

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.

[Per Arne Vollan]

Created attachment 339889 [details]
Patch

[Per Arne Vollan]

(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!

[Simon Fraser (smfr)]

Comment on attachment 339889 [details]
Patch

How does this update if the screen colorspace changes?

[Per Arne Vollan]

(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!

[WebKit Commit Bot]

Comment on attachment 339889 [details]
Patch

Clearing flags on attachment: 339889

Committed r231535: <https://trac.webkit.org/changeset/231535>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.