Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2862123002:
Pass the GUID for the SharedMemoryHandle used by base::FieldTrialList. (Closed)
DescriptionPass the GUID for the SharedMemoryHandle used by base::FieldTrialList.
SharedMemoryHandle now has a GUID used to track shared memory usage across
processes. The SharedMemoryHandle used by base::FieldTrialList is passed
manually by command line flag and Process creation options. This CL appends the
GUID to the command line flag.
BUG=713763
Review-Url: https://codereview.chromium.org/2862123002
Cr-Commit-Position: refs/heads/master@{#470700}
Committed: https://chromium.googlesource.com/chromium/src/+/2525d998c7c50bce5ca17fb934c2aeb8e27e1db0
Patch Set 1 #Patch Set 2 : Add test. #Patch Set 3 : clang format #
Total comments: 9
Patch Set 4 : Comments from asvitkine. #
Total comments: 14
Patch Set 5 : Comments from asvitkine. #
Total comments: 2
Patch Set 6 : Pass string piece by value. #
Dependent Patchsets: Messages
Total messages: 43 (27 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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.
erikchen@chromium.org changed reviewers: + asvitkine@chromium.org, avi@chromium.org
asvitkine: Please review. avi: Please review content/
lgtm https://codereview.chromium.org/2862123002/diff/30001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2862123002/diff/30001/base/metrics/field_tria... base/metrics/field_trial.cc:1121: std::getline(iss, token, ','); You use base::StringToInt, why not base::SplitString?
Thanks! Comments below. https://codereview.chromium.org/2862123002/diff/30001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2862123002/diff/30001/base/metrics/field_tria... base/metrics/field_trial.cc:838: const char* field_trial_switch_value, It's not the switch value - it's the switch name.... If you're just trying to clarify this, suggest using switch_name instead. Or if it's part of an earlier iteration that you decided to change your mind on, please revert. Either way, please make sure the .cc changes are consistent with what's in the .h - which is currently not the case with your CL. https://codereview.chromium.org/2862123002/diff/30001/base/metrics/field_tria... base/metrics/field_trial.cc:1121: std::getline(iss, token, ','); On 2017/05/05 04:30:12, Avi (ping after 24h) wrote: > You use base::StringToInt, why not base::SplitString? +1 - let's use SplitString() https://codereview.chromium.org/2862123002/diff/30001/base/metrics/field_tria... base/metrics/field_trial.cc:1154: const std::string& switch_value) { How about unifying the code for the windows and posix versions? You can pass the int fd params to both and just mention that it's not used on Windows. Then, I think the only ifdef you need to have for win/non-win is the return line. https://codereview.chromium.org/2862123002/diff/30001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2862123002/diff/30001/base/metrics/field_tria... base/metrics/field_trial.h:652: // Serialization and deserialization doesn't actually transport the underlying Please start this comment explaining what serialization/deserialization is for (i.e. used to tell child processes about the segment). The current comment you have makes sense as a second sentence.
asvitkine: PTAL https://codereview.chromium.org/2862123002/diff/30001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2862123002/diff/30001/base/metrics/field_tria... base/metrics/field_trial.cc:838: const char* field_trial_switch_value, On 2017/05/05 18:02:16, Alexei Svitkine (slow) wrote: > It's not the switch value - it's the switch name.... If you're just trying to > clarify this, suggest using switch_name instead. Or if it's part of an earlier > iteration that you decided to change your mind on, please revert. Either way, > please make sure the .cc changes are consistent with what's in the .h - which is > currently not the case with your CL. sed error. https://codereview.chromium.org/2862123002/diff/30001/base/metrics/field_tria... base/metrics/field_trial.cc:1121: std::getline(iss, token, ','); On 2017/05/05 18:02:16, Alexei Svitkine (slow) wrote: > On 2017/05/05 04:30:12, Avi (ping after 24h) wrote: > > You use base::StringToInt, why not base::SplitString? > > +1 - let's use SplitString() Done. https://codereview.chromium.org/2862123002/diff/30001/base/metrics/field_tria... base/metrics/field_trial.cc:1154: const std::string& switch_value) { On 2017/05/05 18:02:16, Alexei Svitkine (slow) wrote: > How about unifying the code for the windows and posix versions? You can pass the > int fd params to both and just mention that it's not used on Windows. Then, I > think the only ifdef you need to have for win/non-win is the return line. I thought about this - there needs to be another ifdef for processing the handle on windows, and now that we use split-string, some more changes w.r.t. counting tokens. overall I think combining causes more confusion for not-that-more code reuse and decided against it. https://codereview.chromium.org/2862123002/diff/30001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2862123002/diff/30001/base/metrics/field_tria... base/metrics/field_trial.h:652: // Serialization and deserialization doesn't actually transport the underlying On 2017/05/05 18:02:16, Alexei Svitkine (slow) wrote: > Please start this comment explaining what serialization/deserialization is for > (i.e. used to tell child processes about the segment). The current comment you > have makes sense as a second sentence. Done.
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...
https://codereview.chromium.org/2862123002/diff/50001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2862123002/diff/50001/base/metrics/field_tria... base/metrics/field_trial.cc:760: const char* field_trial_switch_value, Nit: You still have a sed error here :P https://codereview.chromium.org/2862123002/diff/50001/base/metrics/field_tria... base/metrics/field_trial.cc:1119: std::vector<std::string> tokens = base::SplitString( Nit: Use StringSplitPiece() version https://codereview.chromium.org/2862123002/diff/50001/base/metrics/field_tria... base/metrics/field_trial.cc:1143: const std::string& switch_value) { Please match the same order in .cc as in .h file - specifically have the two Deserialize functions one after the other, rather than trying to group things into the same ifdef. https://codereview.chromium.org/2862123002/diff/50001/base/metrics/field_tria... base/metrics/field_trial.cc:1166: return SharedMemoryHandle(); Nit: I find the following a bit cleaner: uint64_t high = 0; uint64_t low = 0; if (!base::StringToUint64(tokens[0], &high) || !base::StringToUint64(tokens[1], &low)) { return SharedMemoryHandle(); } https://codereview.chromium.org/2862123002/diff/50001/base/metrics/field_tria... base/metrics/field_trial.cc:1168: base::UnguessableToken guid = base::UnguessableToken::Deserialize(high, low); Suggest making a helper anon function to share this logic. If you make the guid always first (i.e. tokens[0] and tokens[1]) then you can call this helper from both the Windows and Posix impls and it can return the guid. This should avoid some of the duplication. https://codereview.chromium.org/2862123002/diff/50001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2862123002/diff/50001/base/metrics/field_tria... base/metrics/field_trial.h:652: // Serialization passes two pieces of information. If passes a reference to Nit: How about "Serialization is used to pass information about the handle to child processes." You can keep the other two sentences the same. https://codereview.chromium.org/2862123002/diff/50001/base/metrics/field_tria... base/metrics/field_trial.h:653: // the relevant OS resource, and it passes a GUID Nit: Add a .
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
asvitkine: PTAL https://codereview.chromium.org/2862123002/diff/50001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2862123002/diff/50001/base/metrics/field_tria... base/metrics/field_trial.cc:760: const char* field_trial_switch_value, On 2017/05/05 22:37:50, Alexei Svitkine (slow) wrote: > Nit: You still have a sed error here :P Done. https://codereview.chromium.org/2862123002/diff/50001/base/metrics/field_tria... base/metrics/field_trial.cc:1119: std::vector<std::string> tokens = base::SplitString( On 2017/05/05 22:37:50, Alexei Svitkine (slow) wrote: > Nit: Use StringSplitPiece() version Done. https://codereview.chromium.org/2862123002/diff/50001/base/metrics/field_tria... base/metrics/field_trial.cc:1143: const std::string& switch_value) { On 2017/05/05 22:37:50, Alexei Svitkine (slow) wrote: > Please match the same order in .cc as in .h file - specifically have the two > Deserialize functions one after the other, rather than trying to group things > into the same ifdef. Done. https://codereview.chromium.org/2862123002/diff/50001/base/metrics/field_tria... base/metrics/field_trial.cc:1166: return SharedMemoryHandle(); On 2017/05/05 22:37:50, Alexei Svitkine (slow) wrote: > Nit: I find the following a bit cleaner: > > uint64_t high = 0; > uint64_t low = 0; > if (!base::StringToUint64(tokens[0], &high) || > !base::StringToUint64(tokens[1], &low)) { > return SharedMemoryHandle(); > } Done. https://codereview.chromium.org/2862123002/diff/50001/base/metrics/field_tria... base/metrics/field_trial.cc:1168: base::UnguessableToken guid = base::UnguessableToken::Deserialize(high, low); On 2017/05/05 22:37:51, Alexei Svitkine (slow) wrote: > Suggest making a helper anon function to share this logic. > > If you make the guid always first (i.e. tokens[0] and tokens[1]) then you can > call this helper from both the Windows and Posix impls and it can return the > guid. > > This should avoid some of the duplication. Done. https://codereview.chromium.org/2862123002/diff/50001/base/metrics/field_trial.h File base/metrics/field_trial.h (right): https://codereview.chromium.org/2862123002/diff/50001/base/metrics/field_tria... base/metrics/field_trial.h:652: // Serialization passes two pieces of information. If passes a reference to On 2017/05/05 22:37:51, Alexei Svitkine (slow) wrote: > Nit: How about "Serialization is used to pass information about the handle to > child processes." > > You can keep the other two sentences the same. Done. https://codereview.chromium.org/2862123002/diff/50001/base/metrics/field_tria... base/metrics/field_trial.h:653: // the relevant OS resource, and it passes a GUID On 2017/05/05 22:37:51, Alexei Svitkine (slow) wrote: > Nit: Add a . Done.
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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
lgtm https://codereview.chromium.org/2862123002/diff/70001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2862123002/diff/70001/base/metrics/field_tria... base/metrics/field_trial.cc:222: bool DeserializeGUIDFromStringPieces(const base::StringPiece& first, Nit: StringPiece is meant to be passed by value - see header file for more info.
https://codereview.chromium.org/2862123002/diff/70001/base/metrics/field_tria... File base/metrics/field_trial.cc (right): https://codereview.chromium.org/2862123002/diff/70001/base/metrics/field_tria... base/metrics/field_trial.cc:222: bool DeserializeGUIDFromStringPieces(const base::StringPiece& first, On 2017/05/09 14:47:40, Alexei Svitkine (slow) wrote: > Nit: StringPiece is meant to be passed by value - see header file for more info. Done.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2862123002/#ps90001 (title: "Pass string piece by value.")
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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by erikchen@chromium.org
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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by erikchen@chromium.org
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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by erikchen@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": 90001, "attempt_start_ts": 1494437298648200,
"parent_rev": "ab183e73769385838a550a55a317d079192758b9", "commit_rev":
"2525d998c7c50bce5ca17fb934c2aeb8e27e1db0"}
Message was sent while issue was closed.
Description was changed from ========== Pass the GUID for the SharedMemoryHandle used by base::FieldTrialList. SharedMemoryHandle now has a GUID used to track shared memory usage across processes. The SharedMemoryHandle used by base::FieldTrialList is passed manually by command line flag and Process creation options. This CL appends the GUID to the command line flag. BUG=713763 ========== to ========== Pass the GUID for the SharedMemoryHandle used by base::FieldTrialList. SharedMemoryHandle now has a GUID used to track shared memory usage across processes. The SharedMemoryHandle used by base::FieldTrialList is passed manually by command line flag and Process creation options. This CL appends the GUID to the command line flag. BUG=713763 Review-Url: https://codereview.chromium.org/2862123002 Cr-Commit-Position: refs/heads/master@{#470700} Committed: https://chromium.googlesource.com/chromium/src/+/2525d998c7c50bce5ca17fb934c2... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:90001) as https://chromium.googlesource.com/chromium/src/+/2525d998c7c50bce5ca17fb934c2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
