Skip to content

Commit

Permalink
[DevTools] Use base::span<const uint8_t> for devtools messages in con…
Browse files Browse the repository at this point in the history
…tent/public

The interfaces in content/public currently use const std::string& for
devtools messages. This requires client code as well as implementation
code to use an std::string for representing the messages (depending on the
code that's providing the const std::string&).

Moving to base::span allows code to use any container that gives
access to a sequence of bytes without having to make a copy, including:
- std::string (just like now)
- std::vector<uint8_t> (base::span<const uint8_t>'s implicit
  constructor takes it).
- the BigBuffer from Mojo (provides a base::span<const uint8_t> already).
- base::span<const uint8_t>
- crdtp::span<uint8_t> (base::span<const uint8_t>'s implicit
  constructor takes it).
- base::StringPiece (going to / from this one requires a reinterpret_cast,
  but at least it's efficient).

The switch removes a few copies of byte sequences (e.g. from BigBuffer
to std::string). It introduces a few conversions between
the non-owning representations, but these are cheap.

This PR includes a roll of third_party/inspector_protocol.
New revision is: 3b0551d3904f7fc067e78905ce697002187fa7a5

Change-Id: I4bea62307378f72ae95fa631db42c3bc95694d2e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1965407
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Stephane Zermatten <szermatt@chromium.org>
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: Yusuf Ozuysal <yusufo@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Johannes Henkel <johannes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#726674}
  • Loading branch information
Johannes Henkel authored and Commit Bot committed Dec 20, 2019
1 parent 33387b0 commit 21e1940
Show file tree
Hide file tree
Showing 53 changed files with 303 additions and 229 deletions.
4 changes: 2 additions & 2 deletions chrome/browser/android/devtools_manager_delegate_android.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class ClientProxy : public content::DevToolsAgentHostClient {
~ClientProxy() override {}

void DispatchProtocolMessage(DevToolsAgentHost* agent_host,
const std::string& message) override {
base::span<const uint8_t> message) override {
proxy_->DispatchOnClientHost(message);
}

Expand Down Expand Up @@ -129,7 +129,7 @@ class TabProxyDelegate : public content::DevToolsExternalAgentProxyDelegate {
}

void SendMessageToBackend(content::DevToolsExternalAgentProxy* proxy,
const std::string& message) override {
base::span<const uint8_t> message) override {
auto it = proxies_.find(proxy);
if (it == proxies_.end())
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ void ChromeDevToolsManagerDelegate::HandleCommand(
DevToolsAgentHost* agent_host,
content::DevToolsAgentHostClient* client,
const std::string& method,
const std::string& message,
base::span<const uint8_t> message,
NotHandledCallback callback) {
if (sessions_.find(client) == sessions_.end()) {
std::move(callback).Run(message);
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/devtools/chrome_devtools_manager_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class ChromeDevToolsManagerDelegate : public content::DevToolsManagerDelegate {
void HandleCommand(content::DevToolsAgentHost* agent_host,
content::DevToolsAgentHostClient* client,
const std::string& method,
const std::string& message,
base::span<const uint8_t> message,
NotHandledCallback callback) override;
std::string GetTargetType(content::WebContents* web_contents) override;
std::string GetTargetTitle(content::WebContents* web_contents) override;
Expand Down
9 changes: 4 additions & 5 deletions chrome/browser/devtools/chrome_devtools_session.cc
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ ChromeDevToolsSession::~ChromeDevToolsSession() = default;

void ChromeDevToolsSession::HandleCommand(
const std::string& method,
const std::string& message,
base::span<const uint8_t> message,
content::DevToolsManagerDelegate::NotHandledCallback callback) {
if (!dispatcher_.canDispatch(method)) {
std::move(callback).Run(message);
Expand All @@ -77,11 +77,10 @@ static void SendProtocolResponseOrNotification(
std::unique_ptr<protocol::Serializable> message) {
std::vector<uint8_t> cbor = std::move(*message).TakeSerialized();
if (client->UsesBinaryProtocol()) {
client->DispatchProtocolMessage(agent_host,
std::string(cbor.begin(), cbor.end()));
client->DispatchProtocolMessage(agent_host, cbor);
return;
}
std::string json;
std::vector<uint8_t> json;
crdtp::Status status =
crdtp::json::ConvertCBORToJSON(crdtp::SpanFrom(cbor), &json);
LOG_IF(ERROR, !status.ok()) << status.ToASCIIString();
Expand All @@ -107,5 +106,5 @@ void ChromeDevToolsSession::fallThrough(int call_id,
crdtp::span<uint8_t> message) {
auto callback = std::move(pending_commands_[call_id]);
pending_commands_.erase(call_id);
std::move(callback).Run(std::string(message.begin(), message.end()));
std::move(callback).Run(message);
}
2 changes: 1 addition & 1 deletion chrome/browser/devtools/chrome_devtools_session.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class ChromeDevToolsSession : public protocol::FrontendChannel {

void HandleCommand(
const std::string& method,
const std::string& message,
base::span<const uint8_t> message,
content::DevToolsManagerDelegate::NotHandledCallback callback);

TargetHandler* target_handler() { return target_handler_.get(); }
Expand Down
17 changes: 9 additions & 8 deletions chrome/browser/devtools/device/devtools_device_discovery.cc
Original file line number Diff line number Diff line change
Expand Up @@ -121,14 +121,15 @@ class WebSocketProxy : public AndroidDeviceManager::AndroidWebSocket::Delegate {
}

void OnFrameRead(const std::string& message) override {
proxy_->DispatchOnClientHost(message);
proxy_->DispatchOnClientHost(base::as_bytes(base::make_span(message)));
}

void OnSocketClosed() override {
std::string message =
"{ \"method\": \"Inspector.detached\", "
"\"params\": { \"reason\": \"Connection lost.\"} }";
proxy_->DispatchOnClientHost(message);
constexpr char kMsg[] =
"{\"method\":\"Inspector.detached\",\"params\":"
"{\"reason\":\"Connection lost.\"}}";
proxy_->DispatchOnClientHost(
base::as_bytes(base::make_span(kMsg, strlen(kMsg))));
web_socket_.reset();
socket_opened_ = false;
proxy_->ConnectionClosed(); // Deletes |this|.
Expand Down Expand Up @@ -176,7 +177,7 @@ class AgentHostDelegate : public content::DevToolsExternalAgentProxyDelegate {
bool Close() override;
base::TimeTicks GetLastActivityTime() override;
void SendMessageToBackend(content::DevToolsExternalAgentProxy* proxy,
const std::string& message) override;
base::span<const uint8_t> message) override;

scoped_refptr<AndroidDeviceManager::Device> device_;
std::string browser_id_;
Expand Down Expand Up @@ -354,12 +355,12 @@ base::TimeTicks AgentHostDelegate::GetLastActivityTime() {

void AgentHostDelegate::SendMessageToBackend(
content::DevToolsExternalAgentProxy* proxy,
const std::string& message) {
base::span<const uint8_t> message) {
auto it = proxies_.find(proxy);
// We could have detached due to physical connection being closed.
if (it == proxies_.end())
return;
it->second->SendMessageToBackend(message);
it->second->SendMessageToBackend(std::string(message.begin(), message.end()));
}

} // namespace
Expand Down
21 changes: 13 additions & 8 deletions chrome/browser/devtools/devtools_ui_bindings.cc
Original file line number Diff line number Diff line change
Expand Up @@ -708,24 +708,27 @@ void DevToolsUIBindings::HandleMessageFromDevToolsFrontend(

// content::DevToolsAgentHostClient implementation --------------------------
void DevToolsUIBindings::DispatchProtocolMessage(
content::DevToolsAgentHost* agent_host, const std::string& message) {
content::DevToolsAgentHost* agent_host,
base::span<const uint8_t> message) {
DCHECK(agent_host == agent_host_.get());
if (!frontend_host_)
return;

if (message.length() < kMaxMessageChunkSize) {
base::StringPiece message_sp(reinterpret_cast<const char*>(message.data()),
message.size());
if (message_sp.length() < kMaxMessageChunkSize) {
std::string param;
base::EscapeJSONString(message, true, &param);
base::EscapeJSONString(message_sp, true, &param);
base::string16 javascript =
base::UTF8ToUTF16("DevToolsAPI.dispatchMessage(" + param + ");");
web_contents_->GetMainFrame()->ExecuteJavaScript(javascript,
base::NullCallback());
return;
}

base::Value total_size(static_cast<int>(message.length()));
for (size_t pos = 0; pos < message.length(); pos += kMaxMessageChunkSize) {
base::Value message_value(message.substr(pos, kMaxMessageChunkSize));
base::Value total_size(static_cast<int>(message_sp.length()));
for (size_t pos = 0; pos < message_sp.length(); pos += kMaxMessageChunkSize) {
base::Value message_value(message_sp.substr(pos, kMaxMessageChunkSize));
CallClientFunction("DevToolsAPI.dispatchMessageChunk",
&message_value, pos ? NULL : &total_size, NULL);
}
Expand Down Expand Up @@ -1206,8 +1209,10 @@ void DevToolsUIBindings::SetOpenNewWindowForPopups(bool value) {

void DevToolsUIBindings::DispatchProtocolMessageFromDevToolsFrontend(
const std::string& message) {
if (agent_host_.get())
agent_host_->DispatchProtocolMessage(this, message);
if (!agent_host_)
return;
agent_host_->DispatchProtocolMessage(
this, base::as_bytes(base::make_span(message)));
}

void DevToolsUIBindings::RecordEnumeratedHistogram(const std::string& name,
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/devtools/devtools_ui_bindings.h
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ class DevToolsUIBindings : public DevToolsEmbedderMessageDispatcher::Delegate,

// content::DevToolsAgentHostClient implementation.
void DispatchProtocolMessage(content::DevToolsAgentHost* agent_host,
const std::string& message) override;
base::span<const uint8_t> message) override;
void AgentHostClosed(content::DevToolsAgentHost* agent_host) override;

// DevToolsEmbedderMessageDispatcher::Delegate implementation.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include "base/base64.h"
#include "base/callback.h"
#include "base/containers/span.h"
#include "base/json/json_reader.h"
#include "base/json/json_writer.h"
#include "base/values.h"
Expand Down Expand Up @@ -43,8 +44,10 @@ class DevToolsProtocolTest : public InProcessBrowserTest,

// DevToolsAgentHostClient interface
void DispatchProtocolMessage(content::DevToolsAgentHost* agent_host,
const std::string& message) override {
auto parsed_message = base::JSONReader::Read(message);
base::span<const uint8_t> message) override {
base::StringPiece message_str(reinterpret_cast<const char*>(message.data()),
message.size());
auto parsed_message = base::JSONReader::Read(message_str);
auto id = parsed_message->FindIntPath("id");
if (id) {
// TODO: implement handling of results from method calls (when needed).
Expand Down Expand Up @@ -72,7 +75,8 @@ class DevToolsProtocolTest : public InProcessBrowserTest,
command.SetKey(kMethodParam, base::Value(method));
std::string json_command;
base::JSONWriter::Write(command, &json_command);
agent_host_->DispatchProtocolMessage(this, json_command);
agent_host_->DispatchProtocolMessage(
this, base::as_bytes(base::make_span(json_command)));
}

void RunLoopUpdatingQuitClosure() {
Expand Down
19 changes: 12 additions & 7 deletions chrome/browser/extensions/api/debugger/debugger_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ class ExtensionDevToolsClientHost : public content::DevToolsAgentHostClient,
// DevToolsAgentHostClient interface.
void AgentHostClosed(DevToolsAgentHost* agent_host) override;
void DispatchProtocolMessage(DevToolsAgentHost* agent_host,
const std::string& message) override;
base::span<const uint8_t> message) override;
bool MayAttachToURL(const GURL& url, bool is_webui) override;
bool MayAttachToBrowser() override;
bool MayReadLocalFiles() override;
Expand Down Expand Up @@ -266,9 +266,11 @@ void ExtensionDevToolsClientHost::SendMessageToBackend(
"params", command_params->additional_properties.CreateDeepCopy());
}

std::string json_args;
base::JSONWriter::Write(protocol_request, &json_args);
agent_host_->DispatchProtocolMessage(this, json_args);
std::string json;
base::JSONWriter::Write(protocol_request, &json);

agent_host_->DispatchProtocolMessage(this,
base::as_bytes(base::make_span(json)));
}

void ExtensionDevToolsClientHost::InfoBarDismissed() {
Expand Down Expand Up @@ -314,15 +316,18 @@ void ExtensionDevToolsClientHost::Observe(
}

void ExtensionDevToolsClientHost::DispatchProtocolMessage(
DevToolsAgentHost* agent_host, const std::string& message) {
DevToolsAgentHost* agent_host,
base::span<const uint8_t> message) {
DCHECK(agent_host == agent_host_.get());
if (!EventRouter::Get(profile_))
return;

base::StringPiece message_str(reinterpret_cast<const char*>(message.data()),
message.size());
std::unique_ptr<base::Value> result = base::JSONReader::ReadDeprecated(
message, base::JSON_REPLACE_INVALID_CHARACTERS);
message_str, base::JSON_REPLACE_INVALID_CHARACTERS);
if (!result || !result->is_dict()) {
LOG(ERROR) << "Tried to send invalid message to extension: " << message;
LOG(ERROR) << "Tried to send invalid message to extension: " << message_str;
return;
}
base::DictionaryValue* dictionary =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ class StubDevToolsAgentHostClient : public content::DevToolsAgentHostClient {
~StubDevToolsAgentHostClient() override {}
void AgentHostClosed(content::DevToolsAgentHost* agent_host) override {}
void DispatchProtocolMessage(content::DevToolsAgentHost* agent_host,
const std::string& message) override {}
base::span<const uint8_t> message) override {}
};

} // namespace
Expand Down Expand Up @@ -297,8 +297,10 @@ IN_PROC_BROWSER_TEST_F(ChromeMimeHandlerViewTest,

// Reload via guest's DevTools, embedder should reload.
content::TestNavigationObserver reload_observer(embedder_web_contents);
constexpr char kMsg[] = R"({"id":1,"method":"Page.reload"})";
devtools_agent_host->DispatchProtocolMessage(
&devtools_agent_host_client, R"({"id":1,"method": "Page.reload"})");
&devtools_agent_host_client,
base::as_bytes(base::make_span(kMsg, strlen(kMsg))));
reload_observer.Wait();
devtools_agent_host->DetachClient(&devtools_agent_host_client);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class TestClient : public content::DevToolsAgentHostClient {
TestClient() {}
~TestClient() override {}
void DispatchProtocolMessage(content::DevToolsAgentHost* agent_host,
const std::string& message) override {}
base::span<const uint8_t> message) override {}
void AgentHostClosed(content::DevToolsAgentHost* agent_host) override {}
};

Expand All @@ -39,8 +39,9 @@ class ScopedDevtoolsOpener {
EXPECT_TRUE(agent_host_);
agent_host_->AttachClient(&test_client_);
// Send Page.enable, which is required before any Page methods.
constexpr char kMsg[] = R"({"id": 0, "method": "Page.enable"})";
agent_host_->DispatchProtocolMessage(
&test_client_, "{\"id\": 0, \"method\": \"Page.enable\"}");
&test_client_, base::as_bytes(base::make_span(kMsg, strlen(kMsg))));
}

explicit ScopedDevtoolsOpener(content::WebContents* web_contents)
Expand All @@ -60,7 +61,8 @@ class ScopedDevtoolsOpener {
std::string json_string;
JSONStringValueSerializer serializer(&json_string);
ASSERT_TRUE(serializer.Serialize(ad_blocking_command));
agent_host_->DispatchProtocolMessage(&test_client_, json_string);
agent_host_->DispatchProtocolMessage(
&test_client_, base::as_bytes(base::make_span(json_string)));
}

private:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,8 @@ void DevtoolsClient::SendMessageWithParams(
std::string json_message;
base::JSONWriter::Write(message, &json_message);

bool success = agent_host_->DispatchProtocolMessage(this, json_message);
bool success = agent_host_->DispatchProtocolMessage(
this, base::as_bytes(base::make_span(json_message)));
DCHECK(success);
}

Expand All @@ -122,11 +123,13 @@ void DevtoolsClient::UnregisterEventHandler(const char* method) {

void DevtoolsClient::DispatchProtocolMessage(
content::DevToolsAgentHost* agent_host,
const std::string& json_message) {
base::span<const uint8_t> json_message) {
DCHECK_EQ(agent_host, agent_host_.get());

base::StringPiece str_message(
reinterpret_cast<const char*>(json_message.data()), json_message.size());
std::unique_ptr<base::Value> message =
base::JSONReader::ReadDeprecated(json_message, base::JSON_PARSE_RFC);
base::JSONReader::ReadDeprecated(str_message, base::JSON_PARSE_RFC);
DCHECK(message && message->is_dict());

const base::DictionaryValue* message_dict;
Expand All @@ -137,7 +140,7 @@ void DevtoolsClient::DispatchProtocolMessage(
? DispatchMessageReply(std::move(message), *message_dict)
: DispatchEvent(std::move(message), *message_dict);
if (!success)
DVLOG(2) << "Unhandled protocol message: " << json_message;
DVLOG(2) << "Unhandled protocol message: " << str_message;
}

bool DevtoolsClient::DispatchMessageReply(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class DevtoolsClient : public MessageDispatcher,

// content::DevToolsAgentHostClient overrides:
void DispatchProtocolMessage(content::DevToolsAgentHost* agent_host,
const std::string& message) override;
base::span<const uint8_t> message) override;
void AgentHostClosed(content::DevToolsAgentHost* agent_host) override;

private:
Expand Down
2 changes: 1 addition & 1 deletion content/browser/devtools/devtools_agent_host_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ bool DevToolsAgentHostImpl::DetachClient(DevToolsAgentHostClient* client) {

bool DevToolsAgentHostImpl::DispatchProtocolMessage(
DevToolsAgentHostClient* client,
const std::string& message) {
base::span<const uint8_t> message) {
DevToolsSession* session = SessionByClient(client);
if (!session)
return false;
Expand Down
2 changes: 1 addition & 1 deletion content/browser/devtools/devtools_agent_host_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class CONTENT_EXPORT DevToolsAgentHostImpl : public DevToolsAgentHost {
bool AttachClient(DevToolsAgentHostClient* client) override;
bool DetachClient(DevToolsAgentHostClient* client) override;
bool DispatchProtocolMessage(DevToolsAgentHostClient* client,
const std::string& message) override;
base::span<const uint8_t> message) override;
bool IsAttached() override;
void InspectElement(RenderFrameHost* frame_host, int x, int y) override;
std::string GetId() override;
Expand Down
Loading

0 comments on commit 21e1940

Please sign in to comment.