Detect debugging target crash.

Detect debugging target crash.


BUG=None
TBR=avi

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190374

[Vladislav Kaznacheev, 2013-03-19 08:23:09]

This relies on Inspector.targetCrashed introduced in
https://bugs.webkit.org/show_bug.cgi?id=112669.

[Vladislav Kaznacheev, 2013-03-19 08:23:48]

Please review. This requires https://bugs.webkit.org/show_bug.cgi?id=112669
committed.

[yurys, 2013-03-19 08:53:31]

lgtm

https://codereview.chromium.org/12906011/diff/1/content/browser/devtools/rend...
File content/browser/devtools/render_view_devtools_agent_host.h (right):

https://codereview.chromium.org/12906011/diff/1/content/browser/devtools/rend...
content/browser/devtools/render_view_devtools_agent_host.h:39: friend class
DevToolsAgentHostWebContentsObserver;
I'd rather made DevToolsAgentHostWebContentsObserver and
DevToolsAgentHostRvhObserver nested classes. You would need just forward
declaration here and wouldn't pollute content namespace with implementation
details.

[yurys, 2013-03-19 09:01:31]

We need to cover http handler with tests, please re-enabled existing ones and
start adding new for the new features.

[Vladislav Kaznacheev, 2013-03-19 13:10:06]

PTAL

[pfeldman, 2013-03-21 06:30:08]

https://codereview.chromium.org/12906011/diff/19002/content/browser/devtools/...
File content/browser/devtools/devtools_protocol.cc (right):

