Convert Pass()→std::move() on Windows

Convert Pass()→std::move() on Windows

Also cleaned up a few stragglers from grepping the source: it's unclear
if these are just building with weird defines, don't build at all, only
build on obscure platforms, or something else… either way, Pass()
itself will be removed shortly after this.

BUG=557422
Committed: https://crrev.com/70c494273fa71d922405cba4dc39f5457652a2db
Cr-Commit-Position: refs/heads/master@{#378872}

[dcheng, 2016-03-02 01:44:40]

Description was changed from

==========
Formatted


Compiles

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

to

==========
Formatted


Compiles

BUG=557422
==========

[dcheng, 2016-03-02 01:44:40]

dcheng@chromium.org changed reviewers:
+ brettw@chromium.org

[dcheng, 2016-03-02 01:45:39]

Description was changed from

==========
Formatted


Compiles

BUG=557422
==========

to

==========
Convert Pass()→std::move() on Windows

Hopefully the last of it…

BUG=557422
==========

[dcheng, 2016-03-02 01:53:49]

Description was changed from

==========
Convert Pass()→std::move() on Windows

Hopefully the last of it…

BUG=557422
==========

to

==========
Convert Pass()→std::move() on Windows

Also cleaned up a few stragglers from grepping the source: it's unclear
if these are just building with weird defines, don't build at all, only
build on obscure platforms, or something else… either way, Pass()
itself will be removed shortly after this.

BUG=557422
==========

[dcheng, 2016-03-02 01:58:05]

dcheng@chromium.org changed reviewers:
+ wfh@chromium.org

[dcheng, 2016-03-02 01:59:58]

+brettw, mind stamping this? After this, I think Pass() is pretty much gone
(other than stale comments and some mojo stuff which /should/ be unrelated)

+wfh as well, for //content/common/sandbox_win.cc

https://codereview.chromium.org/1752233002/diff/20001/components/policy/core/...
File components/policy/core/common/registry_dict_win_unittest.cc (right):

https://codereview.chromium.org/1752233002/diff/20001/components/policy/core/...
components/policy/core/common/registry_dict_win_unittest.cc:23:
test_dict.SetValue("one", int_value.CreateDeepCopy());
Strictly speaking, not directly related, but since I was fixing Pass() here
already, I switched away from some of the other deprecated base::Value apis.

[dcheng, 2016-03-02 02:03:31]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-02 02:04:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1752233002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1752233002/40001

[commit-bot: I haz the power, 2016-03-02 04:01:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-02 04:01:40]

Dry run: This issue passed the CQ dry run.

[Will Harris, 2016-03-02 18:34:46]

//content/common/sandbox_win.cc lgtm

[brettw, 2016-03-02 19:09:08]

LGTM with some fixes.

I'm basing the comments below on the parameter passed to std::move being left in
a "valid but unspecified" state. I'm not sure what state that could be other
than null for these pointers, but we should code so that everything is specified
the way we want.

https://codereview.chromium.org/1752233002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_icons/status_tray_state_changer_win.cc
(right):

https://codereview.chromium.org/1752233002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_state_changer_win.cc:175:
scoped_ptr<NOTIFYITEM> rv(std::move(notify_item_));
This code is pretty suspicious.

With your change it's equivalent to return std::move(notify_item_) and it will
have the effect of leaving notify_item_ in an undefined state.

I think you want the release() call in there as per the old code to guarantee
the state of the old value.

https://codereview.chromium.org/1752233002/diff/40001/sandbox/win/src/job.cc
File sandbox/win/src/job.cc (right):

https://codereview.chromium.org/1752233002/diff/40001/sandbox/win/src/job.cc#...
sandbox/win/src/job.cc:107: return std::move(job_handle_);
Here also we need to write code so that job_handle_ is in a defined cleared
state.

https://codereview.chromium.org/1752233002/diff/40001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/1752233002/diff/40001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:818: base::win::ScopedHICON previous_icon =
std::move(window_icon_);
Ditto

https://codereview.chromium.org/1752233002/diff/40001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:824: base::win::ScopedHICON previous_icon =
std::move(app_icon_);
Ditto

[mmenke, 2016-03-02 19:13:24]

mmenke@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2016-03-02 19:13:25]

https://codereview.chromium.org/1752233002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_icons/status_tray_state_changer_win.cc
(right):

