Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2453313004:
Convert FieldTrialActivated to use mojo. (Closed)
|
Created:
4 years, 1 month ago by nigeltao1 Modified:
3 years, 9 months ago CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert FieldTrialActivated to use mojo.
Manually tested by printf'ing the arguments to FieldTrialActivated. The
args looked the same as for ChromeRenderMessageFilter prior to this CL.
BUG=577685
Committed: https://crrev.com/360d7c5326b5a6b91a4162e4b7137ef6fcb792ce
Cr-Commit-Position: refs/heads/master@{#437142}
Patch Set 1 #Patch Set 2 : Convert FieldTrialActivated to use mojo. #Patch Set 3 : Convert FieldTrialActivated to use mojo. #
Total comments: 2
Patch Set 4 : Convert FieldTrialActivated to use mojo. #
Total comments: 2
Patch Set 5 : Convert FieldTrialActivated to use mojo. #Patch Set 6 : Convert FieldTrialActivated to use mojo. #Patch Set 7 : Convert FieldTrialActivated to use mojo. #Patch Set 8 : Convert FieldTrialActivated to use mojo. #Patch Set 9 : Convert FieldTrialActivated to use mojo. #Patch Set 10 : Convert FieldTrialActivated to use mojo. #
Total comments: 2
Patch Set 11 : Convert FieldTrialActivated to use mojo. #Patch Set 12 : Convert FieldTrialActivated to use mojo. #
Total comments: 1
Messages
Total messages: 83 (54 generated)
The CQ bit was checked by nigeltao@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 ========== Convert FieldTrialActivated to use mojo. Manually tested by printf'ing the arguments to FieldTrialActivated. The args looked the same as for ChromeRenderMessageFilter prior to this CL. BUG=577685 ========== to ========== Convert FieldTrialActivated to use mojo. Manually tested by printf'ing the arguments to FieldTrialActivated. The args looked the same as for ChromeRenderMessageFilter prior to this CL. BUG=577685 ==========
nigeltao@chromium.org changed reviewers: + sammc@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 nigeltao@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_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by nigeltao@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_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by nigeltao@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_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
LGTM Adding reviewers doesn't send mail automatically. I find the publish+mail page useful for adding reviewers and letting them know at the same time (it's what Chromite Butler does too). Sorry for not spotting this sooner. https://codereview.chromium.org/2453313004/diff/40001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_thread_observer.cc (right): https://codereview.chromium.org/2453313004/diff/40001/chrome/renderer/chrome_... chrome/renderer/chrome_render_thread_observer.cc:296: if (!field_trial_recorder_) OnFieldTrialGroupFinalized seems like it should only be called once per process, so |field_trial_recorder_| can just be a local. Otherwise, resetting |field_trial_recorder_| in OnRenderProcessShutdown() above should fix your single-process test threading issues.
The CQ bit was checked by nigeltao@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_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
nigeltao@chromium.org changed reviewers: + sky@chromium.org, tsepez@chromium.org
sky, tsepez, can you review for OWNERS? https://codereview.chromium.org/2453313004/diff/40001/chrome/renderer/chrome_... File chrome/renderer/chrome_render_thread_observer.cc (right): https://codereview.chromium.org/2453313004/diff/40001/chrome/renderer/chrome_... chrome/renderer/chrome_render_thread_observer.cc:296: if (!field_trial_recorder_) On 2016/11/07 06:56:54, Sam McNally wrote: > OnFieldTrialGroupFinalized seems like it should only be called once per process, > so |field_trial_recorder_| can just be a local. Otherwise, resetting > |field_trial_recorder_| in OnRenderProcessShutdown() above should fix your > single-process test threading issues. I made it a local.
mojom LGTM
LGTM https://codereview.chromium.org/2453313004/diff/60001/chrome/browser/field_tr... File chrome/browser/field_trial_recorder_impl.h (right): https://codereview.chromium.org/2453313004/diff/60001/chrome/browser/field_tr... chrome/browser/field_trial_recorder_impl.h:11: class FieldTrialRecorderImpl : public chrome::mojom::FieldTrialRecorder { The reason for the mojom in the namespace is so you don't need to use "impl" or some other suffix/prefix to differentiate the two. Name this FieldTrialRecorder.
https://codereview.chromium.org/2453313004/diff/60001/chrome/browser/field_tr... File chrome/browser/field_trial_recorder_impl.h (right): https://codereview.chromium.org/2453313004/diff/60001/chrome/browser/field_tr... chrome/browser/field_trial_recorder_impl.h:11: class FieldTrialRecorderImpl : public chrome::mojom::FieldTrialRecorder { On 2016/11/10 20:22:56, sky wrote: > The reason for the mojom in the namespace is so you don't need to use "impl" or > some other suffix/prefix to differentiate the two. Name this FieldTrialRecorder. Done.
The CQ bit was checked by nigeltao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org, tsepez@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2453313004/#ps80001 (title: "Convert FieldTrialActivated to use mojo.")
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: linux_chromium_chromeos_ozone_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 nigeltao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, sammc@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2453313004/#ps100001 (title: "Convert FieldTrialActivated to use mojo.")
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by nigeltao@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: linux_chromium_compile_dbg_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 nigeltao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, sammc@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2453313004/#ps120001 (title: "Convert FieldTrialActivated to use mojo.")
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: 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 nigeltao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, sammc@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2453313004/#ps140001 (title: "Convert FieldTrialActivated to use mojo.")
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: linux_chromium_chromeos_ozone_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 nigeltao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, sammc@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2453313004/#ps160001 (title: "Convert FieldTrialActivated to use mojo.")
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: 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 nigeltao@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
rockot@chromium.org changed reviewers: + rockot@chromium.org
As far as I can tell the mash failures you see are purely coincidental. Nothing in your CL is relevant to their behavior. https://codereview.chromium.org/2453313004/diff/180001/chrome/app/mash/chrome... File chrome/app/mash/chrome_mash_content_browser_manifest_overlay.json (right): https://codereview.chromium.org/2453313004/diff/180001/chrome/app/mash/chrome... chrome/app/mash/chrome_mash_content_browser_manifest_overlay.json:16: "renderer": [ this is redundant, please don't add it. this overlay is always applied on top of the chrome_content_browser_manifest_overlay
On Fri, Dec 2, 2016 at 4:29 AM, <rockot@chromium.org> wrote: > As far as I can tell the mash failures you see are purely coincidental. > Nothing > in your CL is relevant to their behavior. Should I therefore just force my way past the commit queue somehow? How do I do so? I'm still relatively new to Chromium build tools mechanism and culture. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
For the record, my CQ woes with this CL are also discussed on the chromium-mojo list: https://groups.google.com/a/chromium.org/d/topic/chromium-mojo/fOPYqYMfJGE/di...
https://codereview.chromium.org/2453313004/diff/180001/chrome/app/mash/chrome... File chrome/app/mash/chrome_mash_content_browser_manifest_overlay.json (right): https://codereview.chromium.org/2453313004/diff/180001/chrome/app/mash/chrome... chrome/app/mash/chrome_mash_content_browser_manifest_overlay.json:16: "renderer": [ On 2016/12/01 17:29:47, Ken Rockot wrote: > this is redundant, please don't add it. this overlay is always applied on top of > the chrome_content_browser_manifest_overlay Done.
The CQ bit was checked by nigeltao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, sammc@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2453313004/#ps200001 (title: "Convert FieldTrialActivated to use mojo.")
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: linux_chromium_chromeos_ozone_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 nigeltao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, sammc@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2453313004/#ps220001 (title: "Convert FieldTrialActivated to use mojo.")
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": 220001, "attempt_start_ts": 1481157691164270,
"parent_rev": "adda1d7943ea3f90c18d4b8a39fa0741b934a56a", "commit_rev":
"e4f3332207ff90f8718b227fcc56cb4a6a16938a"}
Message was sent while issue was closed.
Description was changed from ========== Convert FieldTrialActivated to use mojo. Manually tested by printf'ing the arguments to FieldTrialActivated. The args looked the same as for ChromeRenderMessageFilter prior to this CL. BUG=577685 ========== to ========== Convert FieldTrialActivated to use mojo. Manually tested by printf'ing the arguments to FieldTrialActivated. The args looked the same as for ChromeRenderMessageFilter prior to this CL. BUG=577685 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Convert FieldTrialActivated to use mojo. Manually tested by printf'ing the arguments to FieldTrialActivated. The args looked the same as for ChromeRenderMessageFilter prior to this CL. BUG=577685 ========== to ========== Convert FieldTrialActivated to use mojo. Manually tested by printf'ing the arguments to FieldTrialActivated. The args looked the same as for ChromeRenderMessageFilter prior to this CL. BUG=577685 Committed: https://crrev.com/360d7c5326b5a6b91a4162e4b7137ef6fcb792ce Cr-Commit-Position: refs/heads/master@{#437142} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/360d7c5326b5a6b91a4162e4b7137ef6fcb792ce Cr-Commit-Position: refs/heads/master@{#437142}
Message was sent while issue was closed.
sadrul@chromium.org changed reviewers: + sadrul@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2453313004/diff/220001/chrome/common/render_m... File chrome/common/render_messages.h (left): https://codereview.chromium.org/2453313004/diff/220001/chrome/common/render_m... chrome/common/render_messages.h:605: std::string /* name */) Hi! There are corresponding GpuHostMsg_FieldTrialActivated and PpapiHostMsg_FieldTrialActivated, both of which live in //content. These could easily use the mojom api in this CL if we move this into content. Is there any objects to doing that?
Message was sent while issue was closed.
On 2017/03/03 03:43:47, sadrul wrote: > https://codereview.chromium.org/2453313004/diff/220001/chrome/common/render_m... > File chrome/common/render_messages.h (left): > > https://codereview.chromium.org/2453313004/diff/220001/chrome/common/render_m... > chrome/common/render_messages.h:605: std::string /* name */) > Hi! There are corresponding GpuHostMsg_FieldTrialActivated and > PpapiHostMsg_FieldTrialActivated, both of which live in //content. These could > easily use the mojom api in this CL if we move this into content. Is there any > objects to doing that? I have filed https://bugs.chromium.org/p/chromium/issues/detail?id=698284 to track this work. Please add a comment on the bug if you have any concerns/advice etc.
Message was sent while issue was closed.
On 2017/03/03 03:43:47, sadrul wrote: > https://codereview.chromium.org/2453313004/diff/220001/chrome/common/render_m... > File chrome/common/render_messages.h (left): > > https://codereview.chromium.org/2453313004/diff/220001/chrome/common/render_m... > chrome/common/render_messages.h:605: std::string /* name */) > Hi! There are corresponding GpuHostMsg_FieldTrialActivated and > PpapiHostMsg_FieldTrialActivated, both of which live in //content. These could > easily use the mojom api in this CL if we move this into content. Is there any > objects to doing that? No objection. |
