Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2699323002:
Restrict cross-thread access to gfx::Image and gfx::Font in ResourceBundle (Closed)
DescriptionRestrict cross-thread access to gfx::Image and gfx::Font in ResourceBundle
Since gfx::Image and gfx::Font are not thread safe, cached images and
fonts in ResourceBundle can be used only on UI thread, regardless of
the protection by ResourceBundle::images_and_fonts_lock_.
This CL removes the lock and adds DCHECK to limit the access to UI thread.
BUG=688072, 468010
Review-Url: https://codereview.chromium.org/2699323002
Cr-Commit-Position: refs/heads/master@{#452407}
Committed: https://chromium.googlesource.com/chromium/src/+/645ad78c4d429cda9591f9be1486b911d7076f59
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : cros fix #Patch Set 4 : ReadIconThreadID #Patch Set 5 : rebase #
Total comments: 6
Patch Set 6 : rebase #Patch Set 7 : count -> iterator. comment fix. #Patch Set 8 : rebase #
Depends on Patchset: Messages
Total messages: 41 (35 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Restrict cross-thread access to gfx::Image and gfx::Font in ResourceBundle ========== to ========== Restrict cross-thread access to gfx::Image and gfx::Font in ResourceBundle https://codereview.chromium.org/2701183002/ https://codereview.chromium.org/2699403003/ https://codereview.chromium.org/2707753004/ ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Restrict cross-thread access to gfx::Image and gfx::Font in ResourceBundle https://codereview.chromium.org/2701183002/ https://codereview.chromium.org/2699403003/ https://codereview.chromium.org/2707753004/ ========== to ========== Restrict cross-thread access to gfx::Image and gfx::Font in ResourceBundle Since gfx::Image and gfx::Font are not thread safe, cached images and fonts in ResourceBundle can be used only on UI thread, regardless of the protection by ResourceBundle::images_and_fonts_lock_. This CL removes the lock and adds DCHECK to limit the access to UI thread. ==========
Description was changed from ========== Restrict cross-thread access to gfx::Image and gfx::Font in ResourceBundle Since gfx::Image and gfx::Font are not thread safe, cached images and fonts in ResourceBundle can be used only on UI thread, regardless of the protection by ResourceBundle::images_and_fonts_lock_. This CL removes the lock and adds DCHECK to limit the access to UI thread. ========== to ========== Restrict cross-thread access to gfx::Image and gfx::Font in ResourceBundle Since gfx::Image and gfx::Font are not thread safe, cached images and fonts in ResourceBundle can be used only on UI thread, regardless of the protection by ResourceBundle::images_and_fonts_lock_. This CL removes the lock and adds DCHECK to limit the access to UI thread. BUG=688072, 468010 ==========
tzik@chromium.org changed reviewers: + sky@chromium.org, yukishiino@chromium.org
PTAL
LGTM. https://codereview.chromium.org/2699323002/diff/80001/ui/base/resource/resour... File ui/base/resource/resource_bundle.cc (right): https://codereview.chromium.org/2699323002/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle.cc:777: void ResourceBundle::InitDefaultFontList() { You may want to add DCHECK here, too. https://codereview.chromium.org/2699323002/diff/80001/ui/base/resource/resour... File ui/base/resource/resource_bundle.h (right): https://codereview.chromium.org/2699323002/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle.h:417: // Must be accessed only while holding |images_and_fonts_lock_|. Could you update this comment, too?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
LGTM https://codereview.chromium.org/2699323002/diff/80001/ui/base/resource/resour... File ui/base/resource/resource_bundle_mac.mm (right): https://codereview.chromium.org/2699323002/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle_mac.mm:95: if (images_.count(resource_id)) { Use an iterator to avoid the repeated access? e.g. auto iter = images_.find(resource_id); if (iter != images_.end()) { ... }
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
https://codereview.chromium.org/2699323002/diff/80001/ui/base/resource/resour... File ui/base/resource/resource_bundle.cc (right): https://codereview.chromium.org/2699323002/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle.cc:777: void ResourceBundle::InitDefaultFontList() { On 2017/02/21 06:23:54, Yuki wrote: > You may want to add DCHECK here, too. Done. https://codereview.chromium.org/2699323002/diff/80001/ui/base/resource/resour... File ui/base/resource/resource_bundle.h (right): https://codereview.chromium.org/2699323002/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle.h:417: // Must be accessed only while holding |images_and_fonts_lock_|. On 2017/02/21 06:23:54, Yuki wrote: > Could you update this comment, too? Done. https://codereview.chromium.org/2699323002/diff/80001/ui/base/resource/resour... File ui/base/resource/resource_bundle_mac.mm (right): https://codereview.chromium.org/2699323002/diff/80001/ui/base/resource/resour... ui/base/resource/resource_bundle_mac.mm:95: if (images_.count(resource_id)) { On 2017/02/21 18:01:05, sky wrote: > Use an iterator to avoid the repeated access? e.g. > > auto iter = images_.find(resource_id); > if (iter != images_.end()) { > ... > } Done.
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: 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 tzik@chromium.org
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, yukishiino@chromium.org Link to the patchset: https://codereview.chromium.org/2699323002/#ps140001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487824281332460,
"parent_rev": "86f65d3bc184fa08c9aec92d43710d720ed39b69", "commit_rev":
"645ad78c4d429cda9591f9be1486b911d7076f59"}
Message was sent while issue was closed.
Description was changed from ========== Restrict cross-thread access to gfx::Image and gfx::Font in ResourceBundle Since gfx::Image and gfx::Font are not thread safe, cached images and fonts in ResourceBundle can be used only on UI thread, regardless of the protection by ResourceBundle::images_and_fonts_lock_. This CL removes the lock and adds DCHECK to limit the access to UI thread. BUG=688072, 468010 ========== to ========== Restrict cross-thread access to gfx::Image and gfx::Font in ResourceBundle Since gfx::Image and gfx::Font are not thread safe, cached images and fonts in ResourceBundle can be used only on UI thread, regardless of the protection by ResourceBundle::images_and_fonts_lock_. This CL removes the lock and adds DCHECK to limit the access to UI thread. BUG=688072, 468010 Review-Url: https://codereview.chromium.org/2699323002 Cr-Commit-Position: refs/heads/master@{#452407} Committed: https://chromium.googlesource.com/chromium/src/+/645ad78c4d429cda9591f9be1486... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/645ad78c4d429cda9591f9be1486... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
