DevTools: Protocol handler generator for content

DevTools: Protocol handler generator for content

BUG=405566
Committed: https://crrev.com/c985d95d1be81585c2e3a085838ce459a5b3ce2f
Cr-Commit-Position: refs/heads/master@{#297628}

[vkuzkokov, 2014-09-03 15:24:06]

vkuzkokov@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgozman, 2014-09-09 14:30:59]

https://codereview.chromium.org/508973003/diff/220001/content/browser/devtool...
File content/browser/devtools/devtools_protocol_frontend.h (right):

https://codereview.chromium.org/508973003/diff/220001/content/browser/devtool...
content/browser/devtools/devtools_protocol_frontend.h:20:
RESPONSE_STATUS_SERVER_ERROR,
Add internal error as well.

https://codereview.chromium.org/508973003/diff/220001/content/browser/devtool...
content/browser/devtools/devtools_protocol_frontend.h:24:
Response(ResponseStatus status, const std::string& message);
Let's add

static Response FallThrough();
static Response OK();
static Response InvalidParams(const std::string& message);
static Response ServerError(const std::string& message);
static Response InternalError(const std::string& message);

https://codereview.chromium.org/508973003/diff/220001/content/browser/devtool...
File content/browser/devtools/input_domain_handler.cc (right):

https://codereview.chromium.org/508973003/diff/220001/content/browser/devtool...
content/browser/devtools/input_domain_handler.cc:67:
ResponseStatus::RESPONSE_STATUS_INVALID_PARAMS,
Can this be just RESPONSE_STATUS_XXXX?

https://codereview.chromium.org/508973003/diff/220001/content/browser/devtool...
File content/browser/devtools/input_domain_handler.h (right):

