Bug 207756

Created attachment 390757 [details]
Patch

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Comment on attachment 390757 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Ever :)

What problem was that causing? I don't understand the change.

> Source/WebCore/ChangeLog:14
> +        media element is resumed.

Nice!

> Source/WebCore/html/HTMLMediaElement.h:1215
> +    bool m_shouldUnpauseOnResume { false };

m_shouldPlaybackOnResume ?

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1056
> +     * paused or not.

s/paused or not/paused/.

> Source/cmake/OptionsWPE.cmake:82
> +WEBKIT_OPTION_DEFINE(RENDERING_PAUSE_SIGNAL_ID "number to use for rendering pause signal or 'OFF'" PRIVATE "OFF")

Signal number used to pause rendering, or OFF.

> Source/cmake/OptionsWPE.cmake:83
> +WEBKIT_OPTION_DEFINE(RENDERING_RESUME_SIGNAL_ID "number to use for rendering resume signal or 'OFF'" PRIVATE "OFF")

Ditto.

> ChangeLog:9
> +        configure the background events simulation triggering with POSIX

Why is it beneficial to change these values at build time via CMake rather than as named constants in the source? I don't understand why someone would want RENDERING_PAUSE_SIGNAL=ON and RENDERING_RESUME_SIGNAL=OFF for example, perhaps a more user-friendly option like "INSTALL_RENDERER_PAUSE_SIGNALS)" would be better, rather than exposing two options that both must be set.
Also, "background events simulation triggering" is a tad awkward, I'd be more consistent with your terminology and say ".. to pause rendering of active DOM objects with POSIX signals."

Comment on attachment 390757 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        Ever :)
> 
> What problem was that causing? I don't understand the change.

The task was scheduled but never triggered. I wonder why the build wasn't failing because of this issue, but I'm not an expert in C++ templates.

>> ChangeLog:9
>> +        configure the background events simulation triggering with POSIX
> 
> Why is it beneficial to change these values at build time via CMake rather than as named constants in the source? I don't understand why someone would want RENDERING_PAUSE_SIGNAL=ON and RENDERING_RESUME_SIGNAL=OFF for example, perhaps a more user-friendly option like "INSTALL_RENDERER_PAUSE_SIGNALS)" would be better, rather than exposing two options that both must be set.
> Also, "background events simulation triggering" is a tad awkward, I'd be more consistent with your terminology and say ".. to pause rendering of active DOM objects with POSIX signals."

There are 2 options so that the unix signal IDs for pause/resume can be configured.

Comment on attachment 390757 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:5746
> +        if (!m_pausedInternal) {
> +            m_shouldUnpauseOnResume = true;
> +            setPausedInternal(true);
> +        }

I think it would be a good idea to not fall through here and break.

> Source/WebCore/html/HTMLMediaElement.cpp:5765
> +    if (m_shouldUnpauseOnResume) {
> +        m_shouldUnpauseOnResume = false;
> +        setPausedInternal(false);
> +    }

Ditto

>> Source/WebCore/html/HTMLMediaElement.h:1215
>> +    bool m_shouldUnpauseOnResume { false };
> 
> m_shouldPlaybackOnResume ?

Bikeshedding a bit, for me it would be m_shouldPlayOnResume

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:742
> +static gboolean s_sendPauseRenderingPending = FALSE;
> +static gboolean s_sendResumeRenderingPending = FALSE;

I see no need in using gboolean instead of bool.

>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1056
>> +     * paused or not.
> 
> s/paused or not/paused/.

Yes, too many "nots"

>>> ChangeLog:9
>>> +        configure the background events simulation triggering with POSIX
>> 
>> Why is it beneficial to change these values at build time via CMake rather than as named constants in the source? I don't understand why someone would want RENDERING_PAUSE_SIGNAL=ON and RENDERING_RESUME_SIGNAL=OFF for example, perhaps a more user-friendly option like "INSTALL_RENDERER_PAUSE_SIGNALS)" would be better, rather than exposing two options that both must be set.
>> Also, "background events simulation triggering" is a tad awkward, I'd be more consistent with your terminology and say ".. to pause rendering of active DOM objects with POSIX signals."
> 
> There are 2 options so that the unix signal IDs for pause/resume can be configured.

Agree with Charlie here.

Comment on attachment 390757 [details]
Patch

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

>>>> ChangeLog:9
>>>> +        configure the background events simulation triggering with POSIX
>>> 
>>> Why is it beneficial to change these values at build time via CMake rather than as named constants in the source? I don't understand why someone would want RENDERING_PAUSE_SIGNAL=ON and RENDERING_RESUME_SIGNAL=OFF for example, perhaps a more user-friendly option like "INSTALL_RENDERER_PAUSE_SIGNALS)" would be better, rather than exposing two options that both must be set.
>>> Also, "background events simulation triggering" is a tad awkward, I'd be more consistent with your terminology and say ".. to pause rendering of active DOM objects with POSIX signals."
>> 
>> There are 2 options so that the unix signal IDs for pause/resume can be configured.
> 
> Agree with Charlie here.

