blimp: Add logging for compositor and render widget feature.

blimp: Add logging for compositor and render widget feature.

Add some logging for debugging to the compositor and render widget
features.
BUG=596797
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/7d0ee4cc3bd5d7ab207467fa17e82fa3bb05126e
Cr-Commit-Position: refs/heads/master@{#384477}

[Khushal, 2016-03-30 00:35:23]

Description was changed from

==========
blimp: Add logging for compositor and render widget feature.

Add some logging for debugging to the compositor and render widget
features.
BUG=596797
==========

to

==========
blimp: Add logging for compositor and render widget feature.

Add some logging for debugging to the compositor and render widget
features.
BUG=596797
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Khushal, 2016-03-30 00:36:08]

khushalsagar@chromium.org changed reviewers:
+ haibinlu@chromium.org, vmpstr@chromium.org

[haibinlu, 2016-03-30 18:37:08]

https://codereview.chromium.org/1840843005/diff/1/blimp/client/feature/compos...
File blimp/client/feature/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/1840843005/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.cc:224: LOG(ERROR) <<
"Destroying LayerTreeHost for render widget: "
VLOG(1)

https://codereview.chromium.org/1840843005/diff/1/blimp/client/feature/compos...
File blimp/client/feature/compositor/blimp_compositor_manager.cc (right):

https://codereview.chromium.org/1840843005/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor_manager.cc:89: VLOG(1) <<
"Received render widget initialized message for: "
redundant log given the 2 logs below?

https://codereview.chromium.org/1840843005/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor_manager.cc:125: VLOG(1) <<
"Compositor message received for render_widget: "
this is already logged in net layer.

https://codereview.chromium.org/1840843005/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor_manager.cc:189: VLOG(1) <<
"Sending input event for render_widget: " << render_widget_id;
ditto. logged in net layer.

https://codereview.chromium.org/1840843005/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor_manager.cc:198: VLOG(1) <<
"Sending compositor message for render_widget: "
ditto

https://codereview.chromium.org/1840843005/diff/1/blimp/engine/app/blimp_cont...
File blimp/engine/app/blimp_content_main_delegate.cc (right):

https://codereview.chromium.org/1840843005/diff/1/blimp/engine/app/blimp_cont...
blimp/engine/app/blimp_content_main_delegate.cc:25: "vmodule",
"remote_channel_main=1");
Do we need this? since we have reliable message delivery, main&impl message
exchange will be logged twice, one in Engine and once in Client, and they have
the same info.

[vmpstr, 2016-03-30 18:45:53]

We don't typically do VLOGs in cc. Is it possible for you to use traces instead?

[Khushal, 2016-03-30 18:55:47]

https://codereview.chromium.org/1840843005/diff/1/blimp/client/feature/compos...
File blimp/client/feature/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/1840843005/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.cc:224: LOG(ERROR) <<
"Destroying LayerTreeHost for render widget: "
On 2016/03/30 18:37:08, haibinlu wrote:
> VLOG(1)

Done.

https://codereview.chromium.org/1840843005/diff/1/blimp/client/feature/compos...
File blimp/client/feature/compositor/blimp_compositor_manager.cc (right):

https://codereview.chromium.org/1840843005/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor_manager.cc:89: VLOG(1) <<
"Received render widget initialized message for: "
On 2016/03/30 18:37:08, haibinlu wrote:
> redundant log given the 2 logs below?

Done.

https://codereview.chromium.org/1840843005/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor_manager.cc:125: VLOG(1) <<
"Compositor message received for render_widget: "
On 2016/03/30 18:37:08, haibinlu wrote:
> this is already logged in net layer. 

Does it log the render widget id as well?

https://codereview.chromium.org/1840843005/diff/1/blimp/engine/app/blimp_cont...
File blimp/engine/app/blimp_content_main_delegate.cc (right):

https://codereview.chromium.org/1840843005/diff/1/blimp/engine/app/blimp_cont...
blimp/engine/app/blimp_content_main_delegate.cc:25: "vmodule",
"remote_channel_main=1");
On 2016/03/30 18:37:08, haibinlu wrote:
> Do we need this? since we have reliable message delivery, main&impl message
> exchange will be logged twice, one in Engine and once in Client, and they have
> the same info.

