Componentize component_updater: Move over a bunch of files to the component.

Componentize component_updater: Move over a bunch of files to the component.

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

[tommycli, 2014-08-06 21:04:14]

After https://codereview.chromium.org/421393002/ is done, we have move tons of
files over to the component.

I'm uploading this for reference.

[blundell, 2014-08-07 06:44:07]

woohoo! let us know when this is ready for review.

[tommycli, 2014-08-11 17:33:40]

blundell, sorin, waffles: PTAL. This patch is ready for review.

[blundell, 2014-08-12 12:33:49]

lgtm with comment and question

https://codereview.chromium.org/449643002/diff/120001/components/component_up...
File components/component_updater/DEPS (right):

https://codereview.chromium.org/449643002/diff/120001/components/component_up...
components/component_updater/DEPS:2: "+net",
you should add dependencies corresponding to all of the new deps here

https://codereview.chromium.org/449643002/diff/120001/components/component_up...
File components/component_updater/update_response.cc (left):

https://codereview.chromium.org/449643002/diff/120001/components/component_up...
components/component_updater/update_response.cc:13: #include "libxml/tree.h"
How did this work before?

[tommycli, 2014-08-12 18:38:06]

blundell: Thanks! I addressed your comments below, and I have a Q.

https://codereview.chromium.org/449643002/diff/120001/components/component_up...
File components/component_updater/DEPS (right):

https://codereview.chromium.org/449643002/diff/120001/components/component_up...
components/component_updater/DEPS:2: "+net",
On 2014/08/12 12:33:49, blundell wrote:
> you should add dependencies corresponding to all of the new deps here

Done. Added to component_updater.gypi.

https://codereview.chromium.org/449643002/diff/120001/components/component_up...
File components/component_updater/update_response.cc (left):

https://codereview.chromium.org/449643002/diff/120001/components/component_up...
components/component_updater/update_response.cc:13: #include "libxml/tree.h"
On 2014/08/12 12:33:49, blundell wrote:
> How did this work before?

Done.

https://codereview.chromium.org/449643002/diff/140001/components/component_up...
File components/component_updater.gypi (right):

https://codereview.chromium.org/449643002/diff/140001/components/component_up...
components/component_updater.gypi:18: '../ui/base/ui_base.gyp:ui_base',
I tried adding 'omaha_query_params.gypi:omaha_query_params', here also, but got:

gyp: Duplicate target name "omaha_query_params#target" in directory
"/usr/local/google/home/tommycli/chromium/chromium/src/components" used both in
"omaha_query_params.gypi" and "components.gyp".

[tommycli, 2014-08-12 20:41:28]

waffles/sorin: ping

[blundell, 2014-08-13 07:02:53]

On 2014/08/12 18:38:06, tommycli wrote:
> blundell: Thanks! I addressed your comments below, and I have a Q.
> 
>
https://codereview.chromium.org/449643002/diff/120001/components/component_up...
> File components/component_updater/DEPS (right):
> 
>
https://codereview.chromium.org/449643002/diff/120001/components/component_up...
> components/component_updater/DEPS:2: "+net",
> On 2014/08/12 12:33:49, blundell wrote:
> > you should add dependencies corresponding to all of the new deps here
> 
> Done. Added to component_updater.gypi.
> 
>
https://codereview.chromium.org/449643002/diff/120001/components/component_up...
> File components/component_updater/update_response.cc (left):
> 
>
https://codereview.chromium.org/449643002/diff/120001/components/component_up...
> components/component_updater/update_response.cc:13: #include "libxml/tree.h"
> On 2014/08/12 12:33:49, blundell wrote:
> > How did this work before?
> 
> Done.
> 
>
https://codereview.chromium.org/449643002/diff/140001/components/component_up...
> File components/component_updater.gypi (right):
> 
>
https://codereview.chromium.org/449643002/diff/140001/components/component_up...
> components/component_updater.gypi:18: '../ui/base/ui_base.gyp:ui_base',
> I tried adding 'omaha_query_params.gypi:omaha_query_params', here also, but
got:
> 
> gyp: Duplicate target name "omaha_query_params#target" in directory
> "/usr/local/google/home/tommycli/chromium/chromium/src/components" used both
in
> "omaha_query_params.gypi" and "components.gyp".

