Componentize VersionInfo.

Componentize VersionInfo.

Create a new component version_info that allows to get information about
the current version of Chrome running, except for the channel (which has
to be provided by the embedder).

The method to get the channel is not moved to the component because on
Windows it depends on //chrome/installer which would be non-trivial to
componentize. Since the method is embedder-specific (though it have no
bad dependencies on the other platform) it makes sense to not move it
into the component.

Fixes VersionInfo API to only expose static method (since the class did
not have any state and the API was a mix of "const" and "static" methods)
and to delegate the job to version_info component in order to limit the
API changes.

Mass rename (chrome::)VersionInfo::(Channel|CHANNEL) after the enum was
moved to the version_info namespace in the corresponding component using
tools/git/mffr.py.

Change Channel enumeration to be a "class enum" and add static_cast<int>
where needed (as class enum do not implicitly convert to int).

BUG=511912
Committed: https://crrev.com/6e883e40d456dd422ac08ced1c25e7403f78bada
Cr-Commit-Position: refs/heads/master@{#341085}

[sdefresne, 2015-07-24 08:42:07]

sdefresne@chromium.org changed reviewers:
+ blundell@chromium.org

[sdefresne, 2015-07-24 08:42:08]

Colin can you do a first pass review?

Concentrate your attention on:
- components/
- chrome/chrome_common.gypi
- chrome/common/BUILD.gn
- chrome/common/chrome_version_info*

The rest is either bulk renaming of the enumeration or change to other targets
that used directly the generated file chrome/common/chrome_version_info_values.h
to instead use components/version_info/version_info_values.h.

[vabr (Chromium), 2015-07-24 08:45:02]

(FWIW, renaming in chrome/browser/password_manager/* LGTM. Vaclav)

[blundell, 2015-07-24 11:38:49]

This looks fine to me (although the bots don't seem very happy about it). I'm
not seeing //chrome/common/BUILD.gn.

Info about the motivation for this change in the CL description would be useful.

[sdefresne, 2015-07-24 12:44:53]

sdefresne@chromium.org changed reviewers:
+ vabr@chromium.org

[sdefresne, 2015-07-24 13:06:40]

sdefresne@chromium.org changed reviewers:
+ jochen@chromium.org

[sdefresne, 2015-07-24 13:06:41]

jochen: can you review this change?

Concentrate your attention on:
- components/
- chrome/chrome_common.gypi
- chrome/common/BUILD.gn
- chrome/common/chrome_version_info*

The rest is either bulk renaming of the enumeration or change to other targets
that used directly the generated file chrome/common/chrome_version_info_values.h
to instead use components/version_info/version_info_values.h.

[sdefresne, 2015-07-25 19:35:51]

sdefresne@chromium.org changed reviewers:
+ dpranke@chromium.org

[sdefresne, 2015-07-25 19:35:52]

dprank: can you take a look at the BUILD.gn files?

[blundell, 2015-07-27 08:34:53]

Jochen,

For context, the motivation for this CL is to enable iOS to move away from using
//chrome's version_info* while still retaining as much of the code and build
system logic in common with other platforms as possible.

Thanks,

Colin

[jochen (gone - plz use gerrit), 2015-07-27 08:57:05]

how did you come up with the list of owners for this?

can you point me at the thread with TPMs and infra acknowledging that this won't
break any of the release scripts and bots?

https://codereview.chromium.org/1257633002/diff/40001/components/version_info...
File components/version_info/OWNERS (right):

https://codereview.chromium.org/1257633002/diff/40001/components/version_info...
components/version_info/OWNERS:5: # Temporary for the duration of the favicon
componentization.
that comment doesn't really make sense

[sdefresne, 2015-07-27 10:08:19]

sdefresne@chromium.org changed reviewers:
+ amineer@chromium.org, mmoss@chromium.org

[sdefresne, 2015-07-27 10:08:38]

On 2015/07/27 at 08:57:05, jochen wrote:
> how did you come up with the list of owners for this?

Ah, good catch. I just copied another component to get the structure and forgot
to update the list of OWNERS. I've updated it with the list of OWNERS taken from
//chrome/OWNERS (as there are no specific OWNERS for
//chrome/common/chrome_version_info*).
 
> can you point me at the thread with TPMs and infra acknowledging that this
won't break any of the release scripts and bots?

The discussion was made outside of a mailing-list, but here it is for the
record:

On Tue, Jul 21, 2015 at 12:09 AM, Michael Moss  wrote:
> I think option 2 is the best bet, with a plan to move to option 3 if
> possible (I have no idea why the VERSION file lives under src/chrome/
> instead of just src/, but I suspect it's just some historical artifact).
> Option 1 is problematic because the release process is already finicky
> enough (if reasonably stable), and I don't think we should be introducing
> more opportunities for failure (the automated commits have generally been
> one of the flakiest parts of the process) if we can avoid it.
>
> Michael
>
>
> On Mon, Jul 20, 2015 at 9:11 AM, Sylvain Defresne 
> wrote:
>>
>> Chrome on iOS is developed in a private repository derived from the
>> Chromium repository (i.e. we did clone Chromium repository and then made
>> changes to Chromium code base). This means that bling (project name for
>> Chrome on iOS) needs to do regular merge from Chromium code base which is
>> painful.
>>
>> The unforking project has a goal to remove all change made by bling team
>> in that private repository. This is mostly done by componentizing the
>> features from src/chrome that are used on iOS (i.e. this abstract
>> dependencies on src/content and untie links to other features in
>> src/chrome). For code that is specific to one embedder (i.e. vanilla
>> Chromium or Chrome on iOS), then we have iOS specific implementation (mostly
>> glue between components and injection of src/content abstraction).
>>
>> Regarding the version number, iOS currently has the same version as
>> desktop and probably wants to keep in sync. However, Chrome on iOS cannot
>> directly uses src/chrome/chrome_version_info.{h,cc}. I'm interested in
>> duplicating the implementation of that feature (as componentizing it would
>> be tricky due to the dependency on src/chrome/installer on Windows).
>>
>> Some of the values used by src/chrome/chrome_version_info.{h,cc} comes
>> from src/chrome/VERSION and I need to use those same values in my iOS
>> implementation. I think I have three choices to deal with this:
>> 1. introduce src/ios/chrome/VERSION and change the script to keep it in
>> sync with src/chrome/VERSION,
>> 2. have the iOS implementation depends on src/chrome/VERSION (a bit ugly
>> as src/ios is forbidden to depends on src/chrome for architectural reason in
>> general),
>> 3. move src/chrome/VERSION to a location were it can be safely shared
>> between src/ios/chrome and src/chrome.
>>
>> My original question was thus whether option 1. was technically feasible
>> and could be approved by TPM. If not, then I guess I'll fallback to option
>> 2. (with a comment explaining why this dependency is allowed, and maybe an
>> open bug to see if we can move to option 3. later).
>>
>> Regards,

Added +mmoss and +amineer as reviewers.

>
https://codereview.chromium.org/1257633002/diff/40001/components/version_info...
> File components/version_info/OWNERS (right):
> 
>
https://codereview.chromium.org/1257633002/diff/40001/components/version_info...
> components/version_info/OWNERS:5: # Temporary for the duration of the favicon
componentization.
> that comment doesn't really make sense

[Dirk Pranke, 2015-07-27 16:06:34]

dpranke@chromium.org changed reviewers:
+ brettw@chromium.org

[Dirk Pranke, 2015-07-27 16:06:35]

The changes look plausible to me, but I'd rather have Brett review them, as he
reworked how we did the version lookup in GN not long ago, and as I'm not an
owner for these files anyway.

[brettw, 2015-07-27 19:42:32]

https://codereview.chromium.org/1257633002/diff/60001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/1257633002/diff/60001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:262: import("//chrome/version.gni")
This is kind of weird here, can you move it to the top of the file like we would
a C include?

https://codereview.chromium.org/1257633002/diff/60001/chrome/common/chrome_ve...
File chrome/common/chrome_version_info.h (right):

https://codereview.chromium.org/1257633002/diff/60001/chrome/common/chrome_ve...
chrome/common/chrome_version_info.h:16: class VersionInfo {
I think this class should be deleted, or maybe renamed ChannelInfo with just the
channel function in it.

It wasn't clear from the CL description if you plan to do this. I agree doing
that rename at the same time of this patch isn't desirable, but I think we
should follow-up.

Can you add a note about what you should use instead with a TODO and a bug link,
and then follow up on that after this lands?

https://codereview.chromium.org/1257633002/diff/60001/components/version_info...
File components/version_info/version_info.h (right):

https://codereview.chromium.org/1257633002/diff/60001/components/version_info...
components/version_info/version_info.h:16: CHANNEL_UNKNOWN = 0,  // Probably
blue
These comments copied from the old file seem pretty weird. Can you just delete
them?

https://codereview.chromium.org/1257633002/diff/60001/components/version_info...
components/version_info/version_info.h:49: // an extra |modifier| which may be
empty.
Can you clarify what "modifier" means in this context and what happens with it?

[sdefresne, 2015-07-28 08:58:27]

brettw: thank you for the review, please take another look

mmoss/amineer: as TPM, can you check whether this would introduce any problem?
I've only moved some source files and a generated file, so I think all release
scripts should be unaffected (in particular, src/chrome/VERSION has not been
moved).

jochen: brettw is an OWNERS of src/, would you like to be pinged for a second
review when brettw approves this CL?

https://codereview.chromium.org/1257633002/diff/60001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/1257633002/diff/60001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:262: import("//chrome/version.gni")
On 2015/07/27 at 19:42:31, brettw wrote:
> This is kind of weird here, can you move it to the top of the file like we
would a C include?

Done.

https://codereview.chromium.org/1257633002/diff/60001/chrome/common/chrome_ve...
File chrome/common/chrome_version_info.h (right):

https://codereview.chromium.org/1257633002/diff/60001/chrome/common/chrome_ve...
chrome/common/chrome_version_info.h:16: class VersionInfo {
On 2015/07/27 at 19:42:31, brettw wrote:
> I think this class should be deleted, or maybe renamed ChannelInfo with just
the channel function in it.
> 
> It wasn't clear from the CL description if you plan to do this. I agree doing
that rename at the same time of this patch isn't desirable, but I think we
should follow-up.
> 
> Can you add a note about what you should use instead with a TODO and a bug
link, and then follow up on that after this lands?

It makes sense. Created
https://code.google.com/p/chromium/issues/detail?id=514562 as the CL is already
large enough.

https://codereview.chromium.org/1257633002/diff/60001/components/version_info...
File components/version_info/version_info.h (right):

https://codereview.chromium.org/1257633002/diff/60001/components/version_info...
components/version_info/version_info.h:16: CHANNEL_UNKNOWN = 0,  // Probably
blue
On 2015/07/27 at 19:42:32, brettw wrote:
> These comments copied from the old file seem pretty weird. Can you just delete
them?

Done.

https://codereview.chromium.org/1257633002/diff/60001/components/version_info...
components/version_info/version_info.h:49: // an extra |modifier| which may be
empty.
On 2015/07/27 at 19:42:32, brettw wrote:
> Can you clarify what "modifier" means in this context and what happens with
it?

Done. Is this better?

[jochen (gone - plz use gerrit), 2015-07-28 09:06:13]

i'll defer to brettw

[amineer_google, 2015-07-28 15:18:50]

On 2015/07/28 08:58:27, sdefresne wrote:
> brettw: thank you for the review, please take another look
> 
> mmoss/amineer: as TPM, can you check whether this would introduce any problem?
> I've only moved some source files and a generated file, so I think all release
> scripts should be unaffected (in particular, src/chrome/VERSION has not been
> moved).
> 
> jochen: brettw is an OWNERS of src/, would you like to be pinged for a second
> review when brettw approves this CL?
> 
> https://codereview.chromium.org/1257633002/diff/60001/chrome/common/BUILD.gn
> File chrome/common/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1257633002/diff/60001/chrome/common/BUILD.gn#...
> chrome/common/BUILD.gn:262: import("//chrome/version.gni")
> On 2015/07/27 at 19:42:31, brettw wrote:
> > This is kind of weird here, can you move it to the top of the file like we
> would a C include?
> 
> Done.
> 
>
https://codereview.chromium.org/1257633002/diff/60001/chrome/common/chrome_ve...
> File chrome/common/chrome_version_info.h (right):
> 
>
https://codereview.chromium.org/1257633002/diff/60001/chrome/common/chrome_ve...
> chrome/common/chrome_version_info.h:16: class VersionInfo {
> On 2015/07/27 at 19:42:31, brettw wrote:
> > I think this class should be deleted, or maybe renamed ChannelInfo with just
> the channel function in it.
> > 
> > It wasn't clear from the CL description if you plan to do this. I agree
doing
> that rename at the same time of this patch isn't desirable, but I think we
> should follow-up.
> > 
> > Can you add a note about what you should use instead with a TODO and a bug
> link, and then follow up on that after this lands?
> 
> It makes sense. Created
> https://code.google.com/p/chromium/issues/detail?id=514562 as the CL is
already
> large enough.
> 
>
https://codereview.chromium.org/1257633002/diff/60001/components/version_info...
> File components/version_info/version_info.h (right):
> 
>
https://codereview.chromium.org/1257633002/diff/60001/components/version_info...
> components/version_info/version_info.h:16: CHANNEL_UNKNOWN = 0,  // Probably
> blue
> On 2015/07/27 at 19:42:32, brettw wrote:
> > These comments copied from the old file seem pretty weird. Can you just
delete
> them?
> 
> Done.
> 
>
https://codereview.chromium.org/1257633002/diff/60001/components/version_info...
> components/version_info/version_info.h:49: // an extra |modifier| which may be
> empty.
> On 2015/07/27 at 19:42:32, brettw wrote:
> > Can you clarify what "modifier" means in this context and what happens with
> it?
> 
> Done. Is this better?

I'm going to defer to mmoss@ as he understands the branching scripts far better
than I.  I'll also add laforge@ in case he has any concerns.

[amineer_google, 2015-07-28 15:19:09]

amineer@google.com changed reviewers:
+ laforge@chromium.org

[brettw, 2015-07-28 17:52:58]

LGTM, please do follow up when this lands with removing the duplicate functions,
I don't want that task to get lost.

[dzhioev (left Google), 2015-07-28 18:33:48]

dzhioev@chromium.org changed reviewers:
+ dzhioev@chromium.org

[dzhioev (left Google), 2015-07-28 18:33:49]

https://codereview.chromium.org/1257633002/diff/80001/components/version_info...
File components/version_info/version_info.h (right):

https://codereview.chromium.org/1257633002/diff/80001/components/version_info...
components/version_info/version_info.h:15: enum Channel {
I suggest to replace this enum with a strongly typed enum (enum class), as long
as we are changing Channel usage everywhere anyway. "CHANNEL_" prefix can be
dropped, so the items will be referenced as follows
"version_info::Channel::STABLE".

[sdefresne, 2015-07-29 11:01:17]

Patchset #7 (id:120001) has been deleted

[sdefresne, 2015-07-29 11:15:28]

Patchset #7 (id:140001) has been deleted

[sdefresne, 2015-07-29 12:15:30]

On 2015/07/28 at 18:33:49, dzhioev wrote:
>
https://codereview.chromium.org/1257633002/diff/80001/components/version_info...
> File components/version_info/version_info.h (right):
> 
>
https://codereview.chromium.org/1257633002/diff/80001/components/version_info...
> components/version_info/version_info.h:15: enum Channel {
> I suggest to replace this enum with a strongly typed enum (enum class), as
long as we are changing Channel usage everywhere anyway. "CHANNEL_" prefix can
be dropped, so the items will be referenced as follows
"version_info::Channel::STABLE".

Good idea, done in patchset #7, though I had to add some static_cast<int> to the
following files as "enum class" do not have implicit conversion to int:
- chrome/browser/apps/app_window_intercept_all_keys_uitest.cc
- chrome/browser/extensions/chrome_notification_observer.cc
- chrome/common/chrome_version_info_android.cc
- chrome/common/extensions/chrome_extensions_client.cc

[sdefresne, 2015-07-29 12:16:04]

laforge/mmoss: can one of you take a look?

[Michael Moss, 2015-07-29 14:19:24]

LGTM

[dzhioev (left Google), 2015-07-29 21:30:28]

On 2015/07/29 12:15:30, sdefresne wrote:
> On 2015/07/28 at 18:33:49, dzhioev wrote:
> >
>
https://codereview.chromium.org/1257633002/diff/80001/components/version_info...
> > File components/version_info/version_info.h (right):
> > 
> >
>
https://codereview.chromium.org/1257633002/diff/80001/components/version_info...
> > components/version_info/version_info.h:15: enum Channel {
> > I suggest to replace this enum with a strongly typed enum (enum class), as
> long as we are changing Channel usage everywhere anyway. "CHANNEL_" prefix can
> be dropped, so the items will be referenced as follows
> "version_info::Channel::STABLE".
> 
> Good idea, done in patchset #7, though I had to add some static_cast<int> to
the
> following files as "enum class" do not have implicit conversion to int:
> - chrome/browser/apps/app_window_intercept_all_keys_uitest.cc
> - chrome/browser/extensions/chrome_notification_observer.cc
> - chrome/common/chrome_version_info_android.cc
> - chrome/common/extensions/chrome_extensions_client.cc

Thanks. LGTM

[sdefresne, 2015-07-30 08:00:04]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-07-30 08:00:05]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1257633002/#ps160001
(title: "Convert version_info::Channel to a "class enum"")

[commit-bot: I haz the power, 2015-07-30 08:00:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1257633002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1257633002/160001

[commit-bot: I haz the power, 2015-07-30 08:06:07]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-07-30 08:06:50]

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