[Extensions] Update generated code to support move operations

[Extensions] Update generated code to support move operations

Generated extensions code currently uses linked_ptrs for non-copyable types
stored in containers. Instead, these types should just be movable. Update the
code generator to support move operations for flagged idl files and apply it
to the developerPrivate API as a first step/POC.

BUG=595949
Committed: https://crrev.com/63ef43c0af90ca5f23d0606e2394b0e5f25fc0b5
Cr-Commit-Position: refs/heads/master@{#382704}

[Devlin, 2016-03-18 18:42:23]

Description was changed from

==========
[Extensions] Update generated code to support move operations

Generated extensions code currently uses linked_ptrs for non-copyable types
stored in containers. Instead, these types should just be movable. Update the
code generator to support move operations and apply it to the developerPrivate
API as a first step/POC.

BUG=595949
==========

to

==========
[Extensions] Update generated code to support move operations

Generated extensions code currently uses linked_ptrs for non-copyable types
stored in containers. Instead, these types should just be movable. Update the
code generator to support move operations and apply it to the developerPrivate
API as a first step/POC.

BUG=595949
==========

[Devlin, 2016-03-18 18:42:43]

Description was changed from

==========
[Extensions] Update generated code to support move operations

Generated extensions code currently uses linked_ptrs for non-copyable types
stored in containers. Instead, these types should just be movable. Update the
code generator to support move operations and apply it to the developerPrivate
API as a first step/POC.

BUG=595949
==========

to

==========
[Extensions] Update generated code to support move operations

Generated extensions code currently uses linked_ptrs for non-copyable types
stored in containers. Instead, these types should just be movable. Update the
code generator to support move operations for flagged idl files and apply it
to the developerPrivate API as a first step/POC.

BUG=595949
==========

[Devlin, 2016-03-18 18:43:59]

Patchset #1 (id:1) has been deleted

[Devlin, 2016-03-18 18:44:05]

Patchset #1 (id:20001) has been deleted

[Devlin, 2016-03-18 20:17:39]

rdevlin.cronin@chromium.org changed reviewers:
+ asargent@chromium.org, dcheng@chromium.org

[Devlin, 2016-03-18 20:17:40]

Antony, can you take a look?
Daniel, mind taking a quick glance to double check the c++11-ness (and look at
the comment in extension_info_generator.cc)?

https://codereview.chromium.org/1811413002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/extension_info_generator.cc
(right):

https://codereview.chromium.org/1811413002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:299:
list_ = ExtensionInfoList();
dcheng: what's the right thing to do here?  Would list_.clear() put it back into
a sane state?

https://codereview.chromium.org/1811413002/diff/40001/tools/json_schema_compi...
File tools/json_schema_compiler/h_generator.py (right):

https://codereview.chromium.org/1811413002/diff/40001/tools/json_schema_compi...
tools/json_schema_compiler/h_generator.py:231: if 'use_movable_types' in
type_.namespace.compiler_options:
Note: we could actually generate move ctors/operators for all types and it
compiles (did it to double check that everything was right), but I figured it
would probably be best not to generate them if they're not used (primarily for
binary size reasons - as we transition to moved types we don't have to generate
as many specializations for linked_ptr templates).

[asargent_no_longer_on_chrome, 2016-03-21 21:03:14]

lgtm

https://codereview.chromium.org/1811413002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/1811413002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:402: //
scoped ptr, and so ownership between that and the vector of linked ptrs
is this part of the comment about "vector of linked ptrs" still accurate? (not
sure if it's referring to the ExtensionInfoGenerator::ExtensionInfoList or some
elements within it)

https://codereview.chromium.org/1811413002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/extension_info_generator.cc
(right):

https://codereview.chromium.org/1811413002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:299:
list_ = ExtensionInfoList();
On 2016/03/18 20:17:40, Devlin wrote:
> dcheng: what's the right thing to do here?  Would list_.clear() put it back
into
> a sane state?

(Possibly same issue in ExtensionInfoGenerator::OnImageLoaded as well BTW)

https://codereview.chromium.org/1811413002/diff/40001/tools/json_schema_compi...
File tools/json_schema_compiler/cpp_type_generator.py (right):

