[webkit-reviews] review granted: [Bug 199402] [GTK] Crash in webkitWebViewBaseRenderHostFileDescriptor : [Attachment 374372] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 199402: [GTK] Crash in webkitWebViewBaseRenderHostFileDescriptor
https://bugs.webkit.org/show_bug.cgi?id=199402

Attachment 374372: Patch

https://bugs.webkit.org/attachment.cgi?id=374372&action=review




--- Comment #5 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 374372
  --> https://bugs.webkit.org/attachment.cgi?id=374372
Patch

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

> Source/WebKit/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:76
> -std::unique_ptr<AcceleratedBackingStoreWayland>
AcceleratedBackingStoreWayland::create(WebPageProxy& webPage)
> +bool AcceleratedBackingStoreWayland::checkRequirements()
>  {
>  #if USE(WPE_RENDERER)
>      if (!glImageTargetTexture2D) {
>	   if
(!wpe_fdo_initialize_for_egl_display(PlatformDisplay::sharedDisplay().eglDispla
y()))
> -	       return nullptr;
> +	       return false;
>  
>	   std::unique_ptr<WebCore::GLContext> eglContext =
GLContext::createOffscreenContext();
>	   if (!eglContext)
> -	       return nullptr;
> +	       return false;

Hmm, you sure this is a good idea? This does a lot of serious work under the
guise of a checkRequirements function. I don't think a reasonable programmer
calling AcceleratedBackingStore::checkRequirements would expect a function with
this name to be setting up graphics resources like this. I think it's only a
naming problem; you could call it AcceleratedBackingStore::setup or something
like that, for example. (Although that too is an odd name for a static
method....)