Allow multiple logging::LogMessage{Handler,Listener}s

Allow multiple logging::LogMessage{Handler,Listener}s

Some tests need to add a message handler, but not necessarily want to
replace the existing one. Some tests emulate stack by calling the old
handler if present, while most just ignore the old one. At the end of
the test, some of them restore the old handler, while others just reset
it to nullptr.

Using a stack would preserve the existing handlers, and keeps a
consistent semantics in handler manipulation. Returning true in the
handler still hijacks the handling, and the calling order is from top to
bottom.

Furthermore, sometimes hijacking is not necessary, so a new log message
listener API is added. The listeners couldn't hijack the messages, and
the calling order is not guaranteed.

This enables https://crrev.com/2013573007/ to receive log messages even
when derived classes need their own handler.

BUG=638050
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[wychen, 2016-06-08 05:29:11]

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

[commit-bot: I haz the power, 2016-06-08 05:29:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2034393004/100001

[wychen, 2016-06-08 05:36:55]

Description was changed from

==========
LogMessageHandler in stack

BUG=
==========

to

==========
Turn logging::LogMessageHandler to a stack

Some tests need to override the message handler, but not necessarily want
to replace the existing one. Some of them restore the old handler, while others
just reset it to nullptr.

Using a stack would preserve the existing ones, and keeps a consistent
semantics.
Returning true in the handler still hijack the handling, and the calling order
is from
top to bottom.

This enables https://crrev.com/2013573007/ to receive log messages even when
derived classes need their own handler.

BUG=
==========

[wychen, 2016-06-08 05:40:19]

wychen@chromium.org changed reviewers:
+ brettw@chromium.org, mark@chromium.org, thakis@chromium.org

[wychen, 2016-06-08 05:40:20]

PTAL

[wychen, 2016-06-08 05:44:51]

Description was changed from

==========
Turn logging::LogMessageHandler to a stack

Some tests need to override the message handler, but not necessarily want
to replace the existing one. Some of them restore the old handler, while others
just reset it to nullptr.

Using a stack would preserve the existing ones, and keeps a consistent
semantics.
Returning true in the handler still hijack the handling, and the calling order
is from
top to bottom.

This enables https://crrev.com/2013573007/ to receive log messages even when
derived classes need their own handler.

BUG=
==========

to

==========
Turn logging::LogMessageHandler to a stack

Some tests need to override the message handler, but not necessarily
want to replace the existing one. Some of them restore the old handler,
while others just reset it to nullptr.

Using a stack would preserve the existing ones, and keeps a consistent
semantics. Returning true in the handler still hijack the handling, and
the calling order is from top to bottom.

This enables https://crrev.com/2013573007/ to receive log messages even
when derived classes need their own handler.

BUG=
==========

[wychen, 2016-06-08 06:54:53]

Description was changed from

==========
Turn logging::LogMessageHandler to a stack

Some tests need to override the message handler, but not necessarily
want to replace the existing one. Some of them restore the old handler,
while others just reset it to nullptr.

Using a stack would preserve the existing ones, and keeps a consistent
semantics. Returning true in the handler still hijack the handling, and
the calling order is from top to bottom.

This enables https://crrev.com/2013573007/ to receive log messages even
when derived classes need their own handler.

BUG=
==========

to

==========
Turn logging::LogMessageHandler to a stack

Some tests need to add a message handler, but not necessarily want to
replace the existing one. Some tests emulate stack by calling the old
handler if present, while most just ignore the old one. At the end of
the test, some of them restore the old handler, while others just reset
it to nullptr.

Using a stack would preserve the existing handlers, and keeps a
consistent semantics in handler manipulation. Returning true in the
handler still hijacks the handling, and the calling order is from top to
bottom.

This enables https://crrev.com/2013573007/ to receive log messages even
when derived classes need their own handler.

BUG=
==========

[commit-bot: I haz the power, 2016-06-08 08:51:51]

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

[commit-bot: I haz the power, 2016-06-08 08:51:52]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-06-09 18:52:50]

I think this CL is pretty awesome, and for reviewers: this was partially (or
maybe totally?) motivated by the desire to add 2+ log handlers in the same
browser test (in the same process, a super and a derived class both sniffing
logs).

Super (InProcBrowsertest): https://codereview.chromium.org/2013573007/
Derived ({WebUI,Javascript}BrowserTest):
https://codereview.chromium.org/2045843002/

I've always felt a bit squirmy regarding how WebUI tests have caught console
errors (by overriding a global log handler).  I was basically proposing
something exactly like this yesterday to thestig@/danakj@.

This CL is an great step in improving log sniffing, BUT maybe rather than making
this a stack + pop/push as an API, we instead do something like:

  unique_ptr<logging::ScopedHandle> AddLogMessageHandler(callback)

and when the returned handle goes out of scope, it unregisters that /specific/
callback from getting messages.

This reduces the amount of explicit deregistration cruft and makes the whole
operation less error-prone (in theory) and avoids ordering issues with a global
pop/push (part of this CL checks whether the top handler is the right one to
pop... but most places don't).

wychen/others: what do you think?

https://codereview.chromium.org/2034393004/diff/100001/remoting/client/plugin...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/2034393004/diff/100001/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:1037:
logging::PushLogMessageHandler(&LogToUI);
does this call ever need to be balanced by a PopLogMessageHandler()?

[wychen, 2016-06-09 21:03:35]

On 2016/06/09 18:52:50, Dan Beam wrote:
> I think this CL is pretty awesome, and for reviewers: this was partially (or
> maybe totally?) motivated by the desire to add 2+ log handlers in the same
> browser test (in the same process, a super and a derived class both sniffing
> logs).
> 
> Super (InProcBrowsertest): https://codereview.chromium.org/2013573007/
> Derived ({WebUI,Javascript}BrowserTest):
> https://codereview.chromium.org/2045843002/
> 
> I've always felt a bit squirmy regarding how WebUI tests have caught console
> errors (by overriding a global log handler).  I was basically proposing
> something exactly like this yesterday to thestig@/danakj@.
> 
> This CL is an great step in improving log sniffing, BUT maybe rather than
making
> this a stack + pop/push as an API, we instead do something like:
> 
>   unique_ptr<logging::ScopedHandle> AddLogMessageHandler(callback)
> 
> and when the returned handle goes out of scope, it unregisters that /specific/
> callback from getting messages.
> 
> This reduces the amount of explicit deregistration cruft and makes the whole
> operation less error-prone (in theory) and avoids ordering issues with a
global
> pop/push (part of this CL checks whether the top handler is the right one to
> pop... but most places don't).
> 
> wychen/others: what do you think?

Thanks for your comments!

Message handlers can hijack the message by returning true. The reason I picked
stack is to maintain a good semantics about what it means to hijack. If we
call these handlers from stack top, the most recent one has the highest
priority to hijack it.

Scoped handle is very convenient, but we might need another callback type that
cannot hijack message handling. Otherwise the ordering is hard to determine.
Most of the message handling don't really need to hijack anyway.

The stack is error-prone in the sense that you could pop the wrong one.
What if we add an callback argument to the pop function, and assert it's the
top?

[wychen, 2016-06-09 21:04:18]

https://codereview.chromium.org/2034393004/diff/100001/remoting/client/plugin...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/2034393004/diff/100001/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:1037:
logging::PushLogMessageHandler(&LogToUI);
On 2016/06/09 18:52:50, Dan Beam wrote:
> does this call ever need to be balanced by a PopLogMessageHandler()?

I was a bit curious when I changed this. The handler wasn't reset to nullptr
ever, so I guess not.

[Dan Beam, 2016-07-01 19:28:56]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-07-01 19:28:57]

what's the status on this?

[wychen, 2016-07-06 22:41:26]

On 2016/07/01 19:28:57, Dan Beam wrote:
> what's the status on this?

I plan to add message listeners, which work similar to message handlers, but
return void, so they couldn't hijack the messages.
For message listeners, we use scoped handle like what you proposed to make it
easier to use and less error prone.

For message handlers, we still use stack, so the hijacking order is
well-defined.
To avoid popping the wrong one, besides pop(), we also add one convenient
pop(expected) method to assert |expected| is top().

Does this sound good?

[Dan Beam, 2016-07-06 23:01:02]

On 2016/07/06 22:41:26, wychen wrote:
> On 2016/07/01 19:28:57, Dan Beam wrote:
> > what's the status on this?
> 
> I plan to add message listeners, which work similar to message handlers, but
> return void, so they couldn't hijack the messages.
> For message listeners, we use scoped handle like what you proposed to make it
> easier to use and less error prone.
> 
> For message handlers, we still use stack, so the hijacking order is
> well-defined.
> To avoid popping the wrong one, besides pop(), we also add one convenient
> pop(expected) method to assert |expected| is top().

I think we should only allow pop(expected)

> 
> Does this sound good?

do any tests actually require handling messages?

[wychen, 2016-07-07 00:35:59]

On 2016/07/06 23:01:02, Dan Beam wrote:
> On 2016/07/06 22:41:26, wychen wrote:
> > On 2016/07/01 19:28:57, Dan Beam wrote:
> > > what's the status on this?
> > 
> > I plan to add message listeners, which work similar to message handlers, but
> > return void, so they couldn't hijack the messages.
> > For message listeners, we use scoped handle like what you proposed to make
it
> > easier to use and less error prone.
> > 
> > For message handlers, we still use stack, so the hijacking order is
> > well-defined.
> > To avoid popping the wrong one, besides pop(), we also add one convenient
> > pop(expected) method to assert |expected| is top().
> 
> I think we should only allow pop(expected)
Sounds good.

> > 
> > Does this sound good?
> 
> do any tests actually require handling messages?

Examples here:
https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi...
https://cs.chromium.org/chromium/src/chrome/installer/util/installation_valid...
https://cs.chromium.org/chromium/src/chrome/test/chromedriver/logging.cc?sq=p...
https://cs.chromium.org/chromium/src/components/proximity_auth/logging/loggin...
etc

[wychen, 2016-07-08 00:26:41]

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

[commit-bot: I haz the power, 2016-07-08 00:27:25]

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-07-08 02:29:18]

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

[commit-bot: I haz the power, 2016-07-08 02:29:19]

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

[wychen, 2016-07-08 07:54:38]

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

[commit-bot: I haz the power, 2016-07-08 07:55:19]

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-07-08 11:59:41]

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

[commit-bot: I haz the power, 2016-07-08 11:59:42]

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

[wychen, 2016-07-09 00:33:50]

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

[commit-bot: I haz the power, 2016-07-09 00:34:19]

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

[wychen, 2016-07-09 00:43:12]

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

[commit-bot: I haz the power, 2016-07-09 00:43:31]

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-07-09 01:19:53]

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

[commit-bot: I haz the power, 2016-07-09 01:19:54]

Dry run: Try jobs failed on following builders:
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[wychen, 2016-07-09 07:00:51]

Patchset #12 (id:220001) has been deleted

[wychen, 2016-07-09 07:01:03]

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

[commit-bot: I haz the power, 2016-07-09 07:01:14]

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

[wychen, 2016-07-09 07:05:32]

Description was changed from

==========
Turn logging::LogMessageHandler to a stack

Some tests need to add a message handler, but not necessarily want to
replace the existing one. Some tests emulate stack by calling the old
handler if present, while most just ignore the old one. At the end of
the test, some of them restore the old handler, while others just reset
it to nullptr.

Using a stack would preserve the existing handlers, and keeps a
consistent semantics in handler manipulation. Returning true in the
handler still hijacks the handling, and the calling order is from top to
bottom.

This enables https://crrev.com/2013573007/ to receive log messages even
when derived classes need their own handler.

BUG=
==========

to

==========
Allow multiple logging::LogMessage{Handler,Listener}s

Some tests need to add a message handler, but not necessarily want to
replace the existing one. Some tests emulate stack by calling the old
handler if present, while most just ignore the old one. At the end of
the test, some of them restore the old handler, while others just reset
it to nullptr.

Using a stack would preserve the existing handlers, and keeps a
consistent semantics in handler manipulation. Returning true in the
handler still hijacks the handling, and the calling order is from top to
bottom.

Furthermore, sometimes hijacking is not necessary, so a new log message
listener API is added, including scoped version and traditional add/remove
interfaces. The listeners couldn't hijack the messages, and the calling order
is not guaranteed.

