Move FieldTrialRecorder mojom to content

Move FieldTrialRecorder mojom to content

After this move, we can change PpapiHostMsg_FieldTrialActivated and
GpuHostMsg_FieldTrialActivated to mojo.

To see an example sequence of calls see:
    https://docs.google.com/a/google.com/document/d/18radEJD0pTzz79I7jFsfw8NIuF_jXufNUj6Mr48cO1U/edit?usp=sharing

BUG=698284
Review-Url: https://codereview.chromium.org/2732213004
Cr-Commit-Position: refs/heads/master@{#457644}
Committed: https://chromium.googlesource.com/chromium/src/+/614d70a5b74f107f474dcd4924e64650f9ab6207

[chiniforooshan, 2017-03-07 21:17:45]

chiniforooshan@chromium.org changed reviewers:
+ sadrul@chromium.org

[chiniforooshan, 2017-03-07 21:17:45]

PTAL

[chiniforooshan, 2017-03-07 21:19:01]

Description was changed from

==========
Move FieldTrialRecorder mojom to content

After this move, we can change PpapiHostMsg_FieldTrialActivated and
PpapiHostMsg_FieldTrialActivated to mojo.

BUG=698284
==========

to

==========
Move FieldTrialRecorder mojom to content

After this move, we can change PpapiHostMsg_FieldTrialActivated and
GpuHostMsg_FieldTrialActivated to mojo.

BUG=698284
==========

[chiniforooshan, 2017-03-07 21:19:20]

The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 21:20:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 22:21:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 22:21:07]

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...)

[chiniforooshan, 2017-03-08 01:07:41]

The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 01:08:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2017-03-08 02:19:58]

sadrul@chromium.org changed reviewers:
+ jam@chromium.org

[sadrul, 2017-03-08 02:19:58]

+jam@ for his thoughts: Considering //content/browser will provide this
interface to ppapi and gpu, I can't think of a good reason to provide the
interface to renderer from //chrome/browser. In other words, maybe the interface
registration in //chrome/browser [1] should move into //content/browser [2]?

If we do that, then the mojom and the FieldTrialRecorder implementations should
move into //content/common (instead of in //content/public/common)

1:
https://cs.chromium.org/chromium/src/chrome/browser/chrome_content_browser_cl...
2:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro...

https://codereview.chromium.org/2732213004/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_manifest_overlay.json (right):

https://codereview.chromium.org/2732213004/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_manifest_overlay.json:11:
"content::mojom::FieldTrialRecorder",
This should probably be just removed, and a corresponding entry added in
//content/public/app/mojo/content_browser_manifest.json

https://codereview.chromium.org/2732213004/diff/20001/chrome/browser/field_tr...
File chrome/browser/field_trial_recorder.h (right):

