Logging: pass logging related cmd line options to utility process

Logging: pass logging related cmd line options to utility process

Logging doesn't work in utility process due to the related command line
options are not passed to utility process. For example, run
" ./chrome --vmodule=unpacker=1 --enable-logging=stderr", DVLOG(1)
message in unpacker.cc can not be output correctly. This patch passes the
related command line options to utility process.

BUG=
TEST=--enable-logging=stderr --vmodule=unpacker=1, then open  chrome://extensions, drag a foo.crx in the page, you can see the DVLOG message " Installing extension ".

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162665

[qiankun, 2012-10-11 08:34:40]

please help to review, thanks!

[jam, 2012-10-11 15:25:41]

1) you need to sign the CLA, see
http://dev.chromium.org/developers/contributing-code/external-contributor-che...
for more informatoin

2) please don't add many reviewers, one is enough.

[jam, 2012-10-11 15:27:58]

3) this should be done in a way that it works for all child processes, i.e. in
BrowserChildProcessHostImpl::Launch() we should be copying all the related
command line flags

[qiankun, 2012-10-12 02:18:53]

On 2012/10/11 15:25:41, John Abd-El-Malek wrote:
> 1) you need to sign the CLA, see
>
http://dev.chromium.org/developers/contributing-code/external->contributor-ch...
for more informatoin
Thanks for reminder. I have signed the CLA before.
> 
> 2) please don't add many reviewers, one is enough.

[qiankun, 2012-10-12 02:34:08]

On 2012/10/11 15:27:58, John Abd-El-Malek wrote:
> 3) this should be done in a way that it works for all child 
> processes, i.e. in
> BrowserChildProcessHostImpl::Launch() we should be copying all the 
> related
> command line flags
Plugins/workers process using BrowserChildProcessHostImpl class do the switches
copying in their own implementation plugin/worker_process_host.cc. So should we
move the related code from plugin/worker_process_host.cc to
BrowserChildProcessHostImpl::Launch() or just add the code in
utility_process_host_impl.cc?

[piman, 2012-10-12 17:00:33]

http://codereview.chromium.org/11103028/diff/6001/content/browser/utility_pro...
File content/browser/utility_process_host_impl.cc (right):

http://codereview.chromium.org/11103028/diff/6001/content/browser/utility_pro...
content/browser/utility_process_host_impl.cc:170: switches::kEnableLogging,  //
Support, e.g., --enable-logging=stderr
nit: I don't think this comment adds anything. Please remove?

http://codereview.chromium.org/11103028/diff/6001/content/browser/utility_pro...
content/browser/utility_process_host_impl.cc:173: };
Could you move the flags handling above into here, when it's of the type if
(browser_command_line.HasSwitch(kFlag)) cmd_line->AppendSwitch(kFlag);
It's the same thing, but this way is more concise/readable.

[qiankun, 2012-10-12 21:49:55]

On 2012/10/12 17:00:33, piman wrote:
>
http://codereview.chromium.org/11103028/diff/6001/content/browser/utility_pro...
> File content/browser/utility_process_host_impl.cc (right):
> 
>
http://codereview.chromium.org/11103028/diff/6001/content/browser/utility_pro...
> content/browser/utility_process_host_impl.cc:170: switches::kEnableLogging, 
//
> Support, e.g., --enable-logging=stderr
> nit: I don't think this comment adds anything. Please remove?
> 
> 
Thanks, I will remove this comments.
http://codereview.chromium.org/11103028/diff/6001/content/browser/utility_pro...
> content/browser/utility_process_host_impl.cc:173: };
> Could you move the flags handling above into here, when it's of the type if
> (browser_command_line.HasSwitch(kFlag)) cmd_line->AppendSwitch(kFlag);
> It's the same thing, but this way is more concise/readable.

Do you mean AppendSwitchNative instead of AppendSwitch here? Because
AppendSwitch is only used for no-value flags. --enable-logging may have a value
e.g. --enable-logging=stderr. So I think CopySwitchesFrom is more suitable here.

[qiankun, 2012-10-16 00:35:08]

Hi reviewers, updated comments and patch, please help to review.

[jam, 2012-10-16 14:56:34]

On 2012/10/12 02:34:08, qiankun.miao wrote:
> On 2012/10/11 15:27:58, John Abd-El-Malek wrote:
> > 3) this should be done in a way that it works for all child 
> > processes, i.e. in
> > BrowserChildProcessHostImpl::Launch() we should be copying all the 
> > related
> > command line flags
> Plugins/workers process using BrowserChildProcessHostImpl class do the
switches
> copying in their own implementation plugin/worker_process_host.cc. So should
we
> move the related code from plugin/worker_process_host.cc to
> BrowserChildProcessHostImpl::Launch() or just add the code in
> utility_process_host_impl.cc?

the common switches that we want propagated should be copied in
BrowserChildProcessHostImpl::Launch() instead of having every different host
class for a child process individually doing it

[qiankun, 2012-10-16 15:26:15]

