Bug 204721 – Disable LocalCurrentGraphicsContext when doing DisplayList recording

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Disable LocalCurrentGraphicsContext when doing DisplayList recording

[Tim Horton]

Created attachment 384549 [details]
Patch

[Tim Horton]

Also fixed some deprecated API usage while I was there

[Tim Horton]

Comment on attachment 384549 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384549&action=review

> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:36
> +        m_savedNSGraphicsContext = 0;

Uh... don't know why 0, I just copied from below. I'll make it `nil`.

[Tim Horton]

Also not sure why this doesn't use RetainPtr

[Simon Fraser (smfr)]

Comment on attachment 384549 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384549&action=review

>> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:36
>> +        m_savedNSGraphicsContext = 0;
> 
> Uh... don't know why 0, I just copied from below. I'll make it `nil`.

Should probably WTFLogAlways() or LOG(DisplayLists, ) here to tell us we've done something wrong.

[Tim Horton]

We’re not doing anything wrong. Ideally we would log when someone /reads/ it, but that’s code outside of our control.

[Tim Horton]

Unless you think we should change every callsite to not create a LocalCurrentGraphicsContext in the display list case, but that seems like too much.

[Sam Weinig]

Comment on attachment 384549 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384549&action=review

>>> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:36
>>> +        m_savedNSGraphicsContext = 0;
>> 
>> Uh... don't know why 0, I just copied from below. I'll make it `nil`.
> 
> Should probably WTFLogAlways() or LOG(DisplayLists, ) here to tell us we've done something wrong.

Wouldn't we want to ASSERT() when using DisplayList, so as to not miss subtle issues? If we are going to have to implement functionality that depends on a global graphics context another way, is there really any point in supporting both ways?

[Sam Weinig]

(In reply to Tim Horton from comment #7)
> Unless you think we should change every callsite to not create a
> LocalCurrentGraphicsContext in the display list case, but that seems like
> too much.

What is the point(In reply to Sam Weinig from comment #8)
> Comment on attachment 384549 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384549&action=review
> 
> >>> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:36
> >>> +        m_savedNSGraphicsContext = 0;
> >> 
> >> Uh... don't know why 0, I just copied from below. I'll make it `nil`.
> > 
> > Should probably WTFLogAlways() or LOG(DisplayLists, ) here to tell us we've done something wrong.
> 
> Wouldn't we want to ASSERT() when using DisplayList, so as to not miss
> subtle issues? If we are going to have to implement functionality that
> depends on a global graphics context another way, is there really any point
> in supporting both ways?

Or rather, what is the point of keeping LocalCurrentGraphicsContext if we will have to find alternate implementations for anything that uses it?

[Tim Horton]

(In reply to Sam Weinig from comment #8)
> Comment on attachment 384549 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384549&action=review
> 
> >>> Source/WebCore/platform/mac/LocalCurrentGraphicsContext.mm:36
> >>> +        m_savedNSGraphicsContext = 0;
> >> 
> >> Uh... don't know why 0, I just copied from below. I'll make it `nil`.
> > 
> > Should probably WTFLogAlways() or LOG(DisplayLists, ) here to tell us we've done something wrong.
> 
> Wouldn't we want to ASSERT() when using DisplayList, so as to not miss
> subtle issues? If we are going to have to implement functionality that
> depends on a global graphics context another way, is there really any point
> in supporting both ways?

For the time being, we definitely need to support both ways, since we don't have a solution for things (e.g. form controls) that depend on the global context. Later, I agree! But for now, we can't just rip out the LocalCurrentGraphicsContext invocations, nor can we leave them in unconditionally (because the DL GraphicsContext asserts when you try to get the platformContext).

Since I can't do the ideal thing (log when the bad thing actually happens -- when we read), I'll do the thing that smfr suggested so that we don't forget to address this later.

[Tim Horton]

Created attachment 384590 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 384590 [details]
Patch

Clearing flags on attachment: 384590

Committed r252977: <https://trac.webkit.org/changeset/252977>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57548595>