Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2325623003:
Replace IDMap with std::unordered_map in ResourceManager (Closed)
DescriptionReplace IDMap with std::unordered_map in ResourceManager
BUG=
Committed: https://crrev.com/9daf1acec77f33098c1c5b58e72f71056c21a042
Cr-Commit-Position: refs/heads/master@{#418739}
Patch Set 1 #
Total comments: 8
Patch Set 2 : address comments #Patch Set 3 : nits #
Total comments: 15
Patch Set 4 : address comments #
Total comments: 1
Patch Set 5 : fix GetResource for certain dynamic resources #
Total comments: 2
Patch Set 6 : nit #
Depends on Patchset: Messages
Total messages: 26 (6 generated)
mdjones@chromium.org changed reviewers: + aelias@chromium.org, dtrainor@chromium.org
ptal
https://codereview.chromium.org/2325623003/diff/1/ui/android/resources/resour... File ui/android/resources/resource_manager_impl.cc (right): https://codereview.chromium.org/2325623003/diff/1/ui/android/resources/resour... ui/android/resources/resource_manager_impl.cc:108: Resource* tinted_resource = new Resource(); Can you switch this local variable to a unique_ptr<> too? We should never have a raw pointer control lifetime, raw pointers should only be used for passive readers. Likewise for the two preexisting uses of "new" in this file.
https://codereview.chromium.org/2325623003/diff/1/ui/android/resources/resour... File ui/android/resources/resource_manager_impl.cc (right): https://codereview.chromium.org/2325623003/diff/1/ui/android/resources/resour... ui/android/resources/resource_manager_impl.cc:248: CrushedSpriteResource* resource = new CrushedSpriteResource( +1 to what Alex said. Just use MakeUnique<>(...) for these. https://codereview.chromium.org/2325623003/diff/1/ui/android/resources/resour... ui/android/resources/resource_manager_impl.cc:254: crushed_sprite_resources_[bitmap_res_id] = Indenting looks off https://codereview.chromium.org/2325623003/diff/1/ui/android/resources/resour... File ui/android/resources/resource_manager_impl.h (right): https://codereview.chromium.org/2325623003/diff/1/ui/android/resources/resour... ui/android/resources/resource_manager_impl.h:88: typedef std::unordered_map<int, std::unique_ptr<Resource> > ResourceMap; move to using?
https://codereview.chromium.org/2325623003/diff/1/ui/android/resources/resour... File ui/android/resources/resource_manager_impl.cc (right): https://codereview.chromium.org/2325623003/diff/1/ui/android/resources/resour... ui/android/resources/resource_manager_impl.cc:108: Resource* tinted_resource = new Resource(); On 2016/09/08 22:05:01, aelias wrote: > Can you switch this local variable to a unique_ptr<> too? We should never have > a raw pointer control lifetime, raw pointers should only be used for passive > readers. Likewise for the two preexisting uses of "new" in this file. Done. https://codereview.chromium.org/2325623003/diff/1/ui/android/resources/resour... ui/android/resources/resource_manager_impl.cc:248: CrushedSpriteResource* resource = new CrushedSpriteResource( On 2016/09/09 06:11:32, David Trainor wrote: > +1 to what Alex said. Just use MakeUnique<>(...) for these. Done. https://codereview.chromium.org/2325623003/diff/1/ui/android/resources/resour... ui/android/resources/resource_manager_impl.cc:254: crushed_sprite_resources_[bitmap_res_id] = On 2016/09/09 06:11:32, David Trainor wrote: > Indenting looks off Done. https://codereview.chromium.org/2325623003/diff/1/ui/android/resources/resour... File ui/android/resources/resource_manager_impl.h (right): https://codereview.chromium.org/2325623003/diff/1/ui/android/resources/resour... ui/android/resources/resource_manager_impl.h:88: typedef std::unordered_map<int, std::unique_ptr<Resource> > ResourceMap; On 2016/09/09 06:11:32, David Trainor wrote: > move to using? Done.
Looks decent, just a bunch more minor comments: https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... File ui/android/resources/resource_manager_impl.cc (right): https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... ui/android/resources/resource_manager_impl.cc:70: if (resources_[res_type].find(res_id) != resources_[res_type].end()) { nit: no braces for 1-line into 1-line if statements in C++ style https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... ui/android/resources/resource_manager_impl.cc:105: if (resource_map->find(res_id) != resource_map->end()) Please put the result of the find in a local variable and use it below. https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... ui/android/resources/resource_manager_impl.cc:182: if (resources_[res_type].find(res_id) == resources_[res_type].end()) { Please put the result of the find in a local variable and use it below. https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... ui/android/resources/resource_manager_impl.cc:286: if (crushed_sprite_resources_.find(bitmap_res_id) Please put the result of the find in a local variable and use it below. https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... File ui/android/resources/resource_manager_impl.h (right): https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... ui/android/resources/resource_manager_impl.h:88: using ResourceMap = std::unordered_map<int, std::unique_ptr<Resource> >; Please use "AndroidResourceType" instead of raw int. https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... ui/android/resources/resource_manager_impl.h:90: std::unordered_map<int, std::unique_ptr<CrushedSpriteResource> >; nit: no need for space between ">" characters in C++11 https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... ui/android/resources/resource_manager_impl.h:92: std::unordered_map<int, std::unique_ptr<ResourceMap> >; Please use "SkColor" instead of raw int.
https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... File ui/android/resources/resource_manager_impl.h (right): https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... ui/android/resources/resource_manager_impl.h:88: using ResourceMap = std::unordered_map<int, std::unique_ptr<Resource> >; On 2016/09/09 at 21:50:05, aelias wrote: > Please use "AndroidResourceType" instead of raw int. Sorry, I realized this is not correct. It looks like there is no typedef for this ID space (these are not UIResourceIDs), unfortunately. Not sure it's worth defining one since these IDs are mainly used by Java anyway...
https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... File ui/android/resources/resource_manager_impl.cc (right): https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... ui/android/resources/resource_manager_impl.cc:70: if (resources_[res_type].find(res_id) != resources_[res_type].end()) { On 2016/09/09 21:50:05, aelias wrote: > nit: no braces for 1-line into 1-line if statements in C++ style Done. https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... ui/android/resources/resource_manager_impl.cc:105: if (resource_map->find(res_id) != resource_map->end()) On 2016/09/09 21:50:05, aelias wrote: > Please put the result of the find in a local variable and use it below. Done. https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... ui/android/resources/resource_manager_impl.cc:182: if (resources_[res_type].find(res_id) == resources_[res_type].end()) { On 2016/09/09 21:50:05, aelias wrote: > Please put the result of the find in a local variable and use it below. Done. https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... ui/android/resources/resource_manager_impl.cc:286: if (crushed_sprite_resources_.find(bitmap_res_id) On 2016/09/09 21:50:05, aelias wrote: > Please put the result of the find in a local variable and use it below. Done. https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... File ui/android/resources/resource_manager_impl.h (right): https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... ui/android/resources/resource_manager_impl.h:88: using ResourceMap = std::unordered_map<int, std::unique_ptr<Resource> >; On 2016/09/09 21:59:45, aelias wrote: > On 2016/09/09 at 21:50:05, aelias wrote: > > Please use "AndroidResourceType" instead of raw int. > > Sorry, I realized this is not correct. It looks like there is no typedef for > this ID space (these are not UIResourceIDs), unfortunately. Not sure it's worth > defining one since these IDs are mainly used by Java anyway... At the very least it would help with the understanding of the code, but this might be the only use of it. https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... ui/android/resources/resource_manager_impl.h:90: std::unordered_map<int, std::unique_ptr<CrushedSpriteResource> >; On 2016/09/09 21:50:05, aelias wrote: > nit: no need for space between ">" characters in C++11 Done. https://codereview.chromium.org/2325623003/diff/40001/ui/android/resources/re... ui/android/resources/resource_manager_impl.h:92: std::unordered_map<int, std::unique_ptr<ResourceMap> >; On 2016/09/09 21:50:05, aelias wrote: > Please use "SkColor" instead of raw int. Done. https://codereview.chromium.org/2325623003/diff/60001/ui/android/resources/re... File ui/android/resources/resource_manager_impl.cc (right): https://codereview.chromium.org/2325623003/diff/60001/ui/android/resources/re... ui/android/resources/resource_manager_impl.cc:186: resources_[res_type][res_id] = base::MakeUnique<Resource>(); I think you wanted my to use "item" here to reset the value stored at that location (and update it further down), right? I was having a lot of trouble with the ownership semantics here.
lgtm
Addressed one further issue I found when using dynamic resources. The way unordered_map works, looking up an item via resources_[a][b] will put an empty object in that position. For some dynamic resources, the resource may not be ready/created by the time it is requested; the code in GetResource assumed this would always be the case.
On 2016/09/14 at 17:10:35, mdjones wrote: > Addressed one further issue I found when using dynamic resources. The way unordered_map works, looking up an item via resources_[a][b] will put an empty object in that position. For some dynamic resources, the resource may not be ready/created by the time it is requested; the code in GetResource assumed this would always be the case. OK, thanks for catching that. How about stopping the use [a][b] in the first place (use for example find() instead) instead of compensating for empty objects after the fact?
On 2016/09/14 17:59:01, aelias wrote: > On 2016/09/14 at 17:10:35, mdjones wrote: > > Addressed one further issue I found when using dynamic resources. The way > unordered_map works, looking up an item via resources_[a][b] will put an empty > object in that position. For some dynamic resources, the resource may not be > ready/created by the time it is requested; the code in GetResource assumed this > would always be the case. > > OK, thanks for catching that. How about stopping the use [a][b] in the first > place (use for example find() instead) instead of compensating for empty objects > after the fact? That's how the problem is currently solved. I run find again to make sure the resource is actually there before attempting to access it.
On 2016/09/14 17:59:01, aelias wrote: > On 2016/09/14 at 17:10:35, mdjones wrote: > > Addressed one further issue I found when using dynamic resources. The way > unordered_map works, looking up an item via resources_[a][b] will put an empty > object in that position. For some dynamic resources, the resource may not be > ready/created by the time it is requested; the code in GetResource assumed this > would always be the case. > > OK, thanks for catching that. How about stopping the use [a][b] in the first > place (use for example find() instead) instead of compensating for empty objects > after the fact? That's how the problem is currently solved. I run find again to make sure the resource is actually there before attempting to access it.
OK, sorry it took me a few minutes to understand what the problem was. lgtm modulo minor comment: https://codereview.chromium.org/2325623003/diff/80001/ui/android/resources/re... File ui/android/resources/resource_manager_impl.cc (right): https://codereview.chromium.org/2325623003/diff/80001/ui/android/resources/re... ui/android/resources/resource_manager_impl.cc:70: Resource* resource = nullptr; Could you delete this variable and just do: std::unordered_map<int, std::unique_ptr<Resource>>::iterator item = resources_[res_type].find(res_id); if (item == resources_[res_type].end() || res_type == ANDROID_RESOURCE_TYPE_DYNAMIC || res_type == ANDROID_RESOURCE_TYPE_DYNAMIC_BITMAP) { RequestResourceFromJava(res_type, res_id); item = resources_[res_type].find(res_id); // Check if the resource has been added (some dynamic may not have been). if (item == resources_[res_type].end()) return nullptr; } return item->second.get();
https://codereview.chromium.org/2325623003/diff/80001/ui/android/resources/re... File ui/android/resources/resource_manager_impl.cc (right): https://codereview.chromium.org/2325623003/diff/80001/ui/android/resources/re... ui/android/resources/resource_manager_impl.cc:70: Resource* resource = nullptr; On 2016/09/14 20:14:37, aelias wrote: > Could you delete this variable and just do: > > std::unordered_map<int, std::unique_ptr<Resource>>::iterator item = > resources_[res_type].find(res_id); > if (item == resources_[res_type].end() || > res_type == ANDROID_RESOURCE_TYPE_DYNAMIC || > res_type == ANDROID_RESOURCE_TYPE_DYNAMIC_BITMAP) { > RequestResourceFromJava(res_type, res_id); > item = resources_[res_type].find(res_id); > // Check if the resource has been added (some dynamic may not have been). > if (item == resources_[res_type].end()) > return nullptr; > } > > return item->second.get(); Done.
lgtm!
The CQ bit was checked by mdjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org Link to the patchset: https://codereview.chromium.org/2325623003/#ps100001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by mdjones@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Replace IDMap with std::unordered_map in ResourceManager BUG= ========== to ========== Replace IDMap with std::unordered_map in ResourceManager BUG= Committed: https://crrev.com/9daf1acec77f33098c1c5b58e72f71056c21a042 Cr-Commit-Position: refs/heads/master@{#418739} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9daf1acec77f33098c1c5b58e72f71056c21a042 Cr-Commit-Position: refs/heads/master@{#418739} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