I suppose with some CMake magic a single option with a comma-separated value could be used, but it sounds harder to understand than, say, "-DRENDERING_PAUSE_SIGNAL_ID=35 -DRENDERING_RESUME_SIGNAL_ID=36"

Comment on attachment 390757 [details]
Patch

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

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewWPE.cpp:231
> + * Since: 2.28

As the 2.28 cycle is closing, it seems more prudent to wait 2.30 to enable this API.

Comment on attachment 390757 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.h:962
> +    DeferrableTask<Timer> m_updatePlayStateTask;

Can you split that change?
The goal here is to make sure that we use HTMLMediaElement::enqueueTaskForDispatcher which queues a task to the event loop.
I am not sure why this is not working for you.

Comment on attachment 390757 [details]
Patch

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

Wouldn't it be nice to use D-Bus instead, or literally any imaginable form of IPC other than POSIX signals?

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:841
> +#if HAVE_SIGNAL_H && PLATFORM(WPE) && defined(RENDERING_PAUSE_SIGNAL) && defined(RENDERING_RESUME_SIGNAL)
> +    void (*pauseHandler)(int) = signal(RENDERING_PAUSE_SIGNAL, wpeUIProcessSignalHandler);
> +    if (pauseHandler != SIG_DFL)
> +        signal(RENDERING_PAUSE_SIGNAL, pauseHandler);
> +
> +    void (*resumeHandler)(int) = signal(RENDERING_RESUME_SIGNAL, wpeUIProcessSignalHandler);
> +    if (resumeHandler != SIG_DFL)
> +        signal(RENDERING_RESUME_SIGNAL, resumeHandler);
> +
> +    priv->signalCheckSourceID = g_idle_add_full(G_PRIORITY_DEFAULT, wpeCheckSignals, g_object_ref(webView), g_object_unref);
> +#endif

If you must use POSIX signals, please use g_unix_signal_source_new() so that that you can do your work in the "signal handler" without the need for s_sendPauseRenderingPending/s_sendResumeRenderingPending or worrying about async-signal safety, and so that you don't need this constantly-running idle source. (Doesn't the always-triggering idle source cause CPU churn?)

P.S. I'm not sure if WPE has thread usage restrictions like GTK does? Since we're a library, it's better to manually attach your source to the thread-default main context. I checked all of WebKit/Source and we don't use g_idle_add() nor g_timeout_add() anywhere, so seems better not start now. That's why I did not suggest g_unix_signal_add().

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1064
> +            "is-rendering-pause",

is-rendering-paused

> Source/WebKit/UIProcess/API/wpe/WebKitWebView.h:562
> +WEBKIT_API void
> +webkit_web_view_set_is_rendering_paused              (WebKitWebView *web_view,
> +                                                      gboolean value);
> +
> +WEBKIT_API gboolean
> +webkit_web_view_is_rendering_paused                  (WebKitWebView *web_view);

Parameter alignment should follow GObject style

>> Source/cmake/OptionsWPE.cmake:83
>> +# iOS/Android backgrounding events simulation with POSIX signals.
>> +WEBKIT_OPTION_DEFINE(RENDERING_PAUSE_SIGNAL_ID "number to use for rendering pause signal or 'OFF'" PRIVATE "OFF")
>> +WEBKIT_OPTION_DEFINE(RENDERING_RESUME_SIGNAL_ID "number to use for rendering resume signal or 'OFF'" PRIVATE "OFF")
> 
> Ditto.

If I were picking signals to use here, I'd pick SIGUSR1 and SIGUSR2 and would presumably stomp on JSC thread suspend/resume. Maybe fail the build if someone tries to use SIGUSR1?

Comment on attachment 390757 [details]
Patch

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

>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:841
>> +#endif
> 
> If you must use POSIX signals, please use g_unix_signal_source_new() so that that you can do your work in the "signal handler" without the need for s_sendPauseRenderingPending/s_sendResumeRenderingPending or worrying about async-signal safety, and so that you don't need this constantly-running idle source. (Doesn't the always-triggering idle source cause CPU churn?)
> 
> P.S. I'm not sure if WPE has thread usage restrictions like GTK does? Since we're a library, it's better to manually attach your source to the thread-default main context. I checked all of WebKit/Source and we don't use g_idle_add() nor g_timeout_add() anywhere, so seems better not start now. That's why I did not suggest g_unix_signal_add().

About g_unix_signal_source_new(), it's limited to a hardcoded set of signals, hence useless here. I don't think using SIGUSR would be wanted here. The use-case for these *opt-in at build-time* signals is that some application at the OS level would send user-defined signals to all running apps. This behavior is inspired by this SDL commit, https://hg.libsdl.org/SDL/rev/779b63fc4acb but I think other libs have a similar feature.

