Use kRenderClientId command line flag when starting a render process

Added a command line switch kRenderClientId. It holds the process ID of a browser process and is passed to a renderer process when the render process is created. RenderWidget stores the client ID into its client_id_ field and use it to construct a FrameSinkID when RenderWidgetCompositor is created in RenderWidget::InitializeLayerTreeView.

This change should fix a bug where SurfaceSequence::frame_sink_id is empty in SurfaceLayer::SatisfyDestroySequence().

BUG=656639
Committed: https://crrev.com/067f5824a3d561b9a16774fed995c23b6854405f
Cr-Commit-Position: refs/heads/master@{#430375}

[Alex Z., 2016-10-26 14:27:14]

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

[commit-bot: I haz the power, 2016-10-26 14:27:24]

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

[Alex Z., 2016-10-26 14:42:40]

staraz@chromium.org changed reviewers:
+ fsamuel@chromium.org, piman@chromium.org, tsepez@chromium.org

[Alex Z., 2016-10-26 14:42:40]

fsamuel@chromium.org: This CL adds FrameSinkId to CreateViewParams

piman@chromium.org: Please do an owner review for my changes

tseqez@: Please review my changes in renderer.mojom

[Fady Samuel, 2016-10-26 15:00:18]

not lgtm

https://codereview.chromium.org/2447143003/diff/20001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2447143003/diff/20001/content/renderer/render...
content/renderer/render_widget.cc:323: screen_info, frame_sink_id, false,
hidden,
This isn't right. Each RenderWidget should share the same client_id but get a
different sink ID equal to the routing ID.

I think we should tell a renderer process its client ID via flag when the
process is created, and when a RenderWidget gets a routing ID (via SetRoutingID)
it should get its sink ID.

[Alex Z., 2016-10-27 13:58:05]

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

[commit-bot: I haz the power, 2016-10-27 13:58:28]

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

[Alex Z., 2016-10-27 14:00:05]

Description was changed from

==========
Added FrameSinkId to CreateViewParams as a field.

FrameSinkId is now passed from RenderViewHostImpl to RenderViewImpl as a field
of CreateViewParams. RenderWidget stores the FrameSinkId and calls
OnSetFrameSinkId with it when InitializeLayerTreeView (where
RenderWidgetCompositor is created) is called. This should fix the bug where some
SurfaceSequenceGenerators have empty FrameSinkId.

BUG=656639
==========

to

==========
Added a command line switch kRenderClientId. It holds the process ID of a
browser process and is passed to a renderer process when the render process is
created. RenderWidget stores the client ID into its client_id_ field and use it
to construct a FrameSinkID when RenderWidgetCompositor is created in
RenderWidget::InitializeLayerTreeView.
    
This change should fix a bug where SurfaceSequence::frame_sink_id is empty in
SurfaceLayer::SatisfyDestroySequence().

BUG=656639
==========

[commit-bot: I haz the power, 2016-10-27 15:32:17]

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

[commit-bot: I haz the power, 2016-10-27 15:32:18]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2016-10-27 20:15:55]

https://codereview.chromium.org/2447143003/diff/40001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2447143003/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:250: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
I see no point in saving this here.

https://codereview.chromium.org/2447143003/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:376:
OnSetFrameSinkId(cc::FrameSinkId(client_id_, routing_id_));
I don't believe we have a RenderWidgetCompositor yet at this point. We should
just get rid of OnSetFrameSinkId and the chrome IPC.

https://codereview.chromium.org/2447143003/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:513:
IPC_MESSAGE_HANDLER(ViewMsg_SetFrameSinkId, OnSetFrameSinkId)
Get rid of this.

https://codereview.chromium.org/2447143003/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:1126:
compositor_->SetFrameSinkId(cc::FrameSinkId(client_id_, routing_id_));
We can simply read the client id from commandline here.

[Alex Z., 2016-10-28 18:09:17]

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

[commit-bot: I haz the power, 2016-10-28 18:09:36]

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

[Alex Z., 2016-10-28 18:09:48]

https://codereview.chromium.org/2447143003/diff/40001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2447143003/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:250: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2016/10/27 20:15:55, Fady Samuel wrote:
> I see no point in saving this here. 

