Commit 75fe6378 authored by Findit's avatar Findit

Revert "Add plumbing for mDNS Open Screen support"

This reverts commit 53dfd8ca.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 664404 as the
culprit for failures in the build cycles as shown on:
https://analysis.chromium.org/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtLzUzZGZkOGNhYjU5NmMyYWQ2NjVhMGE1NDQ1MmViNGEzYzgwOTBjMTcM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.linux/Linux%20Builder%20%28dbg%29%2832%29/108140

Sample Failed Step: compile

Original change's description:
> Add plumbing for mDNS Open Screen support
> 
> This patch adds classes to implement the mDNS interfaces
> that Open Screen expects, using the discovery system inside of
> Chromium.
> 
> Change-Id: I450b757241ff99a59ec59f5352a0b604f729b9db
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1590536
> Commit-Queue: Jordan Bayles <jophba@chromium.org>
> Reviewed-by: mark a. foltz <mfoltz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#664404}


Change-Id: I02547456efbfa2cd15ee7732f0467feef7d5e409
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1634421
Cr-Commit-Position: refs/heads/master@{#664423}
parent 8e4ce327
...@@ -142,8 +142,6 @@ static_library("router") { ...@@ -142,8 +142,6 @@ static_library("router") {
if (enable_openscreen) { if (enable_openscreen) {
sources += [ sources += [
"providers/openscreen/discovery/open_screen_listener.cc",
"providers/openscreen/discovery/open_screen_listener.h",
"providers/openscreen/network_service_async_packet_sender.cc", "providers/openscreen/network_service_async_packet_sender.cc",
"providers/openscreen/network_service_async_packet_sender.h", "providers/openscreen/network_service_async_packet_sender.h",
"providers/openscreen/network_service_quic_packet_writer.cc", "providers/openscreen/network_service_quic_packet_writer.cc",
...@@ -154,11 +152,7 @@ static_library("router") { ...@@ -154,11 +152,7 @@ static_library("router") {
configs += configs +=
[ "//third_party/openscreen/src/build:allow_build_from_embedder" ] [ "//third_party/openscreen/src/build:allow_build_from_embedder" ]
deps += [ deps += [ "//third_party/openscreen/src/platform" ]
"//third_party/openscreen/src/osp/public",
"//third_party/openscreen/src/osp_base",
"//third_party/openscreen/src/platform",
]
} }
} }
} }
......
include_rules = [ include_rules = [
"+services/network", "+services/network",
"+third_party/openscreen/src/osp/public", "+third_party/openscreen/src"
"+third_party/openscreen/src/osp_base",
"+third_party/openscreen/src/platform/api",
] ]
// Copyright 2019 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "chrome/browser/media/router/providers/openscreen/discovery/open_screen_listener.h"
#include <utility>
namespace media_router {
namespace {
const char kOpenScreenServiceType[] = "openscreen_.udp_";
openscreen::ServiceInfo ServiceInfoFromServiceDescription(
const local_discovery::ServiceDescription& desc) {
openscreen::ErrorOr<openscreen::IPAddress> address =
openscreen::IPAddress::Parse(desc.address.host());
DCHECK(address);
openscreen::ServiceInfo service_info;
service_info.service_id = desc.service_name;
service_info.friendly_name = desc.instance_name();
if (address.value().IsV4()) {
service_info.v4_endpoint =
openscreen::IPEndpoint{address.value(), desc.address.port()};
service_info.v6_endpoint = {};
} else {
service_info.v4_endpoint = {};
service_info.v6_endpoint =
openscreen::IPEndpoint{address.value(), desc.address.port()};
}
return service_info;
}
} // namespace
OpenScreenListener::OpenScreenListener(std::string service_type)
: service_type_(kOpenScreenServiceType) {}
OpenScreenListener::~OpenScreenListener() {}
bool OpenScreenListener::Start() {
is_running_ = true;
// TODO(jophba): instantiate local_discovery::ServiceDiscoveryClient
for (auto* observer : observers_) {
observer->OnStarted();
}
return true;
}
bool OpenScreenListener::StartAndSuspend() {
for (auto* observer : observers_) {
observer->OnStarted();
observer->OnSuspended();
}
return true;
}
bool OpenScreenListener::Stop() {
DCHECK(is_running_);
is_running_ = false;
for (auto* observer : observers_) {
observer->OnStopped();
}
return true;
}
bool OpenScreenListener::Suspend() {
DCHECK(is_running_);
is_running_ = false;
for (auto* observer : observers_) {
observer->OnSuspended();
}
return true;
}
bool OpenScreenListener::Resume() {
DCHECK(!is_running_);
is_running_ = true;
for (auto* observer : observers_) {
observer->OnStarted();
}
return true;
}
bool OpenScreenListener::SearchNow() {
is_running_ = true;
for (auto* observer : observers_) {
observer->OnSearching();
}
return true;
}
const std::vector<openscreen::ServiceInfo>& OpenScreenListener::GetReceivers()
const {
return receivers_;
}
void OpenScreenListener::AddObserver(ServiceListener::Observer* observer) {
CHECK(observer);
observers_.push_back(observer);
}
void OpenScreenListener::RemoveObserver(ServiceListener::Observer* observer) {
CHECK(observer);
observers_.erase(std::remove(observers_.begin(), observers_.end(), observer),
observers_.end());
}
void OpenScreenListener::RunTasks() {}
void OpenScreenListener::OnDeviceChanged(
const std::string& service_type,
bool added,
const local_discovery::ServiceDescription& service_description) {
CHECK_EQ(service_type, service_type_);
if (!is_running_) {
return;
}
openscreen::ServiceInfo service_info =
ServiceInfoFromServiceDescription(service_description);
if (added) {
receivers_.push_back(std::move(service_info));
const openscreen::ServiceInfo& ref = receivers_.back();
for (auto* observer : observers_) {
observer->OnReceiverAdded(ref);
}
} else {
auto it =
std::find_if(receivers_.begin(), receivers_.end(),
[&service_info](const openscreen::ServiceInfo& info) {
return info.service_id == service_info.service_id;
});
*it = std::move(service_info);
for (auto* observer : observers_) {
observer->OnReceiverChanged(*it);
}
}
}
void OpenScreenListener::OnDeviceRemoved(const std::string& service_type,
const std::string& service_name) {
CHECK(service_type == service_type_);
if (!is_running_) {
return;
}
const auto& removed_it =
std::find_if(receivers_.begin(), receivers_.end(),
[&service_name](openscreen::ServiceInfo& info) {
return info.service_id == service_name;
});
// Move the receiver we want to remove to the end, so we don't have to shift.
DCHECK(removed_it != receivers_.end());
const openscreen::ServiceInfo removed_info = std::move(*removed_it);
if (removed_it != receivers_.end() - 1) {
*removed_it = std::move(receivers_.back());
}
receivers_.pop_back();
for (auto* observer : observers_) {
observer->OnReceiverRemoved(removed_info);
}
}
void OpenScreenListener::OnDeviceCacheFlushed(const std::string& service_type) {
CHECK(service_type == service_type_);
receivers_.clear();
// We still flush even if not running, since it's not going to be accurate.
if (!is_running_) {
return;
}
for (auto* observer : observers_) {
observer->OnAllReceiversRemoved();
}
}
} // namespace media_router
// Copyright 2019 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef CHROME_BROWSER_MEDIA_ROUTER_PROVIDERS_OPENSCREEN_DISCOVERY_OPEN_SCREEN_LISTENER_H_
#define CHROME_BROWSER_MEDIA_ROUTER_PROVIDERS_OPENSCREEN_DISCOVERY_OPEN_SCREEN_LISTENER_H_
#include <string>
#include <vector>
#include "chrome/browser/local_discovery/service_discovery_device_lister.h"
#include "third_party/openscreen/src/osp/public/service_info.h"
#include "third_party/openscreen/src/osp/public/service_listener.h"
#include "third_party/openscreen/src/osp_base/ip_address.h"
namespace media_router {
class OpenScreenListener
: public openscreen::ServiceListener,
local_discovery::ServiceDiscoveryDeviceLister::Delegate {
public:
explicit OpenScreenListener(std::string service_type);
// ServiceListener overrides
~OpenScreenListener() override;
bool Start() override;
bool StartAndSuspend() override;
bool Stop() override;
bool Suspend() override;
bool Resume() override;
bool SearchNow() override;
const std::vector<openscreen::ServiceInfo>& GetReceivers() const override;
void AddObserver(ServiceListener::Observer* observer) override;
void RemoveObserver(ServiceListener::Observer* observer) override;
void RunTasks() override;
// ServiceDiscoveryDeviceLister::Delegate
void OnDeviceChanged(
const std::string& service_type,
bool added,
const local_discovery::ServiceDescription& service_description) override;
void OnDeviceRemoved(const std::string& service_type,
const std::string& service_name) override;
void OnDeviceCacheFlushed(const std::string& service_type) override;
private:
bool is_running_ = false;
const std::string service_type_;
std::vector<openscreen::ServiceInfo> receivers_;
DISALLOW_COPY_AND_ASSIGN(OpenScreenListener);
};
} // namespace media_router
#endif // CHROME_BROWSER_MEDIA_ROUTER_PROVIDERS_OPENSCREEN_DISCOVERY_OPEN_SCREEN_LISTENER_H_
// Copyright 2019 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "chrome/browser/media/router/providers/openscreen/discovery/open_screen_listener.h"
#include "base/time/time.h"
#include "net/base/host_port_pair.h"
#include "net/base/ip_address.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
using ::testing::_;
using ::testing::Invoke;
using ::testing::Return;
using ::testing::StrictMock;
using ::testing::WithArg;
namespace media_router {
const char kServiceType[] = "openscreen_.udp_";
class MockServiceListenerObserver
: public openscreen::ServiceListener::Observer {
public:
MOCK_METHOD0(OnStarted, void());
MOCK_METHOD0(OnStopped, void());
MOCK_METHOD0(OnSuspended, void());
MOCK_METHOD0(OnSearching, void());
MOCK_METHOD1(OnReceiverAdded, void(const openscreen::ServiceInfo&));
MOCK_METHOD1(OnReceiverChanged, void(const openscreen::ServiceInfo&));
MOCK_METHOD1(OnReceiverRemoved, void(const openscreen::ServiceInfo&));
MOCK_METHOD0(OnAllReceiversRemoved, void());
MOCK_METHOD1(OnError, void(openscreen::ServiceListenerError));
MOCK_METHOD1(OnMetrics, void(openscreen::ServiceListener::Metrics));
};
// Although the testing framework can do a byte comparison, when it fails
// it's difficult to figure out *exactly* what is wrong with the actual
// Service Info class.
MATCHER_P(ServiceInfoEquals, expected, "") {
return (expected.service_id == arg.service_id) &&
(expected.friendly_name == arg.friendly_name) &&
(expected.network_interface_index == arg.network_interface_index) &&
(expected.v4_endpoint == arg.v4_endpoint) &&
(expected.v6_endpoint == arg.v6_endpoint);
}
class OpenScreenListenerTest : public ::testing::Test {
protected:
void SetUp() override {
valid_description_.service_name = "mock_service.test_service_type";
valid_description_.address = net::HostPortPair("192.168.0.10", 8888);
valid_description_.metadata = {"foo", "bar", "baz"};
valid_description_.ip_address = net::IPAddress(192, 168, 0, 10);
valid_description_.last_seen = base::Time();
service_info_.service_id = "mock_service.test_service_type";
service_info_.friendly_name = "mock_service";
service_info_.v4_endpoint =
openscreen::IPEndpoint{openscreen::IPAddress(192, 168, 0, 10), 8888};
service_info_.v6_endpoint = {};
}
OpenScreenListenerTest() : listener(kServiceType), observer() {
listener.AddObserver(&observer);
}
void ExpectReceiverAdded(const openscreen::ServiceInfo& info) {
EXPECT_CALL(observer, OnReceiverAdded(ServiceInfoEquals(info)));
}
void ExpectReceiverChanged(const openscreen::ServiceInfo& info) {
EXPECT_CALL(observer, OnReceiverChanged(ServiceInfoEquals(info)));
}
void ExpectReceiverRemoved(const openscreen::ServiceInfo& info) {
EXPECT_CALL(observer, OnReceiverRemoved(ServiceInfoEquals(info)));
}
OpenScreenListener listener;
StrictMock<MockServiceListenerObserver> observer;
local_discovery::ServiceDescription valid_description_;
openscreen::ServiceInfo service_info_;
};
TEST_F(OpenScreenListenerTest, DeviceAddedNotifiesObserversIfStarted) {
listener.OnDeviceChanged(kServiceType, true, valid_description_);
EXPECT_CALL(observer, OnStarted()).Times(1);
listener.Start();
ExpectReceiverAdded(service_info_);
listener.OnDeviceChanged(kServiceType, true, valid_description_);
}
TEST_F(OpenScreenListenerTest, DeviceChangedNotifiesObserversIfStarted) {
listener.OnDeviceChanged(kServiceType, false, valid_description_);
EXPECT_CALL(observer, OnStarted()).Times(1);
listener.Start();
ExpectReceiverAdded(service_info_);
listener.OnDeviceChanged(kServiceType, true, valid_description_);
ExpectReceiverChanged(service_info_);
listener.OnDeviceChanged(kServiceType, false, valid_description_);
}
TEST_F(OpenScreenListenerTest, DeviceRemovedNotifiesObserversIfStarted) {
listener.OnDeviceRemoved(kServiceType, valid_description_.service_name);
EXPECT_CALL(observer, OnStarted()).Times(1);
listener.Start();
ExpectReceiverAdded(service_info_);
listener.OnDeviceChanged(kServiceType, true, valid_description_);
ExpectReceiverRemoved(service_info_);
listener.OnDeviceRemoved(kServiceType, valid_description_.service_name);
}
TEST_F(OpenScreenListenerTest, CachedFlushNotifiesObserversIfStarted) {
listener.OnDeviceCacheFlushed(kServiceType);
EXPECT_CALL(observer, OnStarted()).Times(1);
listener.Start();
EXPECT_CALL(observer, OnAllReceiversRemoved()).Times(1);
listener.OnDeviceCacheFlushed(kServiceType);
}
TEST_F(OpenScreenListenerTest, CachedFlushEmptiesReceiverList) {
EXPECT_CALL(observer, OnStarted()).Times(1);
listener.Start();
ExpectReceiverAdded(service_info_);
listener.OnDeviceChanged(kServiceType, true, valid_description_);
ExpectReceiverAdded(service_info_);
listener.OnDeviceChanged(kServiceType, true, valid_description_);
EXPECT_EQ(2ul, listener.GetReceivers().size());
EXPECT_CALL(observer, OnAllReceiversRemoved()).Times(1);
listener.OnDeviceCacheFlushed(kServiceType);
EXPECT_EQ(0ul, listener.GetReceivers().size());
}
TEST_F(OpenScreenListenerTest, StartNotifiesObservers) {
EXPECT_CALL(observer, OnStarted()).Times(1);
listener.Start();
}
TEST_F(OpenScreenListenerTest, StopNotifiesObservers) {
EXPECT_CALL(observer, OnStarted()).Times(1);
EXPECT_CALL(observer, OnStopped()).Times(1);
listener.Start();
listener.Stop();
}
TEST_F(OpenScreenListenerTest, SuspendNotifiesObservers) {
EXPECT_CALL(observer, OnStarted()).Times(2);
EXPECT_CALL(observer, OnSuspended()).Times(2);
listener.Start();
listener.Suspend();
listener.StartAndSuspend();
}
TEST_F(OpenScreenListenerTest, ResumeNotifiesObservers) {
EXPECT_CALL(observer, OnStarted()).Times(2);
EXPECT_CALL(observer, OnSuspended()).Times(1);
listener.Start();
listener.Suspend();
listener.Resume();
}
TEST_F(OpenScreenListenerTest, SearchingNotifiesObservers) {
EXPECT_CALL(observer, OnStarted()).Times(1);
listener.Start();
EXPECT_CALL(observer, OnSearching()).Times(1);
listener.SearchNow();
}
TEST_F(OpenScreenListenerTest, RemovedObserversDoNotGetNotified) {
listener.RemoveObserver(&observer);
listener.Start();
listener.Stop();
listener.StartAndSuspend();
listener.Resume();
listener.SearchNow();
listener.Suspend();
listener.Resume();
listener.OnDeviceCacheFlushed(kServiceType);
listener.OnDeviceChanged(kServiceType, true, valid_description_);
listener.OnDeviceChanged(kServiceType, true, valid_description_);
listener.OnDeviceRemoved(kServiceType, valid_description_.service_name);
}
TEST_F(OpenScreenListenerTest, DoesNotRecordReceiversIfNotStarted) {
EXPECT_EQ(0ul, listener.GetReceivers().size());
listener.OnDeviceChanged(kServiceType, true, valid_description_);
listener.OnDeviceChanged(kServiceType, false, valid_description_);
listener.OnDeviceChanged(kServiceType, true, valid_description_);
EXPECT_EQ(0ul, listener.GetReceivers().size());
}
} // namespace media_router
...@@ -11,7 +11,7 @@ ...@@ -11,7 +11,7 @@
#include "mojo/public/cpp/bindings/interface_request.h" #include "mojo/public/cpp/bindings/interface_request.h"
namespace media_router { namespace openscreen {
NetworkServiceAsyncPacketSender::NetworkServiceAsyncPacketSender( NetworkServiceAsyncPacketSender::NetworkServiceAsyncPacketSender(
network::mojom::NetworkContext* network_context) { network::mojom::NetworkContext* network_context) {
network::mojom::UDPSocketRequest socket_request(mojo::MakeRequest(&socket_)); network::mojom::UDPSocketRequest socket_request(mojo::MakeRequest(&socket_));
...@@ -39,4 +39,4 @@ net::Error NetworkServiceAsyncPacketSender::SendTo( ...@@ -39,4 +39,4 @@ net::Error NetworkServiceAsyncPacketSender::SendTo(
return net::Error::OK; return net::Error::OK;
} }
} // namespace media_router } // namespace openscreen
...@@ -13,7 +13,7 @@ ...@@ -13,7 +13,7 @@
#include "services/network/public/mojom/network_context.mojom.h" #include "services/network/public/mojom/network_context.mojom.h"
#include "services/network/public/mojom/udp_socket.mojom.h" #include "services/network/public/mojom/udp_socket.mojom.h"
namespace media_router { namespace openscreen {
class AsyncPacketSender { class AsyncPacketSender {
public: public:
virtual ~AsyncPacketSender() {} virtual ~AsyncPacketSender() {}
...@@ -41,6 +41,6 @@ class NetworkServiceAsyncPacketSender : public AsyncPacketSender { ...@@ -41,6 +41,6 @@ class NetworkServiceAsyncPacketSender : public AsyncPacketSender {
DISALLOW_COPY_AND_ASSIGN(NetworkServiceAsyncPacketSender); DISALLOW_COPY_AND_ASSIGN(NetworkServiceAsyncPacketSender);
}; };
} // namespace media_router } // namespace openscreen
#endif // CHROME_BROWSER_MEDIA_ROUTER_PROVIDERS_OPENSCREEN_NETWORK_SERVICE_ASYNC_PACKET_SENDER_H_ #endif // CHROME_BROWSER_MEDIA_ROUTER_PROVIDERS_OPENSCREEN_NETWORK_SERVICE_ASYNC_PACKET_SENDER_H_
...@@ -16,7 +16,7 @@ ...@@ -16,7 +16,7 @@
#include "net/base/ip_endpoint.h" #include "net/base/ip_endpoint.h"
#include "net/third_party/quiche/src/quic/core/quic_constants.h" #include "net/third_party/quiche/src/quic/core/quic_constants.h"
namespace media_router { namespace openscreen {
namespace { namespace {
// Set a reasonable maximum number of packets in flight, for a total of // Set a reasonable maximum number of packets in flight, for a total of
...@@ -165,4 +165,4 @@ void NetworkServiceQuicPacketWriter::WritePacketHelper( ...@@ -165,4 +165,4 @@ void NetworkServiceQuicPacketWriter::WritePacketHelper(
UpdateIsWriteBlocked(); UpdateIsWriteBlocked();
} }
} // namespace media_router } // namespace openscreen
...@@ -19,7 +19,7 @@ ...@@ -19,7 +19,7 @@
#include "chrome/browser/media/router/providers/openscreen/network_service_async_packet_sender.h" #include "chrome/browser/media/router/providers/openscreen/network_service_async_packet_sender.h"
namespace media_router { namespace openscreen {
// Chrome-specific packet writer. Intended for use outside of the Network // Chrome-specific packet writer. Intended for use outside of the Network
// service, this class uses the network service's UdpSocket for sending and // service, this class uses the network service's UdpSocket for sending and
...@@ -117,6 +117,6 @@ class NetworkServiceQuicPacketWriter : quic::QuicPacketWriter { ...@@ -117,6 +117,6 @@ class NetworkServiceQuicPacketWriter : quic::QuicPacketWriter {
DISALLOW_COPY_AND_ASSIGN(NetworkServiceQuicPacketWriter); DISALLOW_COPY_AND_ASSIGN(NetworkServiceQuicPacketWriter);
}; };
} // namespace media_router } // namespace openscreen
#endif // CHROME_BROWSER_MEDIA_ROUTER_PROVIDERS_OPENSCREEN_NETWORK_SERVICE_QUIC_PACKET_WRITER_H_ #endif // CHROME_BROWSER_MEDIA_ROUTER_PROVIDERS_OPENSCREEN_NETWORK_SERVICE_QUIC_PACKET_WRITER_H_
...@@ -13,7 +13,7 @@ ...@@ -13,7 +13,7 @@
#include "media/base/fake_single_thread_task_runner.h" #include "media/base/fake_single_thread_task_runner.h"
#include "net/base/net_errors.h" #include "net/base/net_errors.h"
namespace media_router { namespace openscreen {
using ::testing::_; using ::testing::_;
using ::testing::Invoke; using ::testing::Invoke;
using ::testing::Return; using ::testing::Return;
...@@ -267,4 +267,4 @@ TEST(NetworkServiceQuicPacketWriterTest, TooManyPacketsCausesWriteBlockage) { ...@@ -267,4 +267,4 @@ TEST(NetworkServiceQuicPacketWriterTest, TooManyPacketsCausesWriteBlockage) {
ASSERT_FALSE(test_writer.writer->IsWriteBlocked()); ASSERT_FALSE(test_writer.writer->IsWriteBlocked());
} }
} // namespace media_router } // namespace openscreen
...@@ -3738,6 +3738,10 @@ test("unit_tests") { ...@@ -3738,6 +3738,10 @@ test("unit_tests") {
"../renderer/media/chrome_webrtc_log_message_delegate_unittest.cc", "../renderer/media/chrome_webrtc_log_message_delegate_unittest.cc",
] ]
if (enable_openscreen) {
sources += [ "../browser/media/router/providers/openscreen/network_service_quic_packet_writer_unittest.cc" ]
}
deps += [ deps += [
"//components/bubble:test_support", "//components/bubble:test_support",
"//services/network:test_support", "//services/network:test_support",
...@@ -4844,15 +4848,6 @@ test("unit_tests") { ...@@ -4844,15 +4848,6 @@ test("unit_tests") {
if (is_win || is_mac) { if (is_win || is_mac) {
deps += [ "//chrome/updater:updater_tests" ] deps += [ "//chrome/updater:updater_tests" ]
} }
if (enable_openscreen) {
include_dirs = [ "//third_party/openscreen/src" ]
sources += [
"../browser/media/router/providers/openscreen/discovery/open_screen_listener_unittest.cc",
"../browser/media/router/providers/openscreen/network_service_quic_packet_writer_unittest.cc",
]
}
} }
static_library("test_support_unit") { static_library("test_support_unit") {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment