Bug 204721 - Disable LocalCurrentGraphicsContext when doing DisplayList recording
Summary: Disable LocalCurrentGraphicsContext when doing DisplayList recording
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-30 14:54 PST by Tim Horton
Modified: 2019-12-01 19:33 PST (History)
8 users (show)

See Also:


Attachments
Patch (2.32 KB, patch)
2019-11-30 14:54 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (2.50 KB, patch)
2019-12-01 19:10 PST, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2019-11-30 14:54:15 PST
Disable LocalCurrentGraphicsContext when doing DisplayList recording
Comment 1 Tim Horton 2019-11-30 14:54:25 PST
Created attachment 384549 [details]
Patch
Comment 2 Tim Horton 2019-11-30 14:55:25 PST
Also fixed some deprecated API usage while I was there
Comment 3 Tim Horton 2019-11-30 15:20:13 PST
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`.
Comment 4 Tim Horton 2019-11-30 15:20:24 PST
Also not sure why this doesn't use RetainPtr
Comment 5 Simon Fraser (smfr) 2019-12-01 08:40:17 PST
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.
Comment 6 Tim Horton 2019-12-01 14:45:07 PST
We’re not doing anything wrong. Ideally we would log when someone /reads/ it, but that’s code outside of our control.
Comment 7 Tim Horton 2019-12-01 14:46:03 PST
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 8 Sam Weinig 2019-12-01 15:46:13 PST
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?
Comment 9 Sam Weinig 2019-12-01 15:48:31 PST
(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?
Comment 10 Tim Horton 2019-12-01 19:06:50 PST
(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.
Comment 11 Tim Horton 2019-12-01 19:10:25 PST
Created attachment 384590 [details]
Patch
Comment 12 WebKit Commit Bot 2019-12-01 19:32:33 PST
Comment on attachment 384590 [details]
Patch

Clearing flags on attachment: 384590

Committed r252977: <https://trac.webkit.org/changeset/252977>
Comment 13 WebKit Commit Bot 2019-12-01 19:32:35 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2019-12-01 19:33:19 PST
<rdar://problem/57548595>