Switch cc to std::unordered_*.

Switch cc to std::unordered_*.

This removes all uses of base::hash_*, BASE_HASH_NAMESPACE, and
base::ScopedPtrHashMap in favor of the C++11 versions.

BUG=576864, 579229
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5877ffe39d049730251b7f49f890c820e26643fa
Cr-Commit-Position: refs/heads/master@{#371937}

[davidben, 2016-01-14 20:26:37]

Description was changed from

==========
Switch cc to std::unordered_*.

This removes all uses of base::hash_*, BASE_HASH_NAMESPACE, and
base::ScopedPtrHashMap in favor of the C++11 versions.

BUG=576864
==========

to

==========
Switch cc to std::unordered_*.

This removes all uses of base::hash_*, BASE_HASH_NAMESPACE, and
base::ScopedPtrHashMap in favor of the C++11 versions.

BUG=576864
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[danakj, 2016-01-14 21:12:31]

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

[danakj, 2016-01-14 21:12:32]

LGTM

https://codereview.chromium.org/1587283002/diff/1/cc/animation/animation_regi...
File cc/animation/animation_registrar.h (right):

https://codereview.chromium.org/1587283002/diff/1/cc/animation/animation_regi...
cc/animation/animation_registrar.h:22: typedef std::unordered_map<int,
LayerAnimationController*>
would you mind changing "typedef" to "using" when you change these?

https://codereview.chromium.org/1587283002/diff/1/cc/tiles/picture_layer_tili...
File cc/tiles/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1587283002/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling.cc:188: auto iter = pending_twin->tiles_.begin();
nit: pending_iter?

https://codereview.chromium.org/1587283002/diff/1/cc/tiles/picture_layer_tili...
File cc/tiles/picture_layer_tiling.h (right):

https://codereview.chromium.org/1587283002/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling.h:148: for (const auto& tile_pair : tiles_) {
thanks :)

https://codereview.chromium.org/1587283002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1587283002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:3293: // TODO(davidben): Is this the same as
using operator[]? The check above
You're worried for the case where the scroll_layer_id exists in the map, but the
scoped_ptr value is null? I think that won't happen. The only place the
controller is changed is here and unregister. So it either sets a pointer or
erases the key.

[davidben, 2016-01-14 21:28:19]

(Will address comments and update CL after parent commit goes through to save
myself git contortions.)

https://codereview.chromium.org/1587283002/diff/1/cc/tiles/picture_layer_tili...
File cc/tiles/picture_layer_tiling.h (right):

https://codereview.chromium.org/1587283002/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling.h:148: for (const auto& tile_pair : tiles_) {
On 2016/01/14 21:12:32, danakj wrote:
> thanks :)

All credits goes to the compiler. pairs containing scoped_ptr aren't copyable,
so it doesn't work otherwise.

https://codereview.chromium.org/1587283002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1587283002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:3293: // TODO(davidben): Is this the same as
using operator[]? The check above
On 2016/01/14 21:12:32, danakj wrote:
> You're worried for the case where the scroll_layer_id exists in the map, but
the
> scoped_ptr value is null? I think that won't happen. The only place the
> controller is changed is here and unregister. So it either sets a pointer or
> erases the key.

I'm worried about the case where scroll_layer_id exists in the map.
ScopedPtrHashMap::add acts like unordered_map::insert in that it apparently does
nothing if the key already exists, whereas assigning to operator[] will always
modify the map.

I would be pretty surprised if this code actively wants this behavior, but I
wasn't sure so left it like this to ask you about. Will switch to operator[] if
that's the desired behavior, although it is a little odd that the check on line
3291 isn't quiiiite the right check.

[danakj, 2016-01-14 21:31:54]

https://codereview.chromium.org/1587283002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1587283002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:3293: // TODO(davidben): Is this the same as
using operator[]? The check above
On 2016/01/14 21:28:19, davidben wrote:
> On 2016/01/14 21:12:32, danakj wrote:
> > You're worried for the case where the scroll_layer_id exists in the map, but
> the
> > scoped_ptr value is null? I think that won't happen. The only place the
> > controller is changed is here and unregister. So it either sets a pointer or
> > erases the key.
> 
> I'm worried about the case where scroll_layer_id exists in the map.
> ScopedPtrHashMap::add acts like unordered_map::insert in that it apparently
does
> nothing if the key already exists, whereas assigning to operator[] will always
> modify the map.
> 
> I would be pretty surprised if this code actively wants this behavior, but I
> wasn't sure so left it like this to ask you about. Will switch to operator[]
if
> that's the desired behavior, although it is a little odd that the check on
line
> 3291 isn't quiiiite the right check.