It should just be "omaha_query_params", as component_updater and
omaha_query_params end up as targets in the same gypfile (components.gyp).

[tommycli, 2014-08-13 16:34:47]

On 2014/08/13 07:02:53, blundell wrote:
> On 2014/08/12 18:38:06, tommycli wrote:
> > blundell: Thanks! I addressed your comments below, and I have a Q.
> > 
> >
>
https://codereview.chromium.org/449643002/diff/120001/components/component_up...
> > File components/component_updater/DEPS (right):
> > 
> >
>
https://codereview.chromium.org/449643002/diff/120001/components/component_up...
> > components/component_updater/DEPS:2: "+net",
> > On 2014/08/12 12:33:49, blundell wrote:
> > > you should add dependencies corresponding to all of the new deps here
> > 
> > Done. Added to component_updater.gypi.
> > 
> >
>
https://codereview.chromium.org/449643002/diff/120001/components/component_up...
> > File components/component_updater/update_response.cc (left):
> > 
> >
>
https://codereview.chromium.org/449643002/diff/120001/components/component_up...
> > components/component_updater/update_response.cc:13: #include "libxml/tree.h"
> > On 2014/08/12 12:33:49, blundell wrote:
> > > How did this work before?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/449643002/diff/140001/components/component_up...
> > File components/component_updater.gypi (right):
> > 
> >
>
https://codereview.chromium.org/449643002/diff/140001/components/component_up...
> > components/component_updater.gypi:18: '../ui/base/ui_base.gyp:ui_base',
> > I tried adding 'omaha_query_params.gypi:omaha_query_params', here also, but
> got:
> > 
> > gyp: Duplicate target name "omaha_query_params#target" in directory
> > "/usr/local/google/home/tommycli/chromium/chromium/src/components" used both
> in
> > "omaha_query_params.gypi" and "components.gyp".
> 
> It should just be "omaha_query_params", as component_updater and
> omaha_query_params end up as targets in the same gypfile (components.gyp).

blundell: Thanks, corrected.

[tommycli, 2014-08-13 18:42:17]

darin: May I have a stamp on components/component_updater/DEPS ? This is a
componentization patch for component_updater.

Missing LGTM from OWNERS of dependencies added to DEPS:
    '+courgette',
    '+crypto',
    '+extensions/common',
    '+libxml',
    '+net',
    '+third_party/libxml',
    '+third_party/zlib',
    '+ui',

Suggested missing target path OWNERS:
    darin@chromium.org

[tommycli, 2014-08-14 00:55:59]

sorin / waffles: ping

[blundell, 2014-08-14 07:20:06]

https://codereview.chromium.org/449643002/diff/240001/components/component_up...
File components/component_updater/DEPS (right):

https://codereview.chromium.org/449643002/diff/240001/components/component_up...
components/component_updater/DEPS:6: "+extensions/common",
Oops, I didn't notice this the first time around. What is this for? Code shared
by iOS shouldn't reference extensions.

[tommycli, 2014-08-14 23:13:31]

https://codereview.chromium.org/449643002/diff/240001/components/component_up...
File components/component_updater/DEPS (right):

https://codereview.chromium.org/449643002/diff/240001/components/component_up...
components/component_updater/DEPS:6: "+extensions/common",
On 2014/08/14 07:20:06, blundell (OOO until August 25) wrote:
> Oops, I didn't notice this the first time around. What is this for? Code
shared
> by iOS shouldn't reference extensions.

Sounds good. I have a separate patch going:
https://codereview.chromium.org/474633005/ to decouple the extensions
dependency.

[waffles, 2014-08-18 16:39:22]

Do we need a dependency on components/crx_file in the .gypi?

Also, you said that you removed a dependency on extensions common to eliminate a
cycle, why does extensions common have a dependency on the component updater?

[tommycli, 2014-08-18 18:23:39]

On 2014/08/18 16:39:22, waffles wrote:
> Do we need a dependency on components/crx_file in the .gypi?

Hi. Yes. We determined we could not depend on extensions/, so I broke off the
part of extensions/ that we needed, and put it in crx_file.

It's a standalone component, as the crx_file logic is used by apps, extensions,
and components, so it shouldn't belong in extensions.

> 
> Also, you said that you removed a dependency on extensions common to eliminate
a
> cycle, why does extensions common have a dependency on the component updater?

Here were the cycles:
gyp: Cycles in .gyp file dependency graph detected:
Cycle: content/content_shell_and_tests.gyp -> components/components.gyp ->
extensions/extensions.gyp -> content/content_shell_and_tests.gyp
Cycle: extensions/extensions.gyp -> components/components.gyp ->
extensions/extensions.gyp
Cycle: components/components.gyp -> extensions/extensions.gyp ->
content/content_shell_and_tests.gyp -> components/components.gyp
Cycle: components/components.gyp -> extensions/extensions.gyp ->
components/components.gyp
Cycle: extensions/extensions.gyp -> content/content_shell_and_tests.gyp ->
components/components.gyp -> extensions/extensions.gyp

[waffles, 2014-08-18 18:27:32]

On 2014/08/18 18:23:39, tommycli wrote:
> On 2014/08/18 16:39:22, waffles wrote:
> > Do we need a dependency on components/crx_file in the .gypi?
> 
> Hi. Yes. We determined we could not depend on extensions/, so I broke off the
> part of extensions/ that we needed, and put it in crx_file.
> 
> It's a standalone component, as the crx_file logic is used by apps,
extensions,
> and components, so it shouldn't belong in extensions.

Yes, that is why you moved it - now that it is moved, don't you need to update
the .gypi file in this CL?
(Or is this not ready for review? I thought the ball was in our court here.)
 
> > 
> > Also, you said that you removed a dependency on extensions common to
eliminate
> a
> > cycle, why does extensions common have a dependency on the component
updater?
> 
> Here were the cycles:
> gyp: Cycles in .gyp file dependency graph detected:
> Cycle: content/content_shell_and_tests.gyp -> components/components.gyp ->
> extensions/extensions.gyp -> content/content_shell_and_tests.gyp
> Cycle: extensions/extensions.gyp -> components/components.gyp ->
> extensions/extensions.gyp
> Cycle: components/components.gyp -> extensions/extensions.gyp ->
> content/content_shell_and_tests.gyp -> components/components.gyp
> Cycle: components/components.gyp -> extensions/extensions.gyp ->
> components/components.gyp
> Cycle: extensions/extensions.gyp -> content/content_shell_and_tests.gyp ->
> components/components.gyp -> extensions/extensions.gyp

Ok; now that components/components.gyp -!-> extensions/extensions.gyp, are we
OK?

[tommycli, 2014-08-18 18:39:27]

waffles: see below

On 2014/08/18 18:27:32, waffles wrote:
> On 2014/08/18 18:23:39, tommycli wrote:
> > On 2014/08/18 16:39:22, waffles wrote:
> > > Do we need a dependency on components/crx_file in the .gypi?
> > 
> > Hi. Yes. We determined we could not depend on extensions/, so I broke off
the
> > part of extensions/ that we needed, and put it in crx_file.
> > 
> > It's a standalone component, as the crx_file logic is used by apps,
> extensions,
> > and components, so it shouldn't belong in extensions.
> 
> Yes, that is why you moved it - now that it is moved, don't you need to update
> the .gypi file in this CL?
> (Or is this not ready for review? I thought the ball was in our court here.)
>  

