Use base::Optional for selected video track.

Use base::Optional for selected video track.

Previously we used a one-element std::vector for passing around the
selected video track, but we can use base::Optional instead, since
there is never more than one selected video track.

Review-Url: https://codereview.chromium.org/2718483003
Cr-Commit-Position: refs/heads/master@{#452705}
Committed: https://chromium.googlesource.com/chromium/src/+/9bed660668c954ffd18eee419fba8c5cfaac8789

[servolk, 2017-02-23 18:43:46]

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

[servolk, 2017-02-23 18:43:59]

Description was changed from

==========
Use base::Optional for selected video track.

Previously we used a one-element std::vector for passing around the
selected video track, but we can use base::Optional instead, since
there is never more than one selected video track.
==========

to

==========
Use base::Optional for selected video track.

Previously we used a one-element std::vector for passing around the
selected video track, but we can use base::Optional instead, since
there is never more than one selected video track.
==========

[servolk, 2017-02-23 18:43:59]

servolk@chromium.org changed reviewers:
+ watk@chromium.org

[commit-bot: I haz the power, 2017-02-23 18:44:26]

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-23 20:41:10]

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

[commit-bot: I haz the power, 2017-02-23 20:41:11]

Dry run: This issue passed the CQ dry run.

[watk, 2017-02-23 20:57:54]

lgtm with nits. I'm confused why this code uses camelCase though. Maybe it's a
convention I'm not aware of.

https://codereview.chromium.org/2718483003/diff/1/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/2718483003/diff/1/media/base/pipeline.h#newco...
media/base/pipeline.h:85: // |trackId| is either empty, which means no video
track is selected, or
s/trackId/selectedTrackId/

Also why are we using camelcase here? AFAIK the only place the other style needs
to be used is where we're overriding a blink interface.

Looks like there are a lot of instances of this naming style in the selected
track changed code (currTime too) so I won't comment on them all.

https://codereview.chromium.org/2718483003/diff/1/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2718483003/diff/1/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:603: base::TimeDelta currTime = (state_ == kPlaying)
s/currTime/curr_time/?

https://codereview.chromium.org/2718483003/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2718483003/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:672: base::Optional<MediaTrack::Id>
selectedVideoTrackId;
naming convention

https://codereview.chromium.org/2718483003/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:677: << (selectedVideoTrackId ?
*selectedVideoTrackId : "") << "]";
Could use .value_or("") to make it slightly shorter.

https://codereview.chromium.org/2718483003/diff/1/media/test/pipeline_integra...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/2718483003/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:966:
pipeline_->OnSelectedVideoTrackChanged(base::Optional<MediaTrack::Id>());
should be able to write nullopt here and elsewhere?

[servolk, 2017-02-23 21:14:37]

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

[commit-bot: I haz the power, 2017-02-23 21:15:21]

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

[servolk, 2017-02-23 21:15:39]

On 2017/02/23 20:57:54, watk wrote:
> lgtm with nits. I'm confused why this code uses camelCase though. Maybe it's a
> convention I'm not aware of.
> 
> https://codereview.chromium.org/2718483003/diff/1/media/base/pipeline.h
> File media/base/pipeline.h (right):
> 
>
https://codereview.chromium.org/2718483003/diff/1/media/base/pipeline.h#newco...
> media/base/pipeline.h:85: // |trackId| is either empty, which means no video
> track is selected, or
> s/trackId/selectedTrackId/
> 
> Also why are we using camelcase here? AFAIK the only place the other style
needs
> to be used is where we're overriding a blink interface.
> 
> Looks like there are a lot of instances of this naming style in the selected
> track changed code (currTime too) so I won't comment on them all.
> 
> https://codereview.chromium.org/2718483003/diff/1/media/base/pipeline_impl.cc
> File media/base/pipeline_impl.cc (right):
> 
>
https://codereview.chromium.org/2718483003/diff/1/media/base/pipeline_impl.cc...
> media/base/pipeline_impl.cc:603: base::TimeDelta currTime = (state_ ==
kPlaying)
> s/currTime/curr_time/?
> 
>
https://codereview.chromium.org/2718483003/diff/1/media/blink/webmediaplayer_...
> File media/blink/webmediaplayer_impl.cc (right):
> 
>
https://codereview.chromium.org/2718483003/diff/1/media/blink/webmediaplayer_...
> media/blink/webmediaplayer_impl.cc:672: base::Optional<MediaTrack::Id>
> selectedVideoTrackId;
> naming convention
> 
>
https://codereview.chromium.org/2718483003/diff/1/media/blink/webmediaplayer_...
> media/blink/webmediaplayer_impl.cc:677: << (selectedVideoTrackId ?
> *selectedVideoTrackId : "") << "]";
> Could use .value_or("") to make it slightly shorter.
> 
>
https://codereview.chromium.org/2718483003/diff/1/media/test/pipeline_integra...
> File media/test/pipeline_integration_test.cc (right):
> 
>
https://codereview.chromium.org/2718483003/diff/1/media/test/pipeline_integra...
> media/test/pipeline_integration_test.cc:966:
> pipeline_->OnSelectedVideoTrackChanged(base::Optional<MediaTrack::Id>());
> should be able to write nullopt here and elsewhere?

Thanks for the feedback! All fixed in patchset #2, PTAL.

[servolk, 2017-02-23 21:15:49]

https://codereview.chromium.org/2718483003/diff/1/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/2718483003/diff/1/media/base/pipeline.h#newco...
media/base/pipeline.h:85: // |trackId| is either empty, which means no video
track is selected, or
On 2017/02/23 20:57:54, watk wrote:
> s/trackId/selectedTrackId/
> 
> Also why are we using camelcase here? AFAIK the only place the other style
needs
> to be used is where we're overriding a blink interface.
> 
> Looks like there are a lot of instances of this naming style in the selected
> track changed code (currTime too) so I won't comment on them all.

Done.

https://codereview.chromium.org/2718483003/diff/1/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2718483003/diff/1/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:603: base::TimeDelta currTime = (state_ == kPlaying)
On 2017/02/23 20:57:54, watk wrote:
> s/currTime/curr_time/?

Done.

https://codereview.chromium.org/2718483003/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2718483003/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:672: base::Optional<MediaTrack::Id>
selectedVideoTrackId;
On 2017/02/23 20:57:54, watk wrote:
> naming convention

Done.

https://codereview.chromium.org/2718483003/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:677: << (selectedVideoTrackId ?
*selectedVideoTrackId : "") << "]";
On 2017/02/23 20:57:54, watk wrote:
> Could use .value_or("") to make it slightly shorter.

Done.

https://codereview.chromium.org/2718483003/diff/1/media/test/pipeline_integra...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/2718483003/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:966:
pipeline_->OnSelectedVideoTrackChanged(base::Optional<MediaTrack::Id>());
On 2017/02/23 20:57:54, watk wrote:
> should be able to write nullopt here and elsewhere?

Done.

[watk, 2017-02-23 21:18:34]

Thanks for fixing naming! lgtm

[servolk, 2017-02-23 21:19:09]

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

[commit-bot: I haz the power, 2017-02-23 21:19:21]

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

[servolk, 2017-02-23 21:19:39]

The CQ bit was unchecked by servolk@chromium.org

[servolk, 2017-02-23 21:19:43]

The CQ bit was checked by servolk@chromium.org

[servolk, 2017-02-23 21:19:43]

The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org
Link to the patchset: https://codereview.chromium.org/2718483003/#ps40001
(title: "Updated also enabledTrackIds -> enabled_track_ids")

[commit-bot: I haz the power, 2017-02-23 21:19:46]

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-23 23:24:50]

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

[commit-bot: I haz the power, 2017-02-23 23:24:51]

Failed to apply patch for media/filters/chunk_demuxer.cc:
While running git apply --index -p1;
  error: patch failed: media/filters/chunk_demuxer.cc:708
  error: media/filters/chunk_demuxer.cc: patch does not apply

Patch:       media/filters/chunk_demuxer.cc
Index: media/filters/chunk_demuxer.cc
diff --git a/media/filters/chunk_demuxer.cc b/media/filters/chunk_demuxer.cc
index
0f5bd36c37872708d2bc357feb83269f4920e38a..99690f5c8bbfa964c69ab5afbf165b56e732e94d
100644
--- a/media/filters/chunk_demuxer.cc
+++ b/media/filters/chunk_demuxer.cc
@@ -693,7 +693,7 @@ base::TimeDelta
ChunkDemuxer::GetHighestPresentationTimestamp(
 
 void ChunkDemuxer::OnEnabledAudioTracksChanged(
     const std::vector<MediaTrack::Id>& track_ids,
-    base::TimeDelta currTime) {
+    base::TimeDelta curr_time) {
   base::AutoLock auto_lock(lock_);
   std::set<ChunkDemuxerStream*> enabled_streams;
   for (const auto& id : track_ids) {
@@ -708,24 +708,22 @@ void ChunkDemuxer::OnEnabledAudioTracksChanged(
   for (const auto& stream : audio_streams_) {
     if (enabled_streams.find(stream.get()) == enabled_streams.end()) {
       DVLOG(1) << __func__ << ": disabling stream " << stream.get();
-      stream->set_enabled(false, currTime);
+      stream->set_enabled(false, curr_time);
     }
   }
   for (const auto& stream : enabled_streams) {
     DVLOG(1) << __func__ << ": enabling stream " << stream;
-    stream->set_enabled(true, currTime);
+    stream->set_enabled(true, curr_time);
   }
 }
 
 void ChunkDemuxer::OnSelectedVideoTrackChanged(
-    const std::vector<MediaTrack::Id>& track_ids,
-    base::TimeDelta currTime) {
-  DCHECK_LE(track_ids.size(), 1u);
-
+    base::Optional<MediaTrack::Id> track_id,
+    base::TimeDelta curr_time) {
   base::AutoLock auto_lock(lock_);
   ChunkDemuxerStream* selected_stream = nullptr;
-  if (!track_ids.empty()) {
-    selected_stream = track_id_to_demux_stream_map_[track_ids[0]];
+  if (track_id) {
+    selected_stream = track_id_to_demux_stream_map_[*track_id];
     DCHECK(selected_stream);
     DCHECK_EQ(DemuxerStream::VIDEO, selected_stream->type());
   }
@@ -736,12 +734,12 @@ void ChunkDemuxer::OnSelectedVideoTrackChanged(
     if (stream.get() != selected_stream) {
       DVLOG(1) << __func__ << ": disabling stream " << stream.get();
       DCHECK_EQ(DemuxerStream::VIDEO, stream->type());
-      stream->set_enabled(false, currTime);
+      stream->set_enabled(false, curr_time);
     }
   }
   if (selected_stream) {
     DVLOG(1) << __func__ << ": enabling stream " << selected_stream;
-    selected_stream->set_enabled(true, currTime);
+    selected_stream->set_enabled(true, curr_time);
   }
 }

[servolk, 2017-02-23 23:32:22]

The CQ bit was checked by servolk@chromium.org

[servolk, 2017-02-23 23:32:22]

The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org
Link to the patchset: https://codereview.chromium.org/2718483003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-23 23:33:21]

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-24 01:20:14]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487892742228360,
"parent_rev": "8c94125c2f9fcf51140fb22c1d4034cfa005a193", "commit_rev":
"9bed660668c954ffd18eee419fba8c5cfaac8789"}

[commit-bot: I haz the power, 2017-02-24 01:21:08]

Description was changed from

==========
Use base::Optional for selected video track.

Previously we used a one-element std::vector for passing around the
selected video track, but we can use base::Optional instead, since
there is never more than one selected video track.
==========

to

==========
Use base::Optional for selected video track.

Previously we used a one-element std::vector for passing around the
selected video track, but we can use base::Optional instead, since
there is never more than one selected video track.

Review-Url: https://codereview.chromium.org/2718483003
Cr-Commit-Position: refs/heads/master@{#452705}
Committed:
https://chromium.googlesource.com/chromium/src/+/9bed660668c954ffd18eee419fba...
==========

[commit-bot: I haz the power, 2017-02-24 01:21:09]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/9bed660668c954ffd18eee419fba...

[mlamouri (slow - plz ping), 2017-02-24 11:32:22]

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

[mlamouri (slow - plz ping), 2017-02-24 11:32:22]

base::Optional isn't recommended for parameters, see
https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#When...

Would a pointer have done the job here?

[servolk, 2017-02-24 17:52:00]

On 2017/02/24 11:32:22, mlamouri wrote:
> base::Optional isn't recommended for parameters, see
>
https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#When...
> 
> Would a pointer have done the job here?

Ah, thanks, I wasn't aware of those limitations. I've considered using a
pointer, but a raw pointer isn't good here since we need to pass this value
between the UI and media thread (see PostTask in pipeline_impl.cc), and I didn't
want to deal with lifetime management issues. But I guess we could use
scoped_ptr<MediaTrack::Id> instead, that should work. WDYT about using
scoped_ptr instead of base::Optional?

[watk, 2017-02-24 20:40:02]

On 2017/02/24 17:52:00, servolk wrote:
> On 2017/02/24 11:32:22, mlamouri wrote:
> > base::Optional isn't recommended for parameters, see
> >
>
https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#When...
> > 
> > Would a pointer have done the job here?
> 
> Ah, thanks, I wasn't aware of those limitations. I've considered using a
> pointer, but a raw pointer isn't good here since we need to pass this value
> between the UI and media thread (see PostTask in pipeline_impl.cc), and I
didn't
> want to deal with lifetime management issues. But I guess we could use
> scoped_ptr<MediaTrack::Id> instead, that should work. WDYT about using
> scoped_ptr instead of base::Optional?

mlamouri@, could you help me understand why this is the guidance? I understand
why you might not want to have optionals in the public interface of a library,
but why does it matter between internal pieces? I guess the goal is to avoid
having any layer that has to translate between a pointer and an optional? So
make it a pointer through every layer? Optionals are nice because you don't have
to care about lifetimes.

servolk@, seems like unique_ptr works so I would use that.

[mlamouri (slow - plz ping), 2017-02-27 11:06:38]

On 2017/02/24 at 20:40:02, watk wrote:
> On 2017/02/24 17:52:00, servolk wrote:
> > On 2017/02/24 11:32:22, mlamouri wrote:
> > > base::Optional isn't recommended for parameters, see
> > >
> >
https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#When...
> > > 
> > > Would a pointer have done the job here?
> > 
> > Ah, thanks, I wasn't aware of those limitations. I've considered using a
> > pointer, but a raw pointer isn't good here since we need to pass this value
> > between the UI and media thread (see PostTask in pipeline_impl.cc), and I
didn't
> > want to deal with lifetime management issues. But I guess we could use
> > scoped_ptr<MediaTrack::Id> instead, that should work. WDYT about using
> > scoped_ptr instead of base::Optional?
> 
> mlamouri@, could you help me understand why this is the guidance? I understand
why you might not want to have optionals in the public interface of a library,
but why does it matter between internal pieces? I guess the goal is to avoid
having any layer that has to translate between a pointer and an optional? So
make it a pointer through every layer? Optionals are nice because you don't have
to care about lifetimes.
> 
> servolk@, seems like unique_ptr works so I would use that.

To be clear, I have no strong opinion about this, this is what the //base OWNERS
asked for when I wrote the implementation of base::Optional. They had specific
concerns about Windows (as said in the doc). I wouldn't be surprised that
base::Optional<> has been used as argument's type in other places so it's
probably not the end of the world :)

[watk, 2017-03-01 22:08:48]

On 2017/02/27 11:06:38, mlamouri wrote:
> On 2017/02/24 at 20:40:02, watk wrote:
> > On 2017/02/24 17:52:00, servolk wrote:
> > > On 2017/02/24 11:32:22, mlamouri wrote:
> > > > base::Optional isn't recommended for parameters, see
> > > >
> > >
>
https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#When...
> > > > 
> > > > Would a pointer have done the job here?
> > > 
> > > Ah, thanks, I wasn't aware of those limitations. I've considered using a
> > > pointer, but a raw pointer isn't good here since we need to pass this
value
> > > between the UI and media thread (see PostTask in pipeline_impl.cc), and I
> didn't
> > > want to deal with lifetime management issues. But I guess we could use
> > > scoped_ptr<MediaTrack::Id> instead, that should work. WDYT about using
> > > scoped_ptr instead of base::Optional?
> > 
> > mlamouri@, could you help me understand why this is the guidance? I
understand
> why you might not want to have optionals in the public interface of a library,
> but why does it matter between internal pieces? I guess the goal is to avoid
> having any layer that has to translate between a pointer and an optional? So
> make it a pointer through every layer? Optionals are nice because you don't
have
> to care about lifetimes.
> > 
> > servolk@, seems like unique_ptr works so I would use that.
> 
> To be clear, I have no strong opinion about this, this is what the //base
OWNERS
> asked for when I wrote the implementation of base::Optional. They had specific
> concerns about Windows (as said in the doc). I wouldn't be surprised that
> base::Optional<> has been used as argument's type in other places so it's
> probably not the end of the world :)

Ah ok. danakj@ said that T* should be preferred when possible. But since T*
doesn't work in this case because we're binding into a callback, optional seems
fine.

[servolk, 2017-03-03 22:22:01]

On 2017/03/01 22:08:48, watk wrote:
> On 2017/02/27 11:06:38, mlamouri wrote:
> > On 2017/02/24 at 20:40:02, watk wrote:
> > > On 2017/02/24 17:52:00, servolk wrote:
> > > > On 2017/02/24 11:32:22, mlamouri wrote:
> > > > > base::Optional isn't recommended for parameters, see
> > > > >
> > > >
> >
>
https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#When...
> > > > > 
> > > > > Would a pointer have done the job here?
> > > > 
> > > > Ah, thanks, I wasn't aware of those limitations. I've considered using a
> > > > pointer, but a raw pointer isn't good here since we need to pass this
> value
> > > > between the UI and media thread (see PostTask in pipeline_impl.cc), and
I
> > didn't
> > > > want to deal with lifetime management issues. But I guess we could use
> > > > scoped_ptr<MediaTrack::Id> instead, that should work. WDYT about using
> > > > scoped_ptr instead of base::Optional?
> > > 
> > > mlamouri@, could you help me understand why this is the guidance? I
> understand
> > why you might not want to have optionals in the public interface of a
library,
> > but why does it matter between internal pieces? I guess the goal is to avoid
> > having any layer that has to translate between a pointer and an optional? So
> > make it a pointer through every layer? Optionals are nice because you don't
> have
> > to care about lifetimes.
> > > 
> > > servolk@, seems like unique_ptr works so I would use that.
> > 
> > To be clear, I have no strong opinion about this, this is what the //base
> OWNERS
> > asked for when I wrote the implementation of base::Optional. They had
specific
> > concerns about Windows (as said in the doc). I wouldn't be surprised that
> > base::Optional<> has been used as argument's type in other places so it's
> > probably not the end of the world :)
> 
> Ah ok. danakj@ said that T* should be preferred when possible. But since T*
> doesn't work in this case because we're binding into a callback, optional
seems
> fine.

Sorry for the delay on this. perf ate my time :)
But it sounds like all is good and we can actually keep using the
base::Optional?

[watk, 2017-03-03 22:27:23]

On 2017/03/03 22:22:01, servolk wrote:
> On 2017/03/01 22:08:48, watk wrote:
> > On 2017/02/27 11:06:38, mlamouri wrote:
> > > On 2017/02/24 at 20:40:02, watk wrote:
> > > > On 2017/02/24 17:52:00, servolk wrote:
> > > > > On 2017/02/24 11:32:22, mlamouri wrote:
> > > > > > base::Optional isn't recommended for parameters, see
> > > > > >
> > > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#When...
> > > > > > 
> > > > > > Would a pointer have done the job here?
> > > > > 
> > > > > Ah, thanks, I wasn't aware of those limitations. I've considered using
a
> > > > > pointer, but a raw pointer isn't good here since we need to pass this
> > value
> > > > > between the UI and media thread (see PostTask in pipeline_impl.cc),
and
> I
> > > didn't
> > > > > want to deal with lifetime management issues. But I guess we could use
> > > > > scoped_ptr<MediaTrack::Id> instead, that should work. WDYT about using
> > > > > scoped_ptr instead of base::Optional?
> > > > 
> > > > mlamouri@, could you help me understand why this is the guidance? I
> > understand
> > > why you might not want to have optionals in the public interface of a
> library,
> > > but why does it matter between internal pieces? I guess the goal is to
avoid
> > > having any layer that has to translate between a pointer and an optional?
So
> > > make it a pointer through every layer? Optionals are nice because you
don't
> > have
> > > to care about lifetimes.
> > > > 
> > > > servolk@, seems like unique_ptr works so I would use that.
> > > 
> > > To be clear, I have no strong opinion about this, this is what the //base
> > OWNERS
> > > asked for when I wrote the implementation of base::Optional. They had
> specific
> > > concerns about Windows (as said in the doc). I wouldn't be surprised that
> > > base::Optional<> has been used as argument's type in other places so it's
> > > probably not the end of the world :)
> > 
> > Ah ok. danakj@ said that T* should be preferred when possible. But since T*
> > doesn't work in this case because we're binding into a callback, optional
> seems
> > fine.
> 
> Sorry for the delay on this. perf ate my time :)
> But it sounds like all is good and we can actually keep using the
> base::Optional?

Yep. The conclusion is: prefer T*, if it's not possible carefully consider the
implications of using optional because it may be contagious and make surrounding
code unclear or difficult to work with.

In this case since the alternative is unique_ptr, which IMO is equivalent in
terms of how it affects surrounding code, optional is fine.

[servolk, 2017-03-03 22:29:06]

On 2017/03/03 22:27:23, watk wrote:
> On 2017/03/03 22:22:01, servolk wrote:
> > On 2017/03/01 22:08:48, watk wrote:
> > > On 2017/02/27 11:06:38, mlamouri wrote:
> > > > On 2017/02/24 at 20:40:02, watk wrote:
> > > > > On 2017/02/24 17:52:00, servolk wrote:
> > > > > > On 2017/02/24 11:32:22, mlamouri wrote:
> > > > > > > base::Optional isn't recommended for parameters, see
> > > > > > >
> > > > > >
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#When...
> > > > > > > 
> > > > > > > Would a pointer have done the job here?
> > > > > > 
> > > > > > Ah, thanks, I wasn't aware of those limitations. I've considered
using
> a
> > > > > > pointer, but a raw pointer isn't good here since we need to pass
this
> > > value
> > > > > > between the UI and media thread (see PostTask in pipeline_impl.cc),
> and
> > I
> > > > didn't
> > > > > > want to deal with lifetime management issues. But I guess we could
use
> > > > > > scoped_ptr<MediaTrack::Id> instead, that should work. WDYT about
using
> > > > > > scoped_ptr instead of base::Optional?
> > > > > 
> > > > > mlamouri@, could you help me understand why this is the guidance? I
> > > understand
> > > > why you might not want to have optionals in the public interface of a
> > library,
> > > > but why does it matter between internal pieces? I guess the goal is to
> avoid
> > > > having any layer that has to translate between a pointer and an
optional?
> So
> > > > make it a pointer through every layer? Optionals are nice because you
> don't
> > > have
> > > > to care about lifetimes.
> > > > > 
> > > > > servolk@, seems like unique_ptr works so I would use that.
> > > > 
> > > > To be clear, I have no strong opinion about this, this is what the
//base
> > > OWNERS
> > > > asked for when I wrote the implementation of base::Optional. They had
> > specific
> > > > concerns about Windows (as said in the doc). I wouldn't be surprised
that
> > > > base::Optional<> has been used as argument's type in other places so
it's
> > > > probably not the end of the world :)
> > > 
> > > Ah ok. danakj@ said that T* should be preferred when possible. But since
T*
> > > doesn't work in this case because we're binding into a callback, optional
> > seems
> > > fine.
> > 
> > Sorry for the delay on this. perf ate my time :)
> > But it sounds like all is good and we can actually keep using the
> > base::Optional?
> 
> Yep. The conclusion is: prefer T*, if it's not possible carefully consider the
> implications of using optional because it may be contagious and make
surrounding
> code unclear or difficult to work with.
> 
> In this case since the alternative is unique_ptr, which IMO is equivalent in
> terms of how it affects surrounding code, optional is fine.

Got it, makes sense, thanks!