DevTools: brush up remote debugging browser target: use callbacks, make handlers implementation a m…

DevTools: brush up remote debuggign browser target: use callbacks, make handlers implementation a more pleasant task.
- Migrates command handlers to the callbacks
- Introduces constants for tracing handler
- Removes notifier and replaces it with notifier callback

Test: ./tools/telemetry/run_tests --browser=release testGotTrace  -vvv

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

[pfeldman, 2013-01-16 14:24:55]

Hey. I make handlers more declarative, fixed some namings and make responses
have dictionary results (as in the rest of the remote debugging protocol). Tests
fail though :)

[bulach, 2013-01-16 14:56:21]

lgtm, this is neat, thanks pavel!

some suggestions below, I guess the new callbacks for start / end need a return
value and the tests should be happy! :)

https://codereview.chromium.org/11958010/diff/1/content/browser/devtools/devt...
File content/browser/devtools/devtools_browser_target.cc (right):

https://codereview.chromium.org/11958010/diff/1/content/browser/devtools/devt...
content/browser/devtools/devtools_browser_target.cc:137: if (error)
ultra nit: seems that if (params) .. else if (error) would be neater..

https://codereview.chromium.org/11958010/diff/1/content/browser/devtools/devt...
File content/browser/devtools/devtools_browser_target.h (right):

https://codereview.chromium.org/11958010/diff/1/content/browser/devtools/devt...
content/browser/devtools/devtools_browser_target.h:62: // |error|.
maybe it's simpler to just expose a getter to Notifier, and also add this
comment to the callback definition above?
this extra indirection doesn't seem to add much, wdyt?

https://codereview.chromium.org/11958010/diff/1/content/browser/devtools/devt...
File content/browser/devtools/devtools_tracing_handler.cc (right):

https://codereview.chromium.org/11958010/diff/1/content/browser/devtools/devt...
content/browser/devtools/devtools_tracing_handler.cc:20: static const char*
kTracingDomain = "Tracing";
"const char kFoo[]" rather than "static const char*"

https://codereview.chromium.org/11958010/diff/1/content/browser/devtools/devt...
content/browser/devtools/devtools_tracing_handler.cc:68: return NULL;
here and 75, needs to return base::Value::CreateBooleanValue(true);

[bulach, 2013-01-16 14:56:58]

lgtm,

btw, you may want to flesh out the commit message a bit, and add the test line
(small typo for "debugging" there too)

[pfeldman, 2013-01-16 15:33:38]

Thanks for the review! Found the problem with tests, it is now fixed!

https://codereview.chromium.org/11958010/diff/1/content/browser/devtools/devt...
File content/browser/devtools/devtools_browser_target.cc (right):

https://codereview.chromium.org/11958010/diff/1/content/browser/devtools/devt...
content/browser/devtools/devtools_browser_target.cc:137: if (error)
Actually did it on purpose, so that error had precedence over semi-populated
parameters.

https://codereview.chromium.org/11958010/diff/1/content/browser/devtools/devt...
File content/browser/devtools/devtools_browser_target.h (right):

https://codereview.chromium.org/11958010/diff/1/content/browser/devtools/devt...
content/browser/devtools/devtools_browser_target.h:62: // |error|.
This is a pure convenience method that custom handlers would use. Exposing
notifier() would work, but every handler would need to reach out for it for
every notification. I.e. more glue code...

https://codereview.chromium.org/11958010/diff/1/content/browser/devtools/devt...
File content/browser/devtools/devtools_tracing_handler.cc (right):

https://codereview.chromium.org/11958010/diff/1/content/browser/devtools/devt...
content/browser/devtools/devtools_tracing_handler.cc:20: static const char*
kTracingDomain = "Tracing";
On 2013/01/16 14:56:21, bulach wrote:
> "const char kFoo[]" rather than "static const char*"

Done.

https://codereview.chromium.org/11958010/diff/1/content/browser/devtools/devt...
content/browser/devtools/devtools_tracing_handler.cc:68: return NULL;
I'll try to make harness happy with existing message format (i.e. when ack looks
like {id:42}.

[commit-bot: I haz the power, 2013-01-16 16:37:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pfeldman@chromium.org/11958010/5001

[commit-bot: I haz the power, 2013-01-16 19:00:22]

Change committed as 177188