This enables https://crrev.com/2013573007/ to receive log messages even
when derived classes need their own handler.

BUG=
==========

[commit-bot: I haz the power, 2016-07-09 07:54:15]

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

[commit-bot: I haz the power, 2016-07-09 07:54:15]

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

[wychen, 2016-07-09 11:02:53]

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

[commit-bot: I haz the power, 2016-07-09 11:03:01]

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-07-09 13:03:36]

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

[commit-bot: I haz the power, 2016-07-09 13:03:37]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng 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)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[wychen, 2016-07-09 21:00:57]

Patchset #10 (id:180001) has been deleted

[wychen, 2016-07-09 21:01:15]

Patchset #10 (id:200001) has been deleted

[wychen, 2016-07-09 21:01:31]

Patchset #10 (id:240001) has been deleted

[wychen, 2016-07-09 21:21:20]

PTAL

https://codereview.chromium.org/2034393004/diff/260001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2034393004/diff/260001/base/logging.h#newcode302
base/logging.h:302: ScopedLogMessageListener(LogMessageListenerFunction
listener);
explicit?

https://codereview.chromium.org/2034393004/diff/260001/base/logging_win.cc
File base/logging_win.cc (right):

https://codereview.chromium.org/2034393004/diff/260001/base/logging_win.cc#ne...
base/logging_win.cc:91: // Don't increase verbosity in other log destinations
below this handler
The verbosity is still increased for handlers above the current one in the
stack, and all listeners. Is this filtering still useful? If not, we can convert
this to a listener as well.

[Dan Beam, 2016-07-15 03:11:19]

https://codereview.chromium.org/2034393004/diff/260001/base/test/mock_log.cc
File base/test/mock_log.cc (right):

https://codereview.chromium.org/2034393004/diff/260001/base/test/mock_log.cc#...
base/test/mock_log.cc:12: Lock MockLog::g_lock;
does this class still need to stay global in nature?

https://codereview.chromium.org/2034393004/diff/260001/chrome/browser/media/w...
File chrome/browser/media/webrtc_browsertest_base.cc (right):

https://codereview.chromium.org/2034393004/diff/260001/chrome/browser/media/w...
chrome/browser/media/webrtc_browsertest_base.cc:121: :
listener(JavascriptErrorDetectingLogListener),
awesome

https://codereview.chromium.org/2034393004/diff/260001/chrome/browser/media/w...
File chrome/browser/media/webrtc_browsertest_base.h (right):

https://codereview.chromium.org/2034393004/diff/260001/chrome/browser/media/w...
chrome/browser/media/webrtc_browsertest_base.h:189:
logging::ScopedLogMessageListener listener;
nit: log_listener_

https://codereview.chromium.org/2034393004/diff/260001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/ntp/new_tab_ui_browsertest.cc (right):

https://codereview.chromium.org/2034393004/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/ntp/new_tab_ui_browsertest.cc:50:
logging::ScopedLogMessageListener listener;
all of these should be log_listener_ IMO or at least listener_ <-- we use
trailing _ for private members

https://codereview.chromium.org/2034393004/diff/260001/chrome/installer/util/...
File chrome/installer/util/installation_validation_helper.cc (right):

https://codereview.chromium.org/2034393004/diff/260001/chrome/installer/util/...
chrome/installer/util/installation_validation_helper.cc:60:
logging::PopLogMessageHandler(AddFailureForLogMessage);
awesome

https://codereview.chromium.org/2034393004/diff/260001/remoting/host/native_m...
File remoting/host/native_messaging/log_message_handler.cc (right):

https://codereview.chromium.org/2034393004/diff/260001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:48: }
^ do we still need this?

https://codereview.chromium.org/2034393004/diff/260001/third_party/crashpad/c...
File third_party/crashpad/crashpad/util/thread/OWNERS (right):

https://codereview.chromium.org/2034393004/diff/260001/third_party/crashpad/c...
third_party/crashpad/crashpad/util/thread/OWNERS:2: per-file
*_messages.cc=file://ipc/SECURITY_OWNERS
¿qué?

[wychen, 2016-07-17 23:12:39]

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

[commit-bot: I haz the power, 2016-07-17 23:12:51]

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-07-18 00:48:06]

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

[commit-bot: I haz the power, 2016-07-18 00:48:07]

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

[wychen, 2016-07-18 15:44:13]

https://codereview.chromium.org/2034393004/diff/260001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2034393004/diff/260001/base/logging.h#newcode302
base/logging.h:302: ScopedLogMessageListener(LogMessageListenerFunction
listener);
On 2016/07/09 21:21:19, wychen wrote:
> explicit?

Done.

https://codereview.chromium.org/2034393004/diff/260001/base/test/mock_log.cc
File base/test/mock_log.cc (right):

https://codereview.chromium.org/2034393004/diff/260001/base/test/mock_log.cc#...
base/test/mock_log.cc:12: Lock MockLog::g_lock;
On 2016/07/15 03:11:19, Dan Beam wrote:
> does this class still need to stay global in nature?

Since LazyInstance is thread safe, I think we don't even need to lock.

https://codereview.chromium.org/2034393004/diff/260001/chrome/browser/media/w...
File chrome/browser/media/webrtc_browsertest_base.h (right):

https://codereview.chromium.org/2034393004/diff/260001/chrome/browser/media/w...
chrome/browser/media/webrtc_browsertest_base.h:189:
logging::ScopedLogMessageListener listener;
On 2016/07/15 03:11:19, Dan Beam wrote:
> nit: log_listener_

Done.

https://codereview.chromium.org/2034393004/diff/260001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/ntp/new_tab_ui_browsertest.cc (right):

https://codereview.chromium.org/2034393004/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/ntp/new_tab_ui_browsertest.cc:50:
logging::ScopedLogMessageListener listener;
On 2016/07/15 03:11:19, Dan Beam wrote:
> all of these should be log_listener_ IMO or at least listener_ <-- we use
> trailing _ for private members

Oops.

https://codereview.chromium.org/2034393004/diff/260001/remoting/host/native_m...
File remoting/host/native_messaging/log_message_handler.cc (right):

https://codereview.chromium.org/2034393004/diff/260001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:48: }
On 2016/07/15 03:11:19, Dan Beam wrote:
> ^ do we still need this?

Cleaned this up to simplify logic.

https://codereview.chromium.org/2034393004/diff/260001/third_party/crashpad/c...
File third_party/crashpad/crashpad/util/thread/OWNERS (right):

https://codereview.chromium.org/2034393004/diff/260001/third_party/crashpad/c...
third_party/crashpad/crashpad/util/thread/OWNERS:2: per-file
*_messages.cc=file://ipc/SECURITY_OWNERS
On 2016/07/15 03:11:19, Dan Beam wrote:
> ¿qué?

The presubmit script asked me to add this file.

[Dan Beam, 2016-07-18 20:00:36]

lgtm except for pointer vs lazyinstance distinction

yes, you're right (AFAIK), that if that code used a LazyInstance, that calling
.Get() would be thread-safe, but you need to use the class base::LazyInstance<T>
g_my_t = SOME_INITIALIZER_THING + g_my_t.Get() for it to work correctly

https://codereview.chromium.org/2034393004/diff/260001/base/test/mock_log.cc
File base/test/mock_log.cc (right):

https://codereview.chromium.org/2034393004/diff/260001/base/test/mock_log.cc#...
base/test/mock_log.cc:12: Lock MockLog::g_lock;
On 2016/07/18 15:44:12, wychen wrote:
> On 2016/07/15 03:11:19, Dan Beam wrote:
> > does this class still need to stay global in nature?
> 
> Since LazyInstance is thread safe, I think we don't even need to lock.

right, but this is a raw pointer, not a LazyInstance.

[Primiano Tucci (use gerrit), 2016-07-19 10:00:43]

primiano@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-07-19 10:00:44]

Drive by comment: please note that today the ~LogMessage dtor is very special,
as in the crash server pipeline looks for that to infer the magic signature (see
go/url_for_2034393004). that dtor is what ultimately triggers the ostream
flushing, and IIRC the crash server knows where to peek the bytes for the actual
log message when it sees that.

I am not familiar with the code in this CL, but please make sure that either you
don't change the lifetime of LogMessage or if you do update the server-side
code.

+wfh might be more familiar with this.

[wychen, 2016-07-19 18:10:33]

On 2016/07/19 10:00:44, Primiano Tucci wrote:
> Drive by comment: please note that today the ~LogMessage dtor is very special,
> as in the crash server pipeline looks for that to infer the magic signature
(see
> go/url_for_2034393004). that dtor is what ultimately triggers the ostream
> flushing, and IIRC the crash server knows where to peek the bytes for the
actual
> log message when it sees that.
> 
> I am not familiar with the code in this CL, but please make sure that either
you
> don't change the lifetime of LogMessage or if you do update the server-side
> code.
> 
> +wfh might be more familiar with this.

Thanks for the comment! I believe the lifetime of LogMessage is not changed in
this CL.

The log handler function can consume the message by returning true. Before this
CL, if there's already a consuming handler, we could replace it with a
non-consuming handler, so that all log messages would pass through. After this
CL, this is no longer possible because handlers are in a stack. This should not
be a problem, since there are currently no examples of this kind of usage.

[wychen, 2016-07-19 18:13:07]

https://codereview.chromium.org/2034393004/diff/260001/base/test/mock_log.cc
File base/test/mock_log.cc (right):

https://codereview.chromium.org/2034393004/diff/260001/base/test/mock_log.cc#...
base/test/mock_log.cc:12: Lock MockLog::g_lock;
On 2016/07/18 20:00:35, Dan Beam wrote:
> On 2016/07/18 15:44:12, wychen wrote:
> > On 2016/07/15 03:11:19, Dan Beam wrote:
> > > does this class still need to stay global in nature?
> > 
> > Since LazyInstance is thread safe, I think we don't even need to lock.
> 
> right, but this is a raw pointer, not a LazyInstance.

You are right. g_instance_ is still a raw pointer. I was referring to the
log_message_listeners behind AddLogMessageListener(), but that's not the whole
thing.

Could you clarify the first comment? Do you want to convert g_lock to an
instance variable? As long as g_instance_ is still static, I think g_lock still
need to be static.

[Dan Beam, 2016-07-19 19:07:24]

https://codereview.chromium.org/2034393004/diff/280001/base/test/mock_log.cc
File base/test/mock_log.cc (right):

https://codereview.chromium.org/2034393004/diff/280001/base/test/mock_log.cc#...
base/test/mock_log.cc:14: }
why can't the entirety of this file basically be:

MockLog::MockLog() : is_capturing_logs_(false) {}

MockLog::~MockLog() {
  if (is_capturing_logs_)
    StopCapturingLogs();
}

MockLog::StartCapturingLogs() {
  RAW_CHECK(!is_capturing_logs_);
  logging::AddLogMessageListener(Log);
  is_capturing_logs_ = true;
}

MockLog::StopCapturingLogs() {
  RAW_CHECK(is_capturing_logs_);
  logging::AddLogMessageListener(Log);
  is_capturing_logs_ = false;
}

why do we need g_instance_ or g_lock at all?

[Dan Beam, 2016-07-19 19:07:49]

https://codereview.chromium.org/2034393004/diff/280001/base/test/mock_log.cc
File base/test/mock_log.cc (right):

https://codereview.chromium.org/2034393004/diff/280001/base/test/mock_log.cc#...
base/test/mock_log.cc:14: }
On 2016/07/19 19:07:23, Dan Beam wrote:
> why can't the entirety of this file basically be:
> 
> MockLog::MockLog() : is_capturing_logs_(false) {}
> 
> MockLog::~MockLog() {
>   if (is_capturing_logs_)
>     StopCapturingLogs();
> }
> 
> MockLog::StartCapturingLogs() {
>   RAW_CHECK(!is_capturing_logs_);
>   logging::AddLogMessageListener(Log);
>   is_capturing_logs_ = true;
> }
> 
> MockLog::StopCapturingLogs() {
>   RAW_CHECK(is_capturing_logs_);
>   logging::AddLogMessageListener(Log);

Remove*

>   is_capturing_logs_ = false;
> }
> 
> why do we need g_instance_ or g_lock at all?

