Connect TurnPort and TCPPort to AsyncPacketSocket::SignalSentPacket.

Connect TurnPort and TCPPort to AsyncPacketSocket::SignalSentPacket.

To reduce the risk of future mistakes when connecting Ports, Port::OnSentPacket was made pure virtual to ensure that new implementations take care of it.

BUG=4173
R=pthatcher@webrtc.org

Committed: https://crrev.com/7307952a5bf63311e5f9a2a90089a06177e42716
Cr-Commit-Position: refs/heads/master@{#11247}

[stefan-webrtc, 2016-01-12 13:37:41]

Description was changed from

==========
Connect TurnPort::OnSentPacket to SignalSentPacket.

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

to

==========
Connect TurnPort::OnSentPacket to SignalSentPacket.

BUG=4173
==========

[stefan-webrtc, 2016-01-12 13:37:55]

stefan@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[stefan-webrtc, 2016-01-12 13:46:38]

The CQ bit was checked by stefan@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-12 13:46:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1577873003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1577873003/1

[commit-bot: I haz the power, 2016-01-12 15:47:00]

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

[commit-bot: I haz the power, 2016-01-12 15:47:00]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[stefan-webrtc, 2016-01-13 11:02:40]

Description was changed from

==========
Connect TurnPort::OnSentPacket to SignalSentPacket.

BUG=4173
==========

to

==========
Connect TurnPort and TCPPort to AsyncPacketSocket::SignalSentPacket.

To reduce the risk of future mistakes when connecting Ports, Port::OnSentPacket
was made pure virtual to ensure that new implementations take care of it.

BUG=4173
==========

[pthatcher1, 2016-01-13 19:42:37]

lgtm, assuming you improve the comments a bit.

https://codereview.webrtc.org/1577873003/diff/20001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/1577873003/diff/20001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:285: // PortInterface.
I think this needs to be more explicit about what should connect to what. 
Perhaps:

This is made pure virtual to notify subclasses of Port that they MUST listen to
AsyncPacketSocket::SignalSentPacket and then call PortInterface::OnSentPacket.

https://codereview.webrtc.org/1577873003/diff/20001/webrtc/p2p/base/relayport.h
File webrtc/p2p/base/relayport.h (right):

https://codereview.webrtc.org/1577873003/diff/20001/webrtc/p2p/base/relayport...
webrtc/p2p/base/relayport.h:100: const rtc::SentPacket& sent_packet) override {}
You said it must call PortInterface::OnSentPacket, but then it doesn't.  I know
we don't care much about RelayPort, but at least you could put a comment about
this.

[stefan-webrtc, 2016-01-14 10:13:24]

Improve comments.

[stefan-webrtc, 2016-01-14 10:14:38]

https://codereview.webrtc.org/1577873003/diff/20001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/1577873003/diff/20001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:285: // PortInterface.
On 2016/01/13 19:42:36, pthatcher1 wrote:
> I think this needs to be more explicit about what should connect to what. 
> Perhaps:
> 
> This is made pure virtual to notify subclasses of Port that they MUST listen
to
> AsyncPacketSocket::SignalSentPacket and then call PortInterface::OnSentPacket.

Agree, your text is better. Thanks!

https://codereview.webrtc.org/1577873003/diff/20001/webrtc/p2p/base/relayport.h
File webrtc/p2p/base/relayport.h (right):

https://codereview.webrtc.org/1577873003/diff/20001/webrtc/p2p/base/relayport...
webrtc/p2p/base/relayport.h:100: const rtc::SentPacket& sent_packet) override {}
On 2016/01/13 19:42:36, pthatcher1 wrote:
> You said it must call PortInterface::OnSentPacket, but then it doesn't.  I
know
> we don't care much about RelayPort, but at least you could put a comment about
> this.

Done. To be honest, I'm not entirely sure how or if RelayPort is being used
today. I'm happy to do a follow-up if you have suggestions for improvements.

[stefan-webrtc, 2016-01-14 10:14:45]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2016-01-14 10:14:46]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1577873003/#ps40001 (title:
"Improve comments.")

