Bug 202244 - Add very basic PageCache support for RTCPeerConnection
Summary: Add very basic PageCache support for RTCPeerConnection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 202293
  Show dependency treegraph
 
Reported: 2019-09-25 15:14 PDT by Chris Dumez
Modified: 2019-09-26 15:20 PDT (History)
13 users (show)

See Also:


Attachments
Patch (9.97 KB, patch)
2019-09-25 16:19 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (12.47 KB, patch)
2019-09-25 19:32 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2019-09-25 15:14:33 PDT
Add very basic PageCache support for RTCPeerConnection.
Comment 1 Chris Dumez 2019-09-25 16:19:49 PDT
Created attachment 379588 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2019-09-25 16:20:19 PDT
<rdar://problem/55723524>
Comment 3 Alex Christensen 2019-09-25 17:12:01 PDT
Comment on attachment 379588 [details]
Patch

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

> Source/WebCore/Modules/mediastream/RTCPeerConnection.h:190
> +        promise.whenSettled([pendingActivity = makeRefPtr(makePendingActivity(*this).ptr())] { });

Why not just make whenSettled take a WTF::Function?
Comment 4 Chris Dumez 2019-09-25 19:32:00 PDT
Created attachment 379613 [details]
Patch
Comment 5 Geoffrey Garen 2019-09-25 20:51:05 PDT
Comment on attachment 379613 [details]
Patch

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

r=me

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:502
> +    // FIXME: We should do better here, it is way too easy to prevent PageCache.
>  bool RTCPeerConnection::canSuspendForDocumentSuspension() const
>  {
>      return !hasPendingActivity();

I kinda think we should be able to remove this. Pending promised will be resolved before any navigation. And events are OK to delay.
Comment 6 Chris Dumez 2019-09-25 20:55:49 PDT
(In reply to Geoffrey Garen from comment #5)
> Comment on attachment 379613 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379613&action=review
> 
> r=me
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:502
> > +    // FIXME: We should do better here, it is way too easy to prevent PageCache.
> >  bool RTCPeerConnection::canSuspendForDocumentSuspension() const
> >  {
> >      return !hasPendingActivity();
> 
> I kinda think we should be able to remove this. Pending promised will be
> resolved before any navigation. And events are OK to delay.

Promises are guaranteed to be resolved before navigating away? What if I call a JS function that returns a promise from inside the pagehide event handler?
Comment 7 Geoffrey Garen 2019-09-25 21:00:21 PDT
> Promises are guaranteed to be resolved before navigating away? What if I
> call a JS function that returns a promise from inside the pagehide event
> handler?

Maybe I misread this code. I guess this is a set of promises that will resolve when future events happen? Yeah, I guess we would need to implement an explicit way to delay them if so.
Comment 8 WebKit Commit Bot 2019-09-25 22:32:07 PDT
Comment on attachment 379613 [details]
Patch

Clearing flags on attachment: 379613

Committed r250379: <https://trac.webkit.org/changeset/250379>
Comment 9 WebKit Commit Bot 2019-09-25 22:32:09 PDT
All reviewed patches have been landed.  Closing bug.