Comment on attachment 390757 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.h:962
>> +    DeferrableTask<Timer> m_updatePlayStateTask;
> 
> Can you split that change?
> The goal here is to make sure that we use HTMLMediaElement::enqueueTaskForDispatcher which queues a task to the event loop.
> I am not sure why this is not working for you.

Ok I understand now what's going on :) In EventLoop::run() the task is skipped because its group is flagged as suspended.
So instead of scheduling a task I wonder if I can directly pause playback from HTMLMediaElement::suspend().

Comment on attachment 390757 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.cpp:5765
>> +    }
> 
> Ditto

Ditto of? This is not a switch().

Created attachment 390913 [details]
Patch

Comment on attachment 390757 [details]
Patch

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

>>> Source/WebCore/html/HTMLMediaElement.cpp:5765
>>> +    }
>> 
>> Ditto
> 
> Ditto of? This is not a switch().

Right!

Comment on attachment 390913 [details]
Patch

First of all, I really don't like using signals for this. Signals are
troublesome for many reasons (many of them outlined in the comments
below) and furthermore it is not the business of a library to decide
how a program will behave depending on how the library was built.

Now, on the positive side: I do truly believe that something along the
lines of this patch can be useful in many scenarios, and I would love
to see the functionality landed in WPE (and also the GTK port!) in some
form. Adding API to allow an application to suspend pages (= webviews)
is something that would allow for example to let browsers (like GNOME
Web, or Cog) to completely or partially suspend/throttle pages which
are inactive (for example: non visible browser tabs).

My preference regarding would be to see this patch in WebKit *without*
the signal handling parts. If any application needs to do it using
signals for whatever reason, it's the application where the code
belongs, and if the application cannot be modified, people are still
free to use a patched build of WebKit—but I am convinced that signal
handling must be avoided at all costs in upstream WebKit.

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

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:310
> +    unsigned signalCheckSourceID { 0 };

It does not seem like a great idea to have one source per web view,
when signals are global things. I think it makes more sense to have
a singleton source, and register/unregister web views from a list
of web views to (un)suspend inside webkitWebViewConstructed() and
webkitWebViewDispose().

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:747
> +    signal(sig, wpeUIProcessSignalHandler);

Why is this needed? Signal handlers are never reset automatically,
one has to explicitly call signal(signum, SIG_DFL) to go back to the
default signal handler.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:752
> +        s_sendResumeRenderingPending = true;

else
        UNREACHABLE();

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:831
> +#if HAVE_SIGNAL_H && PLATFORM(WPE) && defined(RENDERING_PAUSE_SIGNAL) && defined(RENDERING_RESUME_SIGNAL)

This could be as well be in the GTK port, I think we should not have
PLATFORM(WPE) guards here.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:832
> +    void (*pauseHandler)(int) = signal(RENDERING_PAUSE_SIGNAL, wpeUIProcessSignalHandler);

It's very unfortunate g_unix_signal_source_new() only allows a small
subset of all possible signal numbers—that is quite silly. At least
if we are going to do signal handling manually, please use sigaction()
because it's safer to than plain ol' signal()... with sigaction() you
must set the SA_RESTART flag, so system calls which might have been
interrupted when the signal handler is called will be restarted—this
is unavailable with signal()—and also with sigaction() the signal being
delivered is temporarily added to the thread's signal mask to avoid
re-deliveries of the signal while the signal is being processed.

Even better would be to use signalfd() on Linux, and then handle that
file descriptor with a GSource created using g_unix_fd_source_new()
in the event loop—this completely bypasses the problem that there are
not that many signal-safe functions. JFTR, the equivalent for *BSD
would be to use kqueue() and an event type with EVFILT_SIGNAL.

I think at least changing the code to use sigaction() is a must.

(Yes, using Unix signals is a perilous affair. TL;DR: never do it
if you can avoid it.)

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:834
> +        signal(RENDERING_PAUSE_SIGNAL, pauseHandler);

This code seems unnecessarily complicated to understand. Why do you even need
to do this? It looks like the idea is to try to use wpeUIProcessSignalHandler
but if there was some signal handler already registered, then configure it
back.

This seems like a terrible idea because if, let's say, a program accidentally
registers a handler for the resume signal, then it's possible to send a signal
to pause rendering, but it won't be possible to ever resume it!

