Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2846893003:
Used a single class for SharedMemoryHandle. (Closed)
DescriptionUsed a single class for SharedMemoryHandle.
This CL is a refactor with no intended behavior change.
Now that the class is defined on all platforms, make it a single class rather
than 3 separate classes. This allows the removal of some duplicate code in the
public interface.
BUG=713763
Review-Url: https://codereview.chromium.org/2846893003
Cr-Commit-Position: refs/heads/master@{#468039}
Committed: https://chromium.googlesource.com/chromium/src/+/d2bc65e52a5d63520f974b98c5efa863b116f4a0
Patch Set 1 #Patch Set 2 : clang format. #
Total comments: 3
Patch Set 3 : Rebase. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 29 (23 generated)
The CQ bit was checked by erikchen@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 ========== Used a shared class for shared memory handle. BUG= ========== to ========== Used a single class for SharedMemoryHandle. Now that the class is defined on all platforms, make it a single class rather than 3 separate classes. This allows the removal of some duplicate code in the public interface. BUG=713763 ==========
Description was changed from ========== Used a single class for SharedMemoryHandle. Now that the class is defined on all platforms, make it a single class rather than 3 separate classes. This allows the removal of some duplicate code in the public interface. BUG=713763 ========== to ========== Used a single class for SharedMemoryHandle. This CL is a refactor with no intended behavior change. Now that the class is defined on all platforms, make it a single class rather than 3 separate classes. This allows the removal of some duplicate code in the public interface. BUG=713763 ==========
erikchen@chromium.org changed reviewers: + thakis@chromium.org
thakis: Please review.
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_...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm (with green bots), thanks https://codereview.chromium.org/2846893003/diff/20001/base/memory/shared_memo... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/2846893003/diff/20001/base/memory/shared_memo... base/memory/shared_memory_handle.h:26: namespace base { Maybe add an #if defined(OS_IOS) #error at the top; previously this used to do nothing on iOS
https://codereview.chromium.org/2846893003/diff/20001/base/memory/shared_memo... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/2846893003/diff/20001/base/memory/shared_memo... base/memory/shared_memory_handle.h:26: namespace base { On 2017/04/27 19:50:09, Nico wrote: > Maybe add an #if defined(OS_IOS) #error at the top; previously this used to do > nothing on iOS There's no behavior change here. We've always had SharedMemoryHandle on iOS, which has always used the POSIX version instead of the Mach version.
https://codereview.chromium.org/2846893003/diff/20001/base/memory/shared_memo... File base/memory/shared_memory_handle.h (right): https://codereview.chromium.org/2846893003/diff/20001/base/memory/shared_memo... base/memory/shared_memory_handle.h:26: namespace base { On 2017/04/27 19:54:28, erikchen wrote: > On 2017/04/27 19:50:09, Nico wrote: > > Maybe add an #if defined(OS_IOS) #error at the top; previously this used to do > > nothing on iOS > > There's no behavior change here. We've always had SharedMemoryHandle on iOS, > which has always used the POSIX version instead of the Mach version. Are you sure? The LHS has #if defined(OS_POSIX) && !(defined(OS_MACOSX) && !defined(OS_IOS)) // posix #elif defined(OS_WIN) // win #else // mac #endif ...oh, you're right. Sorry.
The CQ bit was checked by erikchen@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_asan_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 erikchen@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 erikchen@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: This issue passed the CQ dry run.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2846893003/#ps40001 (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": 40001, "attempt_start_ts": 1493399621937200,
"parent_rev": "1d92f015d0b85e071f0040c53ccd8f145b55e4c1", "commit_rev":
"d2bc65e52a5d63520f974b98c5efa863b116f4a0"}
Message was sent while issue was closed.
Description was changed from ========== Used a single class for SharedMemoryHandle. This CL is a refactor with no intended behavior change. Now that the class is defined on all platforms, make it a single class rather than 3 separate classes. This allows the removal of some duplicate code in the public interface. BUG=713763 ========== to ========== Used a single class for SharedMemoryHandle. This CL is a refactor with no intended behavior change. Now that the class is defined on all platforms, make it a single class rather than 3 separate classes. This allows the removal of some duplicate code in the public interface. BUG=713763 Review-Url: https://codereview.chromium.org/2846893003 Cr-Commit-Position: refs/heads/master@{#468039} Committed: https://chromium.googlesource.com/chromium/src/+/d2bc65e52a5d63520f974b98c5ef... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d2bc65e52a5d63520f974b98c5ef... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
