Add more information to about:version and "About Chromium" dialog

Add more information to about:version and "About Chromium" dialog
- Add the information of an executable full-path, a profile path and an OS to about:version
- Add the information of a build type and an OS to "About Chromium" dialog

(Retry of r84808 with ChromeOS fixed.)

BUG=18679, 37186
TEST=Observe that about:version and "About Chromium" dialog display enough information

Patch by Kentaro Hara <haraken@google.com>;.

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

[haraken1, 2011-04-22 22:30:57]

I am trying to fix the bug 18679 and 37186. 
I found your name in git blame around the function that I modified. 
Could you take a look at it?

I could not test this patch on windows and Mac, since I do not have those
environments now and do not yet have a permission for using trybot. So, if this
bug-fix becomes LGTM, would you send this patch to the trybot before committing
instead of me?

Thanks.

[Evan Martin, 2011-04-22 22:36:40]

It appears you accidentally included string_util changes in this change.

http://codereview.chromium.org/6894037/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6894037/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:4451: <message
name="IDS_ABOUT_VERSION_WEBKIT" desc="label for a WebKit on the about:version
page">
What does "label for a WebKit" mean?

http://codereview.chromium.org/6894037/diff/1/chrome/browser/browser_about_ha...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/6894037/diff/1/chrome/browser/browser_about_ha...
chrome/browser/browser_about_handler.cc:972: bool io_allowed =
base::ThreadRestrictions::SetIOAllowed(true);
I don't like this.  Why not just not call AbsolutePath?

http://codereview.chromium.org/6894037/diff/1/chrome/common/chrome_version_in...
File chrome/common/chrome_version_info.cc (right):

http://codereview.chromium.org/6894037/diff/1/chrome/common/chrome_version_in...
chrome/common/chrome_version_info.cc:95: return "Native Client";
I don't think this actually counts as an OS here.

http://codereview.chromium.org/6894037/diff/1/chrome/common/chrome_version_in...
chrome/common/chrome_version_info.cc:99: return "Windows";
Can you put this one first?  (We usually do Win/Mac/Linux in that order in this
kind of code.)

[haraken1, 2011-04-23 17:05:43]

> It appears you accidentally included string_util changes in this change.

Sorry! I invalidated the changes in the patch set 2. 

>
http://codereview.chromium.org/6894037/diff/1/chrome/app/generated_resources.grd
> File chrome/app/generated_resources.grd (right):
> 
>
http://codereview.chromium.org/6894037/diff/1/chrome/app/generated_resources....
> chrome/app/generated_resources.grd:4451: <message
> name="IDS_ABOUT_VERSION_WEBKIT" desc="label for a WebKit on the about:version
> page">
> What does "label for a WebKit" mean?

I agree. I deleted "label for a WebKit" and directly wrote a string "WebKit" in
about_version.html. Similarly, "label for a JavaScript" has no meaning and so I
deleted it and wrote a string "JavaScript" in about_version.html. 

>
http://codereview.chromium.org/6894037/diff/1/chrome/browser/browser_about_ha...
> File chrome/browser/browser_about_handler.cc (right):
> 
>
http://codereview.chromium.org/6894037/diff/1/chrome/browser/browser_about_ha...
> chrome/browser/browser_about_handler.cc:972: bool io_allowed =
> base::ThreadRestrictions::SetIOAllowed(true);
> I don't like this.  Why not just not call AbsolutePath?

I fixed it. 

>
http://codereview.chromium.org/6894037/diff/1/chrome/common/chrome_version_in...
> File chrome/common/chrome_version_info.cc (right):
> 
>
http://codereview.chromium.org/6894037/diff/1/chrome/common/chrome_version_in...
> chrome/common/chrome_version_info.cc:95: return "Native Client";
> I don't think this actually counts as an OS here.

I fixed it. 

>
http://codereview.chromium.org/6894037/diff/1/chrome/common/chrome_version_in...
> chrome/common/chrome_version_info.cc:99: return "Windows";
> Can you put this one first?  (We usually do Win/Mac/Linux in that order in
this
> kind of code.)

I fixed it. 

Thanks.

[haraken1, 2011-04-27 17:33:17]

Would you take a look at it?