Yah it is a bit awkward. I think operator[] will be fine tho, we shouldn't have
null values in the map since the only place that sets them is here and
CreateScrollbarAnimationController won't return null (without hitting a DCHECK).

[davidben, 2016-01-19 22:31:03]

Description was changed from

==========
Switch cc to std::unordered_*.

This removes all uses of base::hash_*, BASE_HASH_NAMESPACE, and
base::ScopedPtrHashMap in favor of the C++11 versions.

BUG=576864
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Switch cc to std::unordered_*.

This removes all uses of base::hash_*, BASE_HASH_NAMESPACE, and
base::ScopedPtrHashMap in favor of the C++11 versions.

BUG=576864, 579229
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[davidben, 2016-01-21 01:33:58]

Addressed comments. Though, as I write this, it looks like the parent CL is
about to get reverted due to breaking an MSan bot somewhere...

https://codereview.chromium.org/1587283002/diff/1/cc/animation/animation_regi...
File cc/animation/animation_registrar.h (right):

https://codereview.chromium.org/1587283002/diff/1/cc/animation/animation_regi...
cc/animation/animation_registrar.h:22: typedef std::unordered_map<int,
LayerAnimationController*>
On 2016/01/14 21:12:32, danakj wrote:
> would you mind changing "typedef" to "using" when you change these?

Done. There's actually quite a lot of those...

https://codereview.chromium.org/1587283002/diff/1/cc/tiles/picture_layer_tili...
File cc/tiles/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1587283002/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling.cc:188: auto iter = pending_twin->tiles_.begin();
On 2016/01/14 21:12:32, danakj wrote:
> nit: pending_iter?

Done.

https://codereview.chromium.org/1587283002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1587283002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:3293: // TODO(davidben): Is this the same as
using operator[]? The check above
On 2016/01/14 21:31:54, danakj wrote:
> On 2016/01/14 21:28:19, davidben wrote:
> > On 2016/01/14 21:12:32, danakj wrote:
> > > You're worried for the case where the scroll_layer_id exists in the map,
but
> > the
> > > scoped_ptr value is null? I think that won't happen. The only place the
> > > controller is changed is here and unregister. So it either sets a pointer
or
> > > erases the key.
> > 
> > I'm worried about the case where scroll_layer_id exists in the map.
> > ScopedPtrHashMap::add acts like unordered_map::insert in that it apparently
> does
> > nothing if the key already exists, whereas assigning to operator[] will
always
> > modify the map.
> > 
> > I would be pretty surprised if this code actively wants this behavior, but I
> > wasn't sure so left it like this to ask you about. Will switch to operator[]
> if
> > that's the desired behavior, although it is a little odd that the check on
> line
> > 3291 isn't quiiiite the right check.
> 
> Yah it is a bit awkward. I think operator[] will be fine tho, we shouldn't
have
> null values in the map since the only place that sets them is here and
> CreateScrollbarAnimationController won't return null (without hitting a
DCHECK).

Done.

[danakj, 2016-01-21 19:37:36]

LGTM, thanks very much for fixing all the type aliases.

[davidben, 2016-01-22 21:49:23]

Description was changed from

==========
Switch cc to std::unordered_*.

This removes all uses of base::hash_*, BASE_HASH_NAMESPACE, and
base::ScopedPtrHashMap in favor of the C++11 versions.

BUG=576864, 579229
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Switch cc to std::unordered_*.

This removes all uses of base::hash_*, BASE_HASH_NAMESPACE, and
base::ScopedPtrHashMap in favor of the C++11 versions.

BUG=576864, 579229
==========

[davidben, 2016-01-22 21:49:37]

Description was changed from

==========
Switch cc to std::unordered_*.

This removes all uses of base::hash_*, BASE_HASH_NAMESPACE, and
base::ScopedPtrHashMap in favor of the C++11 versions.

BUG=576864, 579229
==========

to

==========
Switch cc to std::unordered_*.

This removes all uses of base::hash_*, BASE_HASH_NAMESPACE, and
base::ScopedPtrHashMap in favor of the C++11 versions.

BUG=576864, 579229
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[davidben, 2016-01-22 23:59:07]

The CQ bit was checked by davidben@chromium.org

[davidben, 2016-01-22 23:59:07]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1587283002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-01-23 00:02:14]

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

[commit-bot: I haz the power, 2016-01-23 01:24:42]

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

[commit-bot: I haz the power, 2016-01-23 01:24:43]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[davidben, 2016-01-27 21:47:48]

