Fixed a TODO in data export of net-internals, added Chrome version and comman...

Fixed a TODO in data export of net-internals, added Chrome version and command line.

BUG=none
TEST=Go to about:net-internals click on dump to text, check if version and command line are good.

Patch contributed by malavv

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

[M-A Ruel, 2010-05-20 15:29:31]

I'm no javascript reviewer so skipping the javascript part. You'll need to find
another reviewer for that.

http://codereview.chromium.org/2104012/diff/6001/7001
File chrome/browser/dom_ui/net_internals_ui.cc (right):

http://codereview.chromium.org/2104012/diff/6001/7001#newcode511
chrome/browser/dom_ui/net_internals_ui.cc:511: std::wstring kVersion = L"N/A";
nit: I usually prefer this format but I don't mind:
std::wstring kVersion(L"N/A");

but all these variables aren't necessary. You should move lines 545-549 over
here with the default values and them just set the actual dict values in the
following lines.

http://codereview.chromium.org/2104012/diff/6001/7001#newcode541
chrome/browser/dom_ui/net_internals_ui.cc:541: kCommandSwitches += L" " +
L(*iter);
You cannot do L(*iter). L is not a function call.

If you need this, I'd rather have this function to be implemented in CommandLine
instead of hacking that in the client code.

[M-A Ruel, 2010-05-20 17:20:43]

http://codereview.chromium.org/2104012/diff/4004/15001
File base/command_line.cc (right):

http://codereview.chromium.org/2104012/diff/4004/15001#newcode306
base/command_line.cc:306: const std::wstring&
CommandLine::getOsIndependantCommandLineString() const {
#if !defined(OS_WIN)
std::string

http://codereview.chromium.org/2104012/diff/4004/15001#newcode308
base/command_line.cc:308: #if defined(OS_WIN)
Remove the windows part.
This function is a member function so you don't need to call the static function
to retrieve an instance of this object.

You cannot return a reference to a local variable. Hence the return type must be
"std::string"

http://codereview.chromium.org/2104012/diff/4004/15001#newcode311
base/command_line.cc:311: std::wstring commandLineStr(L"");
std::string command_line;

http://codereview.chromium.org/2104012/diff/4004/15001#newcode313
base/command_line.cc:313: const ArgvList& argv =
CommandLine::ForCurrentProcess()->argv();
Use argv_ directly.

http://codereview.chromium.org/2104012/diff/4004/15001#newcode317
base/command_line.cc:317: commandLineStr += L" " + *iter;
{
command_line += " ";
command_line += *iter;
}

otherwise you generate 2 unnecessary temporary objects.

http://codereview.chromium.org/2104012/diff/4004/15002
File base/command_line.h (right):

http://codereview.chromium.org/2104012/diff/4004/15002#newcode146
base/command_line.h:146: #elif defined(OS_POSIX)
add the function:

std::string command_line_string();

http://codereview.chromium.org/2104012/diff/4004/15002#newcode154
base/command_line.h:154: const std::wstring& getOsIndependantCommandLineString()
const;
Remove this function.

http://codereview.chromium.org/2104012/diff/4004/15003
File chrome/browser/dom_ui/net_internals_ui.cc (right):

http://codereview.chromium.org/2104012/diff/4004/15003#newcode523
chrome/browser/dom_ui/net_internals_ui.cc:523: dict->SetString(L"COMMAND_LINE",
L"N/A");
Aren't these constants defined somewhere? I mean "VERSION", "OFFICIAL", etc.

http://codereview.chromium.org/2104012/diff/4004/15003#newcode532
chrome/browser/dom_ui/net_internals_ui.cc:532: if
(version_info->is_official_build())
Use accolades.

[malavv, 2010-05-20 17:52:13]

New patch with revised code to solve a net-internals todo.

[M-A Ruel, 2010-05-20 18:31:39]

http://codereview.chromium.org/2104012/diff/20001/21001
File base/command_line.cc (right):

http://codereview.chromium.org/2104012/diff/20001/21001#newcode306
base/command_line.cc:306: const std::string command_line_string() const {
This function must be in a #if !defined(OS_WIN)

http://codereview.chromium.org/2104012/diff/20001/21001#newcode307
base/command_line.cc:307: 
no empty line

http://codereview.chromium.org/2104012/diff/20001/21001#newcode309
base/command_line.cc:309: typedef std::vector<std::string> ArgvList;
typedef is unnecessary for one use.

http://codereview.chromium.org/2104012/diff/20001/21001#newcode319
base/command_line.cc:319: const std::wstring&
CommandLine::getPosixCommandLineString() const {
Remove this function

http://codereview.chromium.org/2104012/diff/20001/21002
File base/command_line.h (right):

http://codereview.chromium.org/2104012/diff/20001/21002#newcode153
base/command_line.h:153: const std::string command_line_string() const;
no need to make the result const.

[malavv, 2010-05-20 18:46:34]

I have fixed inconsistencies and optimization as proposed by maruel.

[M-A Ruel, 2010-05-20 18:48:58]

C++ part lgtm, pushing to Eric for the javascript.

[eroman, 2010-05-20 19:01:36]

Cool, thanks for doing this!

I have some style nits:

http://codereview.chromium.org/2104012/diff/34001/35001
File base/command_line.cc (right):

http://codereview.chromium.org/2104012/diff/34001/35001#newcode312
base/command_line.cc:312: command_line += L" ";
You can simplify this and just do:

return JoinString(argv_, ' ');

(Defined in base/string_util.h).

http://codereview.chromium.org/2104012/diff/34001/35003
File chrome/browser/dom_ui/net_internals_ui.cc (right):

http://codereview.chromium.org/2104012/diff/34001/35003#newcode507
chrome/browser/dom_ui/net_internals_ui.cc:507: // Tell the javascript about the
version of the client and it's
spelling nit: it's --> its

http://codereview.chromium.org/2104012/diff/34001/35003#newcode513
chrome/browser/dom_ui/net_internals_ui.cc:513:
chrome_app::GetChromeVersionInfo());
nit: indent another 2 spaces. (continued lines use 4 spaces).

http://codereview.chromium.org/2104012/diff/34001/35003#newcode515
chrome/browser/dom_ui/net_internals_ui.cc:515: if (version_info == NULL) {
minor nit: I would just do nothing from the C++ side in this case (and let the
javascript handle 'undefined') variables.

http://codereview.chromium.org/2104012/diff/34001/35003#newcode527
chrome/browser/dom_ui/net_internals_ui.cc:527: dict->SetString(L"VERSION",
version_info->file_version());
I recommend using lower-case names for the properties.

http://codereview.chromium.org/2104012/diff/34001/35003#newcode530
chrome/browser/dom_ui/net_internals_ui.cc:530:
UTF16ToWide(platform_util::GetVersionStringModifier()));
nit: indent by 4 spaces.

http://codereview.chromium.org/2104012/diff/34001/35003#newcode534
chrome/browser/dom_ui/net_internals_ui.cc:534:
l10n_util::GetString(IDS_ABOUT_VERSION_OFFICIAL));
nit: indent by 4 spaces.

http://codereview.chromium.org/2104012/diff/34001/35003#newcode541
chrome/browser/dom_ui/net_internals_ui.cc:541:
CommandLine::ForCurrentProcess()->command_line_string());
ditto.

