Bug 88819 – Pass the right color space over to the web process so we can set it on our CA context

• Component: New Bugs
• Status: RESOLVED FIXED

[Anders Carlsson]

Pass the right color space over to the web process so we can set it on our CA context

[Anders Carlsson]

Created attachment 146962 [details]
Patch

[WebKit Review Bot]

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.

[Anders Carlsson]

Committed r120021: <http://trac.webkit.org/changeset/120021>

[Sam Weinig]

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?

[Anders Carlsson]

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

[Sam Weinig]

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

[Anders Carlsson]

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