If you want to keep existing signal handlers working, what you have to do
is keep a reference to them around, and invoke them from your handler in
turn. If you use a global handler as suggested, it's much easier to keep
a single sighandler_t value around for the existing handler, then do:

  static const sighandler_t s_existingSuspendSignalHandler = SIG_DFL;

  static void wpeUIProcessSignalHandler(int sig) {
      if (sig == RENDERING_PAUSE_SIGNAL) {
          s_sendPauseRenderingPending = true;
          if (s_existingSuspendSignalHandler != SIG_DFL)
              s_existingSuspendSignalHandler(sig);
      } else if (sig == RENDERING_RESUME_SIGNAL) {
          // Ditto for resuming.
      } else {
          UNREACHABLE();
      }
  }

Then, when registering the signal:

  s_existingSuspendSignalHandler = signal(REDNERING_SUSPEND_SIGNAL, wpeUIProcessSignalHandler);

This way you never need to play whack-a-mole with the installed signal
handlers, and previously existing handlers will be called without being
in a situation where one of the handlers is the WebKit-provided one and
another is the user-provided one.

Personally, I would go one step further, and use sigaction() passing
NULL as second parameter to obtain the current signal handler information
without changing it, and it is other than SIG_DFL for either signal, I
would just exit forcibly with an error: whoever thinks is a good idea to
use signals for this must let WebKit handle the signals.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:998
> +    }

You can change these for lines to just:

    g_clear_handle_id(&webView->priv->signalCheckSourceID, g_source_remove);

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewWPE.cpp:244
> +    }

Why is WebPage::setIsSuspended() not being used here? I would imagine that
one wants the whole page suspending, and not just media playback. If only
media playback suspension is desired in some cases, then wouldn't it be
better to use a tristate value here (Unpaused, Paused, MediaPaused)?
Then the property would be an “enum” instead of a bool/gboolean value.

> Source/cmake/OptionsWPE.cmake:215
> +    if (CMAKE_MATCH_1 LESS 32)

This check can fail for valid signals in the [SIGRTMIN, SIGRTMAX]
interval, which usually (but not always) has values >32. I would
remove this check and instead only check that the values are:

 - Greater than zero (because zero is not a valid signal number).
 - Different than SIGKILL (because it cannot be handled).
 - Different than SIGBUS or SIGSEGV (because those are used for
   signaling invalid access to memory, we would rather have the
   default bahavior of coredump+abort for those).
 - Different from SIGABRT (because we want abort() calls to work).
 - Different from SIGFPE and SIGILL (similar reasoning as above).
 - Different from SIGTRAP (we want debuggers to always work).
 - Different from SIGALMR (which is used by JSC's profiler code).
 - Different from SIGCLD (used to watch for exited child processes).

Comment on attachment 390913 [details]
Patch

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

>> Source/WebKit/UIProcess/API/wpe/WebKitWebViewWPE.cpp:244
>> +    }
> 
> Why is WebPage::setIsSuspended() not being used here? I would imagine that
> one wants the whole page suspending, and not just media playback. If only
> media playback suspension is desired in some cases, then wouldn't it be
> better to use a tristate value here (Unpaused, Paused, MediaPaused)?
> Then the property would be an “enum” instead of a bool/gboolean value.

setIsSuspended() seems more related with PSON? Besides, I don't see it called by any component.
Also, this property is not only about media playback... Other things get suspended, such as animations and WebGL.

Created attachment 390922 [details]
Patch

There are some existing mechanisms that might be worth piggy-backing.
See for instance WebPageProxy suspendAllMediaPlayback/resumeAllMediaPlayback.

That seems to align pretty well with your intent here, although you also want to resume/suspend other things.
Maybe a dedicated API or extending the current mechanism would be good.

