Support 32-bit subversion fields in base::Version.

Support 32-bit subversion fields in base::Version.

BUG=464610
TEST=base_unittests

Committed: https://crrev.com/bf68f4d5ba5c035cf1aaaa4ee5915cd3e7c15327
Cr-Commit-Position: refs/heads/master@{#319806}

[Will Harris, 2015-03-06 01:52:45]

wfh@chromium.org changed reviewers:
+ thakis@chromium.org

[Will Harris, 2015-03-06 01:53:53]

wfh@chromium.org changed reviewers:
+ jbauman@chromium.org

[Will Harris, 2015-03-06 01:55:04]

PTAL - This is specifically to support a plugin that has a subversion field >
65535.  Can provide more background on request.

[jbauman, 2015-03-06 01:56:36]

content/browser/gpu lgtm

[Nico, 2015-03-06 16:28:10]

This looks incomplete. For example,
content/child/npapi/webplugin_delegate_impl_win.cc and
chrome/browser/web_applications/web_app_mac.mm will need similar changes ( and
more on
https://code.google.com/p/chromium/codesearch#search/&q=%5Cbcomponents%5C(%5C...).

I would try removing the components() method from the header, rebuild
everything, collect a list of files with build failures on all platforms, and
then audit these files.

[Will Harris, 2015-03-08 19:51:39]

wfh@chromium.org changed reviewers:
+ jyasskin@chromium.org

[Will Harris, 2015-03-08 19:52:26]

wfh@chromium.org changed reviewers:
+ ananta@chromium.org

[Will Harris, 2015-03-08 21:36:35]

wfh@chromium.org changed reviewers:
- jyasskin@chromium.org

[Will Harris, 2015-03-08 21:37:56]

On 2015/03/06 16:28:10, Nico wrote:
> This looks incomplete. For example,
> content/child/npapi/webplugin_delegate_impl_win.cc and
> chrome/browser/web_applications/web_app_mac.mm will need similar changes ( and
> more on
>
https://code.google.com/p/chromium/codesearch#search/&q=%5Cbcomponents%5C(%5C...).
> 
> I would try removing the components() method from the header, rebuild
> everything, collect a list of files with build failures on all platforms, and
> then audit these files.

I renamed components() then manually audited all the calls and verified
build/test on all platforms.  Then renamed it back.

PTAL.

+ananta -> content/child/npapi/webplugin_delegate_impl_win.cc

[grt (UTC plus 2), 2015-03-09 13:19:49]

FWIW, this class probably used 16-bit fields since that's what Windows uses:
https://msdn.microsoft.com/library/windows/desktop/aa381058.aspx. Will there be
any resulting oddness in supporting bigger numbers here?

https://codereview.chromium.org/985573003/diff/60001/base/version.cc
File base/version.cc (right):

https://codereview.chromium.org/985573003/diff/60001/base/version.cc#newcode37
base/version.cc:37: if (!StringToInt64(*it, &num))
why not StringToUint? you could static_assert that sizeof(unsigned) >=
sizeof(uint32_t) if you're concerned about that.

https://codereview.chromium.org/985573003/diff/60001/base/version.h
File base/version.h (right):

https://codereview.chromium.org/985573003/diff/60001/base/version.h#newcode60
base/version.h:60: const std::vector<uint32>& components() const { return
components_; }
please use uint32_t (and #include <stdint.h>) as per the comment at the top of
base/basictypes.h.

[Nico, 2015-03-09 14:21:57]

lgtm with more uint32_t (but address grt's concerns too)

https://codereview.chromium.org/985573003/diff/60001/base/version.cc
File base/version.cc (right):

https://codereview.chromium.org/985573003/diff/60001/base/version.cc#newcode37
base/version.cc:37: if (!StringToInt64(*it, &num))
On 2015/03/09 13:19:49, grt wrote:
> why not StringToUint? you could static_assert that sizeof(unsigned) >=
> sizeof(uint32_t) if you're concerned about that.

Is anything wrong with this? It makes it much easier to check for overflows.

https://codereview.chromium.org/985573003/diff/60001/chrome/browser/web_appli...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/985573003/diff/60001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app_mac.mm:505: unsigned int major_version =
0;
why not uint32_t here too?

https://codereview.chromium.org/985573003/diff/60001/content/child/npapi/webp...
File content/child/npapi/webplugin_delegate_impl_win.cc (right):

https://codereview.chromium.org/985573003/diff/60001/content/child/npapi/webp...
content/child/npapi/webplugin_delegate_impl_win.cc:190: unsigned int
GetPluginMajorVersion(const WebPluginInfo& plugin_info) {
uint32_t?

[Will Harris, 2015-03-09 22:52:18]

Thanks for the review.  Nico - please feel free to TAL while I wait for ananta
for content/child/npapi review.

https://codereview.chromium.org/985573003/diff/60001/base/version.cc
File base/version.cc (right):

https://codereview.chromium.org/985573003/diff/60001/base/version.cc#newcode37
base/version.cc:37: if (!StringToInt64(*it, &num))
On 2015/03/09 13:19:49, grt wrote:
> why not StringToUint? you could static_assert that sizeof(unsigned) >=
> sizeof(uint32_t) if you're concerned about that.

Done.

https://codereview.chromium.org/985573003/diff/60001/base/version.cc#newcode37
base/version.cc:37: if (!StringToInt64(*it, &num))
On 2015/03/09 14:21:57, Nico wrote:
> On 2015/03/09 13:19:49, grt wrote:
> > why not StringToUint? you could static_assert that sizeof(unsigned) >=
> > sizeof(uint32_t) if you're concerned about that.
> 
> Is anything wrong with this? It makes it much easier to check for overflows.

grt's way makes the code far simpler - since StringToUint does all the
validation.

https://codereview.chromium.org/985573003/diff/60001/base/version.h
File base/version.h (right):

https://codereview.chromium.org/985573003/diff/60001/base/version.h#newcode60
base/version.h:60: const std::vector<uint32>& components() const { return
components_; }
On 2015/03/09 13:19:49, grt wrote:
> please use uint32_t (and #include <stdint.h>) as per the comment at the top of
> base/basictypes.h.

Done.

https://codereview.chromium.org/985573003/diff/60001/chrome/browser/web_appli...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/985573003/diff/60001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app_mac.mm:505: unsigned int major_version =
0;
On 2015/03/09 14:21:57, Nico wrote:
> why not uint32_t here too?

Done.

https://codereview.chromium.org/985573003/diff/60001/content/child/npapi/webp...
File content/child/npapi/webplugin_delegate_impl_win.cc (right):

https://codereview.chromium.org/985573003/diff/60001/content/child/npapi/webp...
content/child/npapi/webplugin_delegate_impl_win.cc:190: unsigned int
GetPluginMajorVersion(const WebPluginInfo& plugin_info) {
On 2015/03/09 14:21:57, Nico wrote:
> uint32_t?

Done.

[ananta, 2015-03-09 22:56:54]

npapi lgtm

[Nico, 2015-03-09 22:58:00]

lgtm

[Nico, 2015-03-09 22:58:01]

lgtm

[Will Harris, 2015-03-09 22:59:16]

The CQ bit was checked by wfh@chromium.org

[Will Harris, 2015-03-09 22:59:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbauman@chromium.org, ananta@chromium.org
Link to the patchset: https://codereview.chromium.org/985573003/#ps100001
(title: "add a static assert")

[commit-bot: I haz the power, 2015-03-09 23:00:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/985573003/100001

[commit-bot: I haz the power, 2015-03-10 01:33:24]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-10 01:34:14]

Patchset 6 (id:??) landed as
https://crrev.com/bf68f4d5ba5c035cf1aaaa4ee5915cd3e7c15327
Cr-Commit-Position: refs/heads/master@{#319806}