[webkit-reviews] review denied: [Bug 207756] [WPE][GTK] API for pause/resume rendering : [Attachment 397456] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Miguel Gomez
<magomez at igalia.com>'s request for review:
Bug 207756: [WPE][GTK] API for pause/resume rendering
https://bugs.webkit.org/show_bug.cgi?id=207756

Attachment 397456: Patch

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




--- Comment #45 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 397456
  --> https://bugs.webkit.org/attachment.cgi?id=397456
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4555
> + * Pause DOM objects, animations and media playback.

I think it should be clear in the docs that pause/unpause don't need to be
balanced, if pause is called and it's already paused nothing happens, and if
resume is called and it's not pause nothing happens either.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4584
> +    page.resumeActiveDOMObjectsAndAnimations();
> +    page.resumeAllMediaPlayback();
> +    page.resumeRendering();

I see a problem here, these three have a different behavior:

 - ActiveDOMObjectsAndAnimations: pause/resume don't have any effect if the
page doesn't have a running process.
 - AllMediaPlayback: if the page doesn't have a running process the value is
saved and sent to the web process as WebPageCreationParameters.
 - Rendering: doesn't check its current state and crashes if called without a
drawing area.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4600
> +    return page.areActiveDOMObjectsAndAnimationsSuspended();

And we rely only on ActiveDOMObjectsAndAnimations to decided whether we are
paused or not. If pause is called before the web process has been launched,
this will return false, but media playback will be paused and rendering too (if
we didn't crash).

> Source/WebKit/UIProcess/gtk/WebPageProxyGtk.cpp:200
> +    ASSERT(m_drawingArea);
> +    m_drawingArea->suspendRendering();

I think we should save the value to return early if already suspended from the
UI point of view. I think we should also return early if m_drawingArea is
nullptr and suspend the rendering when the drawing area proxy is created if the
saved value is still true.

> Source/WebKit/UIProcess/gtk/WebPageProxyGtk.cpp:206
> +    ASSERT(m_drawingArea);
> +    m_drawingArea->resumeRendering();

Same here. That way we would behave like media playback.

> Source/WebKit/UIProcess/wpe/WebPageProxyWPE.cpp:115
> +void WebPageProxy::suspendRendering()
> +{
> +    ASSERT(m_drawingArea);
> +    m_drawingArea->suspendRendering();
> +}
> +
> +void WebPageProxy::resumeRendering()
> +{
> +    ASSERT(m_drawingArea);
> +    m_drawingArea->resumeRendering();
> +}

In this case I think it would be better to add ifdefs to WebPageProxy or add
WebPageProxyGLib.cpp instead of duplicating the code.

>
Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGrap
hics.cpp:481
> +void DrawingAreaCoordinatedGraphics::suspendRendering()
> +{
> +    suspendPainting();
> +}
> +
> +void DrawingAreaCoordinatedGraphics::resumeRendering()
> +{
> +    resumePainting();
> +}

Can we just call the IPC messages SuspendPainting and ResumePainting? Then we
don't even need this, I guess.