https://codereview.chromium.org/1811413002/diff/40001/tools/json_schema_compi...
tools/json_schema_compiler/cpp_type_generator.py:128: cpp_type == 'base::Value'
or cpp_type == 'base::DictionaryValue'):
should you also have base::ListValue here?

[Devlin, 2016-03-21 22:24:27]

https://codereview.chromium.org/1811413002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/1811413002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:402: //
scoped ptr, and so ownership between that and the vector of linked ptrs
On 2016/03/21 21:03:13, Antony Sargent wrote:
> is this part of the comment about "vector of linked ptrs" still accurate? (not
> sure if it's referring to the ExtensionInfoGenerator::ExtensionInfoList or
some
> elements within it)

Good call!  We can actually completely remove this hack. :)

https://codereview.chromium.org/1811413002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/extension_info_generator.cc
(right):

https://codereview.chromium.org/1811413002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:299:
list_ = ExtensionInfoList();
On 2016/03/21 21:03:13, Antony Sargent wrote:
> On 2016/03/18 20:17:40, Devlin wrote:
> > dcheng: what's the right thing to do here?  Would list_.clear() put it back
> into
> > a sane state?
> 
> (Possibly same issue in ExtensionInfoGenerator::OnImageLoaded as well BTW)
> 

Yep - just too lazy to put two comments. ;)

https://codereview.chromium.org/1811413002/diff/40001/tools/json_schema_compi...
File tools/json_schema_compiler/cpp_type_generator.py (right):

https://codereview.chromium.org/1811413002/diff/40001/tools/json_schema_compi...
tools/json_schema_compiler/cpp_type_generator.py:128: cpp_type == 'base::Value'
or cpp_type == 'base::DictionaryValue'):
On 2016/03/21 21:03:14, Antony Sargent wrote:
> should you also have base::ListValue here?

I don't think we ever use base::ListValue for a generated cpp type here, do we? 
(And if we ever do, we'd get a compiler error and could fix it then.)

[asargent_no_longer_on_chrome, 2016-03-21 22:45:24]

https://codereview.chromium.org/1811413002/diff/40001/tools/json_schema_compi...
File tools/json_schema_compiler/cpp_type_generator.py (right):

https://codereview.chromium.org/1811413002/diff/40001/tools/json_schema_compi...
tools/json_schema_compiler/cpp_type_generator.py:128: cpp_type == 'base::Value'
or cpp_type == 'base::DictionaryValue'):
On 2016/03/21 22:24:27, Devlin wrote:
> On 2016/03/21 21:03:14, Antony Sargent wrote:
> > should you also have base::ListValue here?
> 
> I don't think we ever use base::ListValue for a generated cpp type here, do
we? 
> (And if we ever do, we'd get a compiler error and could fix it then.)

Perhaps we don't. I did a quick grep for linked_ptr piped through to a grep for
ListValue and thought I saw some results in there that used linked_ptr, but upon
closer examination you're right that there don't seem to be any.

[dcheng, 2016-03-22 17:42:35]

Looks good! Just a few minor suggestions.

https://codereview.chromium.org/1811413002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/extension_info_generator.cc
(right):

https://codereview.chromium.org/1811413002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:260:
FROM_HERE, base::Bind(callback, base::Passed(std::move(list_))));
You shouldn't need std::move() with base::Passed() if you pass list_ by pointer,
e.g. base::Passed(&list). Ditto elsewhere.

https://codereview.chromium.org/1811413002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:261:
list_ = ExtensionInfoList();
Just call clear(): it has no preconditions, and |list_| is guaranteed to be in a
valid, though unspecified, state. This will restore it to a known state.

https://codereview.chromium.org/1811413002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:298:
// What should we do here?
See above.

https://codereview.chromium.org/1811413002/diff/80001/tools/json_schema_compi...
File tools/json_schema_compiler/cc_generator.py (right):

https://codereview.chromium.org/1811413002/diff/80001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:131: # Note: we use '_other' because
some API objects have a member 'other'.
It's probably better to call this 'rhs' rather than prefixing it with an
underscore.

https://codereview.chromium.org/1811413002/diff/80001/tools/json_schema_compi...
File tools/json_schema_compiler/util.h (right):