On 2016/01/23 01:24:43, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

The problem seems to be stemming from MSVC trying to emit a copy constructor for
std::pair<TileMapKey, ScopedTilePtr>, which it can't since ScopedTilePtr isn't
copyable. Why it's complaining about something private when the copy constructor
is disabled with = delete, I'm not sure. I'll keep digging, but I don't suppose
this rings a bell?

[danakj, 2016-01-27 21:50:43]

That sounds like
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...

[davidben, 2016-01-27 21:52:10]

On 2016/01/27 21:47:48, davidben (slow) wrote:
> On 2016/01/23 01:24:43, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
> 
> The problem seems to be stemming from MSVC trying to emit a copy constructor
for
> std::pair<TileMapKey, ScopedTilePtr>, which it can't since ScopedTilePtr isn't
> copyable. Why it's complaining about something private when the copy
constructor
> is disabled with = delete, I'm not sure. I'll keep digging, but I don't
suppose
> this rings a bell?

Nevermind, I think I figured it out! The containing class is DISALLOW_ASSIGN,
not DISALLOW_COPY_AND_ASSIGN. MSVC's emitting an implicit copy constructor then
and there which requires that the unordered_map be copyable. Whereas clang and
gcc are probably emitting it on-demand and nothing ever instantiates it.

[danakj, 2016-01-27 21:55:11]

On Wed, Jan 27, 2016 at 1:52 PM, <davidben@chromium.org> wrote:

>
> On 2016/01/27 21:47:48, davidben (slow) wrote:
> > On 2016/01/23 01:24:43, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
> > >
>
>http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_compile_dbg_ng/builds/135389)
> >
> > The problem seems to be stemming from MSVC trying to emit a copy
constructor
> for
> > std::pair<TileMapKey, ScopedTilePtr>, which it can't since ScopedTilePtr
isn't
> > copyable. Why it's complaining about something private when the copy
> constructor
> > is disabled with = delete, I'm not sure. I'll keep digging, but I don't
> suppose
> > this rings a bell?
>
> Nevermind, I think I figured it out! The containing class is DISALLOW_ASSIGN,
> not DISALLOW_COPY_AND_ASSIGN. MSVC's emitting an implicit copy constructor
then
> and there which requires that the unordered_map be copyable. Whereas clang
and
> gcc are probably emitting it on-demand and nothing ever instantiates it.
>
>
Yupyup! MSVC oddities. Sounds like it was a bug to not be
DISALLOW_COPY_AND_ASSIGN tho.

-- 
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.

[davidben, 2016-01-27 22:47:02]

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

[commit-bot: I haz the power, 2016-01-27 22:47:40]

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

[commit-bot: I haz the power, 2016-01-28 00:10:27]

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

[commit-bot: I haz the power, 2016-01-28 00:10:29]

Dry run: This issue passed the CQ dry run.

[davidben, 2016-01-28 00:10:53]

The CQ bit was checked by davidben@chromium.org

[davidben, 2016-01-28 00:10:53]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1587283002/#ps120001
(title: "Fix MSVC build issue")

[commit-bot: I haz the power, 2016-01-28 00:11:23]

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

[commit-bot: I haz the power, 2016-01-28 00:29:59]

Description was changed from

==========
Switch cc to std::unordered_*.

This removes all uses of base::hash_*, BASE_HASH_NAMESPACE, and
base::ScopedPtrHashMap in favor of the C++11 versions.

BUG=576864, 579229
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Switch cc to std::unordered_*.

This removes all uses of base::hash_*, BASE_HASH_NAMESPACE, and
base::ScopedPtrHashMap in favor of the C++11 versions.

BUG=576864, 579229
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-01-28 00:30:00]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-28 00:35:24]

Description was changed from

==========
Switch cc to std::unordered_*.

This removes all uses of base::hash_*, BASE_HASH_NAMESPACE, and
base::ScopedPtrHashMap in favor of the C++11 versions.

BUG=576864, 579229
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Switch cc to std::unordered_*.

This removes all uses of base::hash_*, BASE_HASH_NAMESPACE, and
base::ScopedPtrHashMap in favor of the C++11 versions.

BUG=576864, 579229
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5877ffe39d049730251b7f49f890c820e26643fa
Cr-Commit-Position: refs/heads/master@{#371937}
==========

[commit-bot: I haz the power, 2016-01-28 00:35:30]

Patchset 7 (id:??) landed as
https://crrev.com/5877ffe39d049730251b7f49f890c820e26643fa
Cr-Commit-Position: refs/heads/master@{#371937}