[wychen, 2016-07-19 20:56:23]

https://codereview.chromium.org/2034393004/diff/280001/base/test/mock_log.cc
File base/test/mock_log.cc (right):

https://codereview.chromium.org/2034393004/diff/280001/base/test/mock_log.cc#...
base/test/mock_log.cc:14: }
On 2016/07/19 19:07:49, Dan Beam wrote:
> why do we need g_instance_ or g_lock at all?

I think the reason is that the {Add,Remove}LogMessageListener
can only take static methods, but MOCK_METHOD(Log) is not
static. Therefore, a static wrapper method is introduced to
bridge the signals.

It seems gmock doesn't support static mocked method, and
base::Bind() couldn't be used because LogMessageListener is
a raw function pointer instead of Callback class.

Getting rid of g_instance_ and g_lock would be much nicer,
and could support multiple MockLog instances. Not sure how
useful it is though.

[Dan Beam, 2016-07-19 22:14:17]

On 2016/07/19 20:56:23, wychen wrote:
> https://codereview.chromium.org/2034393004/diff/280001/base/test/mock_log.cc
> File base/test/mock_log.cc (right):
> 
>
https://codereview.chromium.org/2034393004/diff/280001/base/test/mock_log.cc#...
> base/test/mock_log.cc:14: }
> On 2016/07/19 19:07:49, Dan Beam wrote:
> > why do we need g_instance_ or g_lock at all?
> 
> I think the reason is that the {Add,Remove}LogMessageListener
> can only take static methods, but MOCK_METHOD(Log) is not
> static. Therefore, a static wrapper method is introduced to
> bridge the signals.
> 
> It seems gmock doesn't support static mocked method, and
> base::Bind() couldn't be used because LogMessageListener is
> a raw function pointer instead of Callback class.
> 
> Getting rid of g_instance_ and g_lock would be much nicer,
> and could support multiple MockLog instances. Not sure how
> useful it is though.

ok, still lgtm then

[wychen, 2016-07-19 23:03:47]

On 2016/07/19 22:14:17, Dan Beam wrote:
> On 2016/07/19 20:56:23, wychen wrote:
> > https://codereview.chromium.org/2034393004/diff/280001/base/test/mock_log.cc
> > File base/test/mock_log.cc (right):
> > 
> >
>
https://codereview.chromium.org/2034393004/diff/280001/base/test/mock_log.cc#...
> > base/test/mock_log.cc:14: }
> > On 2016/07/19 19:07:49, Dan Beam wrote:
> > > why do we need g_instance_ or g_lock at all?
> > 
> > I think the reason is that the {Add,Remove}LogMessageListener
> > can only take static methods, but MOCK_METHOD(Log) is not
> > static. Therefore, a static wrapper method is introduced to
> > bridge the signals.
> > 
> > It seems gmock doesn't support static mocked method, and
> > base::Bind() couldn't be used because LogMessageListener is
> > a raw function pointer instead of Callback class.
> > 
> > Getting rid of g_instance_ and g_lock would be much nicer,
> > and could support multiple MockLog instances. Not sure how
> > useful it is though.
> 
> ok, still lgtm then

Added the lock back, since g_instance_ still needs it.

[brettw, 2016-07-20 23:36:37]

Sorry to come in here late.

Long background:

I was originally going to suggest trying to keep the logging API the same and
adding code to base test support for the observation functions. This handles
most of the cases exceot for gpu_main and components/crash.

The old code for gpu_main and components/crash both look really suspicious. I
don't see how these interact with the general setup in logging_chrome,
potentially they stomp on each other. Your patch improves this since it turns
both of those cases into an observer instead of a handler, so the ordering is
consistent.

Ideally we'd fix both of these cases (I think this should all be handled by
logging_chrome, but I don't want to completely derail this patch and I think
there's still some argument that this is a general case that we should be able
to support.

Suggestions:

I feel like the API would be cleaner and less easy to mess up if the API exposed
by base was a class that registers and unregisters itself via it's own lifetime,
and base doesn't expose the functions directly.

In the past with these observer functions we've had problems with people
forgetting to unregister in error cases and getting obscure crashes. We went
through quite some effort to convert the notification service to use a
registration object.

So in this scheme you'd have two classes, one for filtering and one for
listening. These have a virtual function LogMessage() on them, the constructor
registers and the destructor unregisters, and there are no other exposed
registration management functions.

What do you think about this?

[wychen, 2016-07-25 05:37:14]

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

[commit-bot: I haz the power, 2016-07-25 05:37:24]

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-07-25 05:38:54]

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

[commit-bot: I haz the power, 2016-07-25 05:38:55]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[wychen, 2016-07-25 05:56:12]

Patchset #13 (id:320001) has been deleted

[wychen, 2016-07-25 05:59:35]

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

[commit-bot: I haz the power, 2016-07-25 05:59:42]

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

[wychen, 2016-07-25 06:01:04]

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

[commit-bot: I haz the power, 2016-07-25 06:01:16]

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-07-25 06:17:30]

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

[commit-bot: I haz the power, 2016-07-25 06:17:31]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[wychen, 2016-07-25 07:46:16]

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

[commit-bot: I haz the power, 2016-07-25 07:46:31]

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

[wychen, 2016-07-25 20:05:27]

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

[wychen, 2016-07-25 20:05:54]

Patchset #16 (id:400001) has been deleted

[commit-bot: I haz the power, 2016-07-25 20:06:11]

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

[wychen, 2016-07-25 20:06:18]

Patchset #15 (id:380001) has been deleted

[wychen, 2016-07-25 20:06:38]

Patchset #14 (id:360001) has been deleted

[wychen, 2016-07-25 21:03:30]

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

[commit-bot: I haz the power, 2016-07-25 21:03:59]

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-07-25 21:38:33]

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

[commit-bot: I haz the power, 2016-07-25 21:38:35]

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

[wychen, 2016-07-25 22:36:26]

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

[commit-bot: I haz the power, 2016-07-25 22:36:49]

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-07-26 00:49:50]

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

[commit-bot: I haz the power, 2016-07-26 00:49:52]

Dry run: This issue passed the CQ dry run.

[wychen, 2016-07-26 01:25:14]

Patchset #15 (id:440001) has been deleted

[wychen, 2016-07-26 01:25:25]

Patchset #14 (id:420001) has been deleted

[wychen, 2016-07-27 02:11:13]

On 2016/07/20 23:36:37, brettw (ping on IM after 24h) wrote:
> Suggestions:
> 
> I feel like the API would be cleaner and less easy to mess up if the API
exposed
> by base was a class that registers and unregisters itself via it's own
lifetime,
> and base doesn't expose the functions directly.
> 
> In the past with these observer functions we've had problems with people
> forgetting to unregister in error cases and getting obscure crashes. We went
> through quite some effort to convert the notification service to use a
> registration object.
> 
> So in this scheme you'd have two classes, one for filtering and one for
> listening. These have a virtual function LogMessage() on them, the constructor
> registers and the destructor unregisters, and there are no other exposed
> registration management functions.
> 
> What do you think about this?

Sounds like a good idea. PTAL and see if this is what you meant.
With this change, the implementation of MockLog is much simpler, and allows
multiple instances. dbeam@, you might want to take a look. Thanks!

[grt (UTC plus 2), 2016-07-27 20:38:21]

