[Mac] Use the app's version as the app shim bundle version.

[Mac] Use the app's version as the app shim bundle version.

This sets CFBundleShortVersionString to the app's version_name.
It was previously the version of Chrome that built the shim.
The Chrome version is now stored in CrBundleVersion.

This also adds |version_for_display| to ShortcutInfo.

BUG=465617
Committed: https://crrev.com/03b8f5b666728f61940ba94ed99ec7b8ad171fd0
Cr-Commit-Position: refs/heads/master@{#321894}

[jackhou1, 2015-03-17 23:19:12]

jackhou@chromium.org changed reviewers:
+ tapted@chromium.org

[jackhou1, 2015-03-17 23:19:12]

tapted, please take a look

[tapted, 2015-03-18 01:49:59]

https://codereview.chromium.org/1016543004/diff/1/chrome/browser/web_applicat...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/1016543004/diff/1/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app.h:56: base::Version version;
There's a comment on the bug about using a string, version_name. I think that
trend comes from Android, and it's becoming more common on web apps.

https://codereview.chromium.org/1016543004/diff/1/chrome/common/mac/app_mode_...
File chrome/common/mac/app_mode_chrome_locator.mm (right):

https://codereview.chromium.org/1016543004/diff/1/chrome/common/mac/app_mode_...
chrome/common/mac/app_mode_chrome_locator.mm:57: [cr_bundle
objectForInfoDictionaryKey:@"CrBundleVersion"]);
kCrBundleVersionKey?

Although I guess that's not linked here. If that's not easy to do, can we
instead pull it out to declared constant at the top of the file, with a comment
saying it comes from app_mode_common.mm and why it's not just #included.

[jackhou1, 2015-03-18 04:43:49]

https://codereview.chromium.org/1016543004/diff/1/chrome/browser/web_applicat...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/1016543004/diff/1/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app.h:56: base::Version version;
On 2015/03/18 01:49:59, tapted wrote:
> There's a comment on the bug about using a string, version_name. I think that
> trend comes from Android, and it's becoming more common on web apps.

Yeah, I considered this. Setting an arbitrary string seems to work, but Apple's
documentation says it should be a 3-part version string:

https://developer.apple.com/library/ios/documentation/General/Reference/InfoP...

There's also a comment here indicating that this is enforced on IOS, and for
CFBundleVersion:

https://code.google.com/p/chromium/codesearch#chromium/src/base/file_version_...

So I'm vaguely worried that it'll become enforced. But if that happens we'd have
to truncate the base::Version's 4th part anyway. Since it's only for display
purposes, maybe we should just use GetVersionForDisplay() and change it if we
ever have to. WDYT?

https://codereview.chromium.org/1016543004/diff/1/chrome/common/mac/app_mode_...
File chrome/common/mac/app_mode_chrome_locator.mm (right):

https://codereview.chromium.org/1016543004/diff/1/chrome/common/mac/app_mode_...
chrome/common/mac/app_mode_chrome_locator.mm:57: [cr_bundle
objectForInfoDictionaryKey:@"CrBundleVersion"]);
On 2015/03/18 01:49:59, tapted wrote:
> kCrBundleVersionKey?
> 
> Although I guess that's not linked here. If that's not easy to do, can we
> instead pull it out to declared constant at the top of the file, with a
comment
> saying it comes from app_mode_common.mm and why it's not just #included.

Done.

[tapted, 2015-03-19 04:10:20]

lgtm, but we should investigate whether we can do a similar thing for
Windows/Linux as well.

https://codereview.chromium.org/1016543004/diff/1/chrome/browser/web_applicat...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/1016543004/diff/1/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app.h:56: base::Version version;
On 2015/03/18 04:43:48, jackhou1 wrote:
> On 2015/03/18 01:49:59, tapted wrote:
> > There's a comment on the bug about using a string, version_name. I think
that
> > trend comes from Android, and it's becoming more common on web apps.
> 
> Yeah, I considered this. Setting an arbitrary string seems to work, but
Apple's
> documentation says it should be a 3-part version string:
> 
>
https://developer.apple.com/library/ios/documentation/General/Reference/InfoP...
> 
> There's also a comment here indicating that this is enforced on IOS, and for
> CFBundleVersion:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/file_version_...
> 
> So I'm vaguely worried that it'll become enforced. But if that happens we'd
have
> to truncate the base::Version's 4th part anyway. Since it's only for display
> purposes, maybe we should just use GetVersionForDisplay() and change it if we
> ever have to. WDYT?

Yeah I agree with you. And there doesn't seem to be another good key choice for
putting in a string.

[jackhou1, 2015-03-23 03:37:15]

https://codereview.chromium.org/1016543004/diff/1/chrome/browser/web_applicat...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/1016543004/diff/1/chrome/browser/web_applicat...
chrome/browser/web_applications/web_app.h:56: base::Version version;
On 2015/03/19 04:10:20, tapted (travelling) wrote:
> On 2015/03/18 04:43:48, jackhou1 wrote:
> > On 2015/03/18 01:49:59, tapted wrote:
> > > There's a comment on the bug about using a string, version_name. I think
> that
> > > trend comes from Android, and it's becoming more common on web apps.
> > 
> > Yeah, I considered this. Setting an arbitrary string seems to work, but
> Apple's
> > documentation says it should be a 3-part version string:
> > 
> >
>
https://developer.apple.com/library/ios/documentation/General/Reference/InfoP...
> > 
> > There's also a comment here indicating that this is enforced on IOS, and for
> > CFBundleVersion:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/file_version_...
> > 
> > So I'm vaguely worried that it'll become enforced. But if that happens we'd
> have
> > to truncate the base::Version's 4th part anyway. Since it's only for display
> > purposes, maybe we should just use GetVersionForDisplay() and change it if
we
> > ever have to. WDYT?
> 
> Yeah I agree with you. And there doesn't seem to be another good key choice
for
> putting in a string.

Done.

[jackhou1, 2015-03-23 03:42:59]

jackhou@chromium.org changed reviewers:
+ rsesek@chromium.org

[jackhou1, 2015-03-23 03:43:00]

rsesek, could you please review for OWNERS in:

chrome/common/mac/

[Robert Sesek, 2015-03-23 15:18:43]

LGTM

[jackhou1, 2015-03-23 22:20:58]

The CQ bit was checked by jackhou@chromium.org

[jackhou1, 2015-03-23 22:20:59]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/1016543004/#ps60001
(title: "Use GetVersionForDisplay.")

[commit-bot: I haz the power, 2015-03-23 22:22:33]

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

[commit-bot: I haz the power, 2015-03-23 23:22:48]

Committed patchset #4 (id:60001)

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

Patchset 4 (id:??) landed as
https://crrev.com/03b8f5b666728f61940ba94ed99ec7b8ad171fd0
Cr-Commit-Position: refs/heads/master@{#321894}