https://codereview.chromium.org/1752233002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_state_changer_win.cc:175:
scoped_ptr<NOTIFYITEM> rv(std::move(notify_item_));
On 2016/03/02 19:09:08, brettw wrote:
> This code is pretty suspicious.
> 
> With your change it's equivalent to return std::move(notify_item_) and it will
> have the effect of leaving notify_item_ in an undefined state.
> 
> I think you want the release() call in there as per the old code to guarantee
> the state of the old value.

Interesting...I was told this was fine for scoped_ptrs when I bought this up on
another of dcheng's CLs:  https://codereview.chromium.org/1545233002/ (Post
#12).  If we want to avoid this, I suspect we'll have to revisit a lot of
std::move calls.

[brettw, 2016-03-02 19:20:42]

In that code review, it was written "The standard guarantees that a move-from
std::unique_ptr will return nullptr for get()." I'm interested in seeing where
that is.

[dcheng, 2016-03-02 19:38:32]

https://codereview.chromium.org/1752233002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/status_icons/status_tray_state_changer_win.cc
(right):

https://codereview.chromium.org/1752233002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/status_icons/status_tray_state_changer_win.cc:175:
scoped_ptr<NOTIFYITEM> rv(std::move(notify_item_));
On 2016/03/02 at 19:13:25, mmenke wrote:
> On 2016/03/02 19:09:08, brettw wrote:
> > This code is pretty suspicious.
> > 
> > With your change it's equivalent to return std::move(notify_item_) and it
will
> > have the effect of leaving notify_item_ in an undefined state.
> > 
> > I think you want the release() call in there as per the old code to
guarantee
> > the state of the old value.
> 
> Interesting...I was told this was fine for scoped_ptrs when I bought this up
on another of dcheng's CLs:  https://codereview.chromium.org/1545233002/ (Post
#12).  If we want to avoid this, I suspect we'll have to revisit a lot of
std::move calls.

In general, you're right: you can't depend on the state of a moved-from object.
From [lib.types.movedfrom/1]:
Unless otherwise specified, such moved-from objects shall be placed in a valid
but unspecified state.

However, std::unique_ptr *is* one of the classes where the moved-from state is
defined. From [unique.ptr/4]:
  Additionally, u can, upon request, transfer ownership to another unique
pointer u2. Upon completion of
  such a transfer, the following postconditions hold:
  — u2.p is equal to the pre-transfer u.p,
  — u.p is equal to nullptr, and
  — if the pre-transfer u.d maintained state, such state has been transferred to
u2.d.

Since we aim for compatibility with std::unique_ptr, scoped_ptr also has the
same behavior.

[brettw, 2016-03-02 20:08:56]

Ah, so the "undefined" thing is really for the implementer of the move
constructor (which might be the compiler) but you can define things more if you
want.

In that case, in status_icons/status_tray_state_changer_win.cc we can just
return std::move(notify_item_); and delete the comment.

Still LGTM

[dcheng, 2016-03-02 21:06:47]

On 2016/03/02 at 20:08:56, brettw wrote:
> Ah, so the "undefined" thing is really for the implementer of the move
constructor (which might be the compiler) but you can define things more if you
want.
> 
> In that case, in status_icons/status_tray_state_changer_win.cc we can just
return std::move(notify_item_); and delete the comment.
> 

Ah, I misread the intent of that comment: fixed!

> Still LGTM

[dcheng, 2016-03-02 21:08:51]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-03-02 21:09:04]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org,
wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1752233002/#ps60001
(title: ".")

[commit-bot: I haz the power, 2016-03-02 21:09:40]

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

[commit-bot: I haz the power, 2016-03-02 23:20:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-02 23:21:58]

Description was changed from

==========
Convert Pass()→std::move() on Windows

Also cleaned up a few stragglers from grepping the source: it's unclear
if these are just building with weird defines, don't build at all, only
build on obscure platforms, or something else… either way, Pass()
itself will be removed shortly after this.

BUG=557422
==========

to

==========
Convert Pass()→std::move() on Windows

Also cleaned up a few stragglers from grepping the source: it's unclear
if these are just building with weird defines, don't build at all, only
build on obscure platforms, or something else… either way, Pass()
itself will be removed shortly after this.

BUG=557422

Committed: https://crrev.com/70c494273fa71d922405cba4dc39f5457652a2db
Cr-Commit-Position: refs/heads/master@{#378872}
==========

[commit-bot: I haz the power, 2016-03-02 23:22:00]

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