some drive-by comments from a very brief partial read. i'm not adding myself as
a reviewer, so do what you will with these comments.

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc#newcod...
base/logging.cc:134:
base::LazyInstance<std::unordered_set<LogMessageListener*>>::Leaky
why an unordered set? are there really so many listeners that you need the extra
memory overhead for O(lgN) insert/remove? vector or deque seems adequate.

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc#newcod...
base/logging.cc:359: unsigned count = handlers.size();
size_t

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc#newcod...
base/logging.cc:571: for (auto* listeners : log_message_listeners.Get()) {
listeners -> listener

https://codereview.chromium.org/2034393004/diff/460001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2034393004/diff/460001/base/logging.h#newcode286
base/logging.h:286: explicit LogMessageHandler();
nit: omit "explicit" and make the ctor protected, moving it down below the
public section as per
https://google.github.io/styleguide/cppguide.html#Declaration_Order.

https://codereview.chromium.org/2034393004/diff/460001/base/logging.h#newcode303
base/logging.h:303: virtual void OnMessage(int severity, const char* file, int
line,
maybe it's simpler to replace this single-method interface with a CallbackList,
and have consumers of this API just add/remove their callback.

https://codereview.chromium.org/2034393004/diff/460001/base/logging_win.cc
File base/logging_win.cc (right):

https://codereview.chromium.org/2034393004/diff/460001/base/logging_win.cc#ne...
base/logging_win.cc:18: static std::unique_ptr<WinLogMessageHandler>
g_log_handler;
move this into the unnamed namespace and remove "static".
also, you can't use unique_ptr at global scope like this since it will introduce
a static ctor/dtor pair.

https://codereview.chromium.org/2034393004/diff/460001/base/logging_win.h
File base/logging_win.h (right):

https://codereview.chromium.org/2034393004/diff/460001/base/logging_win.h#new...
base/logging_win.h:54: class WinLogMessageHandler : LogMessageHandler {
move this into the private: section of LogEventProvider so you don't need the
"friend"?

https://codereview.chromium.org/2034393004/diff/460001/base/logging_win.h#new...
base/logging_win.h:55: bool OnMessage(logging::LogSeverity severity, const char*
file,
public:

[grt (UTC plus 2), 2016-07-28 06:28:48]

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc#newcod...
base/logging.cc:571: for (auto* listeners : log_message_listeners.Get()) {
since logging may happen on any thread at any time, how are you going to
guarantee that one thread isn't adding or removing a listener/handler while
another is doing these iterations?

https://codereview.chromium.org/2034393004/diff/460001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2034393004/diff/460001/base/logging.h#newcode303
base/logging.h:303: virtual void OnMessage(int severity, const char* file, int
line,
On 2016/07/27 20:38:21, grt (UTC plus 2) wrote:
> maybe it's simpler to replace this single-method interface with a
CallbackList,
> and have consumers of this API just add/remove their callback.

I see that this is counter to Brett's suggestion, so nevermind. Please clean up
these class definitions so that they comply with the style guide.

[wychen, 2016-07-30 23:54:06]

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

[commit-bot: I haz the power, 2016-07-30 23:54:12]

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-07-30 23:56:03]

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

[commit-bot: I haz the power, 2016-07-30 23:56:04]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[wychen, 2016-07-31 00:03:53]

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

[commit-bot: I haz the power, 2016-07-31 00:03:59]

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-07-31 00:40:30]

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

[commit-bot: I haz the power, 2016-07-31 00:40:31]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[wychen, 2016-07-31 00:53:49]

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

[commit-bot: I haz the power, 2016-07-31 00:54:00]

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

[wychen, 2016-08-01 07:11:09]

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

[commit-bot: I haz the power, 2016-08-01 07:11:25]

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

[wychen, 2016-08-01 07:13:25]

Patchset #17 (id:520001) has been deleted

[commit-bot: I haz the power, 2016-08-01 10:34:00]

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

[commit-bot: I haz the power, 2016-08-01 10:34:01]

Dry run: This issue passed the CQ dry run.

[wychen, 2016-08-01 16:10:37]

Patchset #17 (id:540001) has been deleted

[wychen, 2016-08-01 16:10:47]

Patchset #16 (id:500001) has been deleted

[wychen, 2016-08-01 16:10:56]

Patchset #15 (id:480001) has been deleted

[wychen, 2016-08-01 16:12:26]

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc#newcod...
base/logging.cc:134:
base::LazyInstance<std::unordered_set<LogMessageListener*>>::Leaky
On 2016/07/27 20:38:20, grt (UTC plus 2) wrote:
> why an unordered set? are there really so many listeners that you need the
extra
> memory overhead for O(lgN) insert/remove? vector or deque seems adequate.

The number of listeners should be very low practically, so I'm not worried about
asymptotic complexity. BTW, I think insert/remove is O(1) here. The reason I
used unordered_set is to avoid subscribers to assume ordering.
LogMessageHandlers are run in stack order, because the handlers can sink the
message. The calling order of LogMessageListeners doesn't matter from the
perspective of LogMessage, but subscribers are free to add side effects inside
the listener functions, and we don't want these side effects to depend on
certain calling order.

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc#newcod...
base/logging.cc:359: unsigned count = handlers.size();
On 2016/07/27 20:38:20, grt (UTC plus 2) wrote:
> size_t

Done.

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc#newcod...
base/logging.cc:571: for (auto* listeners : log_message_listeners.Get()) {
On 2016/07/27 20:38:20, grt (UTC plus 2) wrote:
> listeners -> listener

Done.

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc#newcod...
base/logging.cc:571: for (auto* listeners : log_message_listeners.Get()) {
On 2016/07/28 06:28:47, grt (UTC plus 2) wrote:
> since logging may happen on any thread at any time, how are you going to
> guarantee that one thread isn't adding or removing a listener/handler while
> another is doing these iterations?

I've added Locks.

https://codereview.chromium.org/2034393004/diff/460001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2034393004/diff/460001/base/logging.h#newcode286
base/logging.h:286: explicit LogMessageHandler();
On 2016/07/27 20:38:21, grt (UTC plus 2) wrote:
> nit: omit "explicit" and make the ctor protected, moving it down below the
> public section as per
> https://google.github.io/styleguide/cppguide.html#Declaration_Order.

Done.

https://codereview.chromium.org/2034393004/diff/460001/base/logging.h#newcode303
base/logging.h:303: virtual void OnMessage(int severity, const char* file, int
line,
On 2016/07/27 20:38:21, grt (UTC plus 2) wrote:
> maybe it's simpler to replace this single-method interface with a
CallbackList,
> and have consumers of this API just add/remove their callback.

CallbackList handles thread safety nicely, and I think it is faster than using a
lock. However, its type is only good for listeners, but not handlers, so we'll
have to keep LogMessageHandler anyway.

https://codereview.chromium.org/2034393004/diff/460001/base/logging.h#newcode303
base/logging.h:303: virtual void OnMessage(int severity, const char* file, int
line,
On 2016/07/28 06:28:48, grt (UTC plus 2) wrote:
> On 2016/07/27 20:38:21, grt (UTC plus 2) wrote:
> > maybe it's simpler to replace this single-method interface with a
> CallbackList,
> > and have consumers of this API just add/remove their callback.
> 
> I see that this is counter to Brett's suggestion, so nevermind. Please clean
up
> these class definitions so that they comply with the style guide.

Done.

https://codereview.chromium.org/2034393004/diff/460001/base/logging_win.cc
File base/logging_win.cc (right):

https://codereview.chromium.org/2034393004/diff/460001/base/logging_win.cc#ne...
base/logging_win.cc:18: static std::unique_ptr<WinLogMessageHandler>
g_log_handler;
On 2016/07/27 20:38:21, grt (UTC plus 2) wrote:
> move this into the unnamed namespace and remove "static".
> also, you can't use unique_ptr at global scope like this since it will
introduce
> a static ctor/dtor pair.

Done, and used raw pointer instead.

https://codereview.chromium.org/2034393004/diff/460001/base/logging_win.h
File base/logging_win.h (right):

https://codereview.chromium.org/2034393004/diff/460001/base/logging_win.h#new...
base/logging_win.h:54: class WinLogMessageHandler : LogMessageHandler {
On 2016/07/27 20:38:21, grt (UTC plus 2) wrote:
> move this into the private: section of LogEventProvider so you don't need the
> "friend"?

Done.

https://codereview.chromium.org/2034393004/diff/460001/base/logging_win.h#new...
base/logging_win.h:55: bool OnMessage(logging::LogSeverity severity, const char*
file,
On 2016/07/27 20:38:21, grt (UTC plus 2) wrote:
> public:

Done.

[grt (UTC plus 2), 2016-08-01 21:17:42]

grt@chromium.org changed reviewers:
+ grt@chromium.org

[grt (UTC plus 2), 2016-08-01 21:17:44]

Please review the CL description; I think the CL no longer has a traditional
add/remove API.

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc#newcod...
base/logging.cc:134:
base::LazyInstance<std::unordered_set<LogMessageListener*>>::Leaky
On 2016/08/01 16:12:25, wychen wrote:
> On 2016/07/27 20:38:20, grt (UTC plus 2) wrote:
> > why an unordered set? are there really so many listeners that you need the
> extra
> > memory overhead for O(lgN) insert/remove? vector or deque seems adequate.
> 
> The number of listeners should be very low practically, so I'm not worried
about
> asymptotic complexity. BTW, I think insert/remove is O(1) here. The reason I
> used unordered_set is to avoid subscribers to assume ordering.
> LogMessageHandlers are run in stack order, because the handlers can sink the
> message. The calling order of LogMessageListeners doesn't matter from the
> perspective of LogMessage, but subscribers are free to add side effects inside
> the listener functions, and we don't want these side effects to depend on
> certain calling order. 

Ouch. Apologies for the complexity goof. I haven't looked at the implementation
of unordered set, but my guess is that it uses a hash table. If so, you're
paying the memory cost of this for a small number of elements. I don't think
this is worthwhile. It's sufficient to document that subscribers must not make
any ordering assumptions; you don't need to impose a memory penalty to avoid the
appearance of ordering. If you think the properties of a set are important, use
a std::set; otherwise, use the most lightweight container possible. My guess is
that vector fits the bill.

https://codereview.chromium.org/2034393004/diff/580001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2034393004/diff/580001/base/logging.h#newcode282
base/logging.h:282: // Derive the LogMessageHandler that gets passed every log
message before
Please review the style guide section on comments and rewrite this.

https://codereview.chromium.org/2034393004/diff/580001/base/logging.h#newcode291
base/logging.h:291: virtual ~LogMessageHandler();
Move up; ctors and dtors precede other methods.

https://codereview.chromium.org/2034393004/diff/580001/base/logging.h#newcode296
base/logging.h:296: BASE_EXPORT size_t LogMessageHandlerCountForTesting();
If this is only used by base_unittests, it should not be exported. Same below.

[wychen, 2016-08-01 22:40:35]

Description was changed from

==========
Allow multiple logging::LogMessage{Handler,Listener}s

Some tests need to add a message handler, but not necessarily want to
replace the existing one. Some tests emulate stack by calling the old
handler if present, while most just ignore the old one. At the end of
the test, some of them restore the old handler, while others just reset
it to nullptr.

Using a stack would preserve the existing handlers, and keeps a
consistent semantics in handler manipulation. Returning true in the
handler still hijacks the handling, and the calling order is from top to
bottom.

Furthermore, sometimes hijacking is not necessary, so a new log message
listener API is added, including scoped version and traditional add/remove
interfaces. The listeners couldn't hijack the messages, and the calling order
is not guaranteed.

This enables https://crrev.com/2013573007/ to receive log messages even
when derived classes need their own handler.

BUG=
==========

to

==========
Allow multiple logging::LogMessage{Handler,Listener}s

Some tests need to add a message handler, but not necessarily want to
replace the existing one. Some tests emulate stack by calling the old
handler if present, while most just ignore the old one. At the end of
the test, some of them restore the old handler, while others just reset
it to nullptr.

Using a stack would preserve the existing handlers, and keeps a
consistent semantics in handler manipulation. Returning true in the
handler still hijacks the handling, and the calling order is from top to
bottom.

Furthermore, sometimes hijacking is not necessary, so a new log message
listener API is added. The listeners couldn't hijack the messages, and
the calling order is not guaranteed.

This enables https://crrev.com/2013573007/ to receive log messages even
when derived classes need their own handler.

BUG=
==========

[wychen, 2016-08-01 23:11:04]

Thanks for reviewing. The CL description is updated.

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc#newcod...
base/logging.cc:134:
base::LazyInstance<std::unordered_set<LogMessageListener*>>::Leaky
On 2016/08/01 21:17:44, grt (UTC plus 2) wrote:
> On 2016/08/01 16:12:25, wychen wrote:
> > On 2016/07/27 20:38:20, grt (UTC plus 2) wrote:
> > > why an unordered set? are there really so many listeners that you need the
> > extra
> > > memory overhead for O(lgN) insert/remove? vector or deque seems adequate.
> > 
> > The number of listeners should be very low practically, so I'm not worried
> about
> > asymptotic complexity. BTW, I think insert/remove is O(1) here. The reason I
> > used unordered_set is to avoid subscribers to assume ordering.
> > LogMessageHandlers are run in stack order, because the handlers can sink the
> > message. The calling order of LogMessageListeners doesn't matter from the
> > perspective of LogMessage, but subscribers are free to add side effects
inside
> > the listener functions, and we don't want these side effects to depend on
> > certain calling order. 
> 
> Ouch. Apologies for the complexity goof. I haven't looked at the
implementation
> of unordered set, but my guess is that it uses a hash table. If so, you're
> paying the memory cost of this for a small number of elements. I don't think
> this is worthwhile. It's sufficient to document that subscribers must not make
> any ordering assumptions; you don't need to impose a memory penalty to avoid
the
> appearance of ordering. If you think the properties of a set are important,
use
> a std::set; otherwise, use the most lightweight container possible. My guess
is
> that vector fits the bill.

Unordered set does use a hash table internally. Do you think it's a good idea to
use CallbackList underneath log_message_listeners while keeping the API the
same?

https://codereview.chromium.org/2034393004/diff/580001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2034393004/diff/580001/base/logging.h#newcode282
base/logging.h:282: // Derive the LogMessageHandler that gets passed every log
message before
On 2016/08/01 21:17:44, grt (UTC plus 2) wrote:
> Please review the style guide section on comments and rewrite this.

Done.

https://codereview.chromium.org/2034393004/diff/580001/base/logging.h#newcode291
base/logging.h:291: virtual ~LogMessageHandler();
On 2016/08/01 21:17:44, grt (UTC plus 2) wrote:
> Move up; ctors and dtors precede other methods.

Done.

https://codereview.chromium.org/2034393004/diff/580001/base/logging.h#newcode296
base/logging.h:296: BASE_EXPORT size_t LogMessageHandlerCountForTesting();
On 2016/08/01 21:17:44, grt (UTC plus 2) wrote:
> If this is only used by base_unittests, it should not be exported. Same below.

If I don't export it, component build would fail.

[grt (UTC plus 2), 2016-08-02 07:25:46]

I just looked at the motivation for this change. Can
https://codereview.chromium.org/2013573007/ use ConsoleObserverDelegate instead
of hooking into logging::?

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc#newcod...
base/logging.cc:134:
base::LazyInstance<std::unordered_set<LogMessageListener*>>::Leaky
On 2016/08/01 23:11:03, wychen wrote:
> On 2016/08/01 21:17:44, grt (UTC plus 2) wrote:
> > On 2016/08/01 16:12:25, wychen wrote:
> > > On 2016/07/27 20:38:20, grt (UTC plus 2) wrote:
> > > > why an unordered set? are there really so many listeners that you need
the
> > > extra
> > > > memory overhead for O(lgN) insert/remove? vector or deque seems
adequate.
> > > 
> > > The number of listeners should be very low practically, so I'm not worried
> > about
> > > asymptotic complexity. BTW, I think insert/remove is O(1) here. The reason
I
> > > used unordered_set is to avoid subscribers to assume ordering.
> > > LogMessageHandlers are run in stack order, because the handlers can sink
the
> > > message. The calling order of LogMessageListeners doesn't matter from the
> > > perspective of LogMessage, but subscribers are free to add side effects
> inside
> > > the listener functions, and we don't want these side effects to depend on
> > > certain calling order. 
> > 
> > Ouch. Apologies for the complexity goof. I haven't looked at the
> implementation
> > of unordered set, but my guess is that it uses a hash table. If so, you're
> > paying the memory cost of this for a small number of elements. I don't think
> > this is worthwhile. It's sufficient to document that subscribers must not
make
> > any ordering assumptions; you don't need to impose a memory penalty to avoid
> the
> > appearance of ordering. If you think the properties of a set are important,
> use
> > a std::set; otherwise, use the most lightweight container possible. My guess
> is
> > that vector fits the bill.
> 
> Unordered set does use a hash table internally. Do you think it's a good idea
to
> use CallbackList underneath log_message_listeners while keeping the API the
> same?

I'm a bit divided. On the one hand, I prefer callbacks to single-method
interfaces. On the other hand, the subscription aspect of CallbackList can be a
little clunky to work with and you'd have to introduce Add/Remove methods. I'm
okay with keeping the interface as it is.

https://codereview.chromium.org/2034393004/diff/580001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2034393004/diff/580001/base/logging.h#newcode296
base/logging.h:296: BASE_EXPORT size_t LogMessageHandlerCountForTesting();
On 2016/08/01 23:11:04, wychen wrote:
> On 2016/08/01 21:17:44, grt (UTC plus 2) wrote:
> > If this is only used by base_unittests, it should not be exported. Same
below.
> 
> If I don't export it, component build would fail.

Meh. I didn't realize base_unittests linked to base.dll rather than pulling in
the sources directly. Oh well.

[wychen, 2016-08-03 02:51:19]

On 2016/08/02 07:25:46, grt (UTC plus 2) wrote:
> I just looked at the motivation for this change. Can
> https://codereview.chromium.org/2013573007/ use ConsoleObserverDelegate
instead
> of hooking into logging::?

This sounds like a great idea. I'll give it a try.

[Primiano Tucci (use gerrit), 2016-08-03 09:20:03]

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

[wychen, 2016-08-04 03:53:55]

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc#newcod...
base/logging.cc:134:
base::LazyInstance<std::unordered_set<LogMessageListener*>>::Leaky
On 2016/08/02 07:25:46, grt (UTC plus 2) wrote:
> On 2016/08/01 23:11:03, wychen wrote:
> > Unordered set does use a hash table internally. Do you think it's a good
idea
> to
> > use CallbackList underneath log_message_listeners while keeping the API the
> > same?
> 
> I'm a bit divided. On the one hand, I prefer callbacks to single-method
> interfaces. On the other hand, the subscription aspect of CallbackList can be
a
> little clunky to work with and you'd have to introduce Add/Remove methods. I'm
> okay with keeping the interface as it is.

I meant keeping the single-method interface as is, but it uses CallbackList
internally. It is still scope-based from the users' perspective, and we don't
need to introduce add/remove methods. We add a callback in the constructor of
LogMessageListener, and the registration object would be in unique_ptr<> as a
member data. The unregistration would happen in destruction time automatically.

I think this is better than using locks like what I did.

[grt (UTC plus 2), 2016-08-04 07:11:18]

On 2016/08/04 03:53:55, wychen wrote:
> https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc
> File base/logging.cc (right):
> 
>
https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc#newcod...
> base/logging.cc:134:
> base::LazyInstance<std::unordered_set<LogMessageListener*>>::Leaky
> On 2016/08/02 07:25:46, grt (UTC plus 2) wrote:
> > On 2016/08/01 23:11:03, wychen wrote:
> > > Unordered set does use a hash table internally. Do you think it's a good
> idea
> > to
> > > use CallbackList underneath log_message_listeners while keeping the API
the
> > > same?
> > 
> > I'm a bit divided. On the one hand, I prefer callbacks to single-method
> > interfaces. On the other hand, the subscription aspect of CallbackList can
be
> a
> > little clunky to work with and you'd have to introduce Add/Remove methods.
I'm
> > okay with keeping the interface as it is.
> 
> I meant keeping the single-method interface as is, but it uses CallbackList
> internally. It is still scope-based from the users' perspective, and we don't
> need to introduce add/remove methods. We add a callback in the constructor of
> LogMessageListener, and the registration object would be in unique_ptr<> as a
> member data. The unregistration would happen in destruction time
automatically.
> 
> I think this is better than using locks like what I did.

For now please investigate using ConsoleObserverDelegate in the other CL.
Perhaps this CL isn't needed at all.

[wychen, 2016-08-04 07:31:41]

On 2016/08/04 07:11:18, grt (UTC plus 2) wrote:
> On 2016/08/04 03:53:55, wychen wrote:
> > https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc
> > File base/logging.cc (right):
> > 
> >
>
https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc#newcod...
> > base/logging.cc:134:
> > base::LazyInstance<std::unordered_set<LogMessageListener*>>::Leaky
> > On 2016/08/02 07:25:46, grt (UTC plus 2) wrote:
> > > On 2016/08/01 23:11:03, wychen wrote:
> > > > Unordered set does use a hash table internally. Do you think it's a good
> > idea
> > > to
> > > > use CallbackList underneath log_message_listeners while keeping the API
> the
> > > > same?
> > > 
> > > I'm a bit divided. On the one hand, I prefer callbacks to single-method
> > > interfaces. On the other hand, the subscription aspect of CallbackList can
> be
> > a
> > > little clunky to work with and you'd have to introduce Add/Remove methods.
> I'm
> > > okay with keeping the interface as it is.
> > 
> > I meant keeping the single-method interface as is, but it uses CallbackList
> > internally. It is still scope-based from the users' perspective, and we
don't
> > need to introduce add/remove methods. We add a callback in the constructor
of
> > LogMessageListener, and the registration object would be in unique_ptr<> as
a
> > member data. The unregistration would happen in destruction time
> automatically.
> > 
> > I think this is better than using locks like what I did.
> 
> For now please investigate using ConsoleObserverDelegate in the other CL.
> Perhaps this CL isn't needed at all.

ConsoleObserverDelegate didn't really work out because there's only one delegate
in a WebContents. If it is already set, replacing it would have side-effects.
For WebContents to support multiple delegates, we'll need to change the API from
set to add/remove. It might look a lot like this CL, but in a different place.
I'd argue that tapping in global log is better than in WebContents. If multiple
WebContents are involved in a test, it might be hard to make sure we didn't
accidentally miss one.

[grt (UTC plus 2), 2016-08-04 09:07:55]

On 2016/08/04 07:31:41, wychen wrote:
> On 2016/08/04 07:11:18, grt (UTC plus 2) wrote:
> > On 2016/08/04 03:53:55, wychen wrote:
> > > https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc
> > > File base/logging.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2034393004/diff/460001/base/logging.cc#newcod...
> > > base/logging.cc:134:
> > > base::LazyInstance<std::unordered_set<LogMessageListener*>>::Leaky
> > > On 2016/08/02 07:25:46, grt (UTC plus 2) wrote:
> > > > On 2016/08/01 23:11:03, wychen wrote:
> > > > > Unordered set does use a hash table internally. Do you think it's a
good
> > > idea
> > > > to
> > > > > use CallbackList underneath log_message_listeners while keeping the
API
> > the
> > > > > same?
> > > > 
> > > > I'm a bit divided. On the one hand, I prefer callbacks to single-method
> > > > interfaces. On the other hand, the subscription aspect of CallbackList
can
> > be
> > > a
> > > > little clunky to work with and you'd have to introduce Add/Remove
methods.
> > I'm
> > > > okay with keeping the interface as it is.
> > > 
> > > I meant keeping the single-method interface as is, but it uses
CallbackList
> > > internally. It is still scope-based from the users' perspective, and we
> don't
> > > need to introduce add/remove methods. We add a callback in the constructor
> of
> > > LogMessageListener, and the registration object would be in unique_ptr<>
as
> a
> > > member data. The unregistration would happen in destruction time
> > automatically.
> > > 
> > > I think this is better than using locks like what I did.
> > 
> > For now please investigate using ConsoleObserverDelegate in the other CL.
> > Perhaps this CL isn't needed at all.
> 
> ConsoleObserverDelegate didn't really work out because there's only one
delegate
> in a WebContents. If it is already set, replacing it would have side-effects.
> For WebContents to support multiple delegates, we'll need to change the API
from
> set to add/remove. It might look a lot like this CL, but in a different place.
> I'd argue that tapping in global log is better than in WebContents. If
multiple
> WebContents are involved in a test, it might be hard to make sure we didn't
> accidentally miss one.

Is there another way to more directly observe the thing you care about? I have
to confess to not knowing anything about CSP or much about the renderer side of
the house, but it seems to me that a log message is a side effect of a thing you
care about rather than the thing itself. The tests will break if the log message
changes or if logging of CSP violations in this manner goes away some day. If
you had a more direct way to observe CSP violations, the tests would be
resilient to these things.

[wychen, 2016-08-05 06:38:21]

On 2016/08/04 09:07:55, grt (UTC plus 2) wrote:
> Is there another way to more directly observe the thing you care about? I have
> to confess to not knowing anything about CSP or much about the renderer side
of
> the house, but it seems to me that a log message is a side effect of a thing
you
> care about rather than the thing itself. The tests will break if the log
message
> changes or if logging of CSP violations in this manner goes away some day. If
> you had a more direct way to observe CSP violations, the tests would be
> resilient to these things.

Thanks for bringing this up. I've considered this direction before, but decided
to stick with log message handling. What we want to add is the ability to check
for CSP violations in the tests, not to change the behavior in production. In
order to directly observe the CSP violations, we'd need to pump it through
multiple layers, add interfaces and IPC signals. It seems a bit overkill for a
test-only requirement. One obvious downside of log message handling is silent
failing due to changed messages, but that was fixed by checking the message both
ways, including asserting the existence and nonexistence of CSP messages. If the
log message changes, at least it will be caught by the tests. I think it should
be safe to assume that we will always want the web developpers to know about the
CSP violations by showing these messages in the console, so I wouldn't worry
about this going away too much.

In the hindsight, even if we don't need to check for CSP violations in the
tests, allowing multiple log message handlers still makes sense. We already see
the messy behavior of the users of the old API, where old handlers are just
replaced, which is error-prone.

[grt (UTC plus 2), 2016-08-05 08:53:05]

On 2016/08/05 06:38:21, wychen wrote:
> On 2016/08/04 09:07:55, grt (UTC plus 2) wrote:
> > Is there another way to more directly observe the thing you care about? I
have
> > to confess to not knowing anything about CSP or much about the renderer side
> of
> > the house, but it seems to me that a log message is a side effect of a thing
> you
> > care about rather than the thing itself. The tests will break if the log
> message
> > changes or if logging of CSP violations in this manner goes away some day.
If
> > you had a more direct way to observe CSP violations, the tests would be
> > resilient to these things.
> 
> Thanks for bringing this up. I've considered this direction before, but
decided
> to stick with log message handling. What we want to add is the ability to
check
> for CSP violations in the tests, not to change the behavior in production. In
> order to directly observe the CSP violations, we'd need to pump it through
> multiple layers, add interfaces and IPC signals. It seems a bit overkill for a
> test-only requirement. One obvious downside of log message handling is silent
> failing due to changed messages, but that was fixed by checking the message
both
> ways, including asserting the existence and nonexistence of CSP messages. If
the
> log message changes, at least it will be caught by the tests. I think it
should
> be safe to assume that we will always want the web developpers to know about
the
> CSP violations by showing these messages in the console, so I wouldn't worry
> about this going away too much.

The thing that bothers me a bit is that your test is not observing what web
developers will see in the console. Rather, it's observing a side-effect that
those console messages are also sent through Chrome's logging system. Will this
always and forever be true?

[wychen, 2016-08-05 17:42:14]

On 2016/08/05 08:53:05, grt (UTC plus 2) wrote:
> The thing that bothers me a bit is that your test is not observing what web
> developers will see in the console. Rather, it's observing a side-effect that
> those console messages are also sent through Chrome's logging system. Will
this
> always and forever be true?

I understand your concern. In order to directly observe what web developers see
in the console, we'll have to tap into WebContents in a way similar to
ConsoleObserverDelegate. The worst thing to do is to add tests that could lead
to silent failure, which is even worse than no tests at all because it gives us
false confidence. Therefore, we don't want to miss any WebContents in the tests.
The thing is, it's very easy to forget to tap into extra WebContents in the
tests, like when using multiple tabs, or even some hidden ones in pre-warming or
inside panels. Guarding these at global log level is the safer thing to do. My
goal is to be able to catch all the CSP violations in the tests, without the
test writers to even be aware of such a thing.

I think the reason all console messages are copied to the logging system is that
they are important, and the volume is low enough in general. We could reasonably
assume this would remain the case in the foreseeable future.

[grt (UTC plus 2), 2016-08-05 20:13:30]

Please add a comment to the place where console messages are logged that
explains why the logging must stay; ideally directing the reader to the code
that checks for CSP violations in tests.

A few more comments. I haven't read any of the code outside of base/ yet. Do
other reviewers have that covered?

https://codereview.chromium.org/2034393004/diff/600001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2034393004/diff/600001/base/logging.cc#newcod...
base/logging.cc:133: base::LazyInstance<base::Lock>::Leaky
log_message_handler_lock;
 = LAZY_INSTANCE_INITIALIZER

https://codereview.chromium.org/2034393004/diff/600001/base/logging.cc#newcod...
base/logging.cc:138: base::LazyInstance<base::Lock>::Leaky
log_message_listener_lock;
 = LAZY_INSTANCE_INITIALIZER

https://codereview.chromium.org/2034393004/diff/600001/base/logging.cc#newcod...
base/logging.cc:459: !log_message_handlers.Get().empty() ||
i don't think this is safe to do outside of the lock.

https://codereview.chromium.org/2034393004/diff/600001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2034393004/diff/600001/base/logging.h#newcode292
base/logging.h:292: size_t message_start, const std::string& str) = 0;
please use "git cl format"

https://codereview.chromium.org/2034393004/diff/600001/base/logging.h#newcode293
base/logging.h:293: protected:
nit: blank line before this (below, too)

https://codereview.chromium.org/2034393004/diff/600001/base/logging_win.cc
File base/logging_win.cc (right):

https://codereview.chromium.org/2034393004/diff/600001/base/logging_win.cc#ne...
base/logging_win.cc:110: log_handler_ = new WinLogMessageHandler();
can log_handler_ be a non-static member of the provider?

[wychen, 2016-08-07 09:51:54]

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

[commit-bot: I haz the power, 2016-08-07 09:52:00]

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

[wychen, 2016-08-07 10:00:10]

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

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

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-08-07 11:05:21]

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

[commit-bot: I haz the power, 2016-08-07 11:05:22]

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

[wychen, 2016-08-08 20:23:13]

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

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

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-08-08 20:27:22]

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

[commit-bot: I haz the power, 2016-08-08 20:27:24]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[wychen, 2016-08-08 22:02:28]

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

[commit-bot: I haz the power, 2016-08-08 22:03:02]

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-08-08 22:06:32]

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

[commit-bot: I haz the power, 2016-08-08 22:06:34]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wychen, 2016-08-08 22:23:40]

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

[wychen, 2016-08-08 22:23:52]

Patchset #20 (id:660001) has been deleted

[commit-bot: I haz the power, 2016-08-08 22:24:23]

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

[wychen, 2016-08-08 22:27:24]

Patchset #19 (id:640001) has been deleted

[commit-bot: I haz the power, 2016-08-08 22:41:57]

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

[commit-bot: I haz the power, 2016-08-08 22:41:59]

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

[wychen, 2016-08-08 22:55:27]

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

[commit-bot: I haz the power, 2016-08-08 22:55:53]

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-08-08 23:06:03]

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

[commit-bot: I haz the power, 2016-08-08 23:06:04]

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

[wychen, 2016-08-09 05:51:39]

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

[commit-bot: I haz the power, 2016-08-09 05:51:59]

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-08-09 07:00:47]

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

[commit-bot: I haz the power, 2016-08-09 07:00:48]

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

[wychen, 2016-08-09 20:13:06]

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

[commit-bot: I haz the power, 2016-08-09 20:13:47]

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

[wychen, 2016-08-09 20:15:54]

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

[commit-bot: I haz the power, 2016-08-09 20:16:33]

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-08-09 20:28:04]

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

[commit-bot: I haz the power, 2016-08-09 20:28:06]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[wychen, 2016-08-09 22:40:36]

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

[wychen, 2016-08-09 22:47:44]

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

[commit-bot: I haz the power, 2016-08-09 22:48:15]

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

[wychen, 2016-08-09 22:57:03]

Patchset #21 (id:720001) has been deleted

[commit-bot: I haz the power, 2016-08-10 00:50:45]

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

[commit-bot: I haz the power, 2016-08-10 00:50:48]

Dry run: Try jobs failed on following builders:
  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_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wychen, 2016-08-10 00:57:02]

Patchset #22 (id:760001) has been deleted

[wychen, 2016-08-10 00:57:17]

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

[commit-bot: I haz the power, 2016-08-10 00:58:00]

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

[wychen, 2016-08-10 00:58:10]

Patchset #21 (id:740001) has been deleted

[commit-bot: I haz the power, 2016-08-10 02:25:39]

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

[commit-bot: I haz the power, 2016-08-10 02:25:41]

Dry run: This issue passed the CQ dry run.

[wychen, 2016-08-10 08:07:18]

Patchset #19 (id:680001) has been deleted

[wychen, 2016-08-11 22:32:03]

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

[commit-bot: I haz the power, 2016-08-11 22:33:31]

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-08-11 23:58:20]

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

