[scheduler] Plumb information about active webrtc connections to scheduler

[scheduler] Plumb information about active webrtc connections to scheduler

Notify scheduler about webrtc connections.

BUG=690929
Review-Url: https://codereview.chromium.org/2683993006
Cr-Commit-Position: refs/heads/master@{#449640}
Committed: https://chromium.googlesource.com/chromium/src/+/407bdb243f9dd1d94f7e99318a7f60df1f421dfa

[altimin, 2017-02-10 14:41:23]

Description was changed from

==========
[scheduler] Plumb information about active webrtc connections to scheduler

BUG=690929
==========

to

==========
[scheduler] Plumb information about active webrtc connections to scheduler

Notify scheduler about webrtc connections.

BUG=690929
==========

[altimin, 2017-02-10 14:41:40]

altimin@chromium.org changed reviewers:
+ guidou@chromium.org

[altimin, 2017-02-10 14:41:40]

PTAL

[altimin, 2017-02-10 14:41:47]

The CQ bit was checked by altimin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 14:42:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 16:10:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 16:10:55]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-02-10 16:51:19]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2017-02-10 16:51:20]

LGTM with a question.

https://codereview.chromium.org/2683993006/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2683993006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:526:
document->frame()->frameScheduler()->onActiveConnectionCreated();

Another possible API would be to use an observer pattern like
ActiveScriptWrappable::hasPendingActivity().

class RTCPeerConnection : public HasUnthrottledTask {
  bool hasUnthrottledTask() override { return !m_closed || !m_stopped; }
};

Then developers don't need to write .reset() in many places. The observer
pattern would be less error-prone than the .reset() pattern.

[altimin, 2017-02-10 16:57:01]

Thanks!

https://codereview.chromium.org/2683993006/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/2683993006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:526:
document->frame()->frameScheduler()->onActiveConnectionCreated();
On 2017/02/10 16:51:20, haraken wrote:
> 
> Another possible API would be to use an observer pattern like
> ActiveScriptWrappable::hasPendingActivity().
> 
> class RTCPeerConnection : public HasUnthrottledTask {
>   bool hasUnthrottledTask() override { return !m_closed || !m_stopped; }
> };
> 
> Then developers don't need to write .reset() in many places. The observer
> pattern would be less error-prone than the .reset() pattern.

I would argue that you still need to notify the scheduler when active connection
has gone away (checking all HasActiveTask subclasses is slow and will probably
involve troubles with ownership models and weak pointers). Thus some kind of
call is needed, and resetting handlers is arguably one of the best options: e.g.
calling reset on an empty handle or calling reset twice will work as intended.

[altimin, 2017-02-10 16:57:05]

The CQ bit was checked by altimin@chromium.org

[commit-bot: I haz the power, 2017-02-10 16:57:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 17:04:41]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1486745825524340, "parent_rev":
"35ff702839cfec8b7dfdfad6180f3fedec9d80dd", "commit_rev":
"407bdb243f9dd1d94f7e99318a7f60df1f421dfa"}

[commit-bot: I haz the power, 2017-02-10 17:05:17]

Description was changed from

==========
[scheduler] Plumb information about active webrtc connections to scheduler

Notify scheduler about webrtc connections.

BUG=690929
==========

to

==========
[scheduler] Plumb information about active webrtc connections to scheduler

Notify scheduler about webrtc connections.

BUG=690929