On 2012/10/16 14:56:34, John Abd-El-Malek wrote:
> On 2012/10/12 02:34:08, qiankun.miao wrote:
> > On 2012/10/11 15:27:58, John Abd-El-Malek wrote:
> > > 3) this should be done in a way that it works for all child 
> > > processes, i.e. in
> > > BrowserChildProcessHostImpl::Launch() we should be copying all the 
> > > related
> > > command line flags
> > Plugins/workers process using BrowserChildProcessHostImpl class do the
> switches
> > copying in their own implementation plugin/worker_process_host.cc. So should
> we
> > move the related code from plugin/worker_process_host.cc to
> > BrowserChildProcessHostImpl::Launch() or just add the code in
> > utility_process_host_impl.cc?
> 
> the common switches that we want propagated should be copied in
> BrowserChildProcessHostImpl::Launch() instead of having every different host
> class for a child process individually doing it

Ok, I will move common switches into BrowserChildProcessHostImpl::Launch(). I
think worker/plugin/utility process will call into
BrowserChildProcessHostImpl::Launch(). Do I miss any processes?

[jam, 2012-10-16 21:17:55]

On 2012/10/16 15:26:15, qiankun.miao wrote:
> On 2012/10/16 14:56:34, John Abd-El-Malek wrote:
> > On 2012/10/12 02:34:08, qiankun.miao wrote:
> > > On 2012/10/11 15:27:58, John Abd-El-Malek wrote:
> > > > 3) this should be done in a way that it works for all child 
> > > > processes, i.e. in
> > > > BrowserChildProcessHostImpl::Launch() we should be copying all the 
> > > > related
> > > > command line flags
> > > Plugins/workers process using BrowserChildProcessHostImpl class do the
> > switches
> > > copying in their own implementation plugin/worker_process_host.cc. So
should
> > we
> > > move the related code from plugin/worker_process_host.cc to
> > > BrowserChildProcessHostImpl::Launch() or just add the code in
> > > utility_process_host_impl.cc?
> > 
> > the common switches that we want propagated should be copied in
> > BrowserChildProcessHostImpl::Launch() instead of having every different host
> > class for a child process individually doing it
> 
> Ok, I will move common switches into BrowserChildProcessHostImpl::Launch(). I
> think worker/plugin/utility process will call into
> BrowserChildProcessHostImpl::Launch(). Do I miss any processes?

correct, all child processes call it

[qiankun, 2012-10-17 05:47:09]

patch updated. Please help to review, thanks.

[piman, 2012-10-17 06:10:25]

http://codereview.chromium.org/11103028/diff/18001/content/browser/browser_ch...
File content/browser/browser_child_process_host_impl.cc (right):

http://codereview.chromium.org/11103028/diff/18001/content/browser/browser_ch...
content/browser/browser_child_process_host_impl.cc:126: static const char*
kForwardSwitches[] = {
nit: can you add:
#if defined(OS_POSIX)
    switches::kChildCleanExit,
#endif

here, and remove the corresponding code below?

http://codereview.chromium.org/11103028/diff/18001/content/browser/plugin_pro...
File content/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/11103028/diff/18001/content/browser/plugin_pro...
content/browser/plugin_process_host.cc:189: #if defined(OS_MACOSX)
nit: collapse the 2 #ifdef OS_MACOSX blocks

[qiankun, 2012-10-17 09:47:36]

thanks, modified according your comments.

http://codereview.chromium.org/11103028/diff/18001/content/browser/browser_ch...
File content/browser/browser_child_process_host_impl.cc (right):

http://codereview.chromium.org/11103028/diff/18001/content/browser/browser_ch...
content/browser/browser_child_process_host_impl.cc:126: static const char*
kForwardSwitches[] = {
On 2012/10/17 06:10:25, piman wrote:
> nit: can you add:
> #if defined(OS_POSIX)
>     switches::kChildCleanExit,
> #endif
> 
> here, and remove the corresponding code below?

Done.

http://codereview.chromium.org/11103028/diff/18001/content/browser/plugin_pro...
File content/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/11103028/diff/18001/content/browser/plugin_pro...
content/browser/plugin_process_host.cc:189: #if defined(OS_MACOSX)
On 2012/10/17 06:10:25, piman wrote:
> nit: collapse the 2 #ifdef OS_MACOSX blocks

Done.

[jam, 2012-10-17 15:19:41]

lgtm

https://codereview.chromium.org/11103028/diff/21002/content/browser/browser_c...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/11103028/diff/21002/content/browser/browser_c...
content/browser/browser_child_process_host_impl.cc:139: 
nit: extra line

[piman, 2012-10-17 16:02:38]

LGTM with John's nit addressed.

[commit-bot: I haz the power, 2012-10-18 01:09:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qiankun.miao@intel.com/11103028/21002

[commit-bot: I haz the power, 2012-10-18 01:09:41]

Failed to apply patch for AUTHORS:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file AUTHORS
  Hunk #1 FAILED at 205.
  1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej

Patch:       AUTHORS
Index: AUTHORS
diff --git a/AUTHORS b/AUTHORS
index
671434f50c5fc9c0ae65de10621e2333257b98f3..67e60f70c070f745967a4fdb54f1320793a30b6d
100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -205,3 +205,4 @@ Joshua Lock <joshua.lock@intel.com>
 Dai Chunyang <chunyang.dai@intel.com>
 Joe Thomas <mhx348@motorola.com>
 Ruben Terrazas <rubentopo@gmail.com>
+Qiankun Miao <qiankun.miao@intel.com>

[commit-bot: I haz the power, 2012-10-18 04:50:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qiankun.miao@intel.com/11103028/28002

[commit-bot: I haz the power, 2012-10-18 07:11:17]

Change committed as 162665