A message might be dropped if it exceeds the limit. Also, there might be an
error during the begin main frame and serialization step, would help in
debugging to know what the state the engine compositor was at when an error
occurred.

[Khushal, 2016-03-30 19:00:20]

On 2016/03/30 18:45:53, vmpstr wrote:
> We don't typically do VLOGs in cc. Is it possible for you to use traces
instead?

This is to get more logs in the bug reports captured as the app is being used
regularly. We can only get logs in these reports.

[haibinlu, 2016-03-30 19:33:11]

https://codereview.chromium.org/1840843005/diff/1/blimp/client/feature/compos...
File blimp/client/feature/compositor/blimp_compositor_manager.cc (right):

https://codereview.chromium.org/1840843005/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor_manager.cc:125: VLOG(1) <<
"Compositor message received for render_widget: "
On 2016/03/30 18:55:47, Khushal wrote:
> On 2016/03/30 18:37:08, haibinlu wrote:
> > this is already logged in net layer. 
> 
> Does it log the render widget id as well?

No, it does not. but you can add it in common/logging.cc

lass CompositorLogExtractor : public LogExtractor {
  void ExtractFields(const BlimpMessage& message,
                     LogFields* output) const override {
    AddField("render_widget_id", message.compositor().render_widget_id(),
output);
  }
};

[danakj, 2016-03-30 19:59:14]

On Wed, Mar 30, 2016 at 12:00 PM, <khushalsagar@chromium.org> wrote:

> On 2016/03/30 18:45:53, vmpstr wrote:
> > We don't typically do VLOGs in cc. Is it possible for you to use traces
> instead?
>
> This is to get more logs in the bug reports captured as the app is being
> used
> regularly. We can only get logs in these reports.
>

Why can't you trace?

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

[Khushal, 2016-03-31 01:26:17]

On 2016/03/30 19:59:14, danakj wrote:
> On Wed, Mar 30, 2016 at 12:00 PM, <mailto:khushalsagar@chromium.org> wrote:
> 
> > On 2016/03/30 18:45:53, vmpstr wrote:
> > > We don't typically do VLOGs in cc. Is it possible for you to use traces
> > instead?
> >
> > This is to get more logs in the bug reports captured as the app is being
> > used
> > regularly. We can only get logs in these reports.
> >
> 
> Why can't you trace?

We want to make it easy for users to report bugs, as the app is being tested, by
just uploading an android bug report which would have these logs. These are
VLOGS so they will not be enabled in the actual production builds going forward.

Also, the blimp client doesn't support capturing traces like clank yet...

> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[vmpstr, 2016-03-31 18:29:18]

