[DevTools] Make generated protocol structs wrappers around DictionaryValue

[DevTools] Make generated protocol structs wrappers around DictionaryValue

Protocol structs are now refcounted.

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

[vkuzkokov, 2014-10-16 14:30:09]

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

[dgozman, 2014-10-17 10:51:57]

https://codereview.chromium.org/642263004/diff/1/content/browser/devtools/pro...
File content/browser/devtools/protocol/devtools_protocol_handler_generator.py
(right):

https://codereview.chromium.org/642263004/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:45:
template<class T> class ListBuilder : public ListBuilderBase {
nit: template<...> is usually on the separate line.

https://codereview.chromium.org/642263004/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:54:
template<> class ListBuilder<std::string> : public ListBuilderBase {
ditto

https://codereview.chromium.org/642263004/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:340:
tmpl_arg_opt = string.Template("${param}_found ? &in_${param} : nullptr")
nit: keep the line break

https://codereview.chromium.org/642263004/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:372:
has_${param}_ = true;
We should remove "has_${param}_" entirely, and use base::DictionaryValue::HasKey
instead.

https://codereview.chromium.org/642263004/diff/1/content/browser/devtools/pro...
File content/browser/devtools/protocol/page_handler.cc (right):

https://codereview.chromium.org/642263004/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/page_handler.cc:198:
scoped_ptr<NavigationEntry> entry;
I think, default constructor of scoped_ptr will have nullptr inside. Did you
test this?

[vkuzkokov, 2014-10-17 11:41:26]

https://codereview.chromium.org/642263004/diff/1/content/browser/devtools/pro...
File content/browser/devtools/protocol/devtools_protocol_handler_generator.py
(right):

https://codereview.chromium.org/642263004/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:45:
template<class T> class ListBuilder : public ListBuilderBase {
On 2014/10/17 10:51:57, dgozman wrote:
> nit: template<...> is usually on the separate line.

Done.

https://codereview.chromium.org/642263004/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:54:
template<> class ListBuilder<std::string> : public ListBuilderBase {
On 2014/10/17 10:51:57, dgozman wrote:
> ditto

Done.

https://codereview.chromium.org/642263004/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:340:
tmpl_arg_opt = string.Template("${param}_found ? &in_${param} : nullptr")
On 2014/10/17 10:51:57, dgozman wrote:
> nit: keep the line break

Done.

https://codereview.chromium.org/642263004/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:372:
has_${param}_ = true;
On 2014/10/17 10:51:57, dgozman wrote:
> We should remove "has_${param}_" entirely, and use
base::DictionaryValue::HasKey
> instead.

Done.

https://codereview.chromium.org/642263004/diff/1/content/browser/devtools/pro...
File content/browser/devtools/protocol/page_handler.cc (right):

https://codereview.chromium.org/642263004/diff/1/content/browser/devtools/pro...
content/browser/devtools/protocol/page_handler.cc:198:
scoped_ptr<NavigationEntry> entry;
On 2014/10/17 10:51:57, dgozman wrote:
> I think, default constructor of scoped_ptr will have nullptr inside. Did you
> test this?

Not every method.

[vkuzkokov, 2014-10-22 14:33:51]

That's what it looks like with movables.

[vkuzkokov, 2014-10-23 16:45:21]

Applied to tracing

[dgozman, 2014-10-29 13:27:45]

Looks good from a devtools side. Let's get a review from C++11 expert?

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

https://codereview.chromium.org/642263004/diff/130001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:558:
all_fields = " | ".join(enum_items)
Why not just |all_fields = "1 << req_fields_num - 1"| ?

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

https://codereview.chromium.org/642263004/diff/130001/content/browser/devtool...
content/browser/devtools/protocol/page_handler.cc:525: return;
This changes the semantics - we used to send frame without metadata. Please
don't change semantics in this patch.

https://codereview.chromium.org/642263004/diff/130001/content/browser/devtool...
content/browser/devtools/protocol/page_handler.cc:538:
.set_metadata(ScreencastFrameMetadata::Create()
I'd extract metadata to a separate statement for readability and demonstration
of "Builder" local variables.

https://codereview.chromium.org/642263004/diff/130001/content/browser/devtool...
content/browser/devtools/protocol/page_handler.cc:582:
.set_color(dom::RGBA::Create().set_r(r).set_g(g).set_b(b).set_a(a).Pass())
Please extract color into a separate variable.

[vkuzkokov, 2014-10-29 17:42:03]

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

https://codereview.chromium.org/642263004/diff/130001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:558:
all_fields = " | ".join(enum_items)
On 2014/10/29 13:27:45, dgozman wrote:
> Why not just |all_fields = "1 << req_fields_num - 1"| ?

To emphasize the intent: same way generator in blink does it.

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

https://codereview.chromium.org/642263004/diff/130001/content/browser/devtool...
content/browser/devtools/protocol/page_handler.cc:525: return;
On 2014/10/29 13:27:45, dgozman wrote:
> This changes the semantics - we used to send frame without metadata. Please
> don't change semantics in this patch.

Not exactly: empty ScreencaseFrameMetadata failed to serialize if it was missing
all of its required parameters. Now it fails to compile that way.

https://codereview.chromium.org/642263004/diff/130001/content/browser/devtool...
content/browser/devtools/protocol/page_handler.cc:538:
.set_metadata(ScreencastFrameMetadata::Create()
On 2014/10/29 13:27:45, dgozman wrote:
> I'd extract metadata to a separate statement for readability and demonstration
> of "Builder" local variables.

Done.

https://codereview.chromium.org/642263004/diff/130001/content/browser/devtool...
content/browser/devtools/protocol/page_handler.cc:582:
.set_color(dom::RGBA::Create().set_r(r).set_g(g).set_b(b).set_a(a).Pass())
On 2014/10/29 13:27:45, dgozman wrote:
> Please extract color into a separate variable.

Done.

[vkuzkokov, 2014-11-05 15:01:23]

Added CL with generated code:

https://codereview.chromium.org/683733010/

[dgozman, 2014-11-06 14:20:42]

lgtm

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

https://codereview.chromium.org/642263004/diff/210001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:37:
class ListBuilderBase {
Let's put ListBuilder in content::devtools.

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

https://codereview.chromium.org/642263004/diff/210001/content/browser/devtool...
content/browser/devtools/protocol/page_handler.cc:417:
.set_visible(visible).Pass());
nit: indentation

https://codereview.chromium.org/642263004/diff/210001/content/browser/devtool...
File content/browser/devtools/protocol/usage_and_quota_query.h (right):

https://codereview.chromium.org/642263004/diff/210001/content/browser/devtool...
content/browser/devtools/protocol/usage_and_quota_query.h:50: double
temporary_quota_;
Please initialize this field in constructor.

https://codereview.chromium.org/642263004/diff/210001/content/browser/devtool...
content/browser/devtools/protocol/usage_and_quota_query.h:51: double
persistent_quota_;
ditto

[dgozman, 2014-11-06 14:22:34]

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

[dgozman, 2014-11-06 14:22:34]

dgozman@chromium.org changed required reviewers:
+ thakis@chromium.org

[dgozman, 2014-11-06 14:22:35]

Nico,

Could you please take a look at RValue References and Move Semantics usage in
this patch? Those are added in devtools_protocol_handler_generator.py.

What is going on: we would like to have compile-time checks when constructing
objects used in declarative devtools protocol. See page_handler.cc for usage
examples. For this, we introduce builder template specialized on the fields
already set on the object. This builder owns underlying base::DictionaryValue
and it would be nice to have move semantics for this class.

See the example of generated code in https://codereview.chromium.org/683733010/.

What do you think?

Thank you,
Dmitry

[vkuzkokov, 2014-11-06 16:15:00]

Patchset #13 (id:230001) has been deleted

[vkuzkokov, 2014-11-06 16:30:12]

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

https://codereview.chromium.org/642263004/diff/210001/content/browser/devtool...
content/browser/devtools/protocol/devtools_protocol_handler_generator.py:37:
class ListBuilderBase {
On 2014/11/06 14:20:42, dgozman wrote:
> Let's put ListBuilder in content::devtools.

Done.

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

https://codereview.chromium.org/642263004/diff/210001/content/browser/devtool...
content/browser/devtools/protocol/page_handler.cc:417:
.set_visible(visible).Pass());
On 2014/11/06 14:20:42, dgozman wrote:
> nit: indentation

Done.

https://codereview.chromium.org/642263004/diff/210001/content/browser/devtool...
File content/browser/devtools/protocol/usage_and_quota_query.h (right):

https://codereview.chromium.org/642263004/diff/210001/content/browser/devtool...
content/browser/devtools/protocol/usage_and_quota_query.h:50: double
temporary_quota_;
On 2014/11/06 14:20:42, dgozman wrote:
> Please initialize this field in constructor.

Done.

https://codereview.chromium.org/642263004/diff/210001/content/browser/devtool...
content/browser/devtools/protocol/usage_and_quota_query.h:51: double
persistent_quota_;
On 2014/11/06 14:20:42, dgozman wrote:
> ditto

Done.

[Nico, 2014-11-06 17:41:48]

On 2014/11/06 14:22:35, dgozman wrote:
> Nico,
> 
> Could you please take a look at RValue References and Move Semantics usage in
> this patch? Those are added in devtools_protocol_handler_generator.py.
> 
> What is going on: we would like to have compile-time checks when constructing
> objects used in declarative devtools protocol. See page_handler.cc for usage
> examples. 

Doesn't http://en.cppreference.com/w/cpp/language/copy_elision do what you need
there already?

[vkuzkokov, 2014-11-07 11:32:27]

On 2014/11/06 17:41:48, Nico wrote:
> On 2014/11/06 14:22:35, dgozman wrote:
> > Nico,
> > 
> > Could you please take a look at RValue References and Move Semantics usage
in
> > this patch? Those are added in devtools_protocol_handler_generator.py.
> > 
> > What is going on: we would like to have compile-time checks when
constructing
> > objects used in declarative devtools protocol. See page_handler.cc for usage
> > examples. 
> 
> Doesn't http://en.cppreference.com/w/cpp/language/copy_elision do what you
need
> there already?

No. Optimization is just a nice side effect. The purpose is to have a
compile-time guarantee of uniqueness.

[Nico, 2014-11-07 17:55:54]

I don't understand why you need this.

[vkuzkokov, 2014-11-07 18:33:40]

On 2014/11/07 17:55:54, Nico wrote:
> I don't understand why you need this.

Movable semantics that is? This way we have API that is very hard to misuse (all
relevant instances of base::Value are guaranteed to be deleted exactly once
unless we use scoped_ptr::get/release or JSON Value API, neither of which should
be necessary) and avoid copying which can sometimes be prohibitively expensive.

[Nico, 2014-11-07 23:50:48]

On Fri, Nov 7, 2014 at 10:33 AM, <vkuzkokov@chromium.org> wrote:

> On 2014/11/07 17:55:54, Nico wrote:
>
>> I don't understand why you need this.
>>
>
> Movable semantics that is? This way we have API that is very hard to
> misuse (all
> relevant instances of base::Value are guaranteed to be deleted exactly once
> unless we use scoped_ptr::get/release or JSON Value API, neither of which
> should
> be necessary) and avoid copying which can sometimes be prohibitively
> expensive.
>

- you get the same if your class has a scoped_ptr internally
- does it really matter? this isn't copied a lot, and the objects aren't
huge, right?

I mean, we only want to allow move constructors if there's a demonstrable
performance benefit at this point, and to me it sounds like this isn't the
case here?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dgozman, 2014-11-10 14:04:47]

Alright, ref-counted version seems fine. lgtm

[vkuzkokov, 2014-11-10 14:07:38]

vkuzkokov@chromium.org changed reviewers:
- thakis@chromium.org

[vkuzkokov, 2014-11-10 14:07:38]

vkuzkokov@chromium.org changed required reviewers:
- thakis@chromium.org

[vkuzkokov, 2014-11-10 14:24:06]

Patchset #15 (id:290001) has been deleted

[vkuzkokov, 2014-11-10 15:34:06]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2014-11-10 15:34:34]

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

[commit-bot: I haz the power, 2014-11-10 16:27:47]

Committed patchset #16 (id:330001)

[commit-bot: I haz the power, 2014-11-10 17:12:58]

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