From e8abe3ef1b526dc5e6ed50c0b2bd19bab88baf47 Mon Sep 17 00:00:00 2001 From: nisse Date: Wed, 18 Jan 2017 05:00:34 -0800 Subject: [PATCH] Revert of New method StatsObserver::OnCompleteReports, passing ownership. (patchset #2 id:20001 of https://codereview.webrtc.org/2584553002/ ) Reason for revert: The new method doesn't work as intended. It can't pass ownership, because the StatsReports is a vector of raw pointers to StatReport objects owned by the StatsCollector. Original issue's description: > New method StatsObserver::OnCompleteReports, passing ownership. > > The new name, OnCompleteReports rather than OnComplete, is needed > because in C++ method lookup, overriding a method hides all otherwise > inherited methods with the same name, even if they have a different > signature. And here, the intention is that each subclass should > override one or the other of the two methods, and inherit the method it > doesn't override. > > This cl is a prerequisite for > https://codereview.webrtc.org/2567143003/, because the Chrome glue > code needs to retain the stats report after the OnComplete method has > returned. > > Currently, Chrome makes a copy of the stats mapping (which breaks when > changing ValuePtr from an rtc::linked_ptr to an std::unique_ptr). After > this cl, Chrome can be fixed to take ownership and no longer needs to > copy anything, unblocking cl 2567143003. > > BUG=webrtc:6424 > > Review-Url: https://codereview.webrtc.org/2584553002 > Cr-Commit-Position: refs/heads/master@{#15708} > Committed: https://chromium.googlesource.com/external/webrtc/+/b36ee8d498be2fa58fde3f3f3d69a74e4d3b817d TBR=solenberg@webrtc.org,magjed@webrtc.org,tkchin@webrtc.org,hbos@webrtc.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=webrtc:6424 Review-Url: https://codereview.webrtc.org/2641783002 Cr-Commit-Position: refs/heads/master@{#16144} --- webrtc/api/peerconnection.cc | 6 +++--- webrtc/api/peerconnectioninterface.h | 9 +-------- webrtc/api/test/mockpeerconnectionobservers.h | 6 +++--- webrtc/sdk/android/src/jni/peerconnection_jni.cc | 4 ++-- .../objc/Framework/Classes/RTCPeerConnection+Stats.mm | 6 +++--- 5 files changed, 12 insertions(+), 19 deletions(-) diff --git a/webrtc/api/peerconnection.cc b/webrtc/api/peerconnection.cc index 4be3c7d458..78e6790dd7 100644 --- a/webrtc/api/peerconnection.cc +++ b/webrtc/api/peerconnection.cc @@ -1606,9 +1606,9 @@ void PeerConnection::OnMessage(rtc::Message* msg) { } case MSG_GETSTATS: { GetStatsMsg* param = static_cast(msg->pdata); - std::unique_ptr reports(new StatsReports); - stats_->GetStats(param->track, reports.get()); - param->observer->OnCompleteReports(std::move(reports)); + StatsReports reports; + stats_->GetStats(param->track, &reports); + param->observer->OnComplete(reports); delete param; break; } diff --git a/webrtc/api/peerconnectioninterface.h b/webrtc/api/peerconnectioninterface.h index 7dc8ace8c6..e622e525c4 100644 --- a/webrtc/api/peerconnectioninterface.h +++ b/webrtc/api/peerconnectioninterface.h @@ -109,14 +109,7 @@ class StreamCollectionInterface : public rtc::RefCountInterface { class StatsObserver : public rtc::RefCountInterface { public: - // TODO(nisse, hbos): Old version, not passing ownership. Should - // perhaps be deprecated, but since all of this is a legacy - // interface anyway, probably best to leave as is until this class - // can be deleted. - virtual void OnComplete(const StatsReports& reports) {} - virtual void OnCompleteReports(std::unique_ptr reports) { - OnComplete(*reports); - } + virtual void OnComplete(const StatsReports& reports) = 0; protected: virtual ~StatsObserver() {} diff --git a/webrtc/api/test/mockpeerconnectionobservers.h b/webrtc/api/test/mockpeerconnectionobservers.h index 2bf0a3a83f..23647f6de3 100644 --- a/webrtc/api/test/mockpeerconnectionobservers.h +++ b/webrtc/api/test/mockpeerconnectionobservers.h @@ -108,12 +108,12 @@ class MockStatsObserver : public webrtc::StatsObserver { MockStatsObserver() : called_(false), stats_() {} virtual ~MockStatsObserver() {} - void OnCompleteReports(std::unique_ptr reports) override { + virtual void OnComplete(const StatsReports& reports) { ASSERT(!called_); called_ = true; stats_.Clear(); - stats_.number_of_reports = reports->size(); - for (const auto* r : *reports) { + stats_.number_of_reports = reports.size(); + for (const auto* r : reports) { if (r->type() == StatsReport::kStatsReportTypeSsrc) { stats_.timestamp = r->timestamp(); GetIntValue(r, StatsReport::kStatsValueNameAudioOutputLevel, diff --git a/webrtc/sdk/android/src/jni/peerconnection_jni.cc b/webrtc/sdk/android/src/jni/peerconnection_jni.cc index 11d66b7368..4d98414193 100644 --- a/webrtc/sdk/android/src/jni/peerconnection_jni.cc +++ b/webrtc/sdk/android/src/jni/peerconnection_jni.cc @@ -740,9 +740,9 @@ class StatsObserverWrapper : public StatsObserver { virtual ~StatsObserverWrapper() {} - void OnCompleteReports(std::unique_ptr reports) override { + void OnComplete(const StatsReports& reports) override { ScopedLocalRefFrame local_ref_frame(jni()); - jobjectArray j_reports = ReportsToJava(jni(), *reports); + jobjectArray j_reports = ReportsToJava(jni(), reports); jmethodID m = GetMethodID(jni(), *j_observer_class_, "onComplete", "([Lorg/webrtc/StatsReport;)V"); jni()->CallVoidMethod(*j_observer_global_, m, j_reports); diff --git a/webrtc/sdk/objc/Framework/Classes/RTCPeerConnection+Stats.mm b/webrtc/sdk/objc/Framework/Classes/RTCPeerConnection+Stats.mm index 4eb3e708f1..2c4571e0b3 100644 --- a/webrtc/sdk/objc/Framework/Classes/RTCPeerConnection+Stats.mm +++ b/webrtc/sdk/objc/Framework/Classes/RTCPeerConnection+Stats.mm @@ -29,10 +29,10 @@ class StatsObserverAdapter : public StatsObserver { completion_handler_ = nil; } - void OnCompleteReports(std::unique_ptr reports) override { + void OnComplete(const StatsReports& reports) override { RTC_DCHECK(completion_handler_); - NSMutableArray *stats = [NSMutableArray arrayWithCapacity:reports->size()]; - for (const auto* report : *reports) { + NSMutableArray *stats = [NSMutableArray arrayWithCapacity:reports.size()]; + for (const auto* report : reports) { RTCLegacyStatsReport *statsReport = [[RTCLegacyStatsReport alloc] initWithNativeReport:*report]; [stats addObject:statsReport];