Disable LocalCurrentGraphicsContext when doing DisplayList recording
Created attachment 384549 [details] Patch
Also fixed some deprecated API usage while I was there
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`.
Also not sure why this doesn't use RetainPtr
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.
We’re not doing anything wrong. Ideally we would log when someone /reads/ it, but that’s code outside of our control.
Unless you think we should change every callsite to not create a LocalCurrentGraphicsContext in the display list case, but that seems like too much.
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?
(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?
(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.
Created attachment 384590 [details] Patch
Comment on attachment 384590 [details] Patch Clearing flags on attachment: 384590 Committed r252977: <https://trac.webkit.org/changeset/252977>
All reviewed patches have been landed. Closing bug.
<rdar://problem/57548595>