[commit-bot: I haz the power, 2016-08-11 23:58:22]

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

[wychen, 2016-08-12 19:15:06]

Description was changed from

==========
Allow multiple logging::LogMessage{Handler,Listener}s

Some tests need to add a message handler, but not necessarily want to
replace the existing one. Some tests emulate stack by calling the old
handler if present, while most just ignore the old one. At the end of
the test, some of them restore the old handler, while others just reset
it to nullptr.

Using a stack would preserve the existing handlers, and keeps a
consistent semantics in handler manipulation. Returning true in the
handler still hijacks the handling, and the calling order is from top to
bottom.

Furthermore, sometimes hijacking is not necessary, so a new log message
listener API is added. The listeners couldn't hijack the messages, and
the calling order is not guaranteed.

This enables https://crrev.com/2013573007/ to receive log messages even
when derived classes need their own handler.

BUG=
==========

to

==========
Allow multiple logging::LogMessage{Handler,Listener}s

Some tests need to add a message handler, but not necessarily want to
replace the existing one. Some tests emulate stack by calling the old
handler if present, while most just ignore the old one. At the end of
the test, some of them restore the old handler, while others just reset
it to nullptr.

Using a stack would preserve the existing handlers, and keeps a
consistent semantics in handler manipulation. Returning true in the
handler still hijacks the handling, and the calling order is from top to
bottom.