[eroman, 2010-05-20 19:08:35]

lgtm after the above nits.

http://codereview.chromium.org/2104012/diff/34001/35003
File chrome/browser/dom_ui/net_internals_ui.cc (right):

http://codereview.chromium.org/2104012/diff/34001/35003#newcode515
chrome/browser/dom_ui/net_internals_ui.cc:515: if (version_info == NULL) {
On 2010/05/20 19:01:36, eroman wrote:
> minor nit: I would just do nothing from the C++ side in this case (and let the
> javascript handle 'undefined') variables.

(What I meant by this, is simply remove the code from this block -- the
javascript will then just print "undefined" when it tries to stringize the
properties that don't exist).

[malavv, 2010-05-20 19:24:42]

Hi Eric,

I see what you mean, thanks for the clarifications.

Maxime.

On Thu, May 20, 2010 at 3:08 PM, <eroman@chromium.org> wrote:

> lgtm after the above nits.
>
>
>
> http://codereview.chromium.org/2104012/diff/34001/35003
> File chrome/browser/dom_ui/net_internals_ui.cc (right):
>
> http://codereview.chromium.org/2104012/diff/34001/35003#newcode515
> chrome/browser/dom_ui/net_internals_ui.cc:515: if (version_info == NULL)
> {
> On 2010/05/20 19:01:36, eroman wrote:
>
>> minor nit: I would just do nothing from the C++ side in this case (and
>>
> let the
>
>> javascript handle 'undefined') variables.
>>
>
> (What I meant by this, is simply remove the code from this block -- the
> javascript will then just print "undefined" when it tries to stringize
> the properties that don't exist).
>
>
> http://codereview.chromium.org/2104012/show
>

[eroman, 2010-05-20 23:14:56]

LGTM.

Two more optional nits:

> 522       dict->SetString(L"version_mod",
> 523           UTF16ToWide(platform_util::GetVersionStringModifier()));

How about:
dict->SetStringFromUTF16(L"version_mod",
platform_util::GetVersionStringModifier());

> 214 BrowserBridge.prototype.receivedClientInformationsConstants =
> 215 function(constantsMap) {
> 216   ClientInformations = constantsMap;
> 217 };

I suggest removing the name "constants", and just calling this
"receivedClientInformations", or even better, "receivedClientInfo".
Similarly for the argument, instead of "constantsMap" name it "info".

On Thu, May 20, 2010 at 12:24 PM, Maxime Lavigne <malavv@google.com> wrote:

> Hi Eric,
>
> I see what you mean, thanks for the clarifications.
>
> Maxime.
>
>
> On Thu, May 20, 2010 at 3:08 PM, <eroman@chromium.org> wrote:
>
>> lgtm after the above nits.
>>
>>
>>
>> http://codereview.chromium.org/2104012/diff/34001/35003
>> File chrome/browser/dom_ui/net_internals_ui.cc (right):
>>
>> http://codereview.chromium.org/2104012/diff/34001/35003#newcode515
>> chrome/browser/dom_ui/net_internals_ui.cc:515: if (version_info == NULL)
>> {
>> On 2010/05/20 19:01:36, eroman wrote:
>>
>>> minor nit: I would just do nothing from the C++ side in this case (and
>>>
>> let the
>>
>>> javascript handle 'undefined') variables.
>>>
>>
>> (What I meant by this, is simply remove the code from this block -- the
>> javascript will then just print "undefined" when it tries to stringize
>> the properties that don't exist).
>>
>>
>> http://codereview.chromium.org/2104012/show
>>
>
>

[malavv, 2010-05-21 13:54:50]

Correction of the remaining nits.

http://codereview.chromium.org/2104012/diff/34001/35001
File base/command_line.cc (right):

http://codereview.chromium.org/2104012/diff/34001/35001#newcode312
base/command_line.cc:312: command_line += L" ";
On 2010/05/20 19:01:36, eroman wrote:
> You can simplify this and just do:
> 
> return JoinString(argv_, ' ');
> 
> (Defined in base/string_util.h).

Done.

http://codereview.chromium.org/2104012/diff/34001/35003
File chrome/browser/dom_ui/net_internals_ui.cc (right):

http://codereview.chromium.org/2104012/diff/34001/35003#newcode507
chrome/browser/dom_ui/net_internals_ui.cc:507: // Tell the javascript about the
version of the client and it's
On 2010/05/20 19:01:36, eroman wrote:
> spelling nit: it's --> its

Done.

http://codereview.chromium.org/2104012/diff/34001/35003#newcode513
chrome/browser/dom_ui/net_internals_ui.cc:513:
chrome_app::GetChromeVersionInfo());
On 2010/05/20 19:01:36, eroman wrote:
> nit: indent another 2 spaces. (continued lines use 4 spaces).

Done.

http://codereview.chromium.org/2104012/diff/34001/35003#newcode515
chrome/browser/dom_ui/net_internals_ui.cc:515: if (version_info == NULL) {
On 2010/05/20 19:01:36, eroman wrote:
> minor nit: I would just do nothing from the C++ side in this case (and let the
> javascript handle 'undefined') variables.

Done.

http://codereview.chromium.org/2104012/diff/34001/35003#newcode527
chrome/browser/dom_ui/net_internals_ui.cc:527: dict->SetString(L"VERSION",
version_info->file_version());
On 2010/05/20 19:01:36, eroman wrote:
> I recommend using lower-case names for the properties.

Done.

http://codereview.chromium.org/2104012/diff/34001/35003#newcode530
chrome/browser/dom_ui/net_internals_ui.cc:530:
UTF16ToWide(platform_util::GetVersionStringModifier()));
On 2010/05/20 19:01:36, eroman wrote:
> nit: indent by 4 spaces.

Done.

http://codereview.chromium.org/2104012/diff/34001/35003#newcode534
chrome/browser/dom_ui/net_internals_ui.cc:534:
l10n_util::GetString(IDS_ABOUT_VERSION_OFFICIAL));
On 2010/05/20 19:01:36, eroman wrote:
> nit: indent by 4 spaces.

Done.

http://codereview.chromium.org/2104012/diff/34001/35003#newcode541
chrome/browser/dom_ui/net_internals_ui.cc:541:
CommandLine::ForCurrentProcess()->command_line_string());
On 2010/05/20 19:01:36, eroman wrote:
> ditto.

Done.