https://codereview.chromium.org/508973003/diff/220001/content/browser/devtool...
content/browser/devtools/input_domain_handler.h:23: Response
EmulateTouchFromMouseEvent(std::string in_type,
Remove "in_" prefix.

https://codereview.chromium.org/508973003/diff/220001/content/browser/devtool...
File content/browser/devtools/inspector_domain_handler.h (right):

https://codereview.chromium.org/508973003/diff/220001/content/browser/devtool...
content/browser/devtools/inspector_domain_handler.h:20: void
SetFrontend(scoped_ptr<InspectorDomainFrontend> frontend);
Let's add |frontend_| field and set it.

https://codereview.chromium.org/508973003/diff/220001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/508973003/diff/220001/content/content_browser...
content/content_browser.gypi:446:
'browser/devtools/devtools_protocol_frontend.cc',
Maybe it's time create content/browser/devtools/protocol directory?

[vkuzkokov, 2014-09-10 10:33:12]

https://codereview.chromium.org/508973003/diff/220001/content/browser/devtool...
File content/browser/devtools/devtools_protocol_frontend.h (right):

https://codereview.chromium.org/508973003/diff/220001/content/browser/devtool...
content/browser/devtools/devtools_protocol_frontend.h:20:
RESPONSE_STATUS_SERVER_ERROR,
On 2014/09/09 14:30:59, dgozman wrote:
> Add internal error as well.

Done.

https://codereview.chromium.org/508973003/diff/220001/content/browser/devtool...
content/browser/devtools/devtools_protocol_frontend.h:24:
Response(ResponseStatus status, const std::string& message);
On 2014/09/09 14:30:59, dgozman wrote:
> Let's add
> 
> static Response FallThrough();
> static Response OK();
> static Response InvalidParams(const std::string& message);
> static Response ServerError(const std::string& message);
> static Response InternalError(const std::string& message);

Done.

https://codereview.chromium.org/508973003/diff/220001/content/browser/devtool...
File content/browser/devtools/input_domain_handler.cc (right):

https://codereview.chromium.org/508973003/diff/220001/content/browser/devtool...
content/browser/devtools/input_domain_handler.cc:67:
ResponseStatus::RESPONSE_STATUS_INVALID_PARAMS,
On 2014/09/09 14:30:59, dgozman wrote:
> Can this be just RESPONSE_STATUS_XXXX?

Used Response::InvalidParams instead

https://codereview.chromium.org/508973003/diff/220001/content/browser/devtool...
File content/browser/devtools/input_domain_handler.h (right):

https://codereview.chromium.org/508973003/diff/220001/content/browser/devtool...
content/browser/devtools/input_domain_handler.h:23: Response
EmulateTouchFromMouseEvent(std::string in_type,
On 2014/09/09 14:30:59, dgozman wrote:
> Remove "in_" prefix.

Done.

https://codereview.chromium.org/508973003/diff/220001/content/browser/devtool...
File content/browser/devtools/inspector_domain_handler.h (right):

https://codereview.chromium.org/508973003/diff/220001/content/browser/devtool...
content/browser/devtools/inspector_domain_handler.h:20: void
SetFrontend(scoped_ptr<InspectorDomainFrontend> frontend);
On 2014/09/09 14:30:59, dgozman wrote:
> Let's add |frontend_| field and set it.

Done.

https://codereview.chromium.org/508973003/diff/220001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/508973003/diff/220001/content/content_browser...
content/content_browser.gypi:446:
'browser/devtools/devtools_protocol_frontend.cc',
On 2014/09/09 14:30:59, dgozman wrote:
> Maybe it's time create content/browser/devtools/protocol directory?

Done.

[dgozman, 2014-09-10 13:01:20]

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_frontend.h (right):

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_frontend.h:16: enum
ResponseStatus {
Looks like there is no need to expose this enum. Let's make |int status_|, and
define kResponseStatusXXX in .cc file.

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_frontend.h:31:
ResponseStatus status_;
private:

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_handler_generator.py
(right):

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:599: if
json_param.get("optional"):
I think we should have support for this before landing.

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:634: if
json_param.get("optional"):
ditto

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
File content/browser/devtools/protocol/dom_domain_handler.h (right):

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
content/browser/devtools/protocol/dom_domain_handler.h:14: class Handler {
Now looking at this, I don't like naming. Class name should be equal to file
name. Let's use DOMHandler?

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
File content/browser/devtools/render_view_devtools_agent_host.cc (right):

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
content/browser/devtools/render_view_devtools_agent_host.cc:330:
overrides_handler_->DidAttachInterstitialPage();
RenderViewDTAH should be able to call methods (like this one) on the domain
handlers, so we should expose them.

[vkuzkokov, 2014-09-11 12:53:03]

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_frontend.h (right):

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_frontend.h:16: enum
ResponseStatus {
On 2014/09/10 13:01:20, dgozman wrote:
> Looks like there is no need to expose this enum. Let's make |int status_|, and
> define kResponseStatusXXX in .cc file.

This enum is used in generated .cc

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_frontend.h:31:
ResponseStatus status_;
On 2014/09/10 13:01:20, dgozman wrote:
> private:

Done.

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_handler_generator.py
(right):

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:599: if
json_param.get("optional"):
On 2014/09/10 13:01:20, dgozman wrote:
> I think we should have support for this before landing.

They are not used in protocol. Even if I implement this there would be no way of
saying if it actually works. Added TODO instead.

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:634: if
json_param.get("optional"):
On 2014/09/10 13:01:20, dgozman wrote:
> ditto

ditto

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
File content/browser/devtools/protocol/dom_domain_handler.h (right):

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
content/browser/devtools/protocol/dom_domain_handler.h:14: class Handler {
On 2014/09/10 13:01:20, dgozman wrote:
> Now looking at this, I don't like naming. Class name should be equal to file
> name. Let's use DOMHandler?

Done.

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
File content/browser/devtools/render_view_devtools_agent_host.cc (right):

https://codereview.chromium.org/508973003/diff/240001/content/browser/devtool...
content/browser/devtools/render_view_devtools_agent_host.cc:330:
overrides_handler_->DidAttachInterstitialPage();
On 2014/09/10 13:01:20, dgozman wrote:
> RenderViewDTAH should be able to call methods (like this one) on the domain
> handlers, so we should expose them.

Done.

[dgozman, 2014-09-11 13:43:22]

Let's add async support and structs for events, and this is ready.

https://codereview.chromium.org/508973003/diff/260001/content/browser/devtool...
File content/browser/devtools/BUILD.gn (right):

https://codereview.chromium.org/508973003/diff/260001/content/browser/devtool...
content/browser/devtools/BUILD.gn:84: visibility = ":resources"
[ ":resources" ]

https://codereview.chromium.org/508973003/diff/260001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_handler_generator.py
(right):

https://codereview.chromium.org/508973003/diff/260001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:403: if
(i < len(s)-1) and s[i+1].islower() or (i > 0) and s[i-1].islower():
Does this work for "ContentOfHTMLFile"? We should convert acronyms to lower case
as well.

https://codereview.chromium.org/508973003/diff/260001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:429: if
"properties" in json:
How there may be no "properties" here?

https://codereview.chromium.org/508973003/diff/260001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:483:
elif "type" in json:
I'd split this function into ResolveRef, ResolveArray, ResolveObject and
ResolvePrimitive.

https://codereview.chromium.org/508973003/diff/260001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:521:
DeclareStruct(json_type, mapping)
This call looks strange: json_type is not a json, but a string.

https://codereview.chromium.org/508973003/diff/260001/content/browser/devtool...
File content/browser/devtools/protocol/input_handler.cc (right):

https://codereview.chromium.org/508973003/diff/260001/content/browser/devtool...
content/browser/devtools/protocol/input_handler.cc:45: if (type ==
Input::emulateTouchFromMouseEvent::Type::kEnumMousePressed) {
Why don't we generate enum for this?

[vkuzkokov, 2014-09-11 17:19:20]

https://codereview.chromium.org/508973003/diff/260001/content/browser/devtool...
File content/browser/devtools/BUILD.gn (right):

https://codereview.chromium.org/508973003/diff/260001/content/browser/devtool...
content/browser/devtools/BUILD.gn:84: visibility = ":resources"
On 2014/09/11 13:43:22, dgozman wrote:
> [ ":resources" ]

Done.

https://codereview.chromium.org/508973003/diff/260001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_handler_generator.py
(right):

https://codereview.chromium.org/508973003/diff/260001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:403: if
(i < len(s)-1) and s[i+1].islower() or (i > 0) and s[i-1].islower():
On 2014/09/11 13:43:22, dgozman wrote:
> Does this work for "ContentOfHTMLFile"? We should convert acronyms to lower
case
> as well.

_content_of_html_file. Fixed to remove underscore in the beginning.

https://codereview.chromium.org/508973003/diff/260001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:429: if
"properties" in json:
On 2014/09/11 13:43:22, dgozman wrote:
> How there may be no "properties" here?

API changed. No I pass json["properties"] for types.

https://codereview.chromium.org/508973003/diff/260001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:483:
elif "type" in json:
On 2014/09/11 13:43:22, dgozman wrote:
> I'd split this function into ResolveRef, ResolveArray, ResolveObject and
> ResolvePrimitive.

Done.

https://codereview.chromium.org/508973003/diff/260001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:521:
DeclareStruct(json_type, mapping)
On 2014/09/11 13:43:22, dgozman wrote:
> This call looks strange: json_type is not a json, but a string.

Fixed.

https://codereview.chromium.org/508973003/diff/260001/content/browser/devtool...
File content/browser/devtools/protocol/input_handler.cc (right):

https://codereview.chromium.org/508973003/diff/260001/content/browser/devtool...
content/browser/devtools/protocol/input_handler.cc:45: if (type ==
Input::emulateTouchFromMouseEvent::Type::kEnumMousePressed) {
On 2014/09/11 13:43:22, dgozman wrote:
> Why don't we generate enum for this?

Modifying usages of such enum would be tricky.

[dgozman, 2014-09-12 11:48:46]

Please mention that this patch also changes Input domain to use generated
handler.

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_frontend.h (right):

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_frontend.h:35:
ResponseStatus status_;
Let's add private constructor, so all clients are forced to use factory methods.

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_handler_generator.py
(right):

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:530:
json_type["___mark"] = True;
__mark -> __struct_declared

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:618: if
not ("wrap" in mapping):
I think these default values for "wrap", "append", "prep_req" and "pass_type"
should go to ResolvePrimitive.

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
File content/browser/devtools/protocol/input_handler.cc (right):

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
content/browser/devtools/protocol/input_handler.cc:40: using namespace
emulate_touch_from_mouse_event;
Let's not |using namespace|.

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
content/browser/devtools/protocol/input_handler.cc:90: // TODO(vkuzkokov): make
not optional
full stop please

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
content/browser/devtools/protocol/input_handler.cc:94: if (*button == "none") {
kButtonNone

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
File content/browser/devtools/protocol/input_handler.h (right):

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
content/browser/devtools/protocol/input_handler.h:27: double* deltaX,
delta_x

[vkuzkokov, 2014-09-12 14:22:52]

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_frontend.h (right):

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_frontend.h:35:
ResponseStatus status_;
On 2014/09/12 11:48:45, dgozman wrote:
> Let's add private constructor, so all clients are forced to use factory
methods.

Done.

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_handler_generator.py
(right):

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:530:
json_type["___mark"] = True;
On 2014/09/12 11:48:45, dgozman wrote:
> __mark -> __struct_declared

Done.

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:618: if
not ("wrap" in mapping):
On 2014/09/12 11:48:45, dgozman wrote:
> I think these default values for "wrap", "append", "prep_req" and "pass_type"
> should go to ResolvePrimitive.

Done.

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
File content/browser/devtools/protocol/input_handler.cc (right):

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
content/browser/devtools/protocol/input_handler.cc:40: using namespace
emulate_touch_from_mouse_event;
On 2014/09/12 11:48:45, dgozman wrote:
> Let's not |using namespace|.

Done.

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
content/browser/devtools/protocol/input_handler.cc:90: // TODO(vkuzkokov): make
not optional
On 2014/09/12 11:48:45, dgozman wrote:
> full stop please

removed TODO - done.

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
content/browser/devtools/protocol/input_handler.cc:94: if (*button == "none") {
On 2014/09/12 11:48:45, dgozman wrote:
> kButtonNone

Done.

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
File content/browser/devtools/protocol/input_handler.h (right):

https://codereview.chromium.org/508973003/diff/320001/content/browser/devtool...
content/browser/devtools/protocol/input_handler.h:27: double* deltaX,
On 2014/09/12 11:48:45, dgozman wrote:
> delta_x

Done.

[dgozman, 2014-09-14 14:25:47]

dgozman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[dgozman, 2014-09-14 14:25:49]

Adding Pavel, as this is getting close to handling all we need.

https://codereview.chromium.org/508973003/diff/340001/content/browser/devtool...
File content/browser/devtools/protocol/page_handler.h (right):

https://codereview.chromium.org/508973003/diff/340001/content/browser/devtool...
content/browser/devtools/protocol/page_handler.h:41:
scoped_refptr<DevToolsProtocol::Command> command);
As discussed, let's pass scoped_ptr<CaptureScreenshotResponse> here, which would
have set_xxxx and SendXXXResponse methods, and would store the command inside.
Method could return void then.

[dgozman, 2014-09-16 13:21:25]

https://codereview.chromium.org/508973003/diff/420001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_handler_generator.py
(right):

https://codereview.chromium.org/508973003/diff/420001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:145:
void ${Command}(const ${Command}Params& params);
Let's format this to fit 80:
void ${Command}(
    const ${Command}Params& params);

https://codereview.chromium.org/508973003/diff/420001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:149:
void Send${Command}Response(scoped_refptr<DevToolsProtocol::Command> command,
Let's format this to fit 80:
void Send${Command}Response(
    scoped_refptr<DevToolsProtocol::Command> command,
    const ${Command}Response& params);

https://codereview.chromium.org/508973003/diff/420001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:162:
scoped_refptr<DevToolsProtocol::Response> On${Domain}${Command}(
Let's format this to fit 80:
scoped_refptr<DevToolsProtocol::Response>
On${Domain}${Command}(
    scoped_refptr<DevToolsProtocol::Command> command);

https://codereview.chromium.org/508973003/diff/420001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:330:
bool ${param}_found = params->Get${Type}("${proto_param}", &in_${param});
Let's format to fit 80:
bool ${param}_found = params->Get${Type}(
    "${proto_param}", &in_${param});

https://codereview.chromium.org/508973003/diff/420001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:727:
args = ", ".join(args)))
Let's format to fit 80:
args = "\n      " + ",\n      ".join(args)

[vkuzkokov, 2014-09-16 14:59:38]

https://codereview.chromium.org/508973003/diff/420001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_handler_generator.py
(right):

https://codereview.chromium.org/508973003/diff/420001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:145:
void ${Command}(const ${Command}Params& params);
On 2014/09/16 13:21:23, dgozman wrote:
> Let's format this to fit 80:
> void ${Command}(
>     const ${Command}Params& params);

Done.

https://codereview.chromium.org/508973003/diff/420001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:149:
void Send${Command}Response(scoped_refptr<DevToolsProtocol::Command> command,
On 2014/09/16 13:21:24, dgozman wrote:
> Let's format this to fit 80:
> void Send${Command}Response(
>     scoped_refptr<DevToolsProtocol::Command> command,
>     const ${Command}Response& params);

Done.

https://codereview.chromium.org/508973003/diff/420001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:162:
scoped_refptr<DevToolsProtocol::Response> On${Domain}${Command}(
On 2014/09/16 13:21:24, dgozman wrote:
> Let's format this to fit 80:
> scoped_refptr<DevToolsProtocol::Response>
> On${Domain}${Command}(
>     scoped_refptr<DevToolsProtocol::Command> command);

Done.

https://codereview.chromium.org/508973003/diff/420001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:330:
bool ${param}_found = params->Get${Type}("${proto_param}", &in_${param});
On 2014/09/16 13:21:24, dgozman wrote:
> Let's format to fit 80:
> bool ${param}_found = params->Get${Type}(
>     "${proto_param}", &in_${param});

Done.

https://codereview.chromium.org/508973003/diff/420001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:727:
args = ", ".join(args)))
On 2014/09/16 13:21:24, dgozman wrote:
> Let's format to fit 80:
> args = "\n      " + ",\n      ".join(args)

Done.

[dgozman, 2014-09-16 15:38:03]

dgozman@chromium.org changed reviewers:
+ vsevik@chromium.org, yurys@chromium.org

[dgozman, 2014-09-16 15:38:05]

+vsevik@, yurys@

lgtm

https://codereview.chromium.org/508973003/diff/440001/content/browser/devtool...
File content/browser/devtools/BUILD.gn (right):

https://codereview.chromium.org/508973003/diff/440001/content/browser/devtool...
content/browser/devtools/BUILD.gn:63: script =
"//content/browser/devtools/protocol/devtools_protocol_handler_generator.py"
Fit in 80 please.

https://codereview.chromium.org/508973003/diff/440001/content/browser/devtool...
content/browser/devtools/BUILD.gn:69:
"$target_gen_dir/protocol/devtools_protocol_handler.h",
Let's put .cc first.

https://codereview.chromium.org/508973003/diff/440001/content/browser/devtool...
File content/browser/devtools/devtools_resources.gyp (right):

https://codereview.chromium.org/508973003/diff/440001/content/browser/devtool...
content/browser/devtools/devtools_resources.gyp:79: 'output_h':
'<(SHARED_INTERMEDIATE_DIR)/content/browser/devtools/protocol/devtools_protocol_handler_impl.h',
Let's put .cc first.

https://codereview.chromium.org/508973003/diff/440001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_handler_generator.py
(right):

https://codereview.chromium.org/508973003/diff/440001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:812:
output_h_file.write(template_h.substitute({},
output_h_file.close()

https://codereview.chromium.org/508973003/diff/440001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:819:
output_cc_file.write(template_cc.substitute({},
ditto

[vkuzkokov, 2014-09-16 16:37:55]

https://codereview.chromium.org/508973003/diff/440001/content/browser/devtool...
File content/browser/devtools/BUILD.gn (right):

https://codereview.chromium.org/508973003/diff/440001/content/browser/devtool...
content/browser/devtools/BUILD.gn:63: script =
"//content/browser/devtools/protocol/devtools_protocol_handler_generator.py"
On 2014/09/16 15:38:05, dgozman wrote:
> Fit in 80 please.

Done.

https://codereview.chromium.org/508973003/diff/440001/content/browser/devtool...
content/browser/devtools/BUILD.gn:69:
"$target_gen_dir/protocol/devtools_protocol_handler.h",
On 2014/09/16 15:38:05, dgozman wrote:
> Let's put .cc first.

Done.

https://codereview.chromium.org/508973003/diff/440001/content/browser/devtool...
File content/browser/devtools/devtools_resources.gyp (right):

https://codereview.chromium.org/508973003/diff/440001/content/browser/devtool...
content/browser/devtools/devtools_resources.gyp:79: 'output_h':
'<(SHARED_INTERMEDIATE_DIR)/content/browser/devtools/protocol/devtools_protocol_handler_impl.h',
On 2014/09/16 15:38:05, dgozman wrote:
> Let's put .cc first.

Done.

https://codereview.chromium.org/508973003/diff/440001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_handler_generator.py
(right):

https://codereview.chromium.org/508973003/diff/440001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:812:
output_h_file.write(template_h.substitute({},
On 2014/09/16 15:38:05, dgozman wrote:
> output_h_file.close()

Done.

[vkuzkokov, 2014-09-17 16:20:53]

Pavel, ptal.

[pfeldman, 2014-09-18 10:11:47]

Lets rename "Frontend" to "Client".

https://codereview.chromium.org/508973003/diff/460001/content/browser/devtool...
File content/browser/devtools/devtools_resources.gyp (right):

https://codereview.chromium.org/508973003/diff/460001/content/browser/devtool...
content/browser/devtools/devtools_resources.gyp:75: 'action_name':
'devtools_protocol_handler',
Why is this a part of devtools_resources? devtools_resources is about .grd. I
guess someone erroneously put devtools_protocol_contants here - it should have
been a part of devtools.gyp instead.

https://codereview.chromium.org/508973003/diff/460001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_frontend.h (right):

https://codereview.chromium.org/508973003/diff/460001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_frontend.h:12: class
DevToolsProtocolFrontend {
DevToolsProtocolClient would fit better.

https://codereview.chromium.org/508973003/diff/460001/content/browser/devtool...
File content/browser/devtools/protocol/inspector_handler.h (right):

https://codereview.chromium.org/508973003/diff/460001/content/browser/devtool...
content/browser/devtools/protocol/inspector_handler.h:14: class InspectorHandler
{
Lets remove this and make sure that we can mark domains in protocol.json as
"browser" and have no definitions for them here.

[vkuzkokov, 2014-09-19 17:02:45]

https://codereview.chromium.org/508973003/diff/460001/content/browser/devtool...
File content/browser/devtools/devtools_resources.gyp (right):

https://codereview.chromium.org/508973003/diff/460001/content/browser/devtool...
content/browser/devtools/devtools_resources.gyp:75: 'action_name':
'devtools_protocol_handler',
On 2014/09/18 10:11:47, pfeldman wrote:
> Why is this a part of devtools_resources? devtools_resources is about .grd. I
> guess someone erroneously put devtools_protocol_contants here - it should have
> been a part of devtools.gyp instead.

Done.

https://codereview.chromium.org/508973003/diff/460001/content/browser/devtool...
File content/browser/devtools/protocol/devtools_protocol_frontend.h (right):

https://codereview.chromium.org/508973003/diff/460001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_frontend.h:12: class
DevToolsProtocolFrontend {
On 2014/09/18 10:11:47, pfeldman wrote:
> DevToolsProtocolClient would fit better.

Done.

https://codereview.chromium.org/508973003/diff/460001/content/browser/devtool...
File content/browser/devtools/protocol/inspector_handler.h (right):

https://codereview.chromium.org/508973003/diff/460001/content/browser/devtool...
content/browser/devtools/protocol/inspector_handler.h:14: class InspectorHandler
{
On 2014/09/18 10:11:47, pfeldman wrote:
> Lets remove this and make sure that we can mark domains in protocol.json as
> "browser" and have no definitions for them here.

Although this handler doesn't have commands, it should be used to send events
from Inspector domain.

[dgozman, 2014-09-20 14:53:50]

https://codereview.chromium.org/508973003/diff/460001/content/browser/devtool...
File content/browser/devtools/protocol/inspector_handler.h (right):

https://codereview.chromium.org/508973003/diff/460001/content/browser/devtool...
content/browser/devtools/protocol/inspector_handler.h:14: class InspectorHandler
{
On 2014/09/19 17:02:45, vkuzkokov wrote:
> On 2014/09/18 10:11:47, pfeldman wrote:
> > Lets remove this and make sure that we can mark domains in protocol.json as
> > "browser" and have no definitions for them here.
> 
> Although this handler doesn't have commands, it should be used to send events
> from Inspector domain.

The idea is to move handler creation to the RenderViewDevToolsAgentHost for
greater flexibility, and only add handler files with actual implementation.

https://codereview.chromium.org/508973003/diff/480001/content/browser/devtool...
File content/browser/devtools/devtools.gyp (right):

https://codereview.chromium.org/508973003/diff/480001/content/browser/devtool...
content/browser/devtools/devtools.gyp:1: # Copyright (c) 2012 The Chromium
Authors. All rights reserved.
2014

[vkuzkokov, 2014-09-22 12:32:12]

https://codereview.chromium.org/508973003/diff/460001/content/browser/devtool...
File content/browser/devtools/protocol/inspector_handler.h (right):

https://codereview.chromium.org/508973003/diff/460001/content/browser/devtool...
content/browser/devtools/protocol/inspector_handler.h:14: class InspectorHandler
{
On 2014/09/20 14:53:50, dgozman wrote:
> On 2014/09/19 17:02:45, vkuzkokov wrote:
> > On 2014/09/18 10:11:47, pfeldman wrote:
> > > Lets remove this and make sure that we can mark domains in protocol.json
as
> > > "browser" and have no definitions for them here.
> > 
> > Although this handler doesn't have commands, it should be used to send
events
> > from Inspector domain.
> 
> The idea is to move handler creation to the RenderViewDevToolsAgentHost for
> greater flexibility, and only add handler files with actual implementation.

Done.

https://codereview.chromium.org/508973003/diff/480001/content/browser/devtool...
File content/browser/devtools/devtools.gyp (right):

https://codereview.chromium.org/508973003/diff/480001/content/browser/devtool...
content/browser/devtools/devtools.gyp:1: # Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2014/09/20 14:53:50, dgozman wrote:
> 2014

Done.

[dgozman, 2014-09-22 12:46:06]

https://codereview.chromium.org/508973003/diff/540001/content/browser/devtool...
File content/browser/devtools/protocol/page_handler.cc (right):

https://codereview.chromium.org/508973003/diff/540001/content/browser/devtool...
content/browser/devtools/protocol/page_handler.cc:115: "Unable to capture
screenshot");
nit: indentation

https://codereview.chromium.org/508973003/diff/540001/content/browser/devtool...
File content/browser/devtools/render_view_devtools_agent_host.h (right):

https://codereview.chromium.org/508973003/diff/540001/content/browser/devtool...
content/browser/devtools/render_view_devtools_agent_host.h:39: class
InputHandler;
Let's include them all.

https://codereview.chromium.org/508973003/diff/540001/content/browser/devtool...
content/browser/devtools/render_view_devtools_agent_host.h:120:
scoped_ptr<DevToolsProtocolHandlerImpl> handler_impl_;
Let's put this after all handlers for safe destruction order.

[vkuzkokov, 2014-09-22 12:57:29]

https://codereview.chromium.org/508973003/diff/540001/content/browser/devtool...
File content/browser/devtools/protocol/page_handler.cc (right):

https://codereview.chromium.org/508973003/diff/540001/content/browser/devtool...
content/browser/devtools/protocol/page_handler.cc:115: "Unable to capture
screenshot");
On 2014/09/22 12:46:06, dgozman wrote:
> nit: indentation

Done.

https://codereview.chromium.org/508973003/diff/540001/content/browser/devtool...
File content/browser/devtools/render_view_devtools_agent_host.h (right):

https://codereview.chromium.org/508973003/diff/540001/content/browser/devtool...
content/browser/devtools/render_view_devtools_agent_host.h:39: class
InputHandler;
On 2014/09/22 12:46:06, dgozman wrote:
> Let's include them all.

Done.

https://codereview.chromium.org/508973003/diff/540001/content/browser/devtool...
content/browser/devtools/render_view_devtools_agent_host.h:120:
scoped_ptr<DevToolsProtocolHandlerImpl> handler_impl_;
On 2014/09/22 12:46:06, dgozman wrote:
> Let's put this after all handlers for safe destruction order.

Done.

[pfeldman, 2014-09-23 14:26:53]

lgtm

[vkuzkokov, 2014-09-23 14:30:47]

vkuzkokov@chromium.org changed reviewers:
+ jochen@chromium.org

[vkuzkokov, 2014-09-23 14:32:28]

jochen, PTAL at:

content/browser/BUILD.gn

[vsevik, 2014-09-24 08:54:03]

vsevik@chromium.org changed reviewers:
- vsevik@chromium.org

[jochen (gone - plz use gerrit), 2014-09-24 20:05:42]

lgtm

[vkuzkokov, 2014-09-25 10:53:14]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2014-09-25 10:54:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/508973003/560001

[commit-bot: I haz the power, 2014-09-25 11:49:20]

Committed patchset #29 (id:560001) as cfbc3766dada239abe134710564f59a4a3275191

[commit-bot: I haz the power, 2014-09-25 11:50:01]

Patchset 29 (id:??) landed as
https://crrev.com/fa9990e750100c92ac9f53494b65a3174890487d
Cr-Commit-Position: refs/heads/master@{#296694}

[Mads Ager (chromium), 2014-09-25 13:15:47]

A revert of this CL (patchset #29 id:560001) has been created in
https://codereview.chromium.org/607503002/ by ager@chromium.org.

The reason for reverting is: Looks like this is causing
inspector/timeline/timeline-auto-record.html layout test failures on the
buildbots.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#master=Ch....

[vkuzkokov, 2014-09-25 14:41:37]

Generated code was sending error response if params were missing from command.
Turns out it's valid if all parameters are optional.

[vkuzkokov, 2014-09-25 14:51:09]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2014-09-25 14:51:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/508973003/580001

[commit-bot: I haz the power, 2014-09-25 16:44:33]

Committed patchset #30 (id:580001) as 37bbb8287d5011ee8185b207e272345801685231

[commit-bot: I haz the power, 2014-09-25 16:45:24]

Patchset 30 (id:??) landed as
https://crrev.com/4b0b62979ddd96b65064364eb669ec9dda7e5e5e
Cr-Commit-Position: refs/heads/master@{#296723}

[Zhenyao Mo, 2014-09-26 21:14:34]

A revert of this CL (patchset #30 id:580001) has been created in
https://codereview.chromium.org/607913004/ by zmo@chromium.org.

The reason for reverting is: This causes a lot of layout tests to timeout on
Linux ASAN bot in webkit waterfall..

[Zhenyao Mo, 2014-09-26 21:14:42]

A revert of this CL (patchset #30 id:580001) has been created in
https://codereview.chromium.org/612523004/ by zmo@chromium.org.

The reason for reverting is: This causes a lot of layout tests to timeout on
Linux ASAN bot in webkit waterfall..

[vkuzkokov, 2014-10-01 12:22:31]

The only layout test affected by this patch is
inspector-protocol/input/emulateTouchFromMouseEvent.html which works like a
charm with or without the patch.
Seems like whatever happened with crashes/timeouts that day is unrelated to this
CL.

[vkuzkokov, 2014-10-01 12:23:05]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2014-10-01 12:23:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/508973003/580001

[commit-bot: I haz the power, 2014-10-01 12:24:22]

Committed patchset #30 (id:580001) as 2f932477b738e36292d5c68490fff56fe4630a1f

[commit-bot: I haz the power, 2014-10-01 12:24:56]

Patchset 30 (id:??) landed as
https://crrev.com/c985d95d1be81585c2e3a085838ce459a5b3ce2f
Cr-Commit-Position: refs/heads/master@{#297628}