Furthermore, sometimes hijacking is not necessary, so a new log message
listener API is added. The listeners couldn't hijack the messages, and
the calling order is not guaranteed.

This enables https://crrev.com/2013573007/ to receive log messages even
when derived classes need their own handler.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[wychen, 2016-08-12 19:19:08]

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

[commit-bot: I haz the power, 2016-08-12 19:19:33]

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

[wychen, 2016-08-12 21:32:40]

I added a comment at the place where console log messages are forwarded as a
logging::LogMessage(). As for changes outside of base/, dbeam@ had a look a
while ago, but it's always helpful to have extra pair of eyes on it.

ReadWriteLock is necessary because log message handlers could also emit another
log message. Details of the deadlock is available here:
https://codereview.chromium.org/2034393004/#ps820001

https://codereview.chromium.org/2034393004/diff/600001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2034393004/diff/600001/base/logging.cc#newcod...
base/logging.cc:133: base::LazyInstance<base::Lock>::Leaky
log_message_handler_lock;
On 2016/08/05 20:13:30, grt (UTC plus 2) wrote:
>  = LAZY_INSTANCE_INITIALIZER

Done.

https://codereview.chromium.org/2034393004/diff/600001/base/logging.cc#newcod...
base/logging.cc:138: base::LazyInstance<base::Lock>::Leaky
log_message_listener_lock;
On 2016/08/05 20:13:30, grt (UTC plus 2) wrote:
>  = LAZY_INSTANCE_INITIALIZER

Done.

https://codereview.chromium.org/2034393004/diff/600001/base/logging.cc#newcod...
base/logging.cc:459: !log_message_handlers.Get().empty() ||
On 2016/08/05 20:13:30, grt (UTC plus 2) wrote:
> i don't think this is safe to do outside of the lock.

Done.

https://codereview.chromium.org/2034393004/diff/600001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2034393004/diff/600001/base/logging.h#newcode292
base/logging.h:292: size_t message_start, const std::string& str) = 0;
On 2016/08/05 20:13:30, grt (UTC plus 2) wrote:
> please use "git cl format"

Done.

https://codereview.chromium.org/2034393004/diff/600001/base/logging.h#newcode293
base/logging.h:293: protected:
On 2016/08/05 20:13:30, grt (UTC plus 2) wrote:
> nit: blank line before this (below, too)

Done.

https://codereview.chromium.org/2034393004/diff/600001/base/logging_win.cc
File base/logging_win.cc (right):

https://codereview.chromium.org/2034393004/diff/600001/base/logging_win.cc#ne...
base/logging_win.cc:110: log_handler_ = new WinLogMessageHandler();
On 2016/08/05 20:13:30, grt (UTC plus 2) wrote:
> can log_handler_ be a non-static member of the provider?

Initialize() is a static member, so I think it's natural to put the handler
static as well.

[commit-bot: I haz the power, 2016-08-12 23:22:27]

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

[commit-bot: I haz the power, 2016-08-12 23:22:29]

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

[grt (UTC plus 2), 2016-08-15 07:36:32]

https://codereview.chromium.org/2034393004/diff/600001/base/logging_win.cc
File base/logging_win.cc (right):

https://codereview.chromium.org/2034393004/diff/600001/base/logging_win.cc#ne...
base/logging_win.cc:110: log_handler_ = new WinLogMessageHandler();
On 2016/08/12 21:32:39, wychen wrote:
> On 2016/08/05 20:13:30, grt (UTC plus 2) wrote:
> > can log_handler_ be a non-static member of the provider?
> 
> Initialize() is a static member, so I think it's natural to put the handler
> static as well.

I disagree. Initialize is part of the public API. Presumably, it's static so
that callers can
  LogEventProvider::Initialize(foo);
rather than
  LogEventProvider::GetInstance()->Initialize(foo);

This doesn't mean that implementation details of the provider should also be
statics. old_log_level_, for example, is a data member associated with the
single instance of the LogEventProvider. Making the handler a non-static member
of the provider will also permit you to use std::unique_ptr. It may also make
sense to add a dtor with a DCHECK_EQ(nullptr, log_handler_); to be sure that
Uninitialize was called. If that's important (I'm not sure if it is).

https://codereview.chromium.org/2034393004/diff/840001/base/logging_win.cc
File base/logging_win.cc (right):

https://codereview.chromium.org/2034393004/diff/840001/base/logging_win.cc#ne...
base/logging_win.cc:18: LogEventProvider::WinLogMessageHandler*
LogEventProvider::log_handler_ =
nit:
// static
just before this

https://codereview.chromium.org/2034393004/diff/840001/base/logging_win.cc#ne...
base/logging_win.cc:113: log_handler_ = new WinLogMessageHandler();
  DCHECK_EQ(nullptr, log_handler_);

https://codereview.chromium.org/2034393004/diff/840001/base/logging_win.cc#ne...
base/logging_win.cc:118: delete log_handler_;
  DCHECK_NE(nullptr, log_handler_);

https://codereview.chromium.org/2034393004/diff/840001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2034393004/diff/840001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:916: // Ref:
https://crrev.com/2013573007/
i think it's better to link to a bug rather than a code review. is
https://crbug.com/616181 appropriate here?

[wychen, 2016-08-16 00:59:10]

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

[commit-bot: I haz the power, 2016-08-16 00:59: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, 2016-08-16 01:43:45]

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

[commit-bot: I haz the power, 2016-08-16 01:43:47]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[wychen, 2016-08-17 01:59:58]

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

[commit-bot: I haz the power, 2016-08-17 02:00:17]

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

[wychen, 2016-08-17 02:02:42]

Patchset #26 (id:880002) has been deleted

[wychen, 2016-08-17 02:02:51]

Patchset #25 (id:880001) has been deleted

[commit-bot: I haz the power, 2016-08-17 03:42:36]

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

[commit-bot: I haz the power, 2016-08-17 03:42:38]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[wychen, 2016-08-17 05:03:08]

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

[commit-bot: I haz the power, 2016-08-17 05:03:23]

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

[wychen, 2016-08-17 05:04:12]

Patchset #24 (id:860001) has been deleted

[wychen, 2016-08-17 05:04:39]

Patchset #24 (id:910001) has been deleted

