Bug 181977 - [Cairo] Use GraphicsContextImplCairo for ImageBuffer context
Summary: [Cairo] Use GraphicsContextImplCairo for ImageBuffer context
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: Zan Dobersek
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-23 01:37 PST by Zan Dobersek
Modified: 2018-01-24 23:19 PST (History)
5 users (show)

See Also:


Attachments
WIP (48.81 KB, patch)
2018-01-23 01:38 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (51.75 KB, patch)
2018-01-23 23:44 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2018-01-23 01:37:47 PST
[Cairo] Use GraphicsContextImplCairo for ImageBuffer context
Comment 1 Zan Dobersek 2018-01-23 01:38:30 PST
Created attachment 332015 [details]
WIP
Comment 2 Zan Dobersek 2018-01-23 23:44:25 PST
Created attachment 332128 [details]
Patch
Comment 3 Zan Dobersek 2018-01-23 23:48:46 PST
(In reply to Zan Dobersek from comment #2)
> Created attachment 332128 [details]
> Patch

Given this changes code in GraphicsContext, GraphicsContextImpl and DisplayList::Recorder, I'd prefer if Simon or Myles can review this.
Comment 4 Carlos Garcia Campos 2018-01-24 00:04:16 PST
Comment on attachment 332128 [details]
Patch

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

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:60
> +    bool hasPlatformContext() const override { return false; }
> +    PlatformGraphicsContext* platformContext() const override { return nullptr; }

I wonder why we have hasPlatformContext, since platformContext() returns a pointer we could simply check its return value no?
Comment 5 Carlos Garcia Campos 2018-01-24 00:07:20 PST
(In reply to Zan Dobersek from comment #3)
> (In reply to Zan Dobersek from comment #2)
> > Created attachment 332128 [details]
> > Patch
> 
> Given this changes code in GraphicsContext, GraphicsContextImpl and
> DisplayList::Recorder, I'd prefer if Simon or Myles can review this.

I was already reviewing this when you added this comment. Changes look sane to me, but I agree it would be better if graphics guys double check it.
Comment 6 Simon Fraser (smfr) 2018-01-24 13:54:25 PST
Comment on attachment 332128 [details]
Patch

I like this. At some point I'd like the CG drawing to be a graphicsContextImpl too.
Comment 7 Zan Dobersek 2018-01-24 23:16:33 PST
Comment on attachment 332128 [details]
Patch

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

>> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:60
>> +    PlatformGraphicsContext* platformContext() const override { return nullptr; }
> 
> I wonder why we have hasPlatformContext, since platformContext() returns a pointer we could simply check its return value no?

This was done to exactly mirror the GraphicsContext methods. It might be possible to just deduce this from the platformContext() value like you propose, but I'd wait on that until more implementations (e.g. GraphicsContextImplCG) see whether the extra method is necessary.
Comment 8 Zan Dobersek 2018-01-24 23:18:26 PST
Comment on attachment 332128 [details]
Patch

Clearing flags on attachment: 332128

Committed r227594: <https://trac.webkit.org/changeset/227594>
Comment 9 Zan Dobersek 2018-01-24 23:18:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2018-01-24 23:19:18 PST
<rdar://problem/36856107>