https://codereview.chromium.org/1811413002/diff/80001/tools/json_schema_compi...
tools/json_schema_compiler/util.h:169: out->Append(from.ToValue().release());
No .release().

[Devlin, 2016-03-22 17:58:41]

https://codereview.chromium.org/1811413002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/developer_private/extension_info_generator.cc
(right):

https://codereview.chromium.org/1811413002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:260:
FROM_HERE, base::Bind(callback, base::Passed(std::move(list_))));
On 2016/03/22 17:42:34, dcheng wrote:
> You shouldn't need std::move() with base::Passed() if you pass list_ by
pointer,
> e.g. base::Passed(&list). Ditto elsewhere.

Done.

https://codereview.chromium.org/1811413002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:261:
list_ = ExtensionInfoList();
On 2016/03/22 17:42:34, dcheng wrote:
> Just call clear(): it has no preconditions, and |list_| is guaranteed to be in
a
> valid, though unspecified, state. This will restore it to a known state.

Done.

https://codereview.chromium.org/1811413002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/developer_private/extension_info_generator.cc:298:
// What should we do here?
On 2016/03/22 17:42:34, dcheng wrote:
> See above.

Done.

https://codereview.chromium.org/1811413002/diff/80001/tools/json_schema_compi...
File tools/json_schema_compiler/cc_generator.py (right):

https://codereview.chromium.org/1811413002/diff/80001/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:131: # Note: we use '_other' because
some API objects have a member 'other'.
On 2016/03/22 17:42:34, dcheng wrote:
> It's probably better to call this 'rhs' rather than prefixing it with an
> underscore.

Here's hoping no apis use "rhs" as a parameter ;)  Done.

https://codereview.chromium.org/1811413002/diff/80001/tools/json_schema_compi...
File tools/json_schema_compiler/util.h (right):

https://codereview.chromium.org/1811413002/diff/80001/tools/json_schema_compi...
tools/json_schema_compiler/util.h:169: out->Append(from.ToValue().release());
On 2016/03/22 17:42:34, dcheng wrote:
> No .release().

Done.

[dcheng, 2016-03-22 18:26:51]

lgtm

[Devlin, 2016-03-22 21:13:22]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-03-22 21:13:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1811413002/#ps120001
(title: "compile fix")

[commit-bot: I haz the power, 2016-03-22 21:13:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1811413002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1811413002/120001

[commit-bot: I haz the power, 2016-03-22 22:11:13]

Description was changed from

==========
[Extensions] Update generated code to support move operations

Generated extensions code currently uses linked_ptrs for non-copyable types
stored in containers. Instead, these types should just be movable. Update the
code generator to support move operations for flagged idl files and apply it
to the developerPrivate API as a first step/POC.

BUG=595949
==========

to

==========
[Extensions] Update generated code to support move operations

Generated extensions code currently uses linked_ptrs for non-copyable types
stored in containers. Instead, these types should just be movable. Update the
code generator to support move operations for flagged idl files and apply it
to the developerPrivate API as a first step/POC.

BUG=595949
==========

[commit-bot: I haz the power, 2016-03-22 22:11:14]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-03-22 22:13:21]

Description was changed from

==========
[Extensions] Update generated code to support move operations

Generated extensions code currently uses linked_ptrs for non-copyable types
stored in containers. Instead, these types should just be movable. Update the
code generator to support move operations for flagged idl files and apply it
to the developerPrivate API as a first step/POC.

BUG=595949
==========

to

==========
[Extensions] Update generated code to support move operations

Generated extensions code currently uses linked_ptrs for non-copyable types
stored in containers. Instead, these types should just be movable. Update the
code generator to support move operations for flagged idl files and apply it
to the developerPrivate API as a first step/POC.

BUG=595949

Committed: https://crrev.com/63ef43c0af90ca5f23d0606e2394b0e5f25fc0b5
Cr-Commit-Position: refs/heads/master@{#382704}
==========

[commit-bot: I haz the power, 2016-03-22 22:13:22]

Patchset 5 (id:??) landed as
https://crrev.com/63ef43c0af90ca5f23d0606e2394b0e5f25fc0b5
Cr-Commit-Position: refs/heads/master@{#382704}