[commit-bot: I haz the power, 2016-08-17 05:48:48]

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

[commit-bot: I haz the power, 2016-08-17 05:48:50]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[wychen, 2016-08-17 07:14:26]

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

[commit-bot: I haz the power, 2016-08-17 07:14:43]

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-08-17 08:47:23]

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

[commit-bot: I haz the power, 2016-08-17 08:47:25]

Dry run: This issue passed the CQ dry run.

[wychen, 2016-08-17 16:57:44]

Patchset #24 (id:930001) has been deleted

[wychen, 2016-08-17 17:08:55]

https://codereview.chromium.org/2034393004/diff/600001/base/logging_win.cc
File base/logging_win.cc (right):

https://codereview.chromium.org/2034393004/diff/600001/base/logging_win.cc#ne...
base/logging_win.cc:110: log_handler_ = new WinLogMessageHandler();
On 2016/08/15 07:36:32, grt (UTC plus 2) wrote:
> On 2016/08/12 21:32:39, wychen wrote:
> > On 2016/08/05 20:13:30, grt (UTC plus 2) wrote:
> > > can log_handler_ be a non-static member of the provider?
> > 
> > Initialize() is a static member, so I think it's natural to put the handler
> > static as well.
> 
> I disagree. Initialize is part of the public API. Presumably, it's static so
> that callers can
>   LogEventProvider::Initialize(foo);
> rather than
>   LogEventProvider::GetInstance()->Initialize(foo);
> 
> This doesn't mean that implementation details of the provider should also be
> statics. old_log_level_, for example, is a data member associated with the
> single instance of the LogEventProvider. Making the handler a non-static
member
> of the provider will also permit you to use std::unique_ptr. It may also make
> sense to add a dtor with a DCHECK_EQ(nullptr, log_handler_); to be sure that
> Uninitialize was called. If that's important (I'm not sure if it is).

Done.

https://codereview.chromium.org/2034393004/diff/840001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2034393004/diff/840001/base/logging.cc#newcod...
base/logging.cc:137:
base::LazyInstance<std::unordered_set<LogMessageListener*>>::Leaky
Note to self: use deque.

https://codereview.chromium.org/2034393004/diff/840001/base/logging_unittest.cc
File base/logging_unittest.cc (right):

https://codereview.chromium.org/2034393004/diff/840001/base/logging_unittest....
base/logging_unittest.cc:384: 
Note to self: generate message inside OnMessage().

https://codereview.chromium.org/2034393004/diff/840001/base/logging_win.cc
File base/logging_win.cc (right):

https://codereview.chromium.org/2034393004/diff/840001/base/logging_win.cc#ne...
base/logging_win.cc:18: LogEventProvider::WinLogMessageHandler*
LogEventProvider::log_handler_ =
On 2016/08/15 07:36:32, grt (UTC plus 2) wrote:
> nit:
> // static
> just before this

No longer static.

https://codereview.chromium.org/2034393004/diff/840001/base/logging_win.cc#ne...
base/logging_win.cc:113: log_handler_ = new WinLogMessageHandler();
On 2016/08/15 07:36:32, grt (UTC plus 2) wrote:
>   DCHECK_EQ(nullptr, log_handler_);

Used unique_ptr.

https://codereview.chromium.org/2034393004/diff/840001/base/logging_win.cc#ne...
base/logging_win.cc:118: delete log_handler_;
On 2016/08/15 07:36:32, grt (UTC plus 2) wrote:
>   DCHECK_NE(nullptr, log_handler_);

Used unique_ptr.

https://codereview.chromium.org/2034393004/diff/840001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2034393004/diff/840001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:916: // Ref:
https://crrev.com/2013573007/
On 2016/08/15 07:36:32, grt (UTC plus 2) wrote:
> i think it's better to link to a bug rather than a code review. is
> https://crbug.com/616181 appropriate here?

Neither of the existing bugs in that CL is good.
I've created a new one.

[grt (UTC plus 2), 2016-08-18 08:09:33]

base lgtm

https://codereview.chromium.org/2034393004/diff/840001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2034393004/diff/840001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:916: // Ref:
https://crrev.com/2013573007/
On 2016/08/17 17:08:54, wychen wrote:
> On 2016/08/15 07:36:32, grt (UTC plus 2) wrote:
> > i think it's better to link to a bug rather than a code review. is
> > https://crbug.com/616181 appropriate here?
> 
> Neither of the existing bugs in that CL is good.
> I've created a new one.

Perfect. Please put this issue number in the "BUG=" line of the CL description
as well. Thanks.

[wychen, 2016-08-18 08:46:32]

Description was changed from

==========
Allow multiple logging::LogMessage{Handler,Listener}s

Some tests need to add a message handler, but not necessarily want to
replace the existing one. Some tests emulate stack by calling the old
handler if present, while most just ignore the old one. At the end of
the test, some of them restore the old handler, while others just reset
it to nullptr.

Using a stack would preserve the existing handlers, and keeps a
consistent semantics in handler manipulation. Returning true in the
handler still hijacks the handling, and the calling order is from top to
bottom.

Furthermore, sometimes hijacking is not necessary, so a new log message
listener API is added. The listeners couldn't hijack the messages, and
the calling order is not guaranteed.

This enables https://crrev.com/2013573007/ to receive log messages even
when derived classes need their own handler.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Allow multiple logging::LogMessage{Handler,Listener}s

Some tests need to add a message handler, but not necessarily want to
replace the existing one. Some tests emulate stack by calling the old
handler if present, while most just ignore the old one. At the end of
the test, some of them restore the old handler, while others just reset
it to nullptr.

Using a stack would preserve the existing handlers, and keeps a
consistent semantics in handler manipulation. Returning true in the
handler still hijacks the handling, and the calling order is from top to
bottom.

Furthermore, sometimes hijacking is not necessary, so a new log message
listener API is added. The listeners couldn't hijack the messages, and
the calling order is not guaranteed.

This enables https://crrev.com/2013573007/ to receive log messages even
when derived classes need their own handler.

BUG=638050
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[wychen, 2016-08-18 08:50:30]

brettw@, would you mind having another look? Thanks!

https://codereview.chromium.org/2034393004/diff/840001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2034393004/diff/840001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:916: // Ref:
https://crrev.com/2013573007/
On 2016/08/18 08:09:33, grt (UTC plus 2) wrote:
> On 2016/08/17 17:08:54, wychen wrote:
> > On 2016/08/15 07:36:32, grt (UTC plus 2) wrote:
> > > i think it's better to link to a bug rather than a code review. is
> > > https://crbug.com/616181 appropriate here?
> > 
> > Neither of the existing bugs in that CL is good.
> > I've created a new one.
> 
> Perfect. Please put this issue number in the "BUG=" line of the CL description
> as well. Thanks.

Done.

[brettw, 2016-08-22 23:30:11]

I haven't forgotten about this, I had a discussion about this today but need to
revisit it and I won't have time to get to it before I go home. Will look
tomorrow. Sorry for the delay.

[Dan Beam, 2016-09-29 21:43:10]

"I am so hard to get right without this CL!" - log handlers

[Dan Beam, 2016-11-14 07:47:32]

this CL has been open for 5 MONTHS.  so long that this has come up:
https://codereview.chromium.org/2497833002/

[Mark Mentovai, 2016-11-14 14:40:43]

Brett, did you still want to take a look too?

https://codereview.chromium.org/2034393004/diff/950001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2034393004/diff/950001/base/logging.cc#newcod...
base/logging.cc:376: LogMessageListener::LogMessageListener() {
So much duplication…

https://codereview.chromium.org/2034393004/diff/950001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2034393004/diff/950001/base/logging.h#newcode306
base/logging.h:306: class BASE_EXPORT LogMessageListener {
The difference between Handler and Listener will be confusing in practice. I
don’t believe that we need to support both of them, although I haven’t examined
this entire change to weigh in on each use site.

https://codereview.chromium.org/2034393004/diff/950001/third_party/crashpad/c...
File third_party/crashpad/crashpad/util/thread/OWNERS (right):

https://codereview.chromium.org/2034393004/diff/950001/third_party/crashpad/c...
third_party/crashpad/crashpad/util/thread/OWNERS:1: per-file *_messages.cc=set
noparent
Revert this file. The PRESUBMIT bug that made you add it has been fixed. It
should never have been poking around in third_party to begin with.

https://codereview.chromium.org/2034393004/diff/950001/third_party/crashpad/c...
File third_party/crashpad/crashpad/util/thread/thread_log_messages.cc (right):

https://codereview.chromium.org/2034393004/diff/950001/third_party/crashpad/c...
third_party/crashpad/crashpad/util/thread/thread_log_messages.cc:37: class
ThreadLogMessagesMaster : logging::LogMessageListener {
Note the local change in README.chromium.

[brettw, 2016-11-14 22:00:51]

In my last message I said I hadn't forgotten about this, but then I really did
forget about it. Please feel free to send me a direct email if you don't hear
from me for more than 1 business day.

[brettw, 2016-11-14 22:01:31]

https://codereview.chromium.org/2034393004/diff/950001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2034393004/diff/950001/base/logging.h#newcode306
base/logging.h:306: class BASE_EXPORT LogMessageListener {
I'm inclined to agree. What do you see as the cons of having one one type of
thing?

https://codereview.chromium.org/2034393004/diff/950001/chrome/test/chromedriv...
File chrome/test/chromedriver/logging.cc (right):

https://codereview.chromium.org/2034393004/diff/950001/chrome/test/chromedriv...
chrome/test/chromedriver/logging.cc:249: CHECK(handler);
Normally we wouldn't bother asserting the malloc returns null. I'd just do
  new LogMessageHandler();  // Intentionally leak this instance.
(unless this causes compiler warnings?).

https://codereview.chromium.org/2034393004/diff/950001/components/crash/conte...
File components/crash/content/app/crashpad.cc (right):

https://codereview.chromium.org/2034393004/diff/950001/components/crash/conte...
components/crash/content/app/crashpad.cc:253: CHECK(listener);
Ditto

[wychen, 2016-11-15 23:52:27]

Sorry for suspending this CL for WAY too long.

https://codereview.chromium.org/2034393004/diff/950001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2034393004/diff/950001/base/logging.h#newcode306
base/logging.h:306: class BASE_EXPORT LogMessageListener {
On 2016/11/14 22:01:31, brettw (ping on IM after 24h) wrote:
> I'm inclined to agree. What do you see as the cons of having one one type of
> thing?

The problem we want to solve in this CL is that many agents are trying to
receive and hijack the log messages. Not overriding one another is the very
first step, but it might not be enough.

The model I had in mind is that the handlers are pushed into a stack, so that if
a message is hijacked by a handler, handlers added before it don't receive the
message, but handlers added after it do. In short, recently added handlers have
higher priority. In this case, two-tier system is needed since listeners are
guaranteed to receive the message, even if one of the handlers hijacks it.

Now looking at this CL with fresh eyes, I think we probably don't want handlers
to be able to hijack messages from one another. We can simplify the model by
making all handlers have the same priority, call all of them, and hijack if any
handlers says so. In this case, we don't need listeners.

On the other hand, message handlers with no side effect (listeners, essentially)
is cleaner, but alas there are a few hijacking handlers. I suspect most of them
can be (relatively) safely converted to listeners, but would like to play safe
and do it in stages. This CL preserves the original behavior as much as
possible.

The hijacking handlers are:
- safe_browsing_database_unittest.cc: http://crbug.com/56448
- chrome/test/chromedriver/logging.cc: no apparant reason
- components/proximity_auth/logging/logging_unittest.cc: avoid flooding stdout

These two are gone:
- installation_validation_helper.cc
- installation_validator_unittest.cc

This CL having two types is the intermediate state, and if this CL sticks, we
can gradually deprecate the hijacking one. What do you think about this
approach?

[brettw, 2016-11-16 16:19:18]

If there are only 3 hijacking cases, I'd rather fix them in one go than have an
undesirable intermediate state.

[wychen, 2016-11-17 03:02:50]

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

[commit-bot: I haz the power, 2016-11-17 03:04:15]

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-11-17 04:28:40]

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

[commit-bot: I haz the power, 2016-11-17 04:28:42]

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

[wychen, 2016-11-17 08:20:38]

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

[commit-bot: I haz the power, 2016-11-17 08:21:09]

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

[wychen, 2016-11-17 09:18:32]

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

[commit-bot: I haz the power, 2016-11-17 09:18:58]

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-11-17 09:56:07]

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

[commit-bot: I haz the power, 2016-11-17 09:56:08]

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

[wychen, 2016-11-18 00:54:13]

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

[wychen, 2016-11-18 00:55:21]

Patchset #25 (id:970001) has been deleted

[commit-bot: I haz the power, 2016-11-18 00:56:21]

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-11-18 03:53:14]

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

[commit-bot: I haz the power, 2016-11-18 03:53:16]

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

[wychen, 2016-11-18 07:06:51]

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

[commit-bot: I haz the power, 2016-11-18 07:07:06]

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-11-18 11:41:34]

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

[commit-bot: I haz the power, 2016-11-18 11:41:36]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[wychen, 2016-11-18 21:08:58]

Patchset #26 (id:1010001) has been deleted

[wychen, 2016-11-18 21:14:00]

I'll also look into the linux isolation failure.

https://codereview.chromium.org/2034393004/diff/950001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2034393004/diff/950001/base/logging.cc#newcod...
base/logging.cc:376: LogMessageListener::LogMessageListener() {
On 2016/11/14 14:40:43, Mark Mentovai wrote:
> So much duplication…

No more. :)

https://codereview.chromium.org/2034393004/diff/950001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2034393004/diff/950001/base/logging.h#newcode306
base/logging.h:306: class BASE_EXPORT LogMessageListener {
Only keep listeners now.

https://codereview.chromium.org/2034393004/diff/950001/chrome/test/chromedriv...
File chrome/test/chromedriver/logging.cc (right):

https://codereview.chromium.org/2034393004/diff/950001/chrome/test/chromedriv...
chrome/test/chromedriver/logging.cc:249: CHECK(handler);
On 2016/11/14 22:01:31, brettw (ping on IM after 24h) wrote:
> Normally we wouldn't bother asserting the malloc returns null. I'd just do
>   new LogMessageHandler();  // Intentionally leak this instance.
> (unless this causes compiler warnings?).

Done.

https://codereview.chromium.org/2034393004/diff/950001/components/crash/conte...
File components/crash/content/app/crashpad.cc (right):

https://codereview.chromium.org/2034393004/diff/950001/components/crash/conte...
components/crash/content/app/crashpad.cc:253: CHECK(listener);
On 2016/11/14 22:01:31, brettw (ping on IM after 24h) wrote:
> Ditto

Done.

https://codereview.chromium.org/2034393004/diff/950001/third_party/crashpad/c...
File third_party/crashpad/crashpad/util/thread/OWNERS (right):

https://codereview.chromium.org/2034393004/diff/950001/third_party/crashpad/c...
third_party/crashpad/crashpad/util/thread/OWNERS:1: per-file *_messages.cc=set
noparent
On 2016/11/14 14:40:43, Mark Mentovai wrote:
> Revert this file. The PRESUBMIT bug that made you add it has been fixed. It
> should never have been poking around in third_party to begin with.

Done.

https://codereview.chromium.org/2034393004/diff/1030001/content/browser/frame...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2034393004/diff/1030001/content/browser/frame...
content/browser/frame_host/render_frame_host_impl.cc:966: // In order for CSP
violation detections in the tests to work, console
This comment should probably move to https://crrev.com/2013573007/.

[wychen, 2016-12-30 08:23:51]

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

[commit-bot: I haz the power, 2016-12-30 08:24:05]

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-12-30 12:28:36]

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

[commit-bot: I haz the power, 2016-12-30 12:28:38]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[wychen, 2017-01-03 09:58:14]

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

[commit-bot: I haz the power, 2017-01-03 09:58:22]

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-01-03 11:54:51]

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

