New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 698284 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

FieldTrialRecorder should be used in content

Project Member Reported by sadrul@chromium.org, Mar 3 2017

Issue description

FieldTrialRecorder currently lives in chrome [1]. The renderer process uses it to notify the browser when a field-trial is activated.

There are equivalent chrome IPC messages for pepper [2] and gpu [3] processes. But it should be possible to replace these chrome IPC messages with the mojom API in [1]. To allow for this, the mojom needs to move into somewhere accessible to //content (e.g. maybe in //content).

[1] https://cs.chromium.org/chromium/src/chrome/common/field_trial_recorder.mojom?sq=package:chromium&dr=C&l=8
[2] https://cs.chromium.org/chromium/src/ppapi/proxy/ppapi_messages.h?type=cs&sq=package:chromium&l=2537
[3] https://cs.chromium.org/chromium/src/content/common/gpu_host_messages.h?type=cs&sq=package:chromium&l=172
 
Owner: chiniforooshan@chromium.org
Status: Started (was: Available)
https://codereview.chromium.org/2732213004 moves it to //content/public/common, but I'm not sure, maybe //content/public/browser makes more sense? WDYT?
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/614d70a5b74f107f474dcd4924e64650f9ab6207

commit 614d70a5b74f107f474dcd4924e64650f9ab6207
Author: chiniforooshan <chiniforooshan@chromium.org>
Date: Fri Mar 17 01:19:33 2017

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}

[modify] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/chrome/browser/BUILD.gn
[modify] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/chrome/browser/chrome_content_browser_manifest_overlay.json
[delete] https://crrev.com/f7ccdfd34723ac939c80431e730fda34d948a119/chrome/browser/field_trial_recorder.h
[modify] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/chrome/common/BUILD.gn
[modify] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/chrome/renderer/chrome_render_thread_observer.cc
[modify] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/chrome/renderer/chrome_render_thread_observer.h
[modify] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/content/browser/BUILD.gn
[rename] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/content/browser/field_trial_recorder.cc
[add] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/content/browser/field_trial_recorder.h
[modify] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/content/common/BUILD.gn
[rename] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/content/common/field_trial_recorder.mojom
[modify] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/content/public/app/mojo/content_browser_manifest.json
[modify] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/content/public/renderer/render_thread.h
[modify] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/content/public/test/mock_render_thread.cc
[modify] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/content/public/test/mock_render_thread.h
[modify] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/content/renderer/BUILD.gn
[modify] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/content/renderer/DEPS
[modify] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/content/renderer/render_thread_impl.h
[modify] https://crrev.com/614d70a5b74f107f474dcd4924e64650f9ab6207/content/renderer/render_thread_impl_browsertest.cc

Is this fixed now?
We still need to use this FieldTrialRecorder for gpu and ppapi processes (and remove the relevant IPCs mentioned in OP)
Yeah. https://codereview.chromium.org/2803743002 is for the GPU process. Removing the IPC from the PPAPI process needs just a little bit more work.
Project Member

Comment 7 by bugdroid1@chromium.org, Apr 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/701fa21b5ccf60143b103799f83cae7c51e6b5ee

commit 701fa21b5ccf60143b103799f83cae7c51e6b5ee
Author: chiniforooshan <chiniforooshan@chromium.org>
Date: Thu Apr 06 22:22:49 2017

Mojofy GpuHostMsg_FieldTrialActivated

Also, a little bit of cleaning mojo interface registration in
gpu_process_host.

BUG= 698284 

Review-Url: https://codereview.chromium.org/2803743002
Cr-Commit-Position: refs/heads/master@{#462649}

[modify] https://crrev.com/701fa21b5ccf60143b103799f83cae7c51e6b5ee/content/browser/gpu/gpu_data_manager_impl_private.cc
[modify] https://crrev.com/701fa21b5ccf60143b103799f83cae7c51e6b5ee/content/browser/gpu/gpu_process_host.cc
[modify] https://crrev.com/701fa21b5ccf60143b103799f83cae7c51e6b5ee/content/browser/gpu/gpu_process_host_ui_shim.cc
[modify] https://crrev.com/701fa21b5ccf60143b103799f83cae7c51e6b5ee/content/browser/gpu/gpu_process_host_ui_shim.h
[modify] https://crrev.com/701fa21b5ccf60143b103799f83cae7c51e6b5ee/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/701fa21b5ccf60143b103799f83cae7c51e6b5ee/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/701fa21b5ccf60143b103799f83cae7c51e6b5ee/content/common/BUILD.gn
[modify] https://crrev.com/701fa21b5ccf60143b103799f83cae7c51e6b5ee/content/common/content_message_generator.h
[delete] https://crrev.com/be9dfc362a8b2b90e054b5142c55c28334a6bded/content/common/gpu_host_messages.h
[modify] https://crrev.com/701fa21b5ccf60143b103799f83cae7c51e6b5ee/content/gpu/BUILD.gn
[modify] https://crrev.com/701fa21b5ccf60143b103799f83cae7c51e6b5ee/content/gpu/gpu_child_thread.cc
[modify] https://crrev.com/701fa21b5ccf60143b103799f83cae7c51e6b5ee/content/gpu/gpu_main.cc
[modify] https://crrev.com/701fa21b5ccf60143b103799f83cae7c51e6b5ee/content/public/app/mojo/content_browser_manifest.json
[modify] https://crrev.com/701fa21b5ccf60143b103799f83cae7c51e6b5ee/ipc/ipc_message_start.h

Cc: lukasza@chromium.org
Status: Fixed (was: Started)
I think https://chromium-review.googlesource.com/c/567034 did the remaining part of this bug (and more) and we can close. Thanks Lukasz :)

Sign in to add a comment