(In reply to youenn fablet from comment #18)
> There are some existing mechanisms that might be worth piggy-backing.
> See for instance WebPageProxy suspendAllMediaPlayback/resumeAllMediaPlayback.
> 
> That seems to align pretty well with your intent here, although you also
> want to resume/suspend other things.
> Maybe a dedicated API or extending the current mechanism would be good.

I see that is what you are using in the latest patch, great!

Comment on attachment 390922 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:5772
> +#endif

Do we still need these specific GStreamer changes now that we are using resumeAllMediaPlayback/suspendAllMediaPlayback

Comment on attachment 390922 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:5756
>          // Do nothing, we don't pause media playback in these cases.

Highlight on this comment.

>> Source/WebCore/html/HTMLMediaElement.cpp:5772
>> +#endif
> 
> Do we still need these specific GStreamer changes now that we are using resumeAllMediaPlayback/suspendAllMediaPlayback

Yes, other ports don't do anything ^^

(In reply to youenn fablet from comment #19)
> (In reply to youenn fablet from comment #18)
> > There are some existing mechanisms that might be worth piggy-backing.
> > See for instance WebPageProxy suspendAllMediaPlayback/resumeAllMediaPlayback.
> > 
> > That seems to align pretty well with your intent here, although you also
> > want to resume/suspend other things.
> > Maybe a dedicated API or extending the current mechanism would be good.
> 
> I see that is what you are using in the latest patch, great!

I was using it all along :)

> >> Source/WebCore/html/HTMLMediaElement.cpp:5772
> >> +#endif
> > 
> > Do we still need these specific GStreamer changes now that we are using resumeAllMediaPlayback/suspendAllMediaPlayback
> 
> Yes, other ports don't do anything ^^

Which part is missing there? Should it be implemented?

Looking at the code, most of it seems generic:
Document::suspendAllMediaPlayback -> PlatformMediaSessionManager::suspendAllMediaPlaybackForDocument -> PlatformMediaSession::beginInterruption -> HTMLMediaElement::suspendPlayback

(In reply to youenn fablet from comment #23)
> > >> Source/WebCore/html/HTMLMediaElement.cpp:5772
> > >> +#endif
> > > 
> > > Do we still need these specific GStreamer changes now that we are using resumeAllMediaPlayback/suspendAllMediaPlayback
> > 
> > Yes, other ports don't do anything ^^
> 
> Which part is missing there? 

Without my gst-specific changes, pause() is indeed called, but returns early:

Feb 19 14:16:40 ecto-1 WPEWebProcess[1858493]: HTMLMediaElement::pause(3A006CA22DCC0341)
Feb 19 14:16:40 ecto-1 WPEWebProcess[1858493]: MediaElementSession::playbackPermitted(3A006CA22DCC0341) Returning FALSE because element is suspended

> Should it be implemented?
> 

I wasn't sure what's the intended behavior on mac ports should be.

> Looking at the code, most of it seems generic:
> Document::suspendAllMediaPlayback ->
> PlatformMediaSessionManager::suspendAllMediaPlaybackForDocument ->
> PlatformMediaSession::beginInterruption -> HTMLMediaElement::suspendPlayback

Does it work on mac?

Stack trace btw:

Feb 19 14:41:12 ecto-1 WPEWebProcess[1869965]: 1   0x7f3c8aeb2665 WebCore::HTMLMediaElement::pause()
Feb 19 14:41:12 ecto-1 WPEWebProcess[1869965]: 2   0x7f3c8ae9762e WebCore::HTMLMediaElement::suspendPlayback()
Feb 19 14:41:12 ecto-1 WPEWebProcess[1869965]: 3   0x7f3c8b31d228 WebCore::PlatformMediaSession::beginInterruption(WebCore::PlatformMediaSession::InterruptionType)
Feb 19 14:41:12 ecto-1 WPEWebProcess[1869965]: 4   0x7f3c8b317e0b WebCore::PlatformMediaSessionManager::forEachMatchingSession(WTF::Function<bool (WebCore::PlatformMediaSession const&)> const&, WTF::Function<vo>
Feb 19 14:41:12 ecto-1 WPEWebProcess[1869965]: 5   0x7f3c8b317eda WebCore::PlatformMediaSessionManager::forEachDocumentSession(WTF::ObjectIdentifier<WebCore::DocumentIdentifierType>, WTF::Function<void (WebCore>
Feb 19 14:41:12 ecto-1 WPEWebProcess[1869965]: 6   0x7f3c8b317f96 WebCore::PlatformMediaSessionManager::suspendAllMediaPlaybackForDocument(WTF::ObjectIdentifier<WebCore::DocumentIdentifierType>)
Feb 19 14:41:12 ecto-1 WPEWebProcess[1869965]: 7   0x7f3c8b2217c9 WebCore::Page::forEachDocument(WTF::Function<void (WebCore::Document&)> const&) const
Feb 19 14:41:13 ecto-1 WPEWebProcess[1869965]: 8   0x7f3c8b2228ee WebCore::Page::suspendAllMediaPlayback()
Feb 19 14:41:13 ecto-1 WPEWebProcess[1869965]: 9   0x7f3c89ccdbb7 WebKit::WebPage::didReceiveWebPageMessage(IPC::Connection&, IPC::Decoder&)
Feb 19 14:41:13 ecto-1 WPEWebProcess[1869965]: 10  0x7f3c89dc87da IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&)
Feb 19 14:41:13 ecto-1 WPEWebProcess[1869965]: 11  0x7f3c89ff3397 WebKit::WebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
Feb 19 14:41:13 ecto-1 WPEWebProcess[1869965]: 12  0x7f3c89dc1668 IPC::Connection::dispatchMessage(IPC::Decoder&)
Feb 19 14:41:13 ecto-1 WPEWebProcess[1869965]: 13  0x7f3c89dc2955 IPC::Connection::dispatchMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >)
Feb 19 14:41:13 ecto-1 WPEWebProcess[1869965]: 14  0x7f3c89dc3010 IPC::Connection::dispatchOneIncomingMessage()
Feb 19 14:41:13 ecto-1 WPEWebProcess[1869965]: 15  0x7f3c8cb56efc WTF::RunLoop::performWork()
Feb 19 14:41:13 ecto-1 WPEWebProcess[1869965]: 16  0x7f3c8cbbbd19
Feb 19 14:41:13 ecto-1 WPEWebProcess[1869965]: 17  0x7f3c8757b91f g_main_context_dispatch
Feb 19 14:41:13 ecto-1 WPEWebProcess[1869965]: 18  0x7f3c8757bcb0
Feb 19 14:41:13 ecto-1 WPEWebProcess[1869965]: 19  0x7f3c8757bfc3 g_main_loop_run
Feb 19 14:41:13 ecto-1 WPEWebProcess[1869965]: 20  0x7f3c8cbbc7e0 WTF::RunLoop::run()
Feb 19 14:41:13 ecto-1 WPEWebProcess[1869965]: 21  0x7f3c8a0dd627 WebKit::WebProcessMain(int, char**)
Feb 19 14:41:13 ecto-1 WPEWebProcess[1869965]: 22  0x7f3c8798cbbb __libc_start_main
Feb 19 14:41:13 ecto-1 WPEWebProcess[1869965]: 23  0x560de01ea7da

Created attachment 391155 [details]
WIP

With this change I have suspend working without port-specific ifdefs.

Comment on attachment 390922 [details]
Patch

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

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewWPE.cpp:240
> +        page.suspendAllMediaPlayback();

So MediaElementSession::playbackPermitted() is probably returning false in your case.
What if you call suspendAllMediaPlayback first and suspendActiveDOMObjectsAndAnimations() second?

(In reply to youenn fablet from comment #27)
> Comment on attachment 390922 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390922&action=review
> 
> > Source/WebKit/UIProcess/API/wpe/WebKitWebViewWPE.cpp:240
> > +        page.suspendAllMediaPlayback();
> 
> So MediaElementSession::playbackPermitted() is probably returning false in
> your case.
> What if you call suspendAllMediaPlayback first and
> suspendActiveDOMObjectsAndAnimations() second?

Well. Now that you mention it indeed :) Suspending media before the DOM objects clearly makes more sense. And resuming DOM objects before media perhaps. I'll check tomorrow. Thanks Youenn!

Created attachment 391273 [details]
Patch

Comment on attachment 391273 [details]
Patch

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

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewWPE.cpp:240
> +        page.suspendActiveDOMObjectsAndAnimations();

early return?

Comment on attachment 391273 [details]
Patch

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

Are the WPE port reviewers OK with this new API?

>> Source/WebKit/UIProcess/API/wpe/WebKitWebViewWPE.cpp:240
>> +        page.suspendActiveDOMObjectsAndAnimations();
> 
> early return?

No strong feelings either way. :)

Comment on attachment 391273 [details]
Patch

Patch looks fine, but needs updating the ChangeLog and I have a comment
regarding the public API; it would be great if Carlos García can confirm
that the property should be called “rendering-paused” for consistency,
hence the cq-

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

> Source/WebKit/ChangeLog:14
> +        (wpeCheckSignals):

Please update the ChangeLog entry, as these two functions are not
anymore in the patch :)

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:205
> +#if PLATFORM(WPE)

Is there some reason why this cannot be done for the GTK port as well?

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:999
> +     * WebKitWebView:is-rendering-paused:

Checking other boolean properties in GLib style APIs, they usually don't
have the “is-” prefix, so I would call this just “rendering-paused”.

> Source/WebKit/UIProcess/API/wpe/WebKitWebView.h:562
> +webkit_web_view_is_rendering_paused                  (WebKitWebView               *web_view);

Conversely: webkit_web_view_get_rendering_paused() and
webkit_web_view_set_rendering_paused().

Sorry for the delay commenting about this, I have a several questions (not necessarily for phil, but also for youenn):

 - I find a bit confusing the different suspend methods we have in WebKit and WebCore layers:
  + WebPageProxy::suspendCurrentPageIfPossible: this is only for PSON, right? what's the effect in this case?
  + WebPageProxy::suspendActiveDOMObjectsAndAnimations: does this suspend the painting (layer flush) somehow?
  + WebPageProxy::suspendAllMediaPlayback: why is this separated from the active DOM objects and animations? Is this expected to be used to force a pause of the media instead of suspending the page?
  + WebPage::suspendAllMediaBuffering: do we want to suspend the networking too?
  + Page::suspendScriptedAnimations: this is what we call when suspending painting in the web process, what's the difference with Page::suspendActiveDOMObjectsAndAnimations?
 - We are already suspending the rendering when a page is hidden, what's the use case of this new API? Do we want to suspend pages even if they are currently visible? What's the effect in the web view, do we keep the last frame?

I need to understand the use case of this to review the new API, but I'll add some comments inline.

Comment on attachment 391273 [details]
Patch

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

Patches adding new API must include a unit test.

> Source/WebKit/ChangeLog:10
> +        A new property is added in the WebView to control the rendering
> +        state. When this property is set to TRUE, all animations and media
> +        playback will be paused.

A property is useful when you want to monitor it, to be notified when its value changes, but here you are not emitting the notify signal when it changes, and I don't even know if it's easy to do in this case.

>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:999
>> +     * WebKitWebView:is-rendering-paused:
> 
> Checking other boolean properties in GLib style APIs, they usually don't
> have the “is-” prefix, so I would call this just “rendering-paused”.

Yes, the name sounds more like a read-only property that you can monitor to get notified when the rendering has been pause, more than an action started by you. I wouldn't use a property in this case. I guess suspend/remove are balanced, so setting the property to TRUE twice would require setting it twice to FALSE, which is confusing for a property.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1002
> +     * Whether or not rendering (including media playback, animations, WebGL) is
> +     * paused.

Active DOM objects are not mentioned here, I guess not all active DOM objects are related to rendering, so I would avoid using rendering in the API.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1012
> +            _("Indication of the WebView rendering state"),

This is makes it sound more like a read-only prop to monitor a state.

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewWPE.cpp:229
> + * Pause or resume DOM objects, animations and media playback.

So, what happens if called twice with the same value? I would use webkit_web_view_pause() (or webkit_web_view_suspend()) and webkit_web_view_resume().

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewWPE.cpp:260
> +    return page.areActiveDOMObjectsAndAnimationsSuspended();

We only check active dom objects and animations but not media playback.

Comment on attachment 391273 [details]
Patch

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

>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:205
>> +#if PLATFORM(WPE)
> 
> Is there some reason why this cannot be done for the GTK port as well?

I suppose this would be doable in GTK too, but this patch is not about GTK :)

>>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:999
>>> +     * WebKitWebView:is-rendering-paused:
>> 
>> Checking other boolean properties in GLib style APIs, they usually don't
>> have the “is-” prefix, so I would call this just “rendering-paused”.
> 
> Yes, the name sounds more like a read-only property that you can monitor to get notified when the rendering has been pause, more than an action started by you. I wouldn't use a property in this case. I guess suspend/remove are balanced, so setting the property to TRUE twice would require setting it twice to FALSE, which is confusing for a property.

Attempting to suspend an already suspended page has no effect.

>> Source/WebKit/UIProcess/API/wpe/WebKitWebViewWPE.cpp:229
>> + * Pause or resume DOM objects, animations and media playback.
> 
> So, what happens if called twice with the same value? I would use webkit_web_view_pause() (or webkit_web_view_suspend()) and webkit_web_view_resume().

As mentioned already, nothing happens if this is called twice with the same value.

>> Source/WebKit/UIProcess/API/wpe/WebKitWebViewWPE.cpp:260
>> +    return page.areActiveDOMObjectsAndAnimationsSuspended();
> 
> We only check active dom objects and animations but not media playback.

Right, but as media is suspended/resumed at the same time, I thought simply checking one was enough.

>  - I find a bit confusing the different suspend methods we have in WebKit
> and WebCore layers:
>   + WebPageProxy::suspendCurrentPageIfPossible: this is only for PSON,
> right? what's the effect in this case?

Goal is support b/f cache even in case of process swap, hence different pages in different process.

>   + WebPageProxy::suspendActiveDOMObjectsAndAnimations: does this suspend
> the painting (layer flush) somehow?

I do not think so, though not working on rendering.

>   + WebPageProxy::suspendAllMediaPlayback: why is this separated from the
> active DOM objects and animations? Is this expected to be used to force a
> pause of the media instead of suspending the page?

Right, we have some internal cases doing just that.
This is not a public API though.

>   + WebPage::suspendAllMediaBuffering: do we want to suspend the networking
> too?

It makes sense to me to suspend media buffering when suspending media playback.

>   + Page::suspendScriptedAnimations: this is what we call when suspending
> painting in the web process, what's the difference with
> Page::suspendActiveDOMObjectsAndAnimations?

suspend/resume is an overloaded term in WebKit which makes things difficult to understand.
I believe suspendScriptedAnimations is mostly for RAF and should probably be called as part of/when suspendActiveDOMObjectsAndAnimations is called.

(In reply to Carlos Garcia Campos from comment #33)
>   + WebPage::suspendAllMediaBuffering: do we want to suspend the networking
> too?

Sure!

>   + Page::suspendScriptedAnimations: this is what we call when suspending
> painting in the web process, what's the difference with
> Page::suspendActiveDOMObjectsAndAnimations?
>  - We are already suspending the rendering when a page is hidden, what's the
> use case of this new API?

The use-case was mentioned in comment 10, though reviewers here seem to veto the use of signals, the use-case remains the same.

> Do we want to suspend pages even if they are
> currently visible?

By currently visible, what do you mean?

> What's the effect in the web view, do we keep the last
> frame?
> 

Yes

(In reply to Philippe Normand from comment #35)
> Comment on attachment 391273 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391273&action=review
> 
> >> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:205
> >> +#if PLATFORM(WPE)
> > 
> > Is there some reason why this cannot be done for the GTK port as well?
> 
> I suppose this would be doable in GTK too, but this patch is not about GTK :)

This is adding new GLib API to expose cross-platform functionality, I don't see the reason to make this WPE specific either.

> >>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:999
> >>> +     * WebKitWebView:is-rendering-paused:
> >> 
> >> Checking other boolean properties in GLib style APIs, they usually don't
> >> have the “is-” prefix, so I would call this just “rendering-paused”.
> > 
> > Yes, the name sounds more like a read-only property that you can monitor to get notified when the rendering has been pause, more than an action started by you. I wouldn't use a property in this case. I guess suspend/remove are balanced, so setting the property to TRUE twice would require setting it twice to FALSE, which is confusing for a property.
> 
> Attempting to suspend an already suspended page has no effect.

I has the effect of requiting to set it to FALSE twice, that's not obvious to me when using a property, I alwasy expect to change its value when I set a different one.

> >> Source/WebKit/UIProcess/API/wpe/WebKitWebViewWPE.cpp:229
> >> + * Pause or resume DOM objects, animations and media playback.
> > 
> > So, what happens if called twice with the same value? I would use webkit_web_view_pause() (or webkit_web_view_suspend()) and webkit_web_view_resume().
> 
> As mentioned already, nothing happens if this is called twice with the same
> value.
> 
> >> Source/WebKit/UIProcess/API/wpe/WebKitWebViewWPE.cpp:260
> >> +    return page.areActiveDOMObjectsAndAnimationsSuspended();
> > 
> > We only check active dom objects and animations but not media playback.
> 
> Right, but as media is suspended/resumed at the same time, I thought simply
> checking one was enough.

(In reply to Philippe Normand from comment #37)
> (In reply to Carlos Garcia Campos from comment #33)
> >   + WebPage::suspendAllMediaBuffering: do we want to suspend the networking
> > too?
> 
> Sure!
> 
> >   + Page::suspendScriptedAnimations: this is what we call when suspending
> > painting in the web process, what's the difference with
> > Page::suspendActiveDOMObjectsAndAnimations?
> >  - We are already suspending the rendering when a page is hidden, what's the
> > use case of this new API?
> 
> The use-case was mentioned in comment 10, though reviewers here seem to veto
> the use of signals, the use-case remains the same.

I don't see the use case explained there. The question is what and why, not how. I agree we shouldn't use signals for this, even more if we are adding new API, so apps are free to use signals or whatever. What I don't fully understand yet is why you want to suspend a web view using new API (rendering is already suspended when the web view is hidden, maybe we can just improve that by pausing the media, DOM objects etc, instead of just the scripted animations). And the other question is what do you want to achieve exactly, because I don't see how the rendering is paused in this case, there's no layer flush suspension nor threaded compositor.

> > Do we want to suspend pages even if they are
> > currently visible?
> 
> By currently visible, what do you mean?

A page that is not hidden, the currently visible tab in a browser.

> > What's the effect in the web view, do we keep the last
> > frame?
> > 
> 
> Yes

(In reply to Carlos Garcia Campos from comment #39)
> 
> A page that is not hidden, the currently visible tab in a browser.
> 

How is that supposed to work in a fullscreen, always visible browser like Cog?

(In reply to Philippe Normand from comment #40)
> (In reply to Carlos Garcia Campos from comment #39)
> > 
> > A page that is not hidden, the currently visible tab in a browser.
> > 
> 
> How is that supposed to work in a fullscreen, always visible browser like
> Cog?

That's why I asked if the intention was to suspend visible we views too. I've seen other projects removing the flag wpe_view_activity_state_in_window or wpe_view_activity_state_visible to pause the rendering, for example. I think       it's better to have API for that instead of pretending the web view is hidden when it's not, but the suspension mechanism should be shared with the hidden views, I guess.

Created attachment 397452 [details]
Patch

(In reply to Miguel Gomez from comment #42)
> Created attachment 397452 [details]
> Patch

I talked to cgarcia some things about phil's patch and how we should modify it. We agreed to keep this feature as a pause only, not trying to include here any feature to release resources. This use case is just intended to pause the WebView for a while, and enable it again as fast as possible. So, these are the changes I made:

- Removed the property as it's not needed
- Modified the API methods to pause/unpause so it's cleat they are related (could be changed if people are not happy with it though)
- Stop the compositor loop besides stopping the animations and media playback

Created attachment 397456 [details]
Patch

Comment on attachment 397456 [details]
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/DrawingAreaCoordinatedGraphics.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.