[commit-bot: I haz the power, 2017-01-03 11:54:53]

Dry run: 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_...)

[grt (UTC plus 2), 2017-01-04 09:12:20]

please fix all inheritance; i gave up commenting on them. every class should use
public inheritance, each should declare its inherited methods in a "public:"
section, and each should use the DISALLOW macro at the end of the "private:"
section.

https://codereview.chromium.org/2034393004/diff/1090001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2034393004/diff/1090001/base/logging.cc#newco...
base/logging.cc:368: DCHECK_EQ(count - 1, handlers.size());
this check is a bit bogus. if you think it's possible that |this| is not in the
container, then the erase(remove) pattern will provoke undefined behavior or
worse. rationale: if |this| is not in |handlers|, the call to remove() will
return handlers.end(). end, being a "past the end" iterator cannot be
dereferenced. erase() must not be called with a "first" iterator that cannot be
dereferenced. hence, boom if |this| is not in the container.

that aside, you're only removing one element, so erase(remove) shouldn't be any
more efficient than a single erase. so, please pick one of:

  handlers.erase(handlers.find(handlers.begin(), handlers.end(), this));

or:

  auto it = handlers.find(handlers.begin(), handlers.end(), this);
  if (it != handlers.end())
    handlers.erase(it);
  else
    NOTREACHED();

depending on how confident you are that |this| will always be in the container.

https://codereview.chromium.org/2034393004/diff/1090001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2034393004/diff/1090001/base/logging.h#newcod...
base/logging.h:284: // OnMessage() is called for every log message before it's
sent to other log
nit: i think it's more clear to say messages are sent here after all listeners
and before other destinations. i also think it's more clear to define listeners
first since they are recommended (as per comment in LogMessageListener) and
because they see messages before handlers.

https://codereview.chromium.org/2034393004/diff/1090001/base/logging_win.h
File base/logging_win.h (right):

https://codereview.chromium.org/2034393004/diff/1090001/base/logging_win.h#ne...
base/logging_win.h:73: class WinLogMessageHandler : LogMessageHandler {
https://google.github.io/styleguide/cppguide.html#Inheritance "When using
inheritance, make it public."
" : public LogMessageHandler"

https://codereview.chromium.org/2034393004/diff/1090001/base/logging_win.h#ne...
base/logging_win.h:80: };
 private:
  DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2034393004/diff/1090001/base/test/mock_log.cc
File base/test/mock_log.cc (right):

https://codereview.chromium.org/2034393004/diff/1090001/base/test/mock_log.cc...
base/test/mock_log.cc:11: class MockLogMessageListener :
logging::LogMessageListener {
" : public logging::LogMessageListener"

https://codereview.chromium.org/2034393004/diff/1090001/base/test/mock_log.cc...
base/test/mock_log.cc:24: };
DISALLOW_COPY_AND_ASSIGN (and #include "base/macros.h")

https://codereview.chromium.org/2034393004/diff/1090001/base/trace_event/trac...
File base/trace_event/trace_event_unittest.cc (right):

https://codereview.chromium.org/2034393004/diff/1090001/base/trace_event/trac...
base/trace_event/trace_event_unittest.cc:2827: class MockLogMessageListener :
logging::LogMessageListener {
public

https://codereview.chromium.org/2034393004/diff/1090001/base/trace_event/trac...
base/trace_event/trace_event_unittest.cc:2838: };
DISALLOW

https://codereview.chromium.org/2034393004/diff/1090001/base/trace_event/trac...
base/trace_event/trace_event_unittest.cc:2841: MockLogMessageListener l;
https://google.github.io/styleguide/cppguide.html#General_Naming_Rules "Names
should be descriptive; avoid abbreviation."

https://codereview.chromium.org/2034393004/diff/1090001/base/trace_event/trac...
base/trace_event/trace_event_unittest.cc:2867: class
LogMessageListenerWithTraceEvent : logging::LogMessageListener {
public

https://codereview.chromium.org/2034393004/diff/1090001/base/trace_event/trac...
base/trace_event/trace_event_unittest.cc:2876: };
 private:
  DISALLOW...

https://codereview.chromium.org/2034393004/diff/1090001/chrome/browser/media/...
File chrome/browser/media/webrtc/webrtc_browsertest_base.h (right):

https://codereview.chromium.org/2034393004/diff/1090001/chrome/browser/media/...
chrome/browser/media/webrtc/webrtc_browsertest_base.h:24: class
JavascriptErrorDetectingListener : logging::LogMessageListener {
public

https://codereview.chromium.org/2034393004/diff/1090001/chrome/browser/safe_b...
File chrome/browser/safe_browsing/safe_browsing_database_unittest.cc (right):

https://codereview.chromium.org/2034393004/diff/1090001/chrome/browser/safe_b...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:216: class
ScopedLogMessageIgnorer : logging::LogMessageHandler {
public inheritance and " public:" in the body

https://codereview.chromium.org/2034393004/diff/1090001/chrome/browser/safe_b...
chrome/browser/safe_browsing/safe_browsing_database_unittest.cc:236: };
 private:
  DISALLOW...

[wychen, 2017-01-04 09:47:29]

On 2017/01/04 09:12:20, grt (UTC plus 1) wrote:
> please fix all inheritance; i gave up commenting on them. every class should
use
> public inheritance, each should declare its inherited methods in a "public:"
> section, and each should use the DISALLOW macro at the end of the "private:"
> section.

Thanks for your review. Patch set 27
(https://codereview.chromium.org/2034393004/#ps1050001) is actually my logical
"latest revision".
Patch set 28 and 29 are my attempts to repro trybot failure on
linux_site_isolation. I guess I should've used discardable CLs for this purpose.

Sorry for the confusion, and I'll apply your comments on top of Patch set 27.

[wychen, 2017-01-04 13:31:40]

Patchset #31 (id:1130001) has been deleted

[wychen, 2017-01-04 13:32:04]

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

[commit-bot: I haz the power, 2017-01-04 13:32:18]

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-01-04 17:01:56]

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

[commit-bot: I haz the power, 2017-01-04 17:01:58]

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

[wychen, 2017-01-05 02:12:14]

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

[commit-bot: I haz the power, 2017-01-05 02:12:37]

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-01-05 06:13:37]

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

[commit-bot: I haz the power, 2017-01-05 06:13:39]

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

[wychen, 2017-01-09 13:41:22]

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

[commit-bot: I haz the power, 2017-01-09 13:41:31]

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-01-09 17:11:47]

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

[commit-bot: I haz the power, 2017-01-09 17:11:50]

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

[grt (UTC plus 2), 2017-01-30 15:16:51]

Hi. This CL has the potential to fix https://crbug.com/649257. Are you still
working on this? What's blocking it? Thanks.

[wychen, 2017-01-31 11:10:32]

On 2017/01/30 15:16:51, grt (UTC plus 1) wrote:
> Hi. This CL has the potential to fix https://crbug.com/649257. Are you still
> working on this? What's blocking it? Thanks.

Two months ago, I was investigating the linux isolation failure, which was
caused by layout test timeout due to performance degradation.
Before I got to eliminate this performance degradation, I started to take
parental leave, so this was suspended.
I'll resume working on this CL in a few weeks, and hopefully minimize the
slowdown so that this can be landed.

[grt (UTC plus 2), 2017-01-31 11:33:26]

On 2017/01/31 11:10:32, wychen (APAC time zone) wrote:
> On 2017/01/30 15:16:51, grt (UTC plus 1) wrote:
> > Hi. This CL has the potential to fix https://crbug.com/649257. Are you still
> > working on this? What's blocking it? Thanks.
> 
> Two months ago, I was investigating the linux isolation failure, which was
> caused by layout test timeout due to performance degradation.
> Before I got to eliminate this performance degradation, I started to take
> parental leave, so this was suspended.

Congratulations!

> I'll resume working on this CL in a few weeks, and hopefully minimize the
> slowdown so that this can be landed.

Super, thanks for the update.

[Dan Beam, 2017-06-10 00:38:46]

happy belated birthday to this CL!

[grt (UTC plus 2), 2017-08-03 08:07:58]

wychen: will you be able to continue this?

[wychen, 2017-08-08 04:08:24]

On 2017/08/03 08:07:58, grt (UTC plus 2) wrote:
> wychen: will you be able to continue this?

Apologize for holding this for way too long.
I'm OOO until 8/25, so I won't be working on this in the short term. If anyone
is willing to take it, please feel free to do so. Otherwise I'll try to rebase
and land it when I come back.