Remove ContextLifecycleObserver from MediaStream

Remove ContextLifecycleObserver from MediaStream

MediaStream doesn't need to inherit from ContextLifecycleObserver.
It just needs to hold a strong member to ExecutionContext.

BUG=610176
Committed: https://crrev.com/d4833220d8d9320deb606a0068a332a046486c95
Cr-Commit-Position: refs/heads/master@{#436903}

[haraken, 2016-12-07 06:35:00]

haraken@chromium.org changed reviewers:
+ sigbjornf@opera.com

[haraken, 2016-12-07 06:35:01]

PTAL

https://codereview.chromium.org/2549353005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStream.h (right):

https://codereview.chromium.org/2549353005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStream.h:117:
Member<ExecutionContext> m_executionContext;

Before this CL, this was a weak member. However, I don't see any reason to make
it weak, so I changed it to a strong member.

[sof, 2016-12-07 07:34:16]

lgtm, i like the approach.

https://codereview.chromium.org/2549353005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStream.h (right):

https://codereview.chromium.org/2549353005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStream.h:117:
Member<ExecutionContext> m_executionContext;
On 2016/12/07 06:35:01, haraken wrote:
> 
> Before this CL, this was a weak member. However, I don't see any reason to
make
> it weak, so I changed it to a strong member.

the worry/difference would be that downstream users of this object won't see a
null ExecutionContext after it has been "destroyed", which other implementations
of getExecutionContext() will provide. I haven't investigated if that makes a
substantive difference.

[haraken, 2016-12-07 08:05:52]

https://codereview.chromium.org/2549353005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStream.h (right):

https://codereview.chromium.org/2549353005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStream.h:117:
Member<ExecutionContext> m_executionContext;
On 2016/12/07 07:34:16, sof wrote:
> On 2016/12/07 06:35:01, haraken wrote:
> > 
> > Before this CL, this was a weak member. However, I don't see any reason to
> make
> > it weak, so I changed it to a strong member.
> 
> the worry/difference would be that downstream users of this object won't see a
> null ExecutionContext after it has been "destroyed", which other
implementations
> of getExecutionContext() will provide. I haven't investigated if that makes a
> substantive difference.

Yeah, good point.

Currently EventTarget::getExecutionContext() sometimes returns an
ExecutionContext backed by a WeakMember and sometimes returns an
ExecutionContext backed by a Member. So the difference won't matter.

[haraken, 2016-12-07 08:05:56]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-12-07 08:06:06]

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

[sof, 2016-12-07 08:15:37]

On 2016/12/07 08:05:52, haraken wrote:
>
https://codereview.chromium.org/2549353005/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/mediastream/MediaStream.h (right):
> 
>
https://codereview.chromium.org/2549353005/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/mediastream/MediaStream.h:117:
> Member<ExecutionContext> m_executionContext;
> On 2016/12/07 07:34:16, sof wrote:
> > On 2016/12/07 06:35:01, haraken wrote:
> > > 
> > > Before this CL, this was a weak member. However, I don't see any reason to
> > make
> > > it weak, so I changed it to a strong member.
> > 
> > the worry/difference would be that downstream users of this object won't see
a
> > null ExecutionContext after it has been "destroyed", which other
> implementations
> > of getExecutionContext() will provide. I haven't investigated if that makes
a
> > substantive difference.
> 
> Yeah, good point.
> 
> Currently EventTarget::getExecutionContext() sometimes returns an
> ExecutionContext backed by a WeakMember and sometimes returns an
> ExecutionContext backed by a Member. So the difference won't matter.

notifyContextDestroyed() also clears the context post-contextDestroyed(), so
(Weak)Member reference isn't the main issue.

[haraken, 2016-12-07 08:17:14]

On 2016/12/07 08:15:37, sof wrote:
> On 2016/12/07 08:05:52, haraken wrote:
> >
>
https://codereview.chromium.org/2549353005/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/mediastream/MediaStream.h (right):
> > 
> >
>
https://codereview.chromium.org/2549353005/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/mediastream/MediaStream.h:117:
> > Member<ExecutionContext> m_executionContext;
> > On 2016/12/07 07:34:16, sof wrote:
> > > On 2016/12/07 06:35:01, haraken wrote:
> > > > 
> > > > Before this CL, this was a weak member. However, I don't see any reason
to
> > > make
> > > > it weak, so I changed it to a strong member.
> > > 
> > > the worry/difference would be that downstream users of this object won't
see
> a
> > > null ExecutionContext after it has been "destroyed", which other
> > implementations
> > > of getExecutionContext() will provide. I haven't investigated if that
makes
> a
> > > substantive difference.
> > 
> > Yeah, good point.
> > 
> > Currently EventTarget::getExecutionContext() sometimes returns an
> > ExecutionContext backed by a WeakMember and sometimes returns an
> > ExecutionContext backed by a Member. So the difference won't matter.
> 
> notifyContextDestroyed() also clears the context post-contextDestroyed(), so
> (Weak)Member reference isn't the main issue.

Right. I wanted to mean that EventTarget::getExecutionContext() sometimes return
someElement()->document(), which is not cleared on the context detachment.

[commit-bot: I haz the power, 2016-12-07 09:56:19]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481097956690790,
"parent_rev": "d5b1125f274af94cb5f034a443fbd0a65fe0a97d", "commit_rev":
"8b1512508c487ffa69e6b53052f8ffe12f9e6ff4"}

[commit-bot: I haz the power, 2016-12-07 09:56:45]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-07 09:59:07]

Description was changed from

==========
Remove ContextLifecycleObserver from MediaStream

MediaStream doesn't need to inherit from ContextLifecycleObserver.
It just needs to hold a strong member to ExecutionContext.

BUG=610176
==========

to

==========
Remove ContextLifecycleObserver from MediaStream

MediaStream doesn't need to inherit from ContextLifecycleObserver.
It just needs to hold a strong member to ExecutionContext.

BUG=610176

Committed: https://crrev.com/d4833220d8d9320deb606a0068a332a046486c95
Cr-Commit-Position: refs/heads/master@{#436903}
==========

[commit-bot: I haz the power, 2016-12-07 09:59:08]

Patchset 2 (id:??) landed as
https://crrev.com/d4833220d8d9320deb606a0068a332a046486c95
Cr-Commit-Position: refs/heads/master@{#436903}