Bug 208375 – Add an internal setting to enable or disable canvas rendering in the GPU Process

• Component: Canvas
• Status: RESOLVED FIXED

[Wenson Hsieh]

SSIA

[Wenson Hsieh]

Created attachment 391978 [details]
Patch

[Sam Weinig]

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.

[Said Abou-Hallawa]

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()?

[Wenson Hsieh]

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).

[Tim Horton]

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; }

)

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 392030 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 392030 [details]
Patch

Clearing flags on attachment: 392030

Committed r257677: <https://trac.webkit.org/changeset/257677>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59911910>