Add OS version information to the bug report utility for the Mac version....

Add OS version information to the bug report utility for the Mac version.

BUG= http://crbug.com/26138
TEST= none

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

[Miranda Callahan, 2009-11-03 20:05:33]

I was able to use the existing SysInfo::OperatingSystemVersionNumbers to get
most of the data, and then pulled in GTMSystemVersion in order to get the build
number as well.

[Mark Mentovai, 2009-11-03 20:08:33]

http://codereview.chromium.org/355013/diff/1004/1009
File base/sys_info_mac.mm (right):

http://codereview.chromium.org/355013/diff/1004/1009#newcode4
Line 4: #include <CoreServices/CoreServices.h>
blank line before

http://codereview.chromium.org/355013/diff/1004/1007
File chrome/browser/bug_report_util.cc (right):

http://codereview.chromium.org/355013/diff/1004/1007#newcode79
Line 79: *os_version = StringPrintf("%d.%d.%d %s", major, minor, bugFix);
"%d.%d.%d %s" is four tokens.

I only see three parameters after the format string.

[Mark Mentovai, 2009-11-03 20:12:03]

http://codereview.chromium.org/355013/diff/1004/1009
File base/sys_info_mac.mm (right):

http://codereview.chromium.org/355013/diff/1004/1009#newcode4
Line 4: #include <CoreServices/CoreServices.h>
Actually, the ordering was OK before.  This should be:

"base/sys_info.h"

<CoreServices/CoreServices.h>

"base/sys_string_conversions.h"
"third_party/GTM/Foundation/GTMSystemVersion.h"

The header file corresponding to the implementation file you're working in
should always come first, even before system headers.

This ordering helps uncover hidden problems in headers that don't list all of
the #includes that they need, by putting most headers first in at least one
implementation file.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...

[Avi (use Gerrit), 2009-11-03 20:14:03]

http://codereview.chromium.org/355013/diff/1004/1009
File base/sys_info_mac.mm (right):

http://codereview.chromium.org/355013/diff/1004/1009#newcode21
Line 21: reinterpret_cast<SInt32*>(bugfix_version));
If we're hauling in GTMSystemVersion, we might as well just call through to its
+getMajor:minor:bugFix:.

[Mark Mentovai, 2009-11-03 20:20:10]

There are concerns about sandbox warmup if we change the
implementation of SysInfo::OperatingSystemVersionNumbers.

Mark

avi@chromium.org wrote:
> http://codereview.chromium.org/355013/diff/1004/1009
> File base/sys_info_mac.mm (right):
>
> http://codereview.chromium.org/355013/diff/1004/1009#newcode21
> Line 21: reinterpret_cast<SInt32*>(bugfix_version));
> If we're hauling in GTMSystemVersion, we might as well just call through
> to its +getMajor:minor:bugFix:.

[stuartmorgan, 2009-11-03 20:21:27]

This LGTM with the one fix if we want the build; I'm not sure it's important
enough to bother with since outside of pre-release versions it rarely gives
useful information.

http://codereview.chromium.org/355013/diff/1004/1007
File chrome/browser/bug_report_util.cc (right):

http://codereview.chromium.org/355013/diff/1004/1007#newcode79
Line 79: *os_version = StringPrintf("%d.%d.%d %s", major, minor, bugFix);
The format string shouldn't have the %s

[Miranda Callahan, 2009-11-03 20:29:39]

Mark and Avi, what do you think about Stuart's suggestion to leave out the build
number altogether? If it's not necessary, that makes this change much simpler
(it becomes patch set 1).

[Miranda Callahan, 2009-11-03 20:41:34]

Fixed it all, then reverted to patch set 1 -- a vastly simplified patch, with no
GTM stuff necessary.

http://codereview.chromium.org/355013/diff/1004/1009
File base/sys_info_mac.mm (right):

http://codereview.chromium.org/355013/diff/1004/1009#newcode4
Line 4: #include <CoreServices/CoreServices.h>
On 2009/11/03 20:08:33, Mark Mentovai wrote:
> blank line before

Done.

http://codereview.chromium.org/355013/diff/1004/1009#newcode4
Line 4: #include <CoreServices/CoreServices.h>
On 2009/11/03 20:12:03, Mark Mentovai wrote:
> Actually, the ordering was OK before.  This should be:
> 
> "base/sys_info.h"
> 
> <CoreServices/CoreServices.h>
> 
> "base/sys_string_conversions.h"
> "third_party/GTM/Foundation/GTMSystemVersion.h"
> 
> The header file corresponding to the implementation file you're working in
> should always come first, even before system headers.
> 
> This ordering helps uncover hidden problems in headers that don't list all of
> the #includes that they need, by putting most headers first in at least one
> implementation file.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...

Fixed.

http://codereview.chromium.org/355013/diff/1004/1009#newcode21
Line 21: reinterpret_cast<SInt32*>(bugfix_version));
On 2009/11/03 20:14:03, Avi wrote:
> If we're hauling in GTMSystemVersion, we might as well just call through to
its
> +getMajor:minor:bugFix:.

Done.

http://codereview.chromium.org/355013/diff/1004/1007
File chrome/browser/bug_report_util.cc (right):

http://codereview.chromium.org/355013/diff/1004/1007#newcode79
Line 79: *os_version = StringPrintf("%d.%d.%d %s", major, minor, bugFix);
On 2009/11/03 20:08:33, Mark Mentovai wrote:
> "%d.%d.%d %s" is four tokens.
> 
> I only see three parameters after the format string.

Fixed.

[Mark Mentovai, 2009-11-03 20:42:08]

I don't think that the build number is terribly useful here either,
but it doesn't hurt and you've already done the work...

The part that bugs me is how we drag GTM files one at a time into
targets as needed, when we really should just make it its own target.
That's not your fault or a reasonable problem to fix in this change,
though, and we do it elsewhere.

Mark

mirandac@chromium.org wrote:
> Mark and Avi, what do you think about Stuart's suggestion to leave out the
> build
> number altogether? If it's not necessary, that makes this change much
> simpler
> (it becomes patch set 1).
>
>
>
> http://codereview.chromium.org/355013
>

[pink (ping after 24hrs), 2009-11-04 13:48:55]

Can you file a bug? Then at least we can get it on the radar so we
don't continue to forget about it. kthxbai.

On Tue, Nov 3, 2009 at 3:41 PM, Mark Mentovai <mark@chromium.org> wrote:
>
> I don't think that the build number is terribly useful here either,
> but it doesn't hurt and you've already done the work...
>
> The part that bugs me is how we drag GTM files one at a time into
> targets as needed, when we really should just make it its own target.
> That's not your fault or a reasonable problem to fix in this change,
> though, and we do it elsewhere.
>
> Mark
>
> mirandac@chromium.org wrote:
>> Mark and Avi, what do you think about Stuart's suggestion to leave out the
>> build
>> number altogether? If it's not necessary, that makes this change much
>> simpler
>> (it becomes patch set 1).
>>
>>
>>
>> http://codereview.chromium.org/355013
>>
>



-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com