https://codereview.chromium.org/12906011/diff/19002/content/browser/devtools/...
content/browser/devtools/devtools_protocol.cc:221: DevToolsProtocol::Event*
DevToolsProtocol::CreateEvent(
Make sure order of implementation matches order of declaration.

https://codereview.chromium.org/12906011/diff/19002/content/browser/devtools/...
File content/browser/devtools/devtools_protocol.h (right):

https://codereview.chromium.org/12906011/diff/19002/content/browser/devtools/...
content/browser/devtools/devtools_protocol.h:115: Event(const std::string&
method, base::DictionaryValue* params);
List constructor first. DISALLOW_COPY_AND_ASSIGN missing. Document the fact that
it takes ownership over params.

https://codereview.chromium.org/12906011/diff/19002/content/browser/devtools/...
File content/browser/devtools/render_view_devtools_agent_host.cc (right):

https://codereview.chromium.org/12906011/diff/19002/content/browser/devtools/...
content/browser/devtools/render_view_devtools_agent_host.cc:52: class
RenderViewDevToolsAgentHost::DevToolsAgentHostRvhObserver
Why did this change?

https://codereview.chromium.org/12906011/diff/19002/content/browser/devtools/...
File content/browser/devtools/renderer_overrides_handler.cc (right):

https://codereview.chromium.org/12906011/diff/19002/content/browser/devtools/...
content/browser/devtools/renderer_overrides_handler.cc:31: const char
kPageHandleDialogPromptTextParam[] = "promptText";
It should be there now.

[Vladislav Kaznacheev, 2013-03-21 07:24:02]

PTAL

https://codereview.chromium.org/12906011/diff/19002/content/browser/devtools/...
File content/browser/devtools/devtools_protocol.cc (right):

https://codereview.chromium.org/12906011/diff/19002/content/browser/devtools/...
content/browser/devtools/devtools_protocol.cc:221: DevToolsProtocol::Event*
DevToolsProtocol::CreateEvent(
I believe it does.
On 2013/03/21 06:30:08, pfeldman wrote:
> Make sure order of implementation matches order of declaration.

https://codereview.chromium.org/12906011/diff/19002/content/browser/devtools/...
File content/browser/devtools/devtools_protocol.h (right):

https://codereview.chromium.org/12906011/diff/19002/content/browser/devtools/...
content/browser/devtools/devtools_protocol.h:115: Event(const std::string&
method, base::DictionaryValue* params);
On 2013/03/21 06:30:08, pfeldman wrote:
> List constructor first. DISALLOW_COPY_AND_ASSIGN missing. Document the fact
that
> it takes ownership over params.

Done.

https://codereview.chromium.org/12906011/diff/19002/content/browser/devtools/...
File content/browser/devtools/render_view_devtools_agent_host.cc (right):

https://codereview.chromium.org/12906011/diff/19002/content/browser/devtools/...
content/browser/devtools/render_view_devtools_agent_host.cc:52: class
RenderViewDevToolsAgentHost::DevToolsAgentHostRvhObserver
yurys asked me to do that earlier in this review (see Patch Set 2). Should I
revert this?

On 2013/03/21 06:30:08, pfeldman wrote:
> Why did this change?

https://codereview.chromium.org/12906011/diff/19002/content/browser/devtools/...
File content/browser/devtools/renderer_overrides_handler.cc (right):

https://codereview.chromium.org/12906011/diff/19002/content/browser/devtools/...
content/browser/devtools/renderer_overrides_handler.cc:31: const char
kPageHandleDialogPromptTextParam[] = "promptText";
On 2013/03/21 06:30:08, pfeldman wrote:
> It should be there now.

Done.

[pfeldman, 2013-03-21 15:26:16]

lgtm

[commit-bot: I haz the power, 2013-03-21 15:26:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaznacheev@chromium.org/12906011/32001

[commit-bot: I haz the power, 2013-03-21 15:26:52]

Presubmit check for 12906011-32001 failed and returned exit status 1.

INFO:root:Found 10 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    content/content_browser.gypi

Presubmit checks took 1.2s to calculate.

[commit-bot: I haz the power, 2013-03-21 15:29:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaznacheev@chromium.org/12906011/32001

[commit-bot: I haz the power, 2013-03-22 19:04:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaznacheev@chromium.org/12906011/32001

[commit-bot: I haz the power, 2013-03-22 19:04:34]

Failed to apply patch for
content/browser/devtools/renderer_overrides_handler.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/devtools/renderer_overrides_handler.cc
  Hunk #3 FAILED at 54.
  1 out of 6 hunks FAILED -- saving rejects to file
content/browser/devtools/renderer_overrides_handler.cc.rej

Patch:       content/browser/devtools/renderer_overrides_handler.cc
Index: content/browser/devtools/renderer_overrides_handler.cc
diff --git a/content/browser/devtools/renderer_overrides_handler.cc
b/content/browser/devtools/renderer_overrides_handler.cc
index
bf52e076e94109c57e825ea06240720578a562f3..ede0d713601853875539ea4a1e5e5069465e40f9
100644
--- a/content/browser/devtools/renderer_overrides_handler.cc
+++ b/content/browser/devtools/renderer_overrides_handler.cc
@@ -13,6 +13,7 @@
 #include "base/stringprintf.h"
 #include "base/values.h"
 #include "content/browser/child_process_security_policy_impl.h"
+#include "content/browser/devtools/devtools_protocol_constants.h"
 #include "content/browser/renderer_host/render_view_host_delegate.h"
 #include "content/public/browser/devtools_agent_host.h"
 #include "content/public/browser/javascript_dialog_manager.h"
@@ -27,32 +28,20 @@
 
 namespace content {
 
-namespace {
-
-const char kDOMFileInputCommand[] = "DOM.setFileInputFiles";
-const char kDOMFileInputFilesParam[] = "files";
-const char kPageHandleDialogCommand[] = "Page.handleJavaScriptDialog";
-const char kPageHandleDialogAcceptParam[] = "accept";
-const char kPageHandleDialogPromptTextParam[] = "promptText";
-const char kPageNavigateCommand[] = "Page.navigate";
-const char kPageNavigateUrlParam[] = "url";
-
-}  // namespace
-
 RendererOverridesHandler::RendererOverridesHandler(DevToolsAgentHost* agent)
     : agent_(agent) {
   RegisterCommandHandler(
-      kDOMFileInputCommand,
+      devtools::DOM::setFileInputFiles::kName,
       base::Bind(
           &RendererOverridesHandler::GrantPermissionsForSetFileInputFiles,
           base::Unretained(this)));
   RegisterCommandHandler(
-      kPageHandleDialogCommand,
+      devtools::Page::handleJavaScriptDialog::kName,
       base::Bind(
           &RendererOverridesHandler::PageHandleJavaScriptDialog,
           base::Unretained(this)));
   RegisterCommandHandler(
-      kPageNavigateCommand,
+      devtools::Page::navigate::kName,
       base::Bind(
           &RendererOverridesHandler::PageNavigate,
           base::Unretained(this)));
@@ -65,11 +54,12 @@
RendererOverridesHandler::GrantPermissionsForSetFileInputFiles(
     DevToolsProtocol::Command* command) {
   base::DictionaryValue* params = command->params();
   base::ListValue* file_list;
-  if (!params || !params->GetList(kDOMFileInputFilesParam, &file_list)) {
+  const char* param =
+      devtools::DOM::setFileInputFiles::kParamFiles;
+  if (!params || !params->GetList(param, &file_list)) {
     return command->ErrorResponse(
         DevToolsProtocol::kErrorInvalidParams,
-        base::StringPrintf("Missing or invalid '%s' parameter",
-                           kDOMFileInputFilesParam));
+        base::StringPrintf("Missing or invalid '%s' parameter", param));
   }
   RenderViewHost* host = agent_->GetRenderViewHost();
   if (!host)
@@ -80,8 +70,7 @@
RendererOverridesHandler::GrantPermissionsForSetFileInputFiles(
     if (!file_list->GetString(i, &file)) {
       return command->ErrorResponse(
           DevToolsProtocol::kErrorInvalidParams,
-          base::StringPrintf("'%s' must be a list of strings",
-                             kDOMFileInputFilesParam));
+          base::StringPrintf("'%s' must be a list of strings", param));
     }
     ChildProcessSecurityPolicyImpl::GetInstance()->GrantReadFile(
         host->GetProcess()->GetID(), base::FilePath(file));
@@ -93,17 +82,19 @@ scoped_ptr<DevToolsProtocol::Response>
 RendererOverridesHandler::PageHandleJavaScriptDialog(
     DevToolsProtocol::Command* command) {
   base::DictionaryValue* params = command->params();
+  const char* paramAccept =
+      devtools::Page::handleJavaScriptDialog::kParamAccept;
   bool accept;
-  if (!params || !params->GetBoolean(kPageHandleDialogAcceptParam, &accept)) {
+  if (!params || !params->GetBoolean(paramAccept, &accept)) {
     return command->ErrorResponse(
         DevToolsProtocol::kErrorInvalidParams,
-        base::StringPrintf("Missing or invalid '%s' parameter",
-                           kPageHandleDialogAcceptParam));
+        base::StringPrintf("Missing or invalid '%s' parameter", paramAccept));
   }
   string16 prompt_override;
   string16* prompt_override_ptr = &prompt_override;
-  if (!params || !params->GetString(kPageHandleDialogPromptTextParam,
-                                    prompt_override_ptr)) {
+  if (!params || !params->GetString(
+      devtools::Page::handleJavaScriptDialog::kParamPromptText,
+      prompt_override_ptr)) {
     prompt_override_ptr = NULL;
   }
 
@@ -129,11 +120,12 @@ RendererOverridesHandler::PageNavigate(
     DevToolsProtocol::Command* command) {
   base::DictionaryValue* params = command->params();
   std::string url;
-  if (!params || !params->GetString(kPageNavigateUrlParam, &url)) {
+  const char* param = devtools::Page::navigate::kParamUrl;
+  if (!params || !params->GetString(param, &url)) {
     return command->ErrorResponse(
         DevToolsProtocol::kErrorInvalidParams,
         base::StringPrintf("Missing or invalid '%s' parameter",
-                           kPageNavigateUrlParam));
+                           param));
   }
   GURL gurl(url);
   if (!gurl.is_valid()) {

[commit-bot: I haz the power, 2013-03-25 10:15:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaznacheev@chromium.org/12906011/51001

[commit-bot: I haz the power, 2013-03-25 12:08:31]

Change committed as 190374