Componentize component_updater: Split crx_file code off into its own component.

Componentize component_updater: Split crx_file code off into its own component.

CRX file code currently lives under extensions/. However, 
component_updater also uses them to package components. The 
CRX code should therefore live as its own component.

BUG=371463
TBR=blundell@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289966

[tommycli, 2014-08-14 21:51:55]

Hey guys,

How does this look for splitting off the CrxFile definitions from extensions/?

This is necessary because component updater uses CrxFile also, and shouldn't
depend on extensions/

https://codereview.chromium.org/474633005/diff/20001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_utils.cc (left):

https://codereview.chromium.org/474633005/diff/20001/chrome/browser/component...
chrome/browser/component_updater/component_updater_utils.cc:184:
DCHECK(extensions::Extension::IdIsValid(id));
Is it okay to remove this?

It's not clear how the component updater utils could conceptually depend on
something out of extensions, unless we split this IdIsValid function into
components/crx_file...

[waffles, 2014-08-14 22:26:29]

lgtm

https://codereview.chromium.org/474633005/diff/20001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_utils.cc (left):

https://codereview.chromium.org/474633005/diff/20001/chrome/browser/component...
chrome/browser/component_updater/component_updater_utils.cc:184:
DCHECK(extensions::Extension::IdIsValid(id));
On 2014/08/14 21:51:55, tommycli wrote:
> Is it okay to remove this?
> 
> It's not clear how the component updater utils could conceptually depend on
> something out of extensions, unless we split this IdIsValid function into
> components/crx_file...

I am OK with either removing this or moving it, whichever asargent@ prefers.

I think it makes sense to move it - we're using "extension IDs" in general to
identify components as well.

[tommycli, 2014-08-14 22:50:48]

waffles: Cool thanks

asargent: Ready for your review. Thanks!

[Yoyo Zhou, 2014-08-14 22:51:16]

LGTM

https://codereview.chromium.org/474633005/diff/20001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_utils.cc (left):

https://codereview.chromium.org/474633005/diff/20001/chrome/browser/component...
chrome/browser/component_updater/component_updater_utils.cc:184:
DCHECK(extensions::Extension::IdIsValid(id));
On 2014/08/14 22:26:29, waffles wrote:
> On 2014/08/14 21:51:55, tommycli wrote:
> > Is it okay to remove this?
> > 
> > It's not clear how the component updater utils could conceptually depend on
> > something out of extensions, unless we split this IdIsValid function into
> > components/crx_file...
> 
> I am OK with either removing this or moving it, whichever asargent@ prefers.
> 
> I think it makes sense to move it - we're using "extension IDs" in general to
> identify components as well.

IdIsValid doesn't belong on Extension; I would've put it in
extensions/common/id_util, but that is in extensions. I'm not sure if the rest
of the functions there belong in crx_file as well.

[tommycli, 2014-08-14 23:09:03]

yoz: Thanks for jumping in. I responded below.

https://codereview.chromium.org/474633005/diff/20001/chrome/browser/component...
File chrome/browser/component_updater/component_updater_utils.cc (left):

https://codereview.chromium.org/474633005/diff/20001/chrome/browser/component...
chrome/browser/component_updater/component_updater_utils.cc:184:
DCHECK(extensions::Extension::IdIsValid(id));
On 2014/08/14 22:51:16, Yoyo Zhou wrote:
> On 2014/08/14 22:26:29, waffles wrote:
> > On 2014/08/14 21:51:55, tommycli wrote:
> > > Is it okay to remove this?
> > > 
> > > It's not clear how the component updater utils could conceptually depend
on
> > > something out of extensions, unless we split this IdIsValid function into
> > > components/crx_file...
> > 
> > I am OK with either removing this or moving it, whichever asargent@ prefers.
> > 
> > I think it makes sense to move it - we're using "extension IDs" in general
to
> > identify components as well.
> 
> IdIsValid doesn't belong on Extension; I would've put it in
> extensions/common/id_util, but that is in extensions. I'm not sure if the rest
> of the functions there belong in crx_file as well.

Okay, I agree. Since these IDs are for extensions, apps, and components, they
can all live in crx_file component.

I'd like to move over all the id_util functions in a separate patch, for ease of
reviewing. I've added a TODO to restore the DCHECK once it's moved over. Sound
reasonable?

[Sorin Jianu, 2014-08-15 00:43:51]

Thank you Tommy! As discussed, consider getting feedback from the owners of
src/components when to make source modules build as components vs static
libraries. If acceptable, consider making this module a static library.

[tommycli, 2014-08-15 14:41:42]

sorin: As discussed, I removed the shared library export, since it's such a tiny
piece of code, it can be a static_library. I'll discuss with blundell / jochen
when I get a chance.

blundell / jochen: PTAL. I think you guys are both OOO based on your status, so
I'll send to CQ with a TBR after a while...

[tommycli, 2014-08-15 18:25:51]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-08-15 18:27:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/474633005/60001

[commit-bot: I haz the power, 2014-08-15 19:07:46]

Committed patchset #4 (60001) as 289966

[blundell, 2014-08-25 08:41:20]

It's optional for components to be built as shared libraries.