[commit-bot: I haz the power, 2016-01-14 10:14:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1577873003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1577873003/40001

[commit-bot: I haz the power, 2016-01-14 12:15:15]

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

[commit-bot: I haz the power, 2016-01-14 12:15:15]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[stefan-webrtc, 2016-01-14 12:16:02]

Description was changed from

==========
Connect TurnPort and TCPPort to AsyncPacketSocket::SignalSentPacket.

To reduce the risk of future mistakes when connecting Ports, Port::OnSentPacket
was made pure virtual to ensure that new implementations take care of it.

BUG=4173
==========

to

==========
Connect TurnPort and TCPPort to AsyncPacketSocket::SignalSentPacket.

To reduce the risk of future mistakes when connecting Ports, Port::OnSentPacket
was made pure virtual to ensure that new implementations take care of it.

BUG=4173
R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/7307952a5bf63311e5f9a2a90...
==========

[stefan-webrtc, 2016-01-14 12:16:04]

Committed patchset #3 (id:40001) manually as
7307952a5bf63311e5f9a2a90089a06177e42716 (presubmit successful).

[commit-bot: I haz the power, 2016-01-14 12:16:06]

Description was changed from

==========
Connect TurnPort and TCPPort to AsyncPacketSocket::SignalSentPacket.

To reduce the risk of future mistakes when connecting Ports, Port::OnSentPacket
was made pure virtual to ensure that new implementations take care of it.

BUG=4173
R=pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/7307952a5bf63311e5f9a2a90...
==========

to

==========
Connect TurnPort and TCPPort to AsyncPacketSocket::SignalSentPacket.

To reduce the risk of future mistakes when connecting Ports, Port::OnSentPacket
was made pure virtual to ensure that new implementations take care of it.

BUG=4173
R=pthatcher@webrtc.org

Committed: https://crrev.com/7307952a5bf63311e5f9a2a90089a06177e42716
Cr-Commit-Position: refs/heads/master@{#11247}
==========

[commit-bot: I haz the power, 2016-01-14 12:16:06]

Patchset 3 (id:??) landed as
https://crrev.com/7307952a5bf63311e5f9a2a90089a06177e42716
Cr-Commit-Position: refs/heads/master@{#11247}

[tommi, 2016-01-14 12:55:59]

tommi@webrtc.org changed reviewers:
+ tommi@webrtc.org

[tommi, 2016-01-14 12:55:59]

Looks like this broke chrome:
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

Need to revert since it's blocking the roll.

[tommi, 2016-01-14 12:56:42]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.webrtc.org/1586063002/ by tommi@webrtc.org.

The reason for reverting is: Broke Chrome:

https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

FAILED: cd ../../third_party/libjingle; python
../../native_client/build/build_nexe.py --root ../.. --product-dir
../../out/Debug/xyz --config-name Debug -t ../../native_client/toolchain/ --arch
pnacl --build newlib_plib --name
../../out/Debug/gen/tc_pnacl_newlib/lib/libjingle_nacl.a --objdir
../../out/Debug/obj/third_party/libjingle/libjingle_nacl.gen/pnacl_newlib-pnacl/libjingle_nacl
"--include-dirs=../../out/Debug/gen/tc_pnacl_newlib/include ../..
\"../../out/Debug/gen\" ./source ../ ../../native_client_sdk/src/libraries
../../native_client_sdk/src/libraries/nacl_io/include
../../native_client_sdk/src/libraries/third_party/newlib-extras
../expat/files/lib ../boringssl/src/include" "--compile_flags=-O2 -g -Wall
-fdiagnostics-show-option -Werror  -Wno-unused-function -Wno-char-subscripts
-Wno-c++11-extensions -Wno-unnamed-type-template-args -Wno-extra-semi
-Wno-unused-private-field -Wno-char-subscripts -Wno-unused-function
\"-std=gnu++11\" " --gomadir /b/build/goma "--defines=\"__STDC_LIMIT_MACROS=1\"
\"__STDC_FORMAT_MACROS=1\" \"_GNU_SOURCE=1\" \"_POSIX_C_SOURCE=199506\"
\"_XOPEN_SOURCE=600\" \"DYNAMIC_ANNOTATIONS_ENABLED=1\"
\"DYNAMIC_ANNOTATIONS_PREFIX=NACL_\" \"NACL_BUILD_ARCH=x86\"
V8_DEPRECATION_WARNINGS \"CLD_VERSION=2\" \"_FILE_OFFSET_BITS=64\"
CHROMIUM_BUILD \"CR_CLANG_REVISION=255169-1\" COMPONENT_BUILD
UI_COMPOSITOR_IMAGE_TRANSPORT \"USE_AURA=1\" \"USE_ASH=1\" \"USE_PANGO=1\"
\"USE_CAIRO=1\" \"USE_DEFAULT_RENDER_THEME=1\" \"USE_LIBJPEG_TURBO=1\"
\"USE_X11=1\" \"IMAGE_LOADER_EXTENSION=1\" \"ENABLE_WEBRTC=1\"
\"ENABLE_MEDIA_ROUTER=1\" USE_PROPRIETARY_CODECS ENABLE_PEPPER_CDMS
ENABLE_CONFIGURATION_POLICY ENABLE_NOTIFICATIONS \"ENABLE_HIDPI=1\"
\"ENABLE_TOPCHROME_MD=1\" USE_UDEV DONT_EMBED_BUILD_METADATA
\"DCHECK_ALWAYS_ON=1\" FIELDTRIAL_TESTING_ENABLED \"ENABLE_TASK_MANAGER=1\"
\"ENABLE_EXTENSIONS=1\" \"ENABLE_PDF=1\" \"ENABLE_PLUGINS=1\"
\"ENABLE_SESSION_SERVICE=1\" \"ENABLE_THEMES=1\" \"ENABLE_AUTOFILL_DIALOG=1\"
\"ENABLE_BACKGROUND=1\" \"ENABLE_PRINTING=1\" \"ENABLE_PRINT_PREVIEW=1\"
\"ENABLE_SPELLCHECK=1\" \"ENABLE_CAPTIVE_PORTAL_DETECTION=1\"
\"ENABLE_APP_LIST=1\" \"ENABLE_SUPERVISED_USERS=1\" \"ENABLE_MDNS=1\"
\"ENABLE_SERVICE_DISCOVERY=1\" V8_USE_EXTERNAL_STARTUP_DATA FULL_SAFE_BROWSING
SAFE_BROWSING_CSD SAFE_BROWSING_DB_LOCAL EXPAT_RELATIVE_PATH FEATURE_ENABLE_SSL
GTEST_RELATIVE_PATH HAVE_OPENSSL_SSL_H NO_MAIN_THREAD_WRAPPING NO_SOUND_SYSTEM
WEBRTC_POSIX SRTP_RELATIVE_PATH SSL_USE_OPENSSL USE_WEBRTC_DEV_BRANCH
\"timezone=_timezone\" XML_STATIC \"USE_LIBPCI=1\" \"USE_OPENSSL=1\"
\"USE_OPENSSL_CERTS=1\"" "--link_flags=-B../../out/Debug/gen/tc_pnacl_newlib/lib
 "
"--source-list=../../out/gypfiles/third_party/libjingle/pnacl_newlib.libjingle_nacl.source_list.gypcmd"
In file included from ../webrtc/p2p/base/tcpport.cc:67:
../webrtc/p2p/base/tcpport.h:50:23: error: 'CreateConnection' overrides a member
function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
  virtual Connection* CreateConnection(const Candidate& address,
                      ^
../webrtc/p2p/base/portinterface.h:71:23: note: overridden virtual function is
here
  virtual Connection* CreateConnection(
                      ^
In file included from ../webrtc/p2p/base/tcpport.cc:67:
../webrtc/p2p/base/tcpport.h:53:16: error: 'PrepareAddress' overrides a member
function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
  virtual void PrepareAddress();
               ^
../webrtc/p2p/base/portinterface.h:63:16: note: overridden virtual function is
here
  virtual void PrepareAddress() = 0;
               ^

(etc).