|
|
Chromium Code Reviews
DescriptionReplace deprecated base::NonThreadSafe in chrome/browser/apps/app_shim in favor of ThreadChecker.
Note to crash team: This CL is a refactor and has no intended behavior change.
This change was scripted by https://crbug.com/676387#c8 but was then amended to
favor ThreadChecker over SequenceChecker per reviewer request.
BUG=676387
This CL was uploaded by git cl split.
R=dominickn@chromium.org
Review-Url: https://codereview.chromium.org/2908973004
Cr-Commit-Position: refs/heads/master@{#475947}
Committed: https://chromium.googlesource.com/chromium/src/+/8c5a0ac597cce0b30d7a067e96b909bdfe04a4c5
Patch Set 1 #Patch Set 2 : merge with .h changes #Patch Set 3 : SequenceChecker => ThreadChecker #
Messages
Total messages: 27 (18 generated)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Hi, this is an automated change made by https://crbug.com/676387#c8 and sharded across directories for OWNERS. No human has looked at this specific output yet, see cl description for things to watch for. If it LGTY, please CQ. If it didn't pass CQ dry-run it may be a simple patch failure or an IWYU failure from having this being a partial CL... please review anyways and a human will look at the failure (and ping for re-review if anything major is required..). It is known to compile all targets on Windows at r474679... manual tweaks will likely be required to smoothen things off, bear with me! Thanks! Gab
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...)
There is an IWYU failure. This CL should be combined with crrev.com/2908283002
The CQ bit was checked by gab@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 ========== Replace deprecated base::NonThreadSafe in chrome/browser/apps/app_shim in favor of SequenceChecker. Note to crash team: This CL is a refactor and has no intended behavior change. This change was scripted by https://crbug.com/676387#c8. Note-worthy for the reviewer: * SequenceChecker enforces thread-safety but not thread-affinity! If the classes that were updated are thread-affine (use thread local storage or a third-party API that does) they should be migrated to ThreadChecker instead. * ~NonThreadSafe() used to implcitly check in its destructor ~Sequence/ThreadChecker() doesn't by design. To keep this CL a no-op, an explicit check was added to the destructor of migrated classes. * NonThreadSafe used to provide access to subclasses, as such the |sequence_checker_| member was made protected rather than private where necessary. BUG=676387 This CL was uploaded by git cl split. R=dominickn@chromium.org ========== to ========== Replace deprecated base::NonThreadSafe in chrome/browser/apps/app_shim in favor of SequenceChecker. Note to crash team: This CL is a refactor and has no intended behavior change. This change was scripted by https://crbug.com/676387#c8. Note-worthy for the reviewer: * SequenceChecker enforces thread-safety but not thread-affinity! If the classes that were updated are thread-affine (use thread local storage or a third-party API that does) they should be migrated to ThreadChecker instead. * ~NonThreadSafe() used to implcitly check in its destructor ~Sequence/ThreadChecker() doesn't by design. To keep this CL a no-op, an explicit check was added to the destructor of migrated classes. * NonThreadSafe used to provide access to subclasses, as such the |sequence_checker_| member was made protected rather than private where necessary. BUG=676387 This CL was uploaded by git cl split. R=dominickn@chromium.org ==========
On 2017/05/30 02:56:12, dominickn wrote: > There is an IWYU failure. This CL should be combined with crrev.com/2908283002 Indeed, fixed. Not sure why the script did this (it sometimes happens with per-file OWNERS but that's isn't the case here... CC +fdoray: weird result of git cl split, seems to be unique in its kind...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Having thought about this class a little, it really needs to be thread-affine to the UI thread. It receives events from the IO thread and runs things in response, making the assumption that it is on the UI thread. It shouldn't live elsewhere. I think that means this class should be migrated to ThreadChecker rather than SequenceChecker.
The CQ bit was checked by gab@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...
On 2017/05/30 04:03:38, dominickn wrote: > Having thought about this class a little, it really needs to be thread-affine to > the UI thread. It receives events from the IO thread and runs things in > response, making the assumption that it is on the UI thread. It shouldn't live > elsewhere. > > I think that means this class should be migrated to ThreadChecker rather than > SequenceChecker. Done.
Description was changed from ========== Replace deprecated base::NonThreadSafe in chrome/browser/apps/app_shim in favor of SequenceChecker. Note to crash team: This CL is a refactor and has no intended behavior change. This change was scripted by https://crbug.com/676387#c8. Note-worthy for the reviewer: * SequenceChecker enforces thread-safety but not thread-affinity! If the classes that were updated are thread-affine (use thread local storage or a third-party API that does) they should be migrated to ThreadChecker instead. * ~NonThreadSafe() used to implcitly check in its destructor ~Sequence/ThreadChecker() doesn't by design. To keep this CL a no-op, an explicit check was added to the destructor of migrated classes. * NonThreadSafe used to provide access to subclasses, as such the |sequence_checker_| member was made protected rather than private where necessary. BUG=676387 This CL was uploaded by git cl split. R=dominickn@chromium.org ========== to ========== Replace deprecated base::NonThreadSafe in chrome/browser/apps/app_shim in favor of ThreadChecker. Note to crash team: This CL is a refactor and has no intended behavior change. This change was scripted by https://crbug.com/676387#c8 but was then amended to favor ThreadChecker over SequenceChecker per reviewer request. BUG=676387 This CL was uploaded by git cl split. R=dominickn@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % updating the subject. Thanks!
On 2017/05/30 23:49:55, dominickn wrote: > lgtm % updating the subject. Thanks! done, thanks
The CQ bit was checked by gab@chromium.org
The CQ bit was checked by gab@chromium.org
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": 40001, "attempt_start_ts": 1496248677840410,
"parent_rev": "54cc3544186964efd2ac73c3a66a367203c75744", "commit_rev":
"8c5a0ac597cce0b30d7a067e96b909bdfe04a4c5"}
Message was sent while issue was closed.
Description was changed from ========== Replace deprecated base::NonThreadSafe in chrome/browser/apps/app_shim in favor of ThreadChecker. Note to crash team: This CL is a refactor and has no intended behavior change. This change was scripted by https://crbug.com/676387#c8 but was then amended to favor ThreadChecker over SequenceChecker per reviewer request. BUG=676387 This CL was uploaded by git cl split. R=dominickn@chromium.org ========== to ========== Replace deprecated base::NonThreadSafe in chrome/browser/apps/app_shim in favor of ThreadChecker. Note to crash team: This CL is a refactor and has no intended behavior change. This change was scripted by https://crbug.com/676387#c8 but was then amended to favor ThreadChecker over SequenceChecker per reviewer request. BUG=676387 This CL was uploaded by git cl split. R=dominickn@chromium.org Review-Url: https://codereview.chromium.org/2908973004 Cr-Commit-Position: refs/heads/master@{#475947} Committed: https://chromium.googlesource.com/chromium/src/+/8c5a0ac597cce0b30d7a067e96b9... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/8c5a0ac597cce0b30d7a067e96b9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