Yes, I believe crx_file is already in the component_updater.gypi delta in this
patch.

> > > 
> > > Also, you said that you removed a dependency on extensions common to
> eliminate
> > a
> > > cycle, why does extensions common have a dependency on the component
> updater?
> > 
> > Here were the cycles:
> > gyp: Cycles in .gyp file dependency graph detected:
> > Cycle: content/content_shell_and_tests.gyp -> components/components.gyp ->
> > extensions/extensions.gyp -> content/content_shell_and_tests.gyp
> > Cycle: extensions/extensions.gyp -> components/components.gyp ->
> > extensions/extensions.gyp
> > Cycle: components/components.gyp -> extensions/extensions.gyp ->
> > content/content_shell_and_tests.gyp -> components/components.gyp
> > Cycle: components/components.gyp -> extensions/extensions.gyp ->
> > components/components.gyp
> > Cycle: extensions/extensions.gyp -> content/content_shell_and_tests.gyp ->
> > components/components.gyp -> extensions/extensions.gyp
> 
> Ok; now that components/components.gyp -!-> extensions/extensions.gyp, are we
> OK?

Yes, we should be okay now.

[tommycli, 2014-08-18 18:39:41]

waffles:

https://codereview.chromium.org/449643002/diff/260001/components/component_up...
File components/component_updater.gypi (right):

https://codereview.chromium.org/449643002/diff/260001/components/component_up...
components/component_updater.gypi:18: 'crx_file',
Added crx_file depend here.

[waffles, 2014-08-18 18:40:48]

lgtm

[tommycli, 2014-08-18 18:41:32]

On 2014/08/18 18:40:48, waffles wrote:
> lgtm

waffles: Thanks!

[tommycli, 2014-08-18 18:43:19]

thestig: Do you mind giving a stamp for these files? They should just be include
path changes...

Missing LGTM from an OWNER for these files:
    chrome/browser/DEPS
    chrome/browser/browser_process_impl.cc
    chrome/browser/chrome_browser_main.cc
    chrome/browser/chromeos/login/session/user_session_manager.cc
    chrome/browser/net/crl_set_fetcher.cc
    chrome/browser/net/crl_set_fetcher.h
    chrome/browser/ui/webui/components_ui.cc
    chrome/browser/ui/webui/components_ui.h

[tommycli, 2014-08-18 20:28:04]

darin: ping

May I have a stamp on components/component_updater/DEPS ? This is a
componentization patch for component_updater.

Missing LGTM from OWNERS of dependencies added to DEPS:
    '+courgette',
    '+crypto',
    '+libxml',
    '+net',
    '+third_party/libxml',
    '+third_party/zlib',
    '+ui',

Suggested missing target path OWNERS:
    darin@chromium.org

[Lei Zhang, 2014-08-18 23:35:13]

lgtm

https://codereview.chromium.org/449643002/diff/320001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/449643002/diff/320001/chrome/browser/DEPS#new...
chrome/browser/DEPS:22: "+components/component_updater",
You can remove the redundant entry from c/b/component_updater/DEPS

[darin (slow to review), 2014-08-18 23:48:11]

LGTM for components/component_updater/DEPS

[tommycli, 2014-08-19 00:01:00]

darin, thestig: Thanks!

https://codereview.chromium.org/449643002/diff/320001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/449643002/diff/320001/chrome/browser/DEPS#new...
chrome/browser/DEPS:22: "+components/component_updater",
On 2014/08/18 23:35:12, Lei Zhang wrote:
> You can remove the redundant entry from c/b/component_updater/DEPS

Done.

[tommycli, 2014-08-19 04:02:15]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-08-19 04:02:55]

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

[commit-bot: I haz the power, 2014-08-19 05:26:24]

Committed patchset #18 (360001) as 290499