Since this is only in the blimp specific code, it lgtm (modulo danakj's review).

[Khushal, 2016-03-31 18:42:04]

https://codereview.chromium.org/1840843005/diff/1/blimp/client/feature/compos...
File blimp/client/feature/compositor/blimp_compositor_manager.cc (right):

https://codereview.chromium.org/1840843005/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor_manager.cc:125: VLOG(1) <<
"Compositor message received for render_widget: "
On 2016/03/30 19:33:11, haibinlu wrote:
> On 2016/03/30 18:55:47, Khushal wrote:
> > On 2016/03/30 18:37:08, haibinlu wrote:
> > > this is already logged in net layer. 
> > 
> > Does it log the render widget id as well?
> 
> No, it does not. but you can add it in common/logging.cc
> 
> lass CompositorLogExtractor : public LogExtractor {
>   void ExtractFields(const BlimpMessage& message,
>                      LogFields* output) const override {
>     AddField("render_widget_id", message.compositor().render_widget_id(),
> output);
>   }
> };
> 
> 

That's neat. Done.

[danakj, 2016-03-31 18:44:58]

Description was changed from

==========
blimp: Add logging for compositor and render widget feature.

Add some logging for debugging to the compositor and render widget
features.
BUG=596797
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
blimp: Add logging for compositor and render widget feature.

Add some logging for debugging to the compositor and render widget
features.
BUG=596797
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[danakj, 2016-03-31 18:44:58]

danakj@chromium.org changed reviewers:
- danakj@chromium.org

[danakj, 2016-03-31 18:45:22]

On 2016/03/31 18:29:18, vmpstr wrote:
> Since this is only in the blimp specific code, it lgtm (modulo danakj's
review).

I'm happy if vmpstr's happy.

[vmpstr, 2016-03-31 18:46:37]

cc lgtm

[haibinlu, 2016-03-31 19:06:08]

https://codereview.chromium.org/1840843005/diff/40001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_compositor_manager.cc (right):

https://codereview.chromium.org/1840843005/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_manager.cc:81: VLOG(1) <<
"Creating Blimp Compositor for render widget: "
remove? duplicate msg with RenderWidgetLogExtractor

https://codereview.chromium.org/1840843005/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_manager.cc:103: VLOG(1) <<
"Activating compositor for render widget: " << render_widget_id;
remove? duplicate msg with RenderWidgetLogExtractor

https://codereview.chromium.org/1840843005/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_manager.cc:109: VLOG(1) <<
"Destroying compositor for render widget: " << render_widget_id;
remove? duplicate msg with RenderWidgetLogExtractor

https://codereview.chromium.org/1840843005/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_manager.cc:123: VLOG(1) <<
"Compositor message received for render_widget: "
remove? duplicate msg with CompositorLogExtractor

[Khushal, 2016-03-31 19:11:46]

https://codereview.chromium.org/1840843005/diff/40001/blimp/client/feature/co...
File blimp/client/feature/compositor/blimp_compositor_manager.cc (right):

https://codereview.chromium.org/1840843005/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_manager.cc:81: VLOG(1) <<
"Creating Blimp Compositor for render widget: "
On 2016/03/31 19:06:08, haibinlu wrote:
> remove? duplicate msg with RenderWidgetLogExtractor

Done.

https://codereview.chromium.org/1840843005/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_manager.cc:103: VLOG(1) <<
"Activating compositor for render widget: " << render_widget_id;
On 2016/03/31 19:06:08, haibinlu wrote:
> remove? duplicate msg with RenderWidgetLogExtractor

Done.

https://codereview.chromium.org/1840843005/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_manager.cc:109: VLOG(1) <<
"Destroying compositor for render widget: " << render_widget_id;
On 2016/03/31 19:06:08, haibinlu wrote:
> remove? duplicate msg with RenderWidgetLogExtractor

Done.

https://codereview.chromium.org/1840843005/diff/40001/blimp/client/feature/co...
blimp/client/feature/compositor/blimp_compositor_manager.cc:123: VLOG(1) <<
"Compositor message received for render_widget: "
On 2016/03/31 19:06:08, haibinlu wrote:
> remove? duplicate msg with CompositorLogExtractor

Done.

[haibinlu, 2016-03-31 19:40:15]

lgtm for blimp/

[Khushal, 2016-03-31 19:44:23]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-03-31 19:44:24]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/1840843005/#ps60001
(title: "Remove unnecessary logs.")

[commit-bot: I haz the power, 2016-03-31 19:44:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1840843005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1840843005/60001

[commit-bot: I haz the power, 2016-03-31 21:50:25]

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

[commit-bot: I haz the power, 2016-03-31 21:50:27]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Khushal, 2016-04-01 00:19:51]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-04-01 00:19:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
haibinlu@chromium.org, vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/1840843005/#ps80001
(title: "Missed a test.")

[commit-bot: I haz the power, 2016-04-01 00:20:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1840843005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1840843005/80001

[commit-bot: I haz the power, 2016-04-01 03:39:31]

Description was changed from

==========
blimp: Add logging for compositor and render widget feature.

Add some logging for debugging to the compositor and render widget
features.
BUG=596797
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
blimp: Add logging for compositor and render widget feature.

Add some logging for debugging to the compositor and render widget
features.
BUG=596797
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-04-01 03:39:32]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-01 03:41:25]

Description was changed from

==========
blimp: Add logging for compositor and render widget feature.

Add some logging for debugging to the compositor and render widget
features.
BUG=596797
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
blimp: Add logging for compositor and render widget feature.

Add some logging for debugging to the compositor and render widget
features.
BUG=596797
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/7d0ee4cc3bd5d7ab207467fa17e82fa3bb05126e
Cr-Commit-Position: refs/heads/master@{#384477}
==========

[commit-bot: I haz the power, 2016-04-01 03:41:26]

Patchset 5 (id:??) landed as
https://crrev.com/7d0ee4cc3bd5d7ab207467fa17e82fa3bb05126e
Cr-Commit-Position: refs/heads/master@{#384477}