Create separate extensions_common and extensions_browser GYP targets.

Create separate extensions_common and extensions_browser GYP targets.

BUG=162530
R=benwells@chromium.org

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

[benwells, 2013-09-11 04:33:27]

lgtm

[commit-bot: I haz the power, 2013-09-11 13:17:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/23513035/2001

[commit-bot: I haz the power, 2013-09-11 13:17:32]

Failed to apply patch for chrome/chrome_browser_extensions.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/chrome_browser_extensions.gypi
  Hunk #2 FAILED at 54.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/chrome_browser_extensions.gypi.rej

Patch:       chrome/chrome_browser_extensions.gypi
Index: chrome/chrome_browser_extensions.gypi
diff --git a/chrome/chrome_browser_extensions.gypi
b/chrome/chrome_browser_extensions.gypi
index
e1a7035c5be8ac38fa2e30a16511dbcf1eb83853..714d41f52681d86193030776eb9343713442437a
100644
--- a/chrome/chrome_browser_extensions.gypi
+++ b/chrome/chrome_browser_extensions.gypi
@@ -28,6 +28,7 @@
         '../content/content.gyp:content_browser',
         '../crypto/crypto.gyp:crypto',
         '../device/bluetooth/bluetooth.gyp:device_bluetooth',
+        '../extensions/extensions.gyp:extensions_browser',
         '../net/net.gyp:net',
         '../skia/skia.gyp:skia',
         '../sync/sync.gyp:sync',
@@ -53,15 +54,6 @@
       'sources': [
         # All .cc, .h, .m, and .mm files under browser/extensions except for
         # tests and mocks.
-        '../extensions/browser/extension_prefs_scope.h',
-        '../extensions/browser/extension_error.cc',
-        '../extensions/browser/extension_error.h',
-        '../extensions/browser/file_reader.cc',
-        '../extensions/browser/file_reader.h',
-        '../extensions/browser/pref_names.cc',
-        '../extensions/browser/pref_names.h',
-        '../extensions/browser/view_type_utils.cc',
-        '../extensions/browser/view_type_utils.h',
         'browser/apps/shortcut_manager.cc',
         'browser/apps/shortcut_manager.h',
         'browser/apps/shortcut_manager_factory.cc',

[commit-bot: I haz the power, 2013-09-24 00:15:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/23513035/8001

[commit-bot: I haz the power, 2013-09-24 01:52:28]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-24 21:36:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/23513035/35001

[commit-bot: I haz the power, 2013-09-24 23:28:40]

Retried try job too often on linux_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-09-24 23:33:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/23513035/35001

[commit-bot: I haz the power, 2013-09-25 00:00:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-25 00:12:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/23513035/58001

[commit-bot: I haz the power, 2013-09-25 05:32:43]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-09-25 19:06:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/23513035/58001

[commit-bot: I haz the power, 2013-09-25 21:25:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/23513035/58001

[Yoyo Zhou, 2013-09-25 21:45:59]

Committed patchset #5 manually as r225260 (presubmit successful).

[Nico, 2013-10-23 18:26:58]

https://codereview.chromium.org/23513035/diff/58001/extensions/extensions.gyp
File extensions/extensions.gyp (right):

https://codereview.chromium.org/23513035/diff/58001/extensions/extensions.gyp...
extensions/extensions.gyp:5: {
This is missing chromium_source: 1. Adding this in
https://codereview.chromium.org/37203005

https://codereview.chromium.org/23513035/diff/58001/extensions/extensions.gyp...
extensions/extensions.gyp:9: 'type': 'static_library',
Is this intentionally not a <(component)?

[Yoyo Zhou, 2013-10-24 01:23:42]

https://codereview.chromium.org/23513035/diff/58001/extensions/extensions.gyp
File extensions/extensions.gyp (right):

https://codereview.chromium.org/23513035/diff/58001/extensions/extensions.gyp...
extensions/extensions.gyp:9: 'type': 'static_library',
On 2013/10/23 18:26:59, Nico wrote:
> Is this intentionally not a <(component)?

I think something didn't compile correctly when it was a <(component) - I'll
look again tomorrow.

[benwells, 2013-10-24 03:57:51]

On 2013/10/24 01:23:42, Yoyo Zhou wrote:
> https://codereview.chromium.org/23513035/diff/58001/extensions/extensions.gyp
> File extensions/extensions.gyp (right):
> 
>
https://codereview.chromium.org/23513035/diff/58001/extensions/extensions.gyp...
> extensions/extensions.gyp:9: 'type': 'static_library',
> On 2013/10/23 18:26:59, Nico wrote:
> > Is this intentionally not a <(component)?
> 
> I think something didn't compile correctly when it was a <(component) - I'll
> look again tomorrow.

It might be simpler to leave it as is. This way we can introduce temporary
dependencies back to chrome while moving stuff down to extensions. E.g.
c/c/extensions/permission_set could move now, making c/c/extensions/extension
easier to move. Otherwise we will have to move large independent chunks in one
go.

[Nico, 2013-10-24 04:00:07]

I thought part of the point of components/ was that they have sane
dependencies (at all times, no temporary deps on chrome).


On Wed, Oct 23, 2013 at 8:57 PM, <benwells@chromium.org> wrote:

> On 2013/10/24 01:23:42, Yoyo Zhou wrote:
>
>> https://codereview.chromium.**org/23513035/diff/58001/**
>>
extensions/extensions.gyp<https://codereview.chromium.org/23513035/diff/58001/extensions/extensions.gyp>
>> File extensions/extensions.gyp (right):
>>
>
>
> https://codereview.chromium.**org/23513035/diff/58001/**
>
extensions/extensions.gyp#**newcode9<https://codereview.chromium.org/23513035/diff/58001/extensions/extensions.gyp#newcode9>
>
>> extensions/extensions.gyp:9: 'type': 'static_library',
>> On 2013/10/23 18:26:59, Nico wrote:
>> > Is this intentionally not a <(component)?
>>
>
>  I think something didn't compile correctly when it was a <(component) -
>> I'll
>> look again tomorrow.
>>
>
> It might be simpler to leave it as is. This way we can introduce temporary
> dependencies back to chrome while moving stuff down to extensions. E.g.
> c/c/extensions/permission_set could move now, making
> c/c/extensions/extension
> easier to move. Otherwise we will have to move large independent chunks in
> one
> go.
>
>
https://codereview.chromium.**org/23513035/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[benwells1, 2013-10-24 04:08:49]

Yes, that is the end goal. At the moment we have a component which has sane
dependencies, but which isn't useful as a component. E.g. the extensions
component doesn't contain the Extension class.

There is a case that allowing temporary dependencies back to chrome will
make it easier to move stuff down in more manageable chunks. i.e. large
groups of files with circular dependencies can be moved down one at a time
instead of all in one massive CL.

This also means we can get stuff down sooner, which prevents new
dependencies being added. This approach was recommended for the apps/
"component" and has worked well, but making that it into a true component
is now largely dependent on the extensions/ component.

I just had a look at moving Extension down, and it should be much simpler
to do it if we can introduce a back dependency to
chrome/common/extensions/extension.h, move things like
chrome/common/extensions/permissions/permission_set down, and then as a
last step move down chrome/common/extensions/extension.h.

Once we have a functional component (which is definable by the app_shell
project, which needs both extensions and apps as true components) we would
change these from shared libraries to components.


On Thu, Oct 24, 2013 at 3:00 PM, Nico Weber <thakis@chromium.org> wrote:

> I thought part of the point of components/ was that they have sane
> dependencies (at all times, no temporary deps on chrome).
>
>
> On Wed, Oct 23, 2013 at 8:57 PM, <benwells@chromium.org> wrote:
>
>> On 2013/10/24 01:23:42, Yoyo Zhou wrote:
>>
>>> https://codereview.chromium.**org/23513035/diff/58001/**
>>>
extensions/extensions.gyp<https://codereview.chromium.org/23513035/diff/58001/extensions/extensions.gyp>
>>> File extensions/extensions.gyp (right):
>>>
>>
>>
>> https://codereview.chromium.**org/23513035/diff/58001/**
>>
extensions/extensions.gyp#**newcode9<https://codereview.chromium.org/23513035/diff/58001/extensions/extensions.gyp#newcode9>
>>
>>> extensions/extensions.gyp:9: 'type': 'static_library',
>>> On 2013/10/23 18:26:59, Nico wrote:
>>> > Is this intentionally not a <(component)?
>>>
>>
>>  I think something didn't compile correctly when it was a <(component) -
>>> I'll
>>> look again tomorrow.
>>>
>>
>> It might be simpler to leave it as is. This way we can introduce temporary
>> dependencies back to chrome while moving stuff down to extensions. E.g.
>> c/c/extensions/permission_set could move now, making
>> c/c/extensions/extension
>> easier to move. Otherwise we will have to move large independent chunks
>> in one
>> go.
>>
>>
https://codereview.chromium.**org/23513035/<https://codereview.chromium.org/2...
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2013-10-24 04:16:50]

Ok. I'll check back how things look in 6 months :-)


On Wed, Oct 23, 2013 at 9:08 PM, Ben Wells <benwells@google.com> wrote:

> Yes, that is the end goal. At the moment we have a component which has
> sane dependencies, but which isn't useful as a component. E.g. the
> extensions component doesn't contain the Extension class.
>
> There is a case that allowing temporary dependencies back to chrome will
> make it easier to move stuff down in more manageable chunks. i.e. large
> groups of files with circular dependencies can be moved down one at a time
> instead of all in one massive CL.
>
> This also means we can get stuff down sooner, which prevents new
> dependencies being added. This approach was recommended for the apps/
> "component" and has worked well, but making that it into a true component
> is now largely dependent on the extensions/ component.
>
> I just had a look at moving Extension down, and it should be much simpler
> to do it if we can introduce a back dependency to
> chrome/common/extensions/extension.h, move things like
> chrome/common/extensions/permissions/permission_set down, and then as a
> last step move down chrome/common/extensions/extension.h.
>
> Once we have a functional component (which is definable by the app_shell
> project, which needs both extensions and apps as true components) we would
> change these from shared libraries to components.
>
>
> On Thu, Oct 24, 2013 at 3:00 PM, Nico Weber <thakis@chromium.org> wrote:
>
>> I thought part of the point of components/ was that they have sane
>> dependencies (at all times, no temporary deps on chrome).
>>
>>
>> On Wed, Oct 23, 2013 at 8:57 PM, <benwells@chromium.org> wrote:
>>
>>> On 2013/10/24 01:23:42, Yoyo Zhou wrote:
>>>
>>>> https://codereview.chromium.**org/23513035/diff/58001/**
>>>>
extensions/extensions.gyp<https://codereview.chromium.org/23513035/diff/58001/extensions/extensions.gyp>
>>>> File extensions/extensions.gyp (right):
>>>>
>>>
>>>
>>> https://codereview.chromium.**org/23513035/diff/58001/**
>>>
extensions/extensions.gyp#**newcode9<https://codereview.chromium.org/23513035/diff/58001/extensions/extensions.gyp#newcode9>
>>>
>>>> extensions/extensions.gyp:9: 'type': 'static_library',
>>>> On 2013/10/23 18:26:59, Nico wrote:
>>>> > Is this intentionally not a <(component)?
>>>>
>>>
>>>  I think something didn't compile correctly when it was a <(component) -
>>>> I'll
>>>> look again tomorrow.
>>>>
>>>
>>> It might be simpler to leave it as is. This way we can introduce
>>> temporary
>>> dependencies back to chrome while moving stuff down to extensions. E.g.
>>> c/c/extensions/permission_set could move now, making
>>> c/c/extensions/extension
>>> easier to move. Otherwise we will have to move large independent chunks
>>> in one
>>> go.
>>>
>>>
https://codereview.chromium.**org/23513035/<https://codereview.chromium.org/2...
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.