Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2841433003:
Do not touch ui::ResourceBundle on non-UI thread in web_app_mac.mm (Closed)
DescriptionDo not touch ui::ResourceBundle on non-UI thread in web_app_mac.mm
gfx::ImageStorage has a non-thread-safe ref count, and gfx::Image in
ui::ResourceBundle needs to be copied or destroyed on the UI thread.
This CL removes an access to gfx::Image from non-UI thread in web_app_mac.mm,
to avoid a data race.
BUG=714209, 714280
Review-Url: https://codereview.chromium.org/2841433003
Cr-Commit-Position: refs/heads/master@{#468712}
Committed: https://chromium.googlesource.com/chromium/src/+/08fb0ca7077abf9c7ddfce597a4015244b16d434
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : . #Patch Set 4 : fix #Patch Set 5 : . #Patch Set 6 : fix #Patch Set 7 : . #Patch Set 8 : . #Patch Set 9 : . #
Total comments: 6
Patch Set 10 : unify loop style #Patch Set 11 : fix #Messages
Total messages: 81 (76 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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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...
Patchset #2 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #2 (id:80001) has been deleted
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Patchset #2 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:120001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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 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 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 ========== Do not touch ui::ResourceBundle on non-UI thread in web_app_mac.mm BUG= ========== to ========== Do not touch ui::ResourceBundle on non-UI thread in web_app_mac.mm BUG=714209 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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 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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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 ========== Do not touch ui::ResourceBundle on non-UI thread in web_app_mac.mm BUG=714209 ========== to ========== Do not touch ui::ResourceBundle on non-UI thread in web_app_mac.mm gfx::ImageStorage has a non-thread-safe ref count, and gfx::Image in ui::ResourceBundle needs to be copied or destroyed on the UI thread. This CL removes an access to gfx::Image from non-UI thread in web_app_mac.mm, to avoid a data race. BUG=714209 ==========
tzik@chromium.org changed reviewers: + tapted@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can you add 714280 to BUG= as well? then lgtm - just minor tweaks There's more to do from comments in 714280 but I'm happy to own that, and this seems to be interrupting some developer workflows. (Didn't manage to get to 714280 last week because of a vacation, so thanks for grabbing this!) https://codereview.chromium.org/2841433003/diff/270001/chrome/browser/web_app... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/2841433003/diff/270001/chrome/browser/web_app... chrome/browser/web_applications/web_app_mac.mm:394: int resource_ids[] = { nit: constexpr int kResourceIDs or inline the array literal in the for loop below https://codereview.chromium.org/2841433003/diff/270001/chrome/browser/web_app... chrome/browser/web_applications/web_app_mac.mm:417: auto found = images->find(IDR_APPS_FOLDER_16); for (int small_image_id : {IDR_APPS_FOLDER_16, IDR_APPS_FOLDER_32}) { .. https://codereview.chromium.org/2841433003/diff/270001/chrome/browser/web_app... chrome/browser/web_applications/web_app_mac.mm:433: for (size_t i = 0; i < arraysize(kBrandResourceIds); ++i) { and I guess for (int brand_image_id : {IDR_APPS_FOLDER_OVERLAY_128, IDR_APPS_FOLDER_OVERLAY_512}) { .. so things are consistent
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 ========== Do not touch ui::ResourceBundle on non-UI thread in web_app_mac.mm gfx::ImageStorage has a non-thread-safe ref count, and gfx::Image in ui::ResourceBundle needs to be copied or destroyed on the UI thread. This CL removes an access to gfx::Image from non-UI thread in web_app_mac.mm, to avoid a data race. BUG=714209 ========== to ========== Do not touch ui::ResourceBundle on non-UI thread in web_app_mac.mm gfx::ImageStorage has a non-thread-safe ref count, and gfx::Image in ui::ResourceBundle needs to be copied or destroyed on the UI thread. This CL removes an access to gfx::Image from non-UI thread in web_app_mac.mm, to avoid a data race. BUG=714209, 714280 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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...
https://codereview.chromium.org/2841433003/diff/270001/chrome/browser/web_app... File chrome/browser/web_applications/web_app_mac.mm (right): https://codereview.chromium.org/2841433003/diff/270001/chrome/browser/web_app... chrome/browser/web_applications/web_app_mac.mm:394: int resource_ids[] = { On 2017/05/01 08:43:45, tapted wrote: > nit: constexpr int kResourceIDs > > or inline the array literal in the for loop below Done. Inlined it into the loop. https://codereview.chromium.org/2841433003/diff/270001/chrome/browser/web_app... chrome/browser/web_applications/web_app_mac.mm:417: auto found = images->find(IDR_APPS_FOLDER_16); On 2017/05/01 08:43:45, tapted wrote: > for (int small_image_id : {IDR_APPS_FOLDER_16, IDR_APPS_FOLDER_32}) { > .. Done. https://codereview.chromium.org/2841433003/diff/270001/chrome/browser/web_app... chrome/browser/web_applications/web_app_mac.mm:433: for (size_t i = 0; i < arraysize(kBrandResourceIds); ++i) { On 2017/05/01 08:43:45, tapted wrote: > and I guess > > for (int brand_image_id : {IDR_APPS_FOLDER_OVERLAY_128, > IDR_APPS_FOLDER_OVERLAY_512}) { > .. > > so things are consistent Done.
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
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2841433003/#ps310001 (title: "fix")
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": 310001, "attempt_start_ts": 1493749585500850,
"parent_rev": "ea2deba79c72cbf4bbe74b2f1783d33fdfbbd96b", "commit_rev":
"08fb0ca7077abf9c7ddfce597a4015244b16d434"}
Message was sent while issue was closed.
Description was changed from ========== Do not touch ui::ResourceBundle on non-UI thread in web_app_mac.mm gfx::ImageStorage has a non-thread-safe ref count, and gfx::Image in ui::ResourceBundle needs to be copied or destroyed on the UI thread. This CL removes an access to gfx::Image from non-UI thread in web_app_mac.mm, to avoid a data race. BUG=714209, 714280 ========== to ========== Do not touch ui::ResourceBundle on non-UI thread in web_app_mac.mm gfx::ImageStorage has a non-thread-safe ref count, and gfx::Image in ui::ResourceBundle needs to be copied or destroyed on the UI thread. This CL removes an access to gfx::Image from non-UI thread in web_app_mac.mm, to avoid a data race. BUG=714209, 714280 Review-Url: https://codereview.chromium.org/2841433003 Cr-Commit-Position: refs/heads/master@{#468712} Committed: https://chromium.googlesource.com/chromium/src/+/08fb0ca7077abf9c7ddfce597a40... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:310001) as https://chromium.googlesource.com/chromium/src/+/08fb0ca7077abf9c7ddfce597a40... |