Review-Url: https://codereview.chromium.org/2683993006
Cr-Commit-Position: refs/heads/master@{#449640}
Committed:
https://chromium.googlesource.com/chromium/src/+/407bdb243f9dd1d94f7e99318a7f...
==========

[commit-bot: I haz the power, 2017-02-10 17:05:21]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/407bdb243f9dd1d94f7e99318a7f...

[haraken, 2017-02-10 17:05:55]

On 2017/02/10 16:57:01, altimin wrote:
> Thanks!
> 
>
https://codereview.chromium.org/2683993006/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
> (right):
> 
>
https://codereview.chromium.org/2683993006/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:526:
> document->frame()->frameScheduler()->onActiveConnectionCreated();
> On 2017/02/10 16:51:20, haraken wrote:
> > 
> > Another possible API would be to use an observer pattern like
> > ActiveScriptWrappable::hasPendingActivity().
> > 
> > class RTCPeerConnection : public HasUnthrottledTask {
> >   bool hasUnthrottledTask() override { return !m_closed || !m_stopped; }
> > };
> > 
> > Then developers don't need to write .reset() in many places. The observer
> > pattern would be less error-prone than the .reset() pattern.
> 
> I would argue that you still need to notify the scheduler when active
connection
> has gone away (checking all HasActiveTask subclasses is slow and will probably
> involve troubles with ownership models and weak pointers). Thus some kind of
> call is needed, and resetting handlers is arguably one of the best options:
e.g.
> calling reset on an empty handle or calling reset twice will work as intended.

My worry is that developers may forget to call .reset() in some cases. That's
why ActiveScriptWrappable (which is used to keep a V8 wrapper alive while DOM
has some references) uses the observer pattern instead of .reset() pattern.

[altimin, 2017-02-10 17:32:14]

On 2017/02/10 17:05:55, haraken wrote:
> On 2017/02/10 16:57:01, altimin wrote:
> > Thanks!
> > 
> >
>
https://codereview.chromium.org/2683993006/diff/1/third_party/WebKit/Source/m...
> > File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2683993006/diff/1/third_party/WebKit/Source/m...
> > third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:526:
> > document->frame()->frameScheduler()->onActiveConnectionCreated();
> > On 2017/02/10 16:51:20, haraken wrote:
> > > 
> > > Another possible API would be to use an observer pattern like
> > > ActiveScriptWrappable::hasPendingActivity().
> > > 
> > > class RTCPeerConnection : public HasUnthrottledTask {
> > >   bool hasUnthrottledTask() override { return !m_closed || !m_stopped; }
> > > };
> > > 
> > > Then developers don't need to write .reset() in many places. The observer
> > > pattern would be less error-prone than the .reset() pattern.
> > 
> > I would argue that you still need to notify the scheduler when active
> connection
> > has gone away (checking all HasActiveTask subclasses is slow and will
probably
> > involve troubles with ownership models and weak pointers). Thus some kind of
> > call is needed, and resetting handlers is arguably one of the best options:
> e.g.
> > calling reset on an empty handle or calling reset twice will work as
intended.
> 
> My worry is that developers may forget to call .reset() in some cases. That's
> why ActiveScriptWrappable (which is used to keep a V8 wrapper alive while DOM
> has some references) uses the observer pattern instead of .reset() pattern.

I still prefer current approach. But your point is very accurate, but I think
that we will be able to address it with sufficient test coverage. I improved
existing tests for this: crrev.com/2683993006.

[haraken, 2017-02-11 00:58:43]

On 2017/02/10 17:32:14, altimin wrote:
> On 2017/02/10 17:05:55, haraken wrote:
> > On 2017/02/10 16:57:01, altimin wrote:
> > > Thanks!
> > > 
> > >
> >
>
https://codereview.chromium.org/2683993006/diff/1/third_party/WebKit/Source/m...
> > > File
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2683993006/diff/1/third_party/WebKit/Source/m...
> > >
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:526:
> > > document->frame()->frameScheduler()->onActiveConnectionCreated();
> > > On 2017/02/10 16:51:20, haraken wrote:
> > > > 
> > > > Another possible API would be to use an observer pattern like
> > > > ActiveScriptWrappable::hasPendingActivity().
> > > > 
> > > > class RTCPeerConnection : public HasUnthrottledTask {
> > > >   bool hasUnthrottledTask() override { return !m_closed || !m_stopped; }
> > > > };
> > > > 
> > > > Then developers don't need to write .reset() in many places. The
observer
> > > > pattern would be less error-prone than the .reset() pattern.
> > > 
> > > I would argue that you still need to notify the scheduler when active
> > connection
> > > has gone away (checking all HasActiveTask subclasses is slow and will
> probably
> > > involve troubles with ownership models and weak pointers). Thus some kind
of
> > > call is needed, and resetting handlers is arguably one of the best
options:
> > e.g.
> > > calling reset on an empty handle or calling reset twice will work as
> intended.
> > 
> > My worry is that developers may forget to call .reset() in some cases.
That's
> > why ActiveScriptWrappable (which is used to keep a V8 wrapper alive while
DOM
> > has some references) uses the observer pattern instead of .reset() pattern.
> 
> I still prefer current approach. But your point is very accurate, but I think
> that we will be able to address it with sufficient test coverage. I improved
> existing tests for this: crrev.com/2683993006.

If you really want to go with that approach, can you add a DCHECK to verify that
all handlers have been reset when the scheduler shuts down?