[webkit-reviews] review granted: [Bug 66154] Ownership of canvas's GraphicsContext3D should be moved to PlatformContextSkia : [Attachment 103790] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Stephen White
<senorblanco at chromium.org>'s request for review:
Bug 66154: Ownership of canvas's GraphicsContext3D should be moved to
PlatformContextSkia
https://bugs.webkit.org/show_bug.cgi?id=66154

Attachment 103790: Patch
https://bugs.webkit.org/attachment.cgi?id=103790&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=103790&action=review


>>> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:40
>>> +	 static RefPtr<GraphicsContext3D> context =
GraphicsContext3D::create(attributes, window);
>> 
>> What happens if the window that the GraphicsContext3D is initially created
against goes away?
>> 
>> What happens if the GPU process crashes (context is lost)?
> 
> Good questions.
> 
> All canvases in the same rendering process currently use the same context
(not new to this patch), which means the hostWindow is "wrong" for some of
them.  Apparently, that is ok since the only thing it's used for at creation is
to get the active URL for logging.  (Looking a bit further, it seems we also
use it to determine of accelerated compositing is active.  I'm not sure if we
ever get into a state where that WebViewImpl can be non-composited while the
original one was, not sure, but also not new to this patch).
> 
> Canvas in general cannot handle context loss, and sets the appropriate flag
in context creation, but I'm not sure if we should be doing something further. 
I'll try nuking the GPU process before and after this patch to see if there's
something I missed.

OK. Please give this a little stress testing but otherwise this sounds fine to
me.