https://codereview.chromium.org/2732213004/diff/20001/chrome/browser/field_tr...
chrome/browser/field_trial_recorder.h:11: class FieldTrialRecorder : public
content::mojom::FieldTrialRecorder {
I think this too should move into //content, since gpu and ppapi would also need
an identical implementation of this.

[commit-bot: I haz the power, 2017-03-08 02:26:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 02:26:39]

Dry run: This issue passed the CQ dry run.

[jam, 2017-03-09 16:55:28]

IPCs are considered an internal detail of a component, so we don't send them in
chrome and dispatch them in content, and vice versa

[chiniforooshan, 2017-03-09 18:41:02]

On 2017/03/09 16:55:28, jam wrote:
> IPCs are considered an internal detail of a component, so we don't send them
in
> chrome and dispatch them in content, and vice versa

I see. Thanks. So, the only reason that there would be IPC from chrome to
content is that ChromeRenderThreadObserver registers itself to
components::variations::ChildProcessFieldTrialSyncer. If RenderThreadImpl does
that instead of ChromeRenderThreadObserver, then the IPCs become internal to
content. I will do that if it answers your concern?

[jam, 2017-03-09 20:32:21]

On 2017/03/09 18:41:02, chiniforooshan wrote:
> On 2017/03/09 16:55:28, jam wrote:
> > IPCs are considered an internal detail of a component, so we don't send them
> in
> > chrome and dispatch them in content, and vice versa
> 
> I see. Thanks. So, the only reason that there would be IPC from chrome to
> content is that ChromeRenderThreadObserver registers itself to
> components::variations::ChildProcessFieldTrialSyncer. If RenderThreadImpl does
> that instead of ChromeRenderThreadObserver, then the IPCs become internal to
> content. I will do that if it answers your concern?

yep that sounds good.
i think before we avoided content/ depending on variations but that's not the
case anymore.

[chiniforooshan, 2017-03-14 14:32:14]

Description was changed from

==========
Move FieldTrialRecorder mojom to content

After this move, we can change PpapiHostMsg_FieldTrialActivated and
GpuHostMsg_FieldTrialActivated to mojo.

BUG=698284
==========

to

==========
Move FieldTrialRecorder mojom to content

After this move, we can change PpapiHostMsg_FieldTrialActivated and
GpuHostMsg_FieldTrialActivated to mojo.

To see an example sequence of calls see:
   
https://docs.google.com/a/google.com/document/d/18radEJD0pTzz79I7jFsfw8NIuF_j...

BUG=698284
==========

[chiniforooshan, 2017-03-14 14:34:36]

FieldTrialRecorder messages are now sent and handled in //content.

https://codereview.chromium.org/2732213004/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_manifest_overlay.json (right):

https://codereview.chromium.org/2732213004/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_manifest_overlay.json:11:
"content::mojom::FieldTrialRecorder",
On 2017/03/08 02:19:58, sadrul wrote:
> This should probably be just removed, and a corresponding entry added in
> //content/public/app/mojo/content_browser_manifest.json

Done.

https://codereview.chromium.org/2732213004/diff/20001/chrome/browser/field_tr...
File chrome/browser/field_trial_recorder.h (right):

https://codereview.chromium.org/2732213004/diff/20001/chrome/browser/field_tr...
chrome/browser/field_trial_recorder.h:11: class FieldTrialRecorder : public
content::mojom::FieldTrialRecorder {
On 2017/03/08 02:19:58, sadrul wrote:
> I think this too should move into //content, since gpu and ppapi would also
need
> an identical implementation of this.

Done. Moved it to //content/browser. I think gpu and ppapi just need the mojom
interface. They don't need the implementation, right?

[chiniforooshan, 2017-03-14 15:27:28]

chiniforooshan@chromium.org changed reviewers:
+ rjkroege@chromium.org

[chiniforooshan, 2017-03-14 15:27:29]

+Rob

[jam, 2017-03-16 15:33:40]

lgtm

https://codereview.chromium.org/2732213004/diff/60001/content/browser/field_t...
File content/browser/field_trial_recorder.cc (right):

https://codereview.chromium.org/2732213004/diff/60001/content/browser/field_t...
content/browser/field_trial_recorder.cc:19:
content::mojom::FieldTrialRecorderRequest request) {
ditto

https://codereview.chromium.org/2732213004/diff/60001/content/browser/field_t...
File content/browser/field_trial_recorder.h (right):

https://codereview.chromium.org/2732213004/diff/60001/content/browser/field_t...
content/browser/field_trial_recorder.h:18: static void
Create(content::mojom::FieldTrialRecorderRequest request);
ditto

https://codereview.chromium.org/2732213004/diff/60001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2732213004/diff/60001/content/renderer/render...
content/renderer/render_thread_impl.cc:2060:
content::mojom::FieldTrialRecorderPtr field_trial_recorder;
nit: no content::

[chiniforooshan, 2017-03-16 17:35:46]

https://codereview.chromium.org/2732213004/diff/60001/content/browser/field_t...
File content/browser/field_trial_recorder.cc (right):

https://codereview.chromium.org/2732213004/diff/60001/content/browser/field_t...
content/browser/field_trial_recorder.cc:19:
content::mojom::FieldTrialRecorderRequest request) {
On 2017/03/16 15:33:40, jam wrote:
> ditto

Done.

https://codereview.chromium.org/2732213004/diff/60001/content/browser/field_t...
File content/browser/field_trial_recorder.h (right):

https://codereview.chromium.org/2732213004/diff/60001/content/browser/field_t...
content/browser/field_trial_recorder.h:18: static void
Create(content::mojom::FieldTrialRecorderRequest request);
On 2017/03/16 15:33:40, jam wrote:
> ditto

Done.

https://codereview.chromium.org/2732213004/diff/60001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2732213004/diff/60001/content/renderer/render...
content/renderer/render_thread_impl.cc:2060:
content::mojom::FieldTrialRecorderPtr field_trial_recorder;
On 2017/03/16 15:33:40, jam wrote:
> nit: no content::

Done.

[chiniforooshan, 2017-03-16 18:47:13]

The CQ bit was checked by chiniforooshan@chromium.org

[chiniforooshan, 2017-03-16 18:47:13]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2732213004/#ps80001
(title: "review")

[commit-bot: I haz the power, 2017-03-16 18:47:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 18:52:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 18:52:56]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  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_...)

[chiniforooshan, 2017-03-16 19:00:17]

chiniforooshan@chromium.org changed reviewers:
+ tsepez@chromium.org

[chiniforooshan, 2017-03-16 19:00:18]

+Tom for security review:
    chrome/browser/chrome_content_browser_manifest_overlay.json
chrome/common/field_trial_recorder.mojom
content/common/field_trial_recorder.mojom
content/public/app/mojo/content_browser_manifest.json

[Tom Sepez, 2017-03-16 20:03:43]

lgtm

[chiniforooshan, 2017-03-17 00:10:57]

The CQ bit was checked by chiniforooshan@chromium.org

[chiniforooshan, 2017-03-17 00:10:58]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2732213004/#ps120001
(title: "initialize FieldTrialList for RenderThreadImplBrowserTest")

[commit-bot: I haz the power, 2017-03-17 00:11:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 01:19:38]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489709457674010,
"parent_rev": "f7ccdfd34723ac939c80431e730fda34d948a119", "commit_rev":
"614d70a5b74f107f474dcd4924e64650f9ab6207"}

[commit-bot: I haz the power, 2017-03-17 01:20:31]

Description was changed from

==========
Move FieldTrialRecorder mojom to content

After this move, we can change PpapiHostMsg_FieldTrialActivated and
GpuHostMsg_FieldTrialActivated to mojo.

To see an example sequence of calls see:
   
https://docs.google.com/a/google.com/document/d/18radEJD0pTzz79I7jFsfw8NIuF_j...

BUG=698284
==========

to

==========
Move FieldTrialRecorder mojom to content

After this move, we can change PpapiHostMsg_FieldTrialActivated and
GpuHostMsg_FieldTrialActivated to mojo.

To see an example sequence of calls see:
   
https://docs.google.com/a/google.com/document/d/18radEJD0pTzz79I7jFsfw8NIuF_j...

BUG=698284

Review-Url: https://codereview.chromium.org/2732213004
Cr-Commit-Position: refs/heads/master@{#457644}
Committed:
https://chromium.googlesource.com/chromium/src/+/614d70a5b74f107f474dcd4924e6...
==========

[commit-bot: I haz the power, 2017-03-17 01:20:32]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/614d70a5b74f107f474dcd4924e6...