Pass the right color space over to the web process so we can set it on our CA context
Created attachment 146962 [details] Patch
Attachment 146962 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/Shared/mac/ColorSpaceData.h:32: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r120021: <http://trac.webkit.org/changeset/120021>
Comment on attachment 146962 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146962&action=review Should we sending a color space on creation of the drawing area? As a follow up, and we group all these hosting environment/window properties together somehow (visibility/active state/color space/backing scale factor/layer hosting mode) so they are easier to reason about and less spread out? > Source/WebKit2/UIProcess/API/mac/WKView.mm:206 > + RetainPtr<NSColorSpace> _colorSpace; I think this should include some indication that it is just a cache. > Source/WebKit2/UIProcess/API/mac/WKView.mm:2081 > +- (void)viewDidChangeBackingProperties Why are we using viewDidChangeBackingProperties here rather than _windowDidChangeBackingProperties? It seems like we should be using one or the other. > Source/WebKit2/UIProcess/API/mac/WKView.mm:2089 > + if (DrawingAreaProxy *drawingArea = _data->_page->drawingArea()) > + drawingArea->colorSpaceDidChange(); We have been using viewStateDidChange for things like this lately. Can you explain why that isn't appropriate here?
(In reply to comment #4) > (From update of attachment 146962 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146962&action=review > > Should we sending a color space on creation of the drawing area? As a follow up, and we group all these hosting environment/window properties together somehow (visibility/active state/color space/backing scale factor/layer hosting mode) so they are easier to reason about and less spread out? > > > Source/WebKit2/UIProcess/API/mac/WKView.mm:206 > > + RetainPtr<NSColorSpace> _colorSpace; > > I think this should include some indication that it is just a cache. > It isn't really a cache - it's the last color space we sent to the web process (sort of). > > Source/WebKit2/UIProcess/API/mac/WKView.mm:2081 > > +- (void)viewDidChangeBackingProperties > > Why are we using viewDidChangeBackingProperties here rather than _windowDidChangeBackingProperties? It seems like we should be using one or the other. We should always be using viewDidChangeBackingProperties. I didn't want to get rid of _windowDidChangeBackingProperties: in this patch. > > > Source/WebKit2/UIProcess/API/mac/WKView.mm:2089 > > + if (DrawingAreaProxy *drawingArea = _data->_page->drawingArea()) > > + drawingArea->colorSpaceDidChange(); > > We have been using viewStateDidChange for things like this lately. Can you explain why that isn't appropriate here? The color space isn't really part of the view state, and I didn't want to add platform specific view states.
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 146962 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=146962&action=review > > > > Should we sending a color space on creation of the drawing area? As a follow up, and we group all these hosting environment/window properties together somehow (visibility/active state/color space/backing scale factor/layer hosting mode) so they are easier to reason about and less spread out? > > > > > Source/WebKit2/UIProcess/API/mac/WKView.mm:206 > > > + RetainPtr<NSColorSpace> _colorSpace; > > > > I think this should include some indication that it is just a cache. > > > > It isn't really a cache - it's the last color space we sent to the web process (sort of). > Sure, but it certainly doesn't actually represent the color space of the view in any meaningful way. It should include words from the sentence you just wrote. > > > Source/WebKit2/UIProcess/API/mac/WKView.mm:2081 > > > +- (void)viewDidChangeBackingProperties > > > > Why are we using viewDidChangeBackingProperties here rather than _windowDidChangeBackingProperties? It seems like we should be using one or the other. > > We should always be using viewDidChangeBackingProperties. I didn't want to get rid of _windowDidChangeBackingProperties: in this patch. Fair enough. I take it we never need any of the properties from the notification then? > > > > > > Source/WebKit2/UIProcess/API/mac/WKView.mm:2089 > > > + if (DrawingAreaProxy *drawingArea = _data->_page->drawingArea()) > > > + drawingArea->colorSpaceDidChange(); > > > > We have been using viewStateDidChange for things like this lately. Can you explain why that isn't appropriate here? > > The color space isn't really part of the view state, and I didn't want to add platform specific view states.
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > (From update of attachment 146962 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=146962&action=review > > > > > > Should we sending a color space on creation of the drawing area? As a follow up, and we group all these hosting environment/window properties together somehow (visibility/active state/color space/backing scale factor/layer hosting mode) so they are easier to reason about and less spread out? > > > > > > > Source/WebKit2/UIProcess/API/mac/WKView.mm:206 > > > > + RetainPtr<NSColorSpace> _colorSpace; > > > > > > I think this should include some indication that it is just a cache. > > > > > > > It isn't really a cache - it's the last color space we sent to the web process (sort of). > > > > Sure, but it certainly doesn't actually represent the color space of the view in any meaningful way. It should include words from the sentence you just wrote. Not sure what you mean by "doesn't represent the color space of the view." I'd argue that it does! > > > > > > Source/WebKit2/UIProcess/API/mac/WKView.mm:2081 > > > > +- (void)viewDidChangeBackingProperties > > > > > > Why are we using viewDidChangeBackingProperties here rather than _windowDidChangeBackingProperties? It seems like we should be using one or the other. > > > > We should always be using viewDidChangeBackingProperties. I didn't want to get rid of _windowDidChangeBackingProperties: in this patch. > > Fair enough. I take it we never need any of the properties from the notification then? True.