Use rvalue reference instead of ExtensionBuilder::pass()

Use rvalue reference instead of ExtensionBuilder::pass()

C++ 11 enables rvalue reference with std::move() so that removing legacy ListBuilder::Pass().

BUG=563649
TBR=thankis@chromium.org

Committed: https://crrev.com/704a902182dd1ba3b315563a6a65a7eb107ece79
Cr-Commit-Position: refs/heads/master@{#365969}

[limasdf, 2015-12-09 14:01:31]

Patchset #1 (id:1) has been deleted

[limasdf, 2015-12-09 15:06:43]

Description was changed from

==========
Remove ExtensionBuilder::pass(), use rvalue instead

BUG=
==========

to

==========
Use rvalue reference instead of ExtensionBuilder::pass()

C++ 11 enables rvalue reference with std::move() so that removing legacy
ListBuilder::Pass().

BUG=563649
==========

[limasdf, 2015-12-09 15:06:43]

limasdf@gmail.com changed reviewers:
+ asargent@chromium.org, thakis@chromium.org

[limasdf, 2015-12-09 15:07:21]

limasdf@gmail.com changed reviewers:
+ rdevlin.cronin@chromium.org
- asargent@chromium.org, thakis@chromium.org

[limasdf, 2015-12-09 15:16:14]

limasdf@gmail.com changed reviewers:
+ danakj@chromium.org
- rdevlin.cronin@chromium.org

[limasdf, 2015-12-09 15:20:18]

Dana, Could you take a first look at 	extensions/common/extension_builder.{h,cc}
?

It's pretty similar with previous(extensions::ListBuilder) one.

[danakj, 2015-12-09 18:46:44]

https://codereview.chromium.org/1511103003/diff/20001/extensions/common/exten...
File extensions/common/extension_builder.cc (right):

https://codereview.chromium.org/1511103003/diff/20001/extensions/common/exten...
extensions/common/extension_builder.cc:18: : path_(other.path_),
move the string instead of copying it?

https://codereview.chromium.org/1511103003/diff/20001/extensions/common/exten...
extensions/common/extension_builder.cc:20: manifest_(other.manifest_.release()),
why isn't this manifest_(std::move(other.manifest_))?

https://codereview.chromium.org/1511103003/diff/20001/extensions/common/exten...
extensions/common/extension_builder.cc:22: id_(other.id_) {}
move the string dont copy?

https://codereview.chromium.org/1511103003/diff/20001/extensions/common/exten...
File extensions/common/extension_builder.h (right):

https://codereview.chromium.org/1511103003/diff/20001/extensions/common/exten...
extensions/common/extension_builder.h:27: ExtensionBuilder(ExtensionBuilder&&
other);
you should make a move-operator= when you add a move constructor. (similar rules
to copy constructors)

[limasdf, 2015-12-10 00:42:11]

Patchset #2 (id:40001) has been deleted

[limasdf, 2015-12-10 00:47:10]

Patchset #2 (id:60001) has been deleted

[limasdf, 2015-12-10 01:47:21]

Patchset #2 (id:80001) has been deleted

[limasdf, 2015-12-10 08:55:19]

Patchset #2 (id:10008) has been deleted

[limasdf, 2015-12-16 07:28:00]

Patchset #3 (id:130001) has been deleted

[limasdf, 2015-12-16 07:29:23]

Patchset #3 (id:150001) has been deleted

[limasdf, 2015-12-16 08:01:39]

Patchset #3 (id:170001) has been deleted

[limasdf, 2015-12-16 09:34:54]

Dana, Could you share your wisdom?

It failed only from Windows. 
Here's Backtrace on my Window* PC. 

Exactly `value.DeepCopy()` is problem. and there's no detail log. :(


[ RUN      ] ExtensionBuilderTest.Basic
Backtrace:
        extensions::Extension::Create [0x00E04F2B+283]
(d:\chromium\src\extensions\common\extension.cc:119)
        extensions::ExtensionBuilder::Build [0x00FE3DA6+102]
(d:\chromium\src\extensions\common\extension_builder.cc:43)
        extensions::ExtensionBuilderTest_Basic_Test::TestBody [0x0086415A+58]
(d:\chromium\src\extensions\common\extension_builder_unittest.cc:21)
       
testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,void>
[0x009DDE57+55] (d:\chromium\src\testing\gtest\src\gtest.cc:2458)

https://codereview.chromium.org/1511103003/diff/20001/extensions/common/exten...
File extensions/common/extension_builder.cc (right):

https://codereview.chromium.org/1511103003/diff/20001/extensions/common/exten...
extensions/common/extension_builder.cc:18: : path_(other.path_),
On 2015/12/09 18:46:44, danakj (behind on reviews) wrote:
> move the string instead of copying it?

Done.

https://codereview.chromium.org/1511103003/diff/20001/extensions/common/exten...
extensions/common/extension_builder.cc:20: manifest_(other.manifest_.release()),
On 2015/12/09 18:46:44, danakj (behind on reviews) wrote:
> why isn't this manifest_(std::move(other.manifest_))?

Done.

https://codereview.chromium.org/1511103003/diff/20001/extensions/common/exten...
extensions/common/extension_builder.cc:22: id_(other.id_) {}
On 2015/12/09 18:46:44, danakj (behind on reviews) wrote:
> move the string dont copy?

Done.

https://codereview.chromium.org/1511103003/diff/20001/extensions/common/exten...
File extensions/common/extension_builder.h (right):

https://codereview.chromium.org/1511103003/diff/20001/extensions/common/exten...
extensions/common/extension_builder.h:27: ExtensionBuilder(ExtensionBuilder&&
other);
On 2015/12/09 18:46:44, danakj (behind on reviews) wrote:
> you should make a move-operator= when you add a move constructor. (similar
rules
> to copy constructors)

Yes, https://www.chromium.org/rvalue-references #9 says it. Sorry I forgot it.

Done.

[danakj, 2015-12-16 18:57:49]

https://codereview.chromium.org/1511103003/diff/190001/extensions/common/test...
File extensions/common/test_util.cc (right):

https://codereview.chromium.org/1511103003/diff/190001/extensions/common/test...
extensions/common/test_util.cc:17: return builder
This is now returning a reference to a temporary variable. Return by value.

https://codereview.chromium.org/1511103003/diff/190001/extensions/common/test...
extensions/common/test_util.cc:24: ExtensionBuilder& BuildApp(ExtensionBuilder
builder) {
ditto

[danakj, 2015-12-16 19:06:04]

https://codereview.chromium.org/1511103003/diff/190001/extensions/common/test...
File extensions/common/test_util.cc (right):

https://codereview.chromium.org/1511103003/diff/190001/extensions/common/test...
extensions/common/test_util.cc:17: return builder
On 2015/12/16 18:57:49, danakj (behind on reviews) wrote:
> This is now returning a reference to a temporary variable. Return by value.

er, local, not temporary, but it's bad :) the variable is destroyed when this
function leaves scope

[limasdf, 2015-12-17 06:06:48]

Patchset #4 (id:210001) has been deleted

[limasdf, 2015-12-17 06:17:34]

limasdf@gmail.com changed reviewers:
+ asargent@chromium.org

[limasdf, 2015-12-17 06:19:46]

adding Antony, the owner of extension.


btw, I'm wondering why *only Windows trybot has failed from patchset#2. :/

https://codereview.chromium.org/1511103003/diff/190001/extensions/common/test...
File extensions/common/test_util.cc (right):

https://codereview.chromium.org/1511103003/diff/190001/extensions/common/test...
extensions/common/test_util.cc:17: return builder
On 2015/12/16 19:06:04, danakj (behind on reviews) wrote:
> On 2015/12/16 18:57:49, danakj (behind on reviews) wrote:
> > This is now returning a reference to a temporary variable. Return by value.
> 
> er, local, not temporary, but it's bad :) the variable is destroyed when this
> function leaves scope

Returning rvalue should work.

https://codereview.chromium.org/1511103003/diff/190001/extensions/common/test...
extensions/common/test_util.cc:24: ExtensionBuilder& BuildApp(ExtensionBuilder
builder) {
On 2015/12/16 18:57:49, danakj (behind on reviews) wrote:
> ditto

ditto-ly done.

[asargent_no_longer_on_chrome, 2015-12-17 22:09:08]

lgtm

[danakj, 2015-12-17 22:21:19]

On Wed, Dec 16, 2015 at 10:19 PM, <limasdf@gmail.com> wrote:

> adding Antony, the owner of extension.
>
>
> btw, I'm wondering why *only Windows trybot has failed from patchset#2. :/


Probably it just happened to reuse that memory from the local faster, or
something. I'm sure other bots would explode on the waterfall (esp ASAN
bots).

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-12-17 22:22:02]

LGTM

https://codereview.chromium.org/1511103003/diff/230001/extensions/common/test...
File extensions/common/test_util.cc (right):

https://codereview.chromium.org/1511103003/diff/230001/extensions/common/test...
extensions/common/test_util.cc:17: return
std::move(builder.SetManifest(DictionaryBuilder()
nit: it might be easier to read these if you wrote it as

  builder.SetManifest(....);
  return std::move(builder);

[limasdf, 2015-12-18 00:29:08]

Description was changed from

==========
Use rvalue reference instead of ExtensionBuilder::pass()

C++ 11 enables rvalue reference with std::move() so that removing legacy
ListBuilder::Pass().

BUG=563649
==========

to

==========
Use rvalue reference instead of ExtensionBuilder::pass()

C++ 11 enables rvalue reference with std::move() so that removing legacy
ListBuilder::Pass().

BUG=563649
TBR=thankis@chromium.org
==========

[limasdf, 2015-12-18 00:29:08]

limasdf@gmail.com changed reviewers:
+ thakis@chromium.org

[limasdf, 2015-12-18 00:31:58]

https://codereview.chromium.org/1511103003/diff/230001/extensions/common/test...
File extensions/common/test_util.cc (right):

https://codereview.chromium.org/1511103003/diff/230001/extensions/common/test...
extensions/common/test_util.cc:17: return
std::move(builder.SetManifest(DictionaryBuilder()
On 2015/12/17 22:22:02, danakj (behind on reviews) wrote:
> nit: it might be easier to read these if you wrote it as
> 
>   builder.SetManifest(....);
>   return std::move(builder);

Thank you. |builder| is rvalue, just returning without std::move.

builder.SetManifest(....);
return builder;

[limasdf, 2015-12-18 00:32:03]

The CQ bit was checked by limasdf@gmail.com

[limasdf, 2015-12-18 00:32:03]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/1511103003/#ps250001
(title: "nit change(dana review)")

[commit-bot: I haz the power, 2015-12-18 00:34:13]

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

[danakj, 2015-12-18 00:38:49]

https://codereview.chromium.org/1511103003/diff/230001/extensions/common/test...
File extensions/common/test_util.cc (right):

https://codereview.chromium.org/1511103003/diff/230001/extensions/common/test...
extensions/common/test_util.cc:17: return
std::move(builder.SetManifest(DictionaryBuilder()
On 2015/12/18 00:31:58, limasdf wrote:
> On 2015/12/17 22:22:02, danakj (behind on reviews) wrote:
> > nit: it might be easier to read these if you wrote it as
> > 
> >   builder.SetManifest(....);
> >   return std::move(builder);
> 
> Thank you. |builder| is rvalue, just returning without std::move.
> 
> builder.SetManifest(....);
> return builder;

Ah, right return will make it an rvalue :) Thanks!

[commit-bot: I haz the power, 2015-12-18 01:36:25]

Description was changed from

==========
Use rvalue reference instead of ExtensionBuilder::pass()

C++ 11 enables rvalue reference with std::move() so that removing legacy
ListBuilder::Pass().

BUG=563649
TBR=thankis@chromium.org
==========

to

==========
Use rvalue reference instead of ExtensionBuilder::pass()

C++ 11 enables rvalue reference with std::move() so that removing legacy
ListBuilder::Pass().

BUG=563649
TBR=thankis@chromium.org
==========

[commit-bot: I haz the power, 2015-12-18 01:36:26]

Committed patchset #5 (id:250001)

[commit-bot: I haz the power, 2015-12-18 01:37:26]

Description was changed from

==========
Use rvalue reference instead of ExtensionBuilder::pass()

C++ 11 enables rvalue reference with std::move() so that removing legacy
ListBuilder::Pass().

BUG=563649
TBR=thankis@chromium.org
==========

to

==========
Use rvalue reference instead of ExtensionBuilder::pass()

C++ 11 enables rvalue reference with std::move() so that removing legacy
ListBuilder::Pass().

BUG=563649
TBR=thankis@chromium.org

Committed: https://crrev.com/704a902182dd1ba3b315563a6a65a7eb107ece79
Cr-Commit-Position: refs/heads/master@{#365969}
==========

[commit-bot: I haz the power, 2015-12-18 01:37:27]

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