Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(427)

Issue 7648042: Change Chromoting logger to be setup in plugin's NP_Initialize. (Closed)

Created:
9 years, 4 months ago by garykac
Modified:
9 years, 3 months ago
Reviewers:
Wez
CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, dmaclach+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, ajwong+watch_chromium.org, sergeyu+watch_chromium.org
Visibility:
Public.

Description

Change Chromoting logger to be setup in CreatePlugin. This fixes a crash that occurred when multiple hosts were shared in the same browser. BUG=92078 TEST=manual Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99850

Patch Set 1 #

Total comments: 10

Patch Set 2 : update #

Patch Set 3 : add missing files #

Patch Set 4 : remove comment #

Total comments: 18

Patch Set 5 : review comments #

Patch Set 6 : move client log changes into separate cl #

Total comments: 8

Patch Set 7 : updated patch set #

Total comments: 8

Patch Set 8 : review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+205 lines, -54 lines) Patch
A remoting/host/plugin/host_log_handler.h View 1 2 3 4 5 6 1 chunk +38 lines, -0 lines 0 comments Download
A remoting/host/plugin/host_log_handler.cc View 1 2 3 4 5 6 7 1 chunk +119 lines, -0 lines 0 comments Download
M remoting/host/plugin/host_plugin.cc View 1 2 3 4 4 chunks +16 lines, -0 lines 0 comments Download
M remoting/host/plugin/host_script_object.h View 1 2 3 4 5 6 7 4 chunks +9 lines, -6 lines 0 comments Download
M remoting/host/plugin/host_script_object.cc View 1 2 3 4 5 6 7 6 chunks +21 lines, -48 lines 0 comments Download
M remoting/remoting.gyp View 1 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
garykac
9 years, 4 months ago (2011-08-15 23:59:16 UTC) #1
Wez
LGTM, but please move the register/unregister functions as noted below, and follow up with me ...
9 years, 4 months ago (2011-08-16 00:05:45 UTC) #2
garykac
http://codereview.chromium.org/7648042/diff/1/remoting/host/plugin/host_script_object.cc File remoting/host/plugin/host_script_object.cc (right): http://codereview.chromium.org/7648042/diff/1/remoting/host/plugin/host_script_object.cc#newcode553 remoting/host/plugin/host_script_object.cc:553: LOG(INFO) << "Unregistering global log handler"; On 2011/08/16 00:05:45, ...
9 years, 4 months ago (2011-08-16 00:19:40 UTC) #3
Wez
http://codereview.chromium.org/7648042/diff/1/remoting/host/plugin/host_script_object.cc File remoting/host/plugin/host_script_object.cc (right): http://codereview.chromium.org/7648042/diff/1/remoting/host/plugin/host_script_object.cc#newcode553 remoting/host/plugin/host_script_object.cc:553: LOG(INFO) << "Unregistering global log handler"; On 2011/08/16 00:19:40, ...
9 years, 4 months ago (2011-08-16 00:30:52 UTC) #4
garykac
PTAL. Note that I moved the log handler registration into CreatePlugin and I unregister in ...
9 years, 3 months ago (2011-08-29 23:27:14 UTC) #5
Wez
Looks like some client changes have crept in? http://codereview.chromium.org/7648042/diff/10001/remoting/client/plugin/chromoting_instance.cc File remoting/client/plugin/chromoting_instance.cc (right): http://codereview.chromium.org/7648042/diff/10001/remoting/client/plugin/chromoting_instance.cc#newcode448 remoting/client/plugin/chromoting_instance.cc:448: // ...
9 years, 3 months ago (2011-08-30 05:20:02 UTC) #6
garykac
I moved the client logging cleanup into a separate cl. http://codereview.chromium.org/7648042/diff/10001/remoting/client/plugin/chromoting_instance.cc File remoting/client/plugin/chromoting_instance.cc (right): http://codereview.chromium.org/7648042/diff/10001/remoting/client/plugin/chromoting_instance.cc#newcode448 ...
9 years, 3 months ago (2011-08-31 00:59:00 UTC) #7
Wez
LGTM but please see my comments below. http://codereview.chromium.org/7648042/diff/14009/remoting/host/plugin/host_log_handler.h File remoting/host/plugin/host_log_handler.h (right): http://codereview.chromium.org/7648042/diff/14009/remoting/host/plugin/host_log_handler.h#newcode8 remoting/host/plugin/host_log_handler.h:8: #include "base/string16.h" ...
9 years, 3 months ago (2011-08-31 01:30:16 UTC) #8
garykac
PTAL - I needed to add a check before registering since CreatePlugin is called once ...
9 years, 3 months ago (2011-08-31 23:59:46 UTC) #9
Wez
On 2011/08/31 23:59:46, garykac wrote: > PTAL - I needed to add a check before ...
9 years, 3 months ago (2011-09-01 00:29:05 UTC) #10
garykac
<sigh> it was waiting for a "Message describing this path set"... On Wed, Aug 31, ...
9 years, 3 months ago (2011-09-01 00:33:38 UTC) #11
Wez
LGTM with comments. ;) http://codereview.chromium.org/7648042/diff/19001/remoting/host/plugin/host_log_handler.cc File remoting/host/plugin/host_log_handler.cc (right): http://codereview.chromium.org/7648042/diff/19001/remoting/host/plugin/host_log_handler.cc#newcode36 remoting/host/plugin/host_log_handler.cc:36: LOG(INFO) << "Already have global ...
9 years, 3 months ago (2011-09-01 01:21:23 UTC) #12
garykac
9 years, 3 months ago (2011-09-01 01:30:54 UTC) #13
http://codereview.chromium.org/7648042/diff/19001/remoting/host/plugin/host_l...
File remoting/host/plugin/host_log_handler.cc (right):

http://codereview.chromium.org/7648042/diff/19001/remoting/host/plugin/host_l...
remoting/host/plugin/host_log_handler.cc:36: LOG(INFO) << "Already have global
log handler";
On 2011/09/01 01:21:23, Wez wrote:
> It doesn't seem necessary to log something here, since this is a normal state
of
> affairs, and the "Registering" message will already have been logged.

Done.

http://codereview.chromium.org/7648042/diff/19001/remoting/host/plugin/host_s...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/7648042/diff/19001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:598: // It's safe to check this value
only if we're on the plugin thread.
On 2011/09/01 01:21:23, Wez wrote:
> Actually, we only _need_ to check this value on the plugin thread, since it's
> only the plugin thread on which we might find ourselves doing re-entrant
> logging.

Done.

http://codereview.chromium.org/7648042/diff/19001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:620: printf("ERROR - LogDebugInfo
failed\n");
On 2011/09/01 01:21:23, Wez wrote:
> Now that you have |currently_logging_|, this really can be a LOG(), without
> causing any harm. :)

Done.

http://codereview.chromium.org/7648042/diff/19001/remoting/host/plugin/host_s...
File remoting/host/plugin/host_script_object.h (right):

http://codereview.chromium.org/7648042/diff/19001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.h:184: bool currently_logging_;
On 2011/09/01 01:21:23, Wez wrote:
> nit: Should technically be am_currently_logging_?

Done.

Powered by Google App Engine
This is Rietveld 408576698