| Summary: | Add an internal setting to enable or disable canvas rendering in the GPU Process | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||
| Component: | Canvas | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bdakin, cdumez, commit-queue, dino, esprehn+autocc, ews-watchlist, gyuyoung.kim, sabouhallawa, sam, thorton, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Wenson Hsieh
2020-02-28 07:33:19 PST
Created attachment 391978 [details]
Patch
Comment on attachment 391978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391978&action=review > Source/WebCore/page/Settings.yaml:726 > +useRemoteDrawingForCanvas: > + initial: false This doesn't seem like something WebCore should know or care about. Comment on attachment 391978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391978&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:899 > + bool usesRemoteDrawing = document().settings().useRemoteDrawingForCanvas(); Please remove the FIXME comment. >> Source/WebCore/page/Settings.yaml:726 >> + initial: false > > This doesn't seem like something WebCore should know or care about. Can you please be more explict? The plan is to enable remote rendering for Canvas drawing only. How can we know outside the HTMLCanvasElement::create() that we want to create an ImageBuffer for remote drawing? Should we add a function like this one: HostWindow::createImageBufferForCanvas(...) And call it from HTMLCanvasElement::createImageBuffer()? Comment on attachment 391978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391978&action=review >> Source/WebCore/html/HTMLCanvasElement.cpp:899 >> + bool usesRemoteDrawing = document().settings().useRemoteDrawingForCanvas(); > > Please remove the FIXME comment. Oops — good catch! >>> Source/WebCore/page/Settings.yaml:726 >>> + initial: false >> >> This doesn't seem like something WebCore should know or care about. > > Can you please be more explict? > > The plan is to enable remote rendering for Canvas drawing only. How can we know outside the HTMLCanvasElement::create() that we want to create an ImageBuffer for remote drawing? > > Should we add a function like this one: > > HostWindow::createImageBufferForCanvas(...) > > And call it from HTMLCanvasElement::createImageBuffer()? Interesting...my understanding was that the GPU Process was not something WebCore should care about, but that the notion of "remote drawing" was okay (hence, RemoteAccelerated on RenderingMode). Comment on attachment 391978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391978&action=review >>>> Source/WebCore/page/Settings.yaml:726 >>>> + initial: false >>> >>> This doesn't seem like something WebCore should know or care about. >> >> Can you please be more explict? >> >> The plan is to enable remote rendering for Canvas drawing only. How can we know outside the HTMLCanvasElement::create() that we want to create an ImageBuffer for remote drawing? >> >> Should we add a function like this one: >> >> HostWindow::createImageBufferForCanvas(...) >> >> And call it from HTMLCanvasElement::createImageBuffer()? > > Interesting...my understanding was that the GPU Process was not something WebCore should care about, but that the notion of "remote drawing" was okay (hence, RemoteAccelerated on RenderingMode). I'm with Sam, doesn't seem like we need to infect WebCore with this. Just hide it behind ChromeClient like everything else (see e.g.: // Allows ports to customize the type of graphics layers created by this page. virtual GraphicsLayerFactory* graphicsLayerFactory() const { return nullptr; } ) Comment on attachment 391978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391978&action=review >>>>> Source/WebCore/page/Settings.yaml:726 >>>>> + initial: false >>>> >>>> This doesn't seem like something WebCore should know or care about. >>> >>> Can you please be more explict? >>> >>> The plan is to enable remote rendering for Canvas drawing only. How can we know outside the HTMLCanvasElement::create() that we want to create an ImageBuffer for remote drawing? >>> >>> Should we add a function like this one: >>> >>> HostWindow::createImageBufferForCanvas(...) >>> >>> And call it from HTMLCanvasElement::createImageBuffer()? >> >> Interesting...my understanding was that the GPU Process was not something WebCore should care about, but that the notion of "remote drawing" was okay (hence, RemoteAccelerated on RenderingMode). > > I'm with Sam, doesn't seem like we need to infect WebCore with this. Just hide it behind ChromeClient like everything else (see e.g.: > > // Allows ports to customize the type of graphics layers created by this page. > virtual GraphicsLayerFactory* graphicsLayerFactory() const { return nullptr; } > > ) Okay, I see — it seems we should (at the very least) be asking the client layer for a RenderingMode, then, so it never needs to have a notion of Remote{Acclerated|Unaccelerated} at all. Created attachment 392030 [details]
Patch
Comment on attachment 392030 [details] Patch Clearing flags on attachment: 392030 Committed r257677: <https://trac.webkit.org/changeset/257677> All reviewed patches have been landed. Closing bug. |