From d3a45227842c31243c1ebda079e91258a35b874d Mon Sep 17 00:00:00 2001 From: Sadrul Habib Chowdhury Date: Fri, 22 Sep 2017 04:01:55 +0000 Subject: [PATCH] devtools: Change how commands are handled by the delegate. The DevToolsManagerDelegate is now able to send the protocol response itself when it needs to (through the DevToolsAgentHost). So have the delegate do that, instead of sending the response through the DevToolsSession. This change makes it possible, in follow up CLs, to use UberDispatcher to handle the commands and send the response back using FrontendChannel. BUG=758061 TBR=eseckler@ for //headless, for trivial update to devtools API change. Change-Id: Ib0b521a24c4f4d26f8cd0fe3a6995141acd630ef Reviewed-on: https://chromium-review.googlesource.com/677954 Commit-Queue: Sadrul Chowdhury Reviewed-by: Dmitry Gozman Cr-Commit-Position: refs/heads/master@{#503654} --- .../chrome_devtools_manager_delegate.cc | 49 ++++++++++++++----- .../chrome_devtools_manager_delegate.h | 7 ++- content/browser/devtools/devtools_session.cc | 8 ++- .../browser/devtools_manager_delegate.cc | 9 ++-- .../browser/devtools_manager_delegate.h | 8 +-- .../headless_devtools_manager_delegate.cc | 23 ++++++--- .../headless_devtools_manager_delegate.h | 6 +-- 7 files changed, 71 insertions(+), 39 deletions(-) diff --git a/chrome/browser/devtools/chrome_devtools_manager_delegate.cc b/chrome/browser/devtools/chrome_devtools_manager_delegate.cc index 826a55cd8b47b2..fb9618a1b64602 100644 --- a/chrome/browser/devtools/chrome_devtools_manager_delegate.cc +++ b/chrome/browser/devtools/chrome_devtools_manager_delegate.cc @@ -6,6 +6,7 @@ #include +#include "base/json/json_writer.h" #include "base/memory/ptr_util.h" #include "base/strings/utf_string_conversions.h" #include "build/build_config.h" @@ -118,6 +119,12 @@ void ToggleAdBlocking(bool enabled, content::DevToolsAgentHost* agent_host) { } } +std::string ToString(std::unique_ptr value) { + std::string json; + base::JSONWriter::Write(*value, &json); + return json; +} + } // namespace // static @@ -347,7 +354,7 @@ void ChromeDevToolsManagerDelegate::Inspect( DevToolsWindow::OpenDevToolsWindow(agent_host, nullptr); } -base::DictionaryValue* ChromeDevToolsManagerDelegate::HandleCommand( +bool ChromeDevToolsManagerDelegate::HandleCommand( DevToolsAgentHost* agent_host, int session_id, base::DictionaryValue* command_dict) { @@ -355,26 +362,44 @@ base::DictionaryValue* ChromeDevToolsManagerDelegate::HandleCommand( std::string method; base::DictionaryValue* params = nullptr; if (!DevToolsProtocol::ParseCommand(command_dict, &id, &method, ¶ms)) - return nullptr; + return false; // Do not actually handle the enable/disable commands, just keep track of the // enable state. - if (method == chrome::devtools::Page::enable::kName) + if (method == chrome::devtools::Page::enable::kName) { TogglePageEnable(true /* enable */, agent_host); - if (method == chrome::devtools::Page::disable::kName) + return false; + } + + if (method == chrome::devtools::Page::disable::kName) { TogglePageEnable(false /* enable */, agent_host); + return false; + } - auto* result = HandleBrowserCommand(id, method, params).release(); - if (result) - return result; + auto result = HandleBrowserCommand(id, method, params); + if (result) { + agent_host->SendProtocolMessageToClient(session_id, + ToString(std::move(result))); + return true; + } - if (method == chrome::devtools::Page::setAdBlockingEnabled::kName) - return SetAdBlockingEnabled(agent_host, id, params).release(); + if (method == chrome::devtools::Page::setAdBlockingEnabled::kName) { + result = SetAdBlockingEnabled(agent_host, id, params); + DCHECK(result); + agent_host->SendProtocolMessageToClient(session_id, + ToString(std::move(result))); + return true; + } - if (method == chrome::devtools::Target::setRemoteLocations::kName) - return SetRemoteLocations(agent_host, id, params).release(); + if (method == chrome::devtools::Target::setRemoteLocations::kName) { + result = SetRemoteLocations(agent_host, id, params); + DCHECK(result); + agent_host->SendProtocolMessageToClient(session_id, + ToString(std::move(result))); + return true; + } - return nullptr; + return false; } std::string ChromeDevToolsManagerDelegate::GetTargetType( diff --git a/chrome/browser/devtools/chrome_devtools_manager_delegate.h b/chrome/browser/devtools/chrome_devtools_manager_delegate.h index 666dd6b53704d0..3c1b1d1fd988f7 100644 --- a/chrome/browser/devtools/chrome_devtools_manager_delegate.h +++ b/chrome/browser/devtools/chrome_devtools_manager_delegate.h @@ -34,10 +34,9 @@ class ChromeDevToolsManagerDelegate : // content::DevToolsManagerDelegate implementation. void Inspect(content::DevToolsAgentHost* agent_host) override; - base::DictionaryValue* HandleCommand( - content::DevToolsAgentHost* agent_host, - int session_id, - base::DictionaryValue* command_dict) override; + bool HandleCommand(content::DevToolsAgentHost* agent_host, + int session_id, + base::DictionaryValue* command_dict) override; std::string GetTargetType(content::WebContents* web_contents) override; std::string GetTargetTitle(content::WebContents* web_contents) override; scoped_refptr CreateNewTarget( diff --git a/content/browser/devtools/devtools_session.cc b/content/browser/devtools/devtools_session.cc index 19ff3cdaf1b1c3..023d48903c886d 100644 --- a/content/browser/devtools/devtools_session.cc +++ b/content/browser/devtools/devtools_session.cc @@ -80,12 +80,10 @@ protocol::Response::Status DevToolsSession::Dispatch( if (value && value->IsType(base::Value::Type::DICTIONARY) && delegate) { base::DictionaryValue* dict_value = static_cast(value.get()); - std::unique_ptr response( - delegate->HandleCommand(agent_host_, session_id_, dict_value)); - if (response) { - SendResponse(std::move(response)); + + if (delegate->HandleCommand(agent_host_, session_id_, dict_value)) return protocol::Response::kSuccess; - } + if (delegate->HandleAsyncCommand(agent_host_, session_id_, dict_value, base::Bind(&DevToolsSession::SendResponse, weak_factory_.GetWeakPtr()))) { diff --git a/content/public/browser/devtools_manager_delegate.cc b/content/public/browser/devtools_manager_delegate.cc index b572162e1e8f25..ebd1ad6fd3a0e6 100644 --- a/content/public/browser/devtools_manager_delegate.cc +++ b/content/public/browser/devtools_manager_delegate.cc @@ -39,11 +39,10 @@ void DevToolsManagerDelegate::SessionDestroyed( content::DevToolsAgentHost* agent_host, int session_id) {} -base::DictionaryValue* DevToolsManagerDelegate::HandleCommand( - DevToolsAgentHost* agent_host, - int session_id, - base::DictionaryValue* command) { - return nullptr; +bool DevToolsManagerDelegate::HandleCommand(DevToolsAgentHost* agent_host, + int session_id, + base::DictionaryValue* command) { + return false; } bool DevToolsManagerDelegate::HandleAsyncCommand( diff --git a/content/public/browser/devtools_manager_delegate.h b/content/public/browser/devtools_manager_delegate.h index 92498ecdabd2ba..1136340bc13245 100644 --- a/content/public/browser/devtools_manager_delegate.h +++ b/content/public/browser/devtools_manager_delegate.h @@ -49,10 +49,10 @@ class CONTENT_EXPORT DevToolsManagerDelegate { virtual void SessionDestroyed(content::DevToolsAgentHost* agent_host, int session_id); - // Result ownership is passed to the caller. - virtual base::DictionaryValue* HandleCommand(DevToolsAgentHost* agent_host, - int session_id, - base::DictionaryValue* command); + // Returns true if the command has been handled, false otherwise. + virtual bool HandleCommand(DevToolsAgentHost* agent_host, + int session_id, + base::DictionaryValue* command); using CommandCallback = base::Callback response)>; diff --git a/headless/lib/browser/headless_devtools_manager_delegate.cc b/headless/lib/browser/headless_devtools_manager_delegate.cc index 74e0f817e4b4a6..3e39b54c674238 100644 --- a/headless/lib/browser/headless_devtools_manager_delegate.cc +++ b/headless/lib/browser/headless_devtools_manager_delegate.cc @@ -7,6 +7,7 @@ #include #include +#include "base/json/json_writer.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/devtools_agent_host.h" #include "content/public/browser/devtools_frontend_host.h" @@ -100,6 +101,12 @@ void PDFCreated( } #endif +std::string ToString(std::unique_ptr value) { + std::string json; + base::JSONWriter::Write(*value, &json); + return json; +} + } // namespace #if BUILDFLAG(ENABLE_BASIC_PRINTING) @@ -199,33 +206,37 @@ HeadlessDevToolsManagerDelegate::HeadlessDevToolsManagerDelegate( HeadlessDevToolsManagerDelegate::~HeadlessDevToolsManagerDelegate() {} -base::DictionaryValue* HeadlessDevToolsManagerDelegate::HandleCommand( +bool HeadlessDevToolsManagerDelegate::HandleCommand( content::DevToolsAgentHost* agent_host, int session_id, base::DictionaryValue* command) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); if (!browser_) - return nullptr; + return false; int id; std::string method; if (!command->GetInteger("id", &id) || !command->GetString("method", &method)) - return nullptr; + return false; auto find_it = command_map_.find(method); if (find_it == command_map_.end()) - return nullptr; + return false; // Handle Browser domain commands only from Browser DevToolsAgentHost. if (method.find("Browser.") == 0 && agent_host->GetType() != content::DevToolsAgentHost::kTypeBrowser) - return nullptr; + return false; const base::DictionaryValue* params = nullptr; command->GetDictionary("params", ¶ms); auto cmd_result = find_it->second.Run(id, params); - return cmd_result.release(); + if (!cmd_result) + return false; + agent_host->SendProtocolMessageToClient(session_id, + ToString(std::move(cmd_result))); + return true; } bool HeadlessDevToolsManagerDelegate::HandleAsyncCommand( diff --git a/headless/lib/browser/headless_devtools_manager_delegate.h b/headless/lib/browser/headless_devtools_manager_delegate.h index 72177e2b795810..43de7da5b0f605 100644 --- a/headless/lib/browser/headless_devtools_manager_delegate.h +++ b/headless/lib/browser/headless_devtools_manager_delegate.h @@ -39,9 +39,9 @@ class HeadlessDevToolsManagerDelegate ~HeadlessDevToolsManagerDelegate() override; // DevToolsManagerDelegate implementation: - base::DictionaryValue* HandleCommand(content::DevToolsAgentHost* agent_host, - int session_id, - base::DictionaryValue* command) override; + bool HandleCommand(content::DevToolsAgentHost* agent_host, + int session_id, + base::DictionaryValue* command) override; bool HandleAsyncCommand(content::DevToolsAgentHost* agent_host, int session_id, base::DictionaryValue* command,