WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
226170
Virtualize GraphicsContext
https://bugs.webkit.org/show_bug.cgi?id=226170
Summary
Virtualize GraphicsContext
Tim Horton
Reported
2021-05-24 02:22:24 PDT
Virtualize GraphicsContext
Attachments
Patch
(348.62 KB, patch)
2021-05-24 02:23 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
Patch
(358.37 KB, patch)
2021-05-24 13:53 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
Patch
(357.72 KB, patch)
2021-05-24 15:06 PDT
,
Tim Horton
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(357.21 KB, patch)
2021-05-24 15:47 PDT
,
Tim Horton
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(357.21 KB, patch)
2021-05-24 16:31 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
Patch
(361.93 KB, patch)
2021-05-24 17:26 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
Patch
(369.37 KB, patch)
2021-05-24 19:19 PDT
,
Tim Horton
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(370.49 KB, patch)
2021-05-24 20:49 PDT
,
Tim Horton
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(370.55 KB, patch)
2021-05-24 23:27 PDT
,
Tim Horton
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(376.14 KB, patch)
2021-05-25 00:40 PDT
,
Tim Horton
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(376.27 KB, patch)
2021-05-25 01:40 PDT
,
Tim Horton
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(376.39 KB, patch)
2021-05-25 02:08 PDT
,
Tim Horton
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(377.30 KB, patch)
2021-05-25 03:09 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
Patch
(417.63 KB, patch)
2021-05-25 12:41 PDT
,
Tim Horton
simon.fraser
: review+
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(417.63 KB, patch)
2021-05-25 12:59 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
Patch
(417.64 KB, patch)
2021-05-25 13:03 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
Patch
(422.60 KB, patch)
2021-05-25 15:17 PDT
,
Tim Horton
no flags
Details
Formatted Diff
Diff
Show Obsolete
(16)
View All
Add attachment
proposed patch, testcase, etc.
Tim Horton
Comment 1
2021-05-24 02:23:15 PDT
Created
attachment 429514
[details]
Patch
Tim Horton
Comment 2
2021-05-24 02:23:53 PDT
Not even close to ready for review, I just want to fiddle with EWS (and use it to get Windows building).
Tim Horton
Comment 3
2021-05-24 13:53:39 PDT
Created
attachment 429560
[details]
Patch
Tim Horton
Comment 4
2021-05-24 15:06:58 PDT
Created
attachment 429569
[details]
Patch
Tim Horton
Comment 5
2021-05-24 15:47:42 PDT
Created
attachment 429578
[details]
Patch
Tim Horton
Comment 6
2021-05-24 16:31:50 PDT
Created
attachment 429587
[details]
Patch
Tim Horton
Comment 7
2021-05-24 17:26:49 PDT
Created
attachment 429598
[details]
Patch
Tim Horton
Comment 8
2021-05-24 19:19:44 PDT
Created
attachment 429612
[details]
Patch
Tim Horton
Comment 9
2021-05-24 20:49:37 PDT
Created
attachment 429623
[details]
Patch
Tim Horton
Comment 10
2021-05-24 23:27:31 PDT
Created
attachment 429631
[details]
Patch
Tim Horton
Comment 11
2021-05-25 00:40:55 PDT
Created
attachment 429635
[details]
Patch
Tim Horton
Comment 12
2021-05-25 01:40:22 PDT
Created
attachment 429638
[details]
Patch
Tim Horton
Comment 13
2021-05-25 02:08:03 PDT
Created
attachment 429639
[details]
Patch
Tim Horton
Comment 14
2021-05-25 03:09:42 PDT
Created
attachment 429640
[details]
Patch
Tim Horton
Comment 15
2021-05-25 12:41:46 PDT
Created
attachment 429680
[details]
Patch
Tim Horton
Comment 16
2021-05-25 12:59:33 PDT
Created
attachment 429683
[details]
Patch
Tim Horton
Comment 17
2021-05-25 13:03:09 PDT
Created
attachment 429684
[details]
Patch
Simon Fraser (smfr)
Comment 18
2021-05-25 13:35:11 PDT
Comment on
attachment 429680
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=429680&action=review
> Source/WebCore/platform/graphics/GraphicsContext.h:341 > + void setDrawLuminanceMask(bool drawLuminanceMask) { m_state.drawLuminanceMask = drawLuminanceMask; updateState(m_state, GraphicsContextState::DrawLuminanceMaskChange); } > + bool drawLuminanceMask() const { return m_state.drawLuminanceMask; }
Maybe we could rename these to reduce ambiguity: setIsDrawingLuminanceMask/isDrawingLuminanceMask
> Source/WebCore/platform/graphics/NullGraphicsContext.h:52 > + bool paintingDisabled() const override { return true; }
Why not final?
> Source/WebCore/platform/graphics/NullGraphicsContext.h:155 > + const PaintInvalidationReasons m_paintInvalidationReasons { PaintInvalidationReasons::None };
This could be an OptionSet<> in future.
> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:105 > + bool fill = mode.contains(TextDrawingMode::Fill); > + bool stroke = mode.contains(TextDrawingMode::Stroke); > + if (fill && stroke) > + return kCGTextFillStroke; > + if (fill) > + return kCGTextFill; > + return kCGTextStroke;
Could be simplified with some OptionSet<> goop.
> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:53 > +class Recorder : public GraphicsContext {
I think we should rename this class to RecordingContext
> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:95 > +#if USE(CG) || USE(DIRECT2D) > + void setIsCALayerContext(bool) override { } > + bool isCALayerContext() const override { return false; } > + void setIsAcceleratedContext(bool) override { } > +#endif > + > + void fillRoundedRectImpl(const FloatRoundedRect&, const Color&) override { ASSERT_NOT_REACHED(); } > + void drawLineForText(const FloatRect&, bool, bool, StrokeStyle) override { ASSERT_NOT_REACHED(); }
Can everything be final here?
Said Abou-Hallawa
Comment 19
2021-05-25 14:41:29 PDT
Comment on
attachment 429684
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=429684&action=review
> Source/WebCore/ChangeLog:13 > + In preparation for future patches which introduce new kinds of GraphicsContexts, > + virtualize GraphicsContext, moving platform implementations into subclasses > + GraphicsContext{CG,Cairo,Direct2D}, NullGraphicsContext, DisplayList::Recorder, > + and Nicosia::CairoOperationRecorder.
I think this is great work and it is going to save us from worrying about the paintingDisabled(), m_impl is not null, and the calling the platform function in very function of GraphicsContext. But I do not like the way the superclasses of GraphicsContext are created. I think they should be pointers from now on and the caller should only keep a std::unique_ptr< GraphicsContext > or RefPtr<GraphicsContext> instead. The creation can be the responsibility of GraphicsContext also. For example: class GraphicsContext { public: static std::unique_ptr< GraphicsContext > create(CGContextRef); static std::unique_ptr< GraphicsContext > create(PlatformContextCairo&); static std::unique_ptr< GraphicsContext > create(PlatformContextCairo*); static std::unique_ptr< GraphicsContext > create(cairo_t*); static std::unique_ptr< GraphicsContext > create(DisplayList&, const GraphicsContextState&, const FloatRect& initialClip, const AffineTransform&, Delegate*, DrawGlyphsRecorder::DrawGlyphsDeconstruction); }; And this case a caller like the one in GraphicsLayerCA.cpp will do just this: auto context = GraphicsContext::create(*m_displayList, GraphicsContextState(), initialClip, AffineTransform()); paintGraphicsLayerContents(*context, FloatRect(FloatPoint(), size())); The reason for me preferring this approach over something like: GraphicsContextCG gc(context.get()); is we have to declare the variable to be GraphicsContextCG just to allocate the memory of the object correctly. But I do not think we will ever need to access the members the superclass directly. So using the the base type GraphicsContext is more appropriate and having it a pointer will solve the allocation issue.
Tim Horton
Comment 20
2021-05-25 15:17:59 PDT
Created
attachment 429698
[details]
Patch
Tim Horton
Comment 21
2021-05-25 16:30:09 PDT
(In reply to Simon Fraser (smfr) from
comment #18
)
> Comment on
attachment 429680
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=429680&action=review
> > > Source/WebCore/platform/graphics/GraphicsContext.h:341 > > + void setDrawLuminanceMask(bool drawLuminanceMask) { m_state.drawLuminanceMask = drawLuminanceMask; updateState(m_state, GraphicsContextState::DrawLuminanceMaskChange); } > > + bool drawLuminanceMask() const { return m_state.drawLuminanceMask; } > > Maybe we could rename these to reduce ambiguity: > setIsDrawingLuminanceMask/isDrawingLuminanceMask
Leaving it for now.
> > Source/WebCore/platform/graphics/NullGraphicsContext.h:52 > > + bool paintingDisabled() const override { return true; } > > Why not final?
Copy paste-o.
> > Source/WebCore/platform/graphics/NullGraphicsContext.h:155 > > + const PaintInvalidationReasons m_paintInvalidationReasons { PaintInvalidationReasons::None }; > > This could be an OptionSet<> in future. > > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:105 > > + bool fill = mode.contains(TextDrawingMode::Fill); > > + bool stroke = mode.contains(TextDrawingMode::Stroke); > > + if (fill && stroke) > > + return kCGTextFillStroke; > > + if (fill) > > + return kCGTextFill; > > + return kCGTextStroke; > > Could be simplified with some OptionSet<> goop.
Both true.
> > Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:53 > > +class Recorder : public GraphicsContext { > > I think we should rename this class to RecordingContext
Strongly Agree. Leaving it for now.
> > Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.h:95 > > +#if USE(CG) || USE(DIRECT2D) > > + void setIsCALayerContext(bool) override { } > > + bool isCALayerContext() const override { return false; } > > + void setIsAcceleratedContext(bool) override { } > > +#endif > > + > > + void fillRoundedRectImpl(const FloatRoundedRect&, const Color&) override { ASSERT_NOT_REACHED(); } > > + void drawLineForText(const FloatRect&, bool, bool, StrokeStyle) override { ASSERT_NOT_REACHED(); } > > Can everything be final here?
Yes, done. (In reply to Said Abou-Hallawa from
comment #19
)
> Comment on
attachment 429684
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=429684&action=review
> But I do not think we will ever need to > access the members the superclass directly. So using the the base type > GraphicsContext is more appropriate and having it a pointer will solve the > allocation issue.
I think I agree with the premise (we don't need to call things on the subclass, and it would be nice if we just had a `GraphicsContext`). I don't want to de-stack-allocate all of the GraphicsContexts in this patch, though, that's a separate similarly large change :) Also I don't think it's SO crazy to say `GraphicsContextCG` when *you are passing it a CGContextRef*. The code is already platform-specific.
Tim Horton
Comment 22
2021-05-25 16:32:41 PDT
(Windows being the painfully obvious counterpoint to my last point there)
EWS
Comment 23
2021-05-25 17:01:37 PDT
Committed
r278062
(
238144@main
): <
https://commits.webkit.org/238144@main
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 429698
[details]
.
Radar WebKit Bug Importer
Comment 24
2021-05-25 17:02:32 PDT
<
rdar://problem/78483981
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug