Rename webrtc::PacketInfo to webrtc::PacketFeedback

Rename webrtc::PacketInfo to webrtc::PacketFeedback. This resolves ambiguity with a similarly named RTCPReceiver::PacketInformation and RtpPacketizerVp9::PacketInfo.

BUG=None

Review-Url: https://codereview.webrtc.org/2710093004
Cr-Commit-Position: refs/heads/master@{#17066}
Committed: https://chromium.googlesource.com/external/webrtc/+/f94900083484c53bc9717c66794f0b74f5e4f471

[elad.alon_webrtc.org, 2017-02-23 17:32:24]

elad.alon@webrtc.org changed reviewers:
+ michaelt@webrtc.org, minyue@webrtc.org, ossu@webrtc.org, sprang@webrtc.org,
stefan@webrtc.org

[elad.alon_webrtc.org, 2017-02-23 17:32:25]

Ready for review.
Note: The actual thing changed is quite small. We just rename to make things
clearer (more so down the stack).

[danilchap, 2017-02-23 17:35:01]

Description was changed from

==========
s/PacketInfo/PacketFeedback

BUG=
==========

to

==========
s/PacketInfo/PacketFeedback

BUG=
==========

[elad.alon_webrtc.org, 2017-02-23 17:42:18]

Might be worth noting that besides making some naming choices easier down the
line, this helps distinguish from 1. RtpPacketizerVp9::PacketInfo and 2.
RTCPReceiver::PacketInformation. A known "issue" is that we still keep
PacedPacketInfo named like that. Stefan consulted on this and agrees that it can
stay.

[minyue-webrtc, 2017-02-23 19:17:44]

On 2017/02/23 17:32:25, elad.alon wrote:
> Ready for review.
> Note: The actual thing changed is quite small. We just rename to make things
> clearer (more so down the stack).

It would be good to change the title of the CL and add a reason for the
renaming.

[elad.alon_webrtc.org, 2017-02-23 19:28:17]

Description was changed from

==========
s/PacketInfo/PacketFeedback

BUG=
==========

to

==========
Rename webrtc::PacketInfo to webrtc::PacketFeedback. This resolves ambiguity
with a similarly named RTCPReceiver::PacketInformation and
RtpPacketizerVp9::PacketInfo.

BUG=
==========

[elad.alon_webrtc.org, 2017-02-23 19:28:44]

On 2017/02/23 19:17:44, minyue-webrtc wrote:
> On 2017/02/23 17:32:25, elad.alon wrote:
> > Ready for review.
> > Note: The actual thing changed is quite small. We just rename to make things
> > clearer (more so down the stack).
> 
> It would be good to change the title of the CL and add a reason for the
> renaming.

Done.

[stefan-webrtc, 2017-02-24 08:42:11]

stefan@webrtc.org changed reviewers:
+ philipel@webrtc.org
- michaelt@webrtc.org

[stefan-webrtc, 2017-02-24 08:42:12]

lgtm

https://codereview.webrtc.org/2710093004/diff/20001/webrtc/modules/bitrate_co...
File webrtc/modules/bitrate_controller/include/bitrate_controller.h (right):

https://codereview.webrtc.org/2710093004/diff/20001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/include/bitrate_controller.h:29: struct
PacketFeedback;
Delete

[elad.alon_webrtc.org, 2017-02-27 09:36:31]

https://codereview.webrtc.org/2710093004/diff/20001/webrtc/modules/bitrate_co...
File webrtc/modules/bitrate_controller/include/bitrate_controller.h (right):

https://codereview.webrtc.org/2710093004/diff/20001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/include/bitrate_controller.h:29: struct
PacketFeedback;
On 2017/02/24 08:42:12, stefan-webrtc wrote:
> Delete

Done.

[philipel, 2017-02-27 09:51:01]

Is PacketFeedback really a good name? It sounds like a struct that holds
feedback specific info, while this struct holds mostly non-feedback info.

I know PacketInfo is kind of a non-descriptive name, but I think the risk of
making incorrect assumptions about the struct is much lower.

What do you think of "SendReceiveInfo" or "PacketSendReceiveInfo"?

[elad.alon_webrtc.org, 2017-02-27 10:52:53]

On 2017/02/27 09:51:01, philipel wrote:
> Is PacketFeedback really a good name? It sounds like a struct that holds
> feedback specific info, while this struct holds mostly non-feedback info.
> 
> I know PacketInfo is kind of a non-descriptive name, but I think the risk of
> making incorrect assumptions about the struct is much lower.
> 
> What do you think of "SendReceiveInfo" or "PacketSendReceiveInfo"?

Good suggestions. I think "SendReceiveInfo" might be problematic in that it
seems to imply that the packet was not lost, and "PacketSendReceiveInfo" is a
bit long. What do you think about "PacketFeedbackInfo", or "PacketTwccInfo"?
(TWCC - Transport-wide Congestion Control.)

[stefan-webrtc, 2017-02-27 13:36:22]

On 2017/02/27 10:52:53, elad.alon wrote:
> On 2017/02/27 09:51:01, philipel wrote:
> > Is PacketFeedback really a good name? It sounds like a struct that holds
> > feedback specific info, while this struct holds mostly non-feedback info.
> > 
> > I know PacketInfo is kind of a non-descriptive name, but I think the risk of
> > making incorrect assumptions about the struct is much lower.
> > 
> > What do you think of "SendReceiveInfo" or "PacketSendReceiveInfo"?
> 
> Good suggestions. I think "SendReceiveInfo" might be problematic in that it
> seems to imply that the packet was not lost, and "PacketSendReceiveInfo" is a
> bit long. What do you think about "PacketFeedbackInfo", or "PacketTwccInfo"?
> (TWCC - Transport-wide Congestion Control.)

I don't like PacketTwccInfo or PacketInfo. The first isn't transport agnostic,
and the latter is too generic. If you think of it as "this is information about
a packet that we now have received feedback about", I think it's possible to
argue that PacketFeedback is a reasonable name. PacketSendReceiveInfo sounds
pretty bad too... Any other options, or could we go with PacketFeedback? :)

[elad.alon_webrtc.org, 2017-03-01 13:20:13]

On 2017/02/27 13:36:22, stefan-webrtc wrote:
> On 2017/02/27 10:52:53, elad.alon wrote:
> > On 2017/02/27 09:51:01, philipel wrote:
> > > Is PacketFeedback really a good name? It sounds like a struct that holds
> > > feedback specific info, while this struct holds mostly non-feedback info.
> > > 
> > > I know PacketInfo is kind of a non-descriptive name, but I think the risk
of
> > > making incorrect assumptions about the struct is much lower.
> > > 
> > > What do you think of "SendReceiveInfo" or "PacketSendReceiveInfo"?
> > 
> > Good suggestions. I think "SendReceiveInfo" might be problematic in that it
> > seems to imply that the packet was not lost, and "PacketSendReceiveInfo" is
a
> > bit long. What do you think about "PacketFeedbackInfo", or "PacketTwccInfo"?
> > (TWCC - Transport-wide Congestion Control.)
> 
> I don't like PacketTwccInfo or PacketInfo. The first isn't transport agnostic,
> and the latter is too generic. If you think of it as "this is information
about
> a packet that we now have received feedback about", I think it's possible to
> argue that PacketFeedback is a reasonable name. PacketSendReceiveInfo sounds
> pretty bad too... Any other options, or could we go with PacketFeedback? :)

Do we have a decision? :-) Need a second "lgtm" to land.

[minyue-webrtc, 2017-03-01 14:10:00]

On 2017/03/01 13:20:13, elad.alon wrote:
> On 2017/02/27 13:36:22, stefan-webrtc wrote:
> > On 2017/02/27 10:52:53, elad.alon wrote:
> > > On 2017/02/27 09:51:01, philipel wrote:
> > > > Is PacketFeedback really a good name? It sounds like a struct that holds
> > > > feedback specific info, while this struct holds mostly non-feedback
info.
> > > > 
> > > > I know PacketInfo is kind of a non-descriptive name, but I think the
risk
> of
> > > > making incorrect assumptions about the struct is much lower.
> > > > 
> > > > What do you think of "SendReceiveInfo" or "PacketSendReceiveInfo"?
> > > 
> > > Good suggestions. I think "SendReceiveInfo" might be problematic in that
it
> > > seems to imply that the packet was not lost, and "PacketSendReceiveInfo"
is
> a
> > > bit long. What do you think about "PacketFeedbackInfo", or
"PacketTwccInfo"?
> > > (TWCC - Transport-wide Congestion Control.)
> > 
> > I don't like PacketTwccInfo or PacketInfo. The first isn't transport
agnostic,
> > and the latter is too generic. If you think of it as "this is information
> about
> > a packet that we now have received feedback about", I think it's possible to
> > argue that PacketFeedback is a reasonable name. PacketSendReceiveInfo sounds
> > pretty bad too... Any other options, or could we go with PacketFeedback? :)
> 
> Do we have a decision? :-) Need a second "lgtm" to land.

If philips agree, I have no concern

[philipel, 2017-03-01 15:59:36]

On 2017/03/01 14:10:00, minyue-webrtc wrote:
> On 2017/03/01 13:20:13, elad.alon wrote:
> > On 2017/02/27 13:36:22, stefan-webrtc wrote:
> > > On 2017/02/27 10:52:53, elad.alon wrote:
> > > > On 2017/02/27 09:51:01, philipel wrote:
> > > > > Is PacketFeedback really a good name? It sounds like a struct that
holds
> > > > > feedback specific info, while this struct holds mostly non-feedback
> info.
> > > > > 
> > > > > I know PacketInfo is kind of a non-descriptive name, but I think the
> risk
> > of
> > > > > making incorrect assumptions about the struct is much lower.
> > > > > 
> > > > > What do you think of "SendReceiveInfo" or "PacketSendReceiveInfo"?
> > > > 
> > > > Good suggestions. I think "SendReceiveInfo" might be problematic in that
> it
> > > > seems to imply that the packet was not lost, and "PacketSendReceiveInfo"
> is
> > a
> > > > bit long. What do you think about "PacketFeedbackInfo", or
> "PacketTwccInfo"?
> > > > (TWCC - Transport-wide Congestion Control.)
> > > 
> > > I don't like PacketTwccInfo or PacketInfo. The first isn't transport
> agnostic,
> > > and the latter is too generic. If you think of it as "this is information
> > about
> > > a packet that we now have received feedback about", I think it's possible
to
> > > argue that PacketFeedback is a reasonable name. PacketSendReceiveInfo
sounds
> > > pretty bad too... Any other options, or could we go with PacketFeedback?
:)
> > 
> > Do we have a decision? :-) Need a second "lgtm" to land.
> 
> If philips agree, I have no concern

I'm sorry, but I still don't think PacketFeedback is a good name for the same
reason I mentioned earlier. I can expand a bit and maybe you will understand
why.

I think the motivation for renaming this struct is somewhat weak. We have
different namespaces which allows us to name things in a sensible way (if we can
figure out what those names are, that's the hard part of course) and not having
to come up with contrived names due to collisions.

Yes, I agree that it is annoying to have multiple classes with the same name
under different namespaces, but I think it's better than having contrived names.
In this case in particular I would even say that the name is misleading.

This looks like a case of "naming is hard", since none of us can come up with a
name that makes sense for the others.

If elad.alon@ think it makes sense and if stefan@ thinks it makes sense then I
think we should land this CL.

[ossu, 2017-03-02 00:30:09]

On 2017/03/01 15:59:36, philipel wrote:
> On 2017/03/01 14:10:00, minyue-webrtc wrote:
> > On 2017/03/01 13:20:13, elad.alon wrote:
> > > On 2017/02/27 13:36:22, stefan-webrtc wrote:
> > > > On 2017/02/27 10:52:53, elad.alon wrote:
> > > > > On 2017/02/27 09:51:01, philipel wrote:
> > > > > > Is PacketFeedback really a good name? It sounds like a struct that
> holds
> > > > > > feedback specific info, while this struct holds mostly non-feedback
> > info.
> > > > > > 
> > > > > > I know PacketInfo is kind of a non-descriptive name, but I think the
> > risk
> > > of
> > > > > > making incorrect assumptions about the struct is much lower.
> > > > > > 
> > > > > > What do you think of "SendReceiveInfo" or "PacketSendReceiveInfo"?
> > > > > 
> > > > > Good suggestions. I think "SendReceiveInfo" might be problematic in
that
> > it
> > > > > seems to imply that the packet was not lost, and
"PacketSendReceiveInfo"
> > is
> > > a
> > > > > bit long. What do you think about "PacketFeedbackInfo", or
> > "PacketTwccInfo"?
> > > > > (TWCC - Transport-wide Congestion Control.)
> > > > 
> > > > I don't like PacketTwccInfo or PacketInfo. The first isn't transport
> > agnostic,
> > > > and the latter is too generic. If you think of it as "this is
information
> > > about
> > > > a packet that we now have received feedback about", I think it's
possible
> to
> > > > argue that PacketFeedback is a reasonable name. PacketSendReceiveInfo
> sounds
> > > > pretty bad too... Any other options, or could we go with PacketFeedback?
> :)
> > > 
> > > Do we have a decision? :-) Need a second "lgtm" to land.
> > 
> > If philips agree, I have no concern
> 
> I'm sorry, but I still don't think PacketFeedback is a good name for the same
> reason I mentioned earlier. I can expand a bit and maybe you will understand
> why.
> 
> I think the motivation for renaming this struct is somewhat weak. We have
> different namespaces which allows us to name things in a sensible way (if we
can
> figure out what those names are, that's the hard part of course) and not
having
> to come up with contrived names due to collisions.
> 
> Yes, I agree that it is annoying to have multiple classes with the same name
> under different namespaces, but I think it's better than having contrived
names.
> In this case in particular I would even say that the name is misleading.
> 
> This looks like a case of "naming is hard", since none of us can come up with
a
> name that makes sense for the others.
> 
> If elad.alon@ think it makes sense and if stefan@ thinks it makes sense then I
> think we should land this CL.

What information is the struct supposed to capture? PacketInfo is probably not
wrong, but it's very, very generic. PacketFeedback is less overloaded, but it's
not very clear. Does it capture just the feedback information received about a
packet, or are some things filled out regardless of if feedback was received?
PacketFeedbackInfo is maybe better? PacketTransmission...Info? Status?

[elad.alon_webrtc.org, 2017-03-02 12:51:20]

On 2017/03/02 00:30:09, ossu wrote:
> On 2017/03/01 15:59:36, philipel wrote:
> > On 2017/03/01 14:10:00, minyue-webrtc wrote:
> > > On 2017/03/01 13:20:13, elad.alon wrote:
> > > > On 2017/02/27 13:36:22, stefan-webrtc wrote:
> > > > > On 2017/02/27 10:52:53, elad.alon wrote:
> > > > > > On 2017/02/27 09:51:01, philipel wrote:
> > > > > > > Is PacketFeedback really a good name? It sounds like a struct that
> > holds
> > > > > > > feedback specific info, while this struct holds mostly
non-feedback
> > > info.
> > > > > > > 
> > > > > > > I know PacketInfo is kind of a non-descriptive name, but I think
the
> > > risk
> > > > of
> > > > > > > making incorrect assumptions about the struct is much lower.
> > > > > > > 
> > > > > > > What do you think of "SendReceiveInfo" or "PacketSendReceiveInfo"?
> > > > > > 
> > > > > > Good suggestions. I think "SendReceiveInfo" might be problematic in
> that
> > > it
> > > > > > seems to imply that the packet was not lost, and
> "PacketSendReceiveInfo"
> > > is
> > > > a
> > > > > > bit long. What do you think about "PacketFeedbackInfo", or
> > > "PacketTwccInfo"?
> > > > > > (TWCC - Transport-wide Congestion Control.)
> > > > > 
> > > > > I don't like PacketTwccInfo or PacketInfo. The first isn't transport
> > > agnostic,
> > > > > and the latter is too generic. If you think of it as "this is
> information
> > > > about
> > > > > a packet that we now have received feedback about", I think it's
> possible
> > to
> > > > > argue that PacketFeedback is a reasonable name. PacketSendReceiveInfo
> > sounds
> > > > > pretty bad too... Any other options, or could we go with
PacketFeedback?
> > :)
> > > > 
> > > > Do we have a decision? :-) Need a second "lgtm" to land.
> > > 
> > > If philips agree, I have no concern
> > 
> > I'm sorry, but I still don't think PacketFeedback is a good name for the
same
> > reason I mentioned earlier. I can expand a bit and maybe you will understand
> > why.
> > 
> > I think the motivation for renaming this struct is somewhat weak. We have
> > different namespaces which allows us to name things in a sensible way (if we
> can
> > figure out what those names are, that's the hard part of course) and not
> having
> > to come up with contrived names due to collisions.
> > 
> > Yes, I agree that it is annoying to have multiple classes with the same name
> > under different namespaces, but I think it's better than having contrived
> names.
> > In this case in particular I would even say that the name is misleading.
> > 
> > This looks like a case of "naming is hard", since none of us can come up
with
> a
> > name that makes sense for the others.
> > 
> > If elad.alon@ think it makes sense and if stefan@ thinks it makes sense then
I
> > think we should land this CL.
> 
> What information is the struct supposed to capture? PacketInfo is probably not
> wrong, but it's very, very generic. PacketFeedback is less overloaded, but
it's
> not very clear. Does it capture just the feedback information received about a
> packet, or are some things filled out regardless of if feedback was received?
> PacketFeedbackInfo is maybe better? PacketTransmission...Info? Status?

I'm okay with either PacketFeedback or PacketFeedbackInfo.
PacketTransmissionInfo sounds too long for me.
These are my cents - all two of them. How is the decision to be made?

[elad.alon_webrtc.org, 2017-03-03 13:42:24]

Would it be OK for me to land this as-is, then, and we can rename to something
better when/if we find such a better name?

[stefan-webrtc, 2017-03-03 17:19:03]

Let's land it as is and then rename it again if we find a better name. Some
other methods that are about to be added will have more sensible names if we
call this PacketFeedback, so I think it's an improvement to PacketInfo.

[elad.alon_webrtc.org, 2017-03-06 12:37:58]

Description was changed from

==========
Rename webrtc::PacketInfo to webrtc::PacketFeedback. This resolves ambiguity
with a similarly named RTCPReceiver::PacketInformation and
RtpPacketizerVp9::PacketInfo.

BUG=
==========

to

==========
Rename webrtc::PacketInfo to webrtc::PacketFeedback. This resolves ambiguity
with a similarly named RTCPReceiver::PacketInformation and
RtpPacketizerVp9::PacketInfo.

BUG=None
==========

[elad.alon_webrtc.org, 2017-03-06 13:02:19]

The CQ bit was checked by elad.alon@webrtc.org

[elad.alon_webrtc.org, 2017-03-06 13:02:19]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2710093004/#ps60001 (title:
"Rebased")

[commit-bot: I haz the power, 2017-03-06 13:02:25]

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

[commit-bot: I haz the power, 2017-03-06 13:32:25]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488805339483500,
"parent_rev": "d1587ad244af3388c1282a715cdf05032ba0c2fc", "commit_rev":
"f94900083484c53bc9717c66794f0b74f5e4f471"}

[commit-bot: I haz the power, 2017-03-06 13:32:28]

Description was changed from

==========
Rename webrtc::PacketInfo to webrtc::PacketFeedback. This resolves ambiguity
with a similarly named RTCPReceiver::PacketInformation and
RtpPacketizerVp9::PacketInfo.

BUG=None
==========

to

==========
Rename webrtc::PacketInfo to webrtc::PacketFeedback. This resolves ambiguity
with a similarly named RTCPReceiver::PacketInformation and
RtpPacketizerVp9::PacketInfo.

BUG=None

Review-Url: https://codereview.webrtc.org/2710093004
Cr-Commit-Position: refs/heads/master@{#17066}
Committed:
https://chromium.googlesource.com/external/webrtc/+/f94900083484c53bc9717c667...
==========

[commit-bot: I haz the power, 2017-03-06 13:32:29]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/f94900083484c53bc9717c667...