Done.

https://codereview.chromium.org/2447143003/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:376:
OnSetFrameSinkId(cc::FrameSinkId(client_id_, routing_id_));
On 2016/10/27 20:15:55, Fady Samuel wrote:
> I don't believe we have a RenderWidgetCompositor yet at this point. We should
> just get rid of OnSetFrameSinkId and the chrome IPC.

Done.

https://codereview.chromium.org/2447143003/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:513:
IPC_MESSAGE_HANDLER(ViewMsg_SetFrameSinkId, OnSetFrameSinkId)
On 2016/10/27 20:15:55, Fady Samuel wrote:
> Get rid of this.

Done.

https://codereview.chromium.org/2447143003/diff/40001/content/renderer/render...
content/renderer/render_widget.cc:1126:
compositor_->SetFrameSinkId(cc::FrameSinkId(client_id_, routing_id_));
On 2016/10/27 20:15:55, Fady Samuel wrote:
> We can simply read the client id from commandline here.

Done.

[Fady Samuel, 2016-10-28 18:14:40]

https://codereview.chromium.org/2447143003/diff/60001/content/renderer/render...
File content/renderer/render_widget.cc (left):

https://codereview.chromium.org/2447143003/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1511: void
RenderWidget::OnSetFrameSinkId(const cc::FrameSinkId& frame_sink_id) {
delete this and the existing IPC. We don't need it.

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

https://codereview.chromium.org/2447143003/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1118: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
DCHECK that we have the switch.

https://codereview.chromium.org/2447143003/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1121:
std::stoi(base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
Use base::StringToInt
https://cs.chromium.org/chromium/src/base/strings/string_number_conversions.h...

[Alex Z., 2016-10-28 18:35:08]

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

[commit-bot: I haz the power, 2016-10-28 18:35:40]

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

[Alex Z., 2016-10-28 18:36:11]

https://codereview.chromium.org/2447143003/diff/60001/content/renderer/render...
File content/renderer/render_widget.cc (left):

https://codereview.chromium.org/2447143003/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1511: void
RenderWidget::OnSetFrameSinkId(const cc::FrameSinkId& frame_sink_id) {
On 2016/10/28 18:14:40, Fady Samuel wrote:
> delete this and the existing IPC. We don't need it.

Done.

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

https://codereview.chromium.org/2447143003/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1118: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2016/10/28 18:14:40, Fady Samuel wrote:
> DCHECK that we have the switch.

Done.

https://codereview.chromium.org/2447143003/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:1121:
std::stoi(base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
On 2016/10/28 18:14:40, Fady Samuel wrote:
> Use base::StringToInt
>
https://cs.chromium.org/chromium/src/base/strings/string_number_conversions.h...

Done.

[commit-bot: I haz the power, 2016-10-28 19:08:09]

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

[commit-bot: I haz the power, 2016-10-28 19:08:10]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Alex Z., 2016-10-28 19:33:13]

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

[commit-bot: I haz the power, 2016-10-28 19:33:48]

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, 2016-10-28 20:00:45]

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

[commit-bot: I haz the power, 2016-10-28 20:00:46]

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

[Fady Samuel, 2016-10-28 20:59:34]

https://codereview.chromium.org/2447143003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(left):

https://codereview.chromium.org/2447143003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4134:
TEST_F(RenderWidgetHostViewAuraTest, FrameSinkIdInitialized) {
I don't see the point of this test.

https://codereview.chromium.org/2447143003/diff/80001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2447143003/diff/80001/content/renderer/render...
content/renderer/render_widget.cc:1106: compositor_ =
RenderWidgetCompositor::Create(this, device_scale_factor_,
Can we just pass the FrameSinkId into here?

https://codereview.chromium.org/2447143003/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(left):

https://codereview.chromium.org/2447143003/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4134:
TEST_F(RenderWidgetHostViewAuraTest, FrameSinkIdInitialized) {
just delete this test. It's irrelevant I think.

https://codereview.chromium.org/2447143003/diff/100001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2447143003/diff/100001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1587: int32_t
RenderThreadImpl::GetClientId() {
if (base::CommandLine::ForCurrentProcess()->HasSwitch(
          switches::kSingleProcess))) {
  return 1;
}

In single-process mode used for debugging, we don't pass a rendererclientid via
command line because RenderThreadImpl lives in the same process as the browser.

https://codereview.chromium.org/2447143003/diff/100001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2447143003/diff/100001/content/renderer/rende...
content/renderer/render_widget.cc:21: #include
"base/strings/string_number_conversions.h"
This is no longer necessary.

https://codereview.chromium.org/2447143003/diff/100001/content/renderer/rende...
content/renderer/render_widget.cc:1117: compositor_->SetFrameSinkId(
DCHECK_NE(MSG_ROUTING_NONE, routing_id_);

[Alex Z., 2016-10-31 14:08:12]

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

[commit-bot: I haz the power, 2016-10-31 14:08:32]

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, 2016-10-31 14:55:41]

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

[commit-bot: I haz the power, 2016-10-31 14:55:42]

Dry run: This issue passed the CQ dry run.

[Alex Z., 2016-10-31 15:14:23]

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

[commit-bot: I haz the power, 2016-10-31 15:14:46]

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

[Alex Z., 2016-10-31 15:19:23]

https://codereview.chromium.org/2447143003/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(left):

https://codereview.chromium.org/2447143003/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4134:
TEST_F(RenderWidgetHostViewAuraTest, FrameSinkIdInitialized) {
On 2016/10/28 20:59:33, Fady Samuel wrote:
> just delete this test. It's irrelevant I think.

Done.

https://codereview.chromium.org/2447143003/diff/100001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2447143003/diff/100001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1587: int32_t
RenderThreadImpl::GetClientId() {
On 2016/10/28 20:59:34, Fady Samuel wrote:
> if (base::CommandLine::ForCurrentProcess()->HasSwitch(
>           switches::kSingleProcess))) {
>   return 1;
> }
> 
> In single-process mode used for debugging, we don't pass a rendererclientid
via
> command line because RenderThreadImpl lives in the same process as the
browser.

Done.

https://codereview.chromium.org/2447143003/diff/100001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/2447143003/diff/100001/content/renderer/rende...
content/renderer/render_widget.cc:21: #include
"base/strings/string_number_conversions.h"
On 2016/10/28 20:59:34, Fady Samuel wrote:
> This is no longer necessary.

Done.

https://codereview.chromium.org/2447143003/diff/100001/content/renderer/rende...
content/renderer/render_widget.cc:1117: compositor_->SetFrameSinkId(
On 2016/10/28 20:59:34, Fady Samuel wrote:
> DCHECK_NE(MSG_ROUTING_NONE, routing_id_);

Done.

[Fady Samuel, 2016-10-31 15:32:38]

https://codereview.chromium.org/2447143003/diff/140001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2447143003/diff/140001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1589: switches::kRendererClientId))
nit: braces here.

https://codereview.chromium.org/2447143003/diff/140001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1591: int client_id;
Initialize this to 0.

[Alex Z., 2016-10-31 15:41:33]

https://codereview.chromium.org/2447143003/diff/140001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2447143003/diff/140001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1589: switches::kRendererClientId))
On 2016/10/31 15:32:38, Fady Samuel wrote:
> nit: braces here.

Done.

https://codereview.chromium.org/2447143003/diff/140001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1591: int client_id;
On 2016/10/31 15:32:38, Fady Samuel wrote:
> Initialize this to 0.

Done.

[Fady Samuel, 2016-10-31 16:01:56]

lgtm.

[Alex Z., 2016-10-31 16:58:00]

This CL has been redone since it was first uploaded.

tseqez@: please review my changes in  content/common/view_messages.h

piman@: please review my changes in all files except view_messages.h

[Tom Sepez, 2016-10-31 20:12:00]

RS LGTM on deleting messages.

[piman, 2016-10-31 20:34:09]

https://codereview.chromium.org/2447143003/diff/160001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2447143003/diff/160001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1589: switches::kRendererClientId)) {
nit: When would this happen? Only tests? If so, can you document that it's the
case?

https://codereview.chromium.org/2447143003/diff/160001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1595: &client_id);
Parsing strings (twice) seems like a bit of a heavy cost for something that
looks cheap ("get client id"). Can we do this in the constructor?

[Alex Z., 2016-11-07 19:37:37]

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

[Alex Z., 2016-11-07 19:37:44]

https://codereview.chromium.org/2447143003/diff/160001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2447143003/diff/160001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1589: switches::kRendererClientId)) {
On 2016/10/31 20:34:09, piman wrote:
> nit: When would this happen? Only tests? If so, can you document that it's the
> case?

This happens when running in single-process mode for debugging purposes. I have
documented it.

https://codereview.chromium.org/2447143003/diff/160001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1595: &client_id);
On 2016/10/31 20:34:09, piman wrote:
> Parsing strings (twice) seems like a bit of a heavy cost for something that
> looks cheap ("get client id"). Can we do this in the constructor?

Done.

[commit-bot: I haz the power, 2016-11-07 19:38:22]

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

[piman, 2016-11-07 20:02:34]

lgtm

[commit-bot: I haz the power, 2016-11-07 20:59:01]

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

[commit-bot: I haz the power, 2016-11-07 20:59:02]

Dry run: This issue passed the CQ dry run.

[Alex Z., 2016-11-07 20:59:59]

The CQ bit was checked by staraz@chromium.org

[Alex Z., 2016-11-07 21:00:00]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2447143003/#ps180001
(title: "Added RenderThreadImpl::client_id_ to store client ID and avoid
repetitive string parsing")

[commit-bot: I haz the power, 2016-11-07 21:00:30]

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, 2016-11-07 21:07:41]

Description was changed from

==========
Added a command line switch kRenderClientId. It holds the process ID of a
browser process and is passed to a renderer process when the render process is
created. RenderWidget stores the client ID into its client_id_ field and use it
to construct a FrameSinkID when RenderWidgetCompositor is created in
RenderWidget::InitializeLayerTreeView.
    
This change should fix a bug where SurfaceSequence::frame_sink_id is empty in
SurfaceLayer::SatisfyDestroySequence().

BUG=656639
==========

to

==========
Added a command line switch kRenderClientId. It holds the process ID of a
browser process and is passed to a renderer process when the render process is
created. RenderWidget stores the client ID into its client_id_ field and use it
to construct a FrameSinkID when RenderWidgetCompositor is created in
RenderWidget::InitializeLayerTreeView.
    
This change should fix a bug where SurfaceSequence::frame_sink_id is empty in
SurfaceLayer::SatisfyDestroySequence().

BUG=656639
==========

[commit-bot: I haz the power, 2016-11-07 21:07:43]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-11-07 21:19:15]

Description was changed from

==========
Added a command line switch kRenderClientId. It holds the process ID of a
browser process and is passed to a renderer process when the render process is
created. RenderWidget stores the client ID into its client_id_ field and use it
to construct a FrameSinkID when RenderWidgetCompositor is created in
RenderWidget::InitializeLayerTreeView.
    
This change should fix a bug where SurfaceSequence::frame_sink_id is empty in
SurfaceLayer::SatisfyDestroySequence().

BUG=656639
==========

to

==========
Added a command line switch kRenderClientId. It holds the process ID of a
browser process and is passed to a renderer process when the render process is
created. RenderWidget stores the client ID into its client_id_ field and use it
to construct a FrameSinkID when RenderWidgetCompositor is created in
RenderWidget::InitializeLayerTreeView.

This change should fix a bug where SurfaceSequence::frame_sink_id is empty in
SurfaceLayer::SatisfyDestroySequence().

BUG=656639

Committed: https://crrev.com/067f5824a3d561b9a16774fed995c23b6854405f
Cr-Commit-Position: refs/heads/master@{#430375}
==========

[commit-bot: I haz the power, 2016-11-07 21:19:16]

Patchset 10 (id:??) landed as
https://crrev.com/067f5824a3d561b9a16774fed995c23b6854405f
Cr-Commit-Position: refs/heads/master@{#430375}