[Evan Martin, 2011-04-27 18:00:23]

http://codereview.chromium.org/6894037/diff/1003/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6894037/diff/1003/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:4460: <message name="IDS_ABOUT_VERSION_OS"
desc="label for an OS on the about:version page">
All of the "a"/"an" in these descriptions ("a user agent", "an OS") should
instead be "the".

I am sorry about English, the rules are stupid.  :(

http://codereview.chromium.org/6894037/diff/1003/chrome/browser/browser_about...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/6894037/diff/1003/chrome/browser/browser_about...
chrome/browser/browser_about_handler.cc:993: if (!_wfullpath(path_buf,
executable_path.value().c_str(), MAX_PATH)) {
See base/file_util.h , AbsolutePath()

http://codereview.chromium.org/6894037/diff/1003/chrome/common/chrome_version...
File chrome/common/chrome_version_info.cc (right):

http://codereview.chromium.org/6894037/diff/1003/chrome/common/chrome_version...
chrome/common/chrome_version_info.cc:105: return
l10n_util::GetStringUTF16(IDS_PRODUCT_OS_NAME);
I believe OS_LINUX is true when OS_CHROMEOS is true, so this needs to be above
the OS_LINUX test.

[haraken1, 2011-04-28 00:37:02]

>
http://codereview.chromium.org/6894037/diff/1003/chrome/app/generated_resourc...
> File chrome/app/generated_resources.grd (right):
>
>
http://codereview.chromium.org/6894037/diff/1003/chrome/app/generated_resourc...
> chrome/app/generated_resources.grd:4460: <message
> name="IDS_ABOUT_VERSION_OS" desc="label for an OS on the about:version
> page">
> All of the "a"/"an" in these descriptions ("a user agent", "an OS")
> should instead be "the".

I fixed it.

>
http://codereview.chromium.org/6894037/diff/1003/chrome/browser/browser_about...
> File chrome/browser/browser_about_handler.cc (right):
>
>
http://codereview.chromium.org/6894037/diff/1003/chrome/browser/browser_about...
> chrome/browser/browser_about_handler.cc:993: if (!_wfullpath(path_buf,
> executable_path.value().c_str(), MAX_PATH)) {
> See base/file_util.h , AbsolutePath()

I fixed it.

>
http://codereview.chromium.org/6894037/diff/1003/chrome/common/chrome_version...
> File chrome/common/chrome_version_info.cc (right):
>
>
http://codereview.chromium.org/6894037/diff/1003/chrome/common/chrome_version...
> chrome/common/chrome_version_info.cc:105: return
> l10n_util::GetStringUTF16(IDS_PRODUCT_OS_NAME);
> I believe OS_LINUX is true when OS_CHROMEOS is true, so this needs to be
> above the OS_LINUX test.

I fixed it.

[Evan Martin, 2011-05-03 01:16:24]

I almost lost this change, sorry!  Please ping me if I don't respond to your
reviews quickly (within one day)

http://codereview.chromium.org/6894037/diff/7002/chrome/browser/browser_about...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/6894037/diff/7002/chrome/browser/browser_about...
chrome/browser/browser_about_handler.cc:993: if (_wfullpath(path_buf,
executable_path.value().c_str(), MAX_PATH)) {
It appears you didn't upload the change with AbsolutePath in it.

[haraken1, 2011-05-04 05:42:38]

I am very sorry but I am confused.

>
http://codereview.chromium.org/6894037/diff/7002/chrome/browser/browser_about...
> File chrome/browser/browser_about_handler.cc (right):
>
>
http://codereview.chromium.org/6894037/diff/7002/chrome/browser/browser_about...
> chrome/browser/browser_about_handler.cc:993: if (_wfullpath(path_buf,
> executable_path.value().c_str(), MAX_PATH)) {
> It appears you didn't upload the change with AbsolutePath in it.

Do you mean that I should use file_util::AbsolutePath()
instead of directly using _wfullpath (for WIN) or realpath (for POSIX)?

However, since this function can be called by the UI thread,
we cannot directly issue AbsolutePath().
The first possible way is not using AbsolutePath(), like the code that I wrote.
The second possible way is setting base::ThreadRestrictions::SetIOAllowed(true)
temporarily and then using AbsolutePath().
In the Patch set 1, I used the second way
and then you advised me not to take the second way, as follows.

>
http://codereview.chromium.org/6894037/diff/1/chrome/browser/browser_about_ha...
> chrome/browser/browser_about_handler.cc:972: bool io_allowed =
> base::ThreadRestrictions::SetIOAllowed(true);
> I don't like this.  Why not just not call AbsolutePath?

I took the meaning of this advice
as not using AbsolutePath() but using _wfullpath()
or realpath()... but am I wrong?


On the other hand, I begin to think that
essentially neither the first way nor the second way is not good.
Rather, since the absolute path of the executable path
does not change during one program execution,
maybe we should store the absolute path into the private member
of CommandLine class (base/command_line.h) just after the program begins.
Then we can use the absolute path from the UI thread without using
AbsolutePath().

Thanks.

[Evan Martin, 2011-05-04 22:09:52]

On Tue, May 3, 2011 at 10:42 PM, Kentaro Hara <haraken@google.com> wrote:
> Do you mean that I should use file_util::AbsolutePath()
> instead of directly using _wfullpath (for WIN) or realpath (for POSIX)?
>
> However, since this function can be called by the UI thread,
> we cannot directly issue AbsolutePath().
> The first possible way is not using AbsolutePath(), like the code that I
wrote.
> The second possible way is setting
base::ThreadRestrictions::SetIOAllowed(true)
> temporarily and then using AbsolutePath().
> In the Patch set 1, I used the second way
> and then you advised me not to take the second way, as follows.
>
>>
http://codereview.chromium.org/6894037/diff/1/chrome/browser/browser_about_ha...
>> chrome/browser/browser_about_handler.cc:972: bool io_allowed =
>> base::ThreadRestrictions::SetIOAllowed(true);
>> I don't like this.  Why not just not call AbsolutePath?
>
> I took the meaning of this advice
> as not using AbsolutePath() but using _wfullpath()
> or realpath()... but am I wrong?
>
>
> On the other hand, I begin to think that
> essentially neither the first way nor the second way is not good.
> Rather, since the absolute path of the executable path
> does not change during one program execution,
> maybe we should store the absolute path into the private member
> of CommandLine class (base/command_line.h) just after the program begins.
> Then we can use the absolute path from the UI thread without using
> AbsolutePath().

I think for now you can use ScopedAllowIO and not worry about it.
Please file a bug about it though.  Though AbsolutePath can access the
disk, I don't think it's a major issue for this part of the program.
It is only one call, and it is for a path that is unlikely to be very
complicated.

I am sorry for delaying your change for so long.

[haraken1, 2011-05-05 03:12:10]

Thank you very much for the clear explanation.

> I think for now you can use ScopedAllowIO and not worry about it.
> Please file a bug about it though.  Though AbsolutePath can access the
> disk, I don't think it's a major issue for this part of the program.
> It is only one call, and it is for a path that is unlikely to be very
> complicated.

I fixed it so that it uses ScopedAllowIO and calls AbsolutePath().
Since I found that this bug is already filed as bug 79458, I just
added a comment to the bug about our issue:

http://code.google.com/p/chromium/issues/detail?id=79458&q=id%3A79458&colspec...

[haraken1, 2011-05-08 11:05:27]

ping?

[Evan Martin, 2011-05-09 18:52:28]

LGTM

Really sorry I took so long to review.  :(

http://codereview.chromium.org/6894037/diff/14001/chrome/browser/browser_abou...
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/6894037/diff/14001/chrome/browser/browser_abou...
chrome/browser/browser_about_handler.cc:211: Profile* GetProfile() { return
profile_; }
As a simple getter, this can be:

Profile* profile() const { return profile_; }

[commit-bot: I haz the power, 2011-05-10 01:37:48]

Presubmit check for 6894037-14001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  chrome/browser/resources/about_version.html, line 78, 124 chars \
  chrome/browser/resources/about_version.html, line 81, 85 chars \
  chrome/browser/resources/about_version.html, line 84, 128 chars \
  chrome/browser/resources/about_version.html, line 93, 83 chars \
  chrome/browser/resources/about_version.html, line 95, 84 chars

[haraken1, 2011-05-10 08:19:30]

Dear Evan

Would you put this patch set into the commit queue instead of me?

If I put this patch set into the commit queue by clicking "Commit" checkbox,
then the patch set is rejected because of the presubmit's warnings. These
warnings are about the number of characters per line but ignorable since the
warned file is not a program but an HTML. (I am afraid that restricting the
number of characters per line of about_version.html will decrease readability.)

[Evan Martin, 2011-05-10 17:53:55]

I committed it.  I am again really sorry this took so long.  :(  I promise that
normally the code review to commit process only takes a day...

[Evan Martin, 2011-05-10 18:09:43]

This failed to compile on ARM:
http://build.chromium.org/p/chromium/builders/Arm/builds/10664/steps/compile/...

chrome/common/chrome_version_info.cc: In member function 'std::string
chrome::VersionInfo::OSType() const':
chrome/common/chrome_version_info.cc:97:36:error: 'IDS_PRODUCT_OS_NAME' was not
declared in this scope
cc1plus: warnings being treated as errors
chrome/common/chrome_version_info.cc:109:1:error: control reaches end of
non-void function
make: *** [out/Release/obj.target/common/chrome/common/chrome_version_info.o]
Error 1

[Evan Martin, 2011-05-10 18:13:26]

On 2011/05/10 18:09:43, Evan Martin wrote:
> This failed to compile on ARM:
>
http://build.chromium.org/p/chromium/builders/Arm/builds/10664/steps/compile/...
> 
> chrome/common/chrome_version_info.cc: In member function 'std::string
> chrome::VersionInfo::OSType() const':
> chrome/common/chrome_version_info.cc:97:36:error: 'IDS_PRODUCT_OS_NAME' was
not
> declared in this scope
> cc1plus: warnings being treated as errors
> chrome/common/chrome_version_info.cc:109:1:error: control reaches end of
> non-void function
> make: *** [out/Release/obj.target/common/chrome/common/chrome_version_info.o]
> Error 1

I think it is a simple problem, and it will be difficult for you to test, so I
am going to fix it and recommit the patch.

[Evan Martin, 2011-05-10 18:39:49]

On 2011/05/10 18:13:26, Evan Martin wrote:
> On 2011/05/10 18:09:43, Evan Martin wrote:
> > This failed to compile on ARM:
> >
>
http://build.chromium.org/p/chromium/builders/Arm/builds/10664/steps/compile/...
> > 
> > chrome/common/chrome_version_info.cc: In member function 'std::string
> > chrome::VersionInfo::OSType() const':
> > chrome/common/chrome_version_info.cc:97:36:error: 'IDS_PRODUCT_OS_NAME' was
> not
> > declared in this scope
> > cc1plus: warnings being treated as errors
> > chrome/common/chrome_version_info.cc:109:1:error: control reaches end of
> > non-void function
> > make: ***
[out/Release/obj.target/common/chrome/common/chrome_version_info.o]
> > Error 1
> 
> I think it is a simple problem, and it will be difficult for you to test, so I
> am going to fix it and recommit the patch.

Committed with this change

--- a/chrome/common/chrome_version_info.cc
+++ b/chrome/common/chrome_version_info.cc
@@ -10,6 +10,7 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
+#include "grit/chromium_strings.h"
 
 namespace chrome {
 
@@ -94,7 +95,7 @@ std::string VersionInfo::OSType() const {
 #elif defined(OS_MACOSX)
   return "Mac OS";
 #elif defined(OS_CHROMEOS)
-  return l10n_util::GetStringUTF16(IDS_PRODUCT_OS_NAME);
+  return UTF16ToASCII(l10n_util::GetStringUTF16(IDS_PRODUCT_OS_NAME));
 #elif defined(OS_LINUX)
   return "Linux";
 #elif defined(OS_FREEBSD)

[haraken1, 2011-05-11 00:16:17]

Thanks a lot!!!

[Mark Mentovai, 2011-06-30 01:28:39]

This seems to have been done for all platforms except Mac, and nobody mentioned
Mac or filed a bug about Mac.

[haraken1, 2011-06-30 01:35:58]

I am trying to unify the version string on "About Chromium" dialog on all
platforms in http://codereview.chromium.org/7104106/