Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(380)

Issue 24994002: Don't pull in content_switches.cc to whole chrome code base (Closed)

Created:
7 years, 2 months ago by Rafał Chłodnicki
Modified:
7 years, 2 months ago
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Don't pull in content_switches.cc to whole chrome code base The point of this change is remove compilation of content_switches.cc that was done in installer_util target. This is a static library that other parts of chrome pull in and that makes the Chrome code that references those switches compile even if the switches itself are not exported. Installer_util doesn't even seem to uses those switches AFAIK. Dropped compilation of these files so these problems are actually exposed and addressed by the person adding the switch. The main offender that referenced those switches was chrome's about_flags.cc. BUG= Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226921

Patch Set 1 #

Total comments: 3

Patch Set 2 : Additional exporting/address compile failures #

Total comments: 3

Patch Set 3 : rebase #

Patch Set 4 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+35 lines, -49 lines) Patch
M chrome/chrome_exe.gypi View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M chrome/chrome_installer_util.gypi View 4 chunks +0 lines, -16 lines 0 comments Download
M chrome/common_constants.gyp View 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/content_switches.h View 1 2 3 12 chunks +32 lines, -32 lines 0 comments Download

Messages

Total messages: 15 (0 generated)
Rafał Chłodnicki
No idea who should I CC on this so please help with adding appropriate reviewers. ...
7 years, 2 months ago (2013-09-27 08:40:00 UTC) #1
jam
lgtm. if this doesn't break any try or build bots, great! regarding exporting all the ...
7 years, 2 months ago (2013-09-27 20:03:58 UTC) #2
Rafał Chłodnicki
On 2013/09/27 20:03:58, jam wrote: > lgtm. if this doesn't break any try or build ...
7 years, 2 months ago (2013-09-28 10:05:39 UTC) #3
Rafał Chłodnicki
https://codereview.chromium.org/24994002/diff/1/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): https://codereview.chromium.org/24994002/diff/1/chrome/chrome_exe.gypi#newcode31 chrome/chrome_exe.gypi:31: '<(DEPTH)/content/public/common/content_switches.h', On 2013/09/27 20:03:58, jam wrote: > nit: drop ...
7 years, 2 months ago (2013-09-28 10:05:47 UTC) #4
jam
https://codereview.chromium.org/24994002/diff/1/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): https://codereview.chromium.org/24994002/diff/1/chrome/chrome_exe.gypi#newcode31 chrome/chrome_exe.gypi:31: '<(DEPTH)/content/public/common/content_switches.h', On 2013/09/28 10:05:47, Rafał Chłodnicki wrote: > On ...
7 years, 2 months ago (2013-10-01 16:13:19 UTC) #5
jam
On 2013/09/28 10:05:39, Rafał Chłodnicki wrote: > On 2013/09/27 20:03:58, jam wrote: > > lgtm. ...
7 years, 2 months ago (2013-10-01 16:13:45 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rchlodnicki@opera.com/24994002/1
7 years, 2 months ago (2013-10-01 16:14:09 UTC) #7
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 2 months ago (2013-10-01 16:54:48 UTC) #8
Rafał Chłodnicki
https://codereview.chromium.org/24994002/diff/32001/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): https://codereview.chromium.org/24994002/diff/32001/chrome/chrome_exe.gypi#newcode585 chrome/chrome_exe.gypi:585: '../content/content.gyp:content_common', Alternatively I could just add content_switches to sources. ...
7 years, 2 months ago (2013-10-01 18:38:17 UTC) #9
jam
https://codereview.chromium.org/24994002/diff/32001/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): https://codereview.chromium.org/24994002/diff/32001/chrome/chrome_exe.gypi#newcode585 chrome/chrome_exe.gypi:585: '../content/content.gyp:content_common', On 2013/10/01 18:38:17, Rafał Chłodnicki wrote: > Alternatively ...
7 years, 2 months ago (2013-10-03 00:34:35 UTC) #10
Rafał Chłodnicki
https://codereview.chromium.org/24994002/diff/32001/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): https://codereview.chromium.org/24994002/diff/32001/chrome/chrome_exe.gypi#newcode585 chrome/chrome_exe.gypi:585: '../content/content.gyp:content_common', On 2013/10/03 00:34:36, jam wrote: > On 2013/10/01 ...
7 years, 2 months ago (2013-10-03 08:33:55 UTC) #11
Rafał Chłodnicki
There is a conflict when rebasing to master so I need to fix that first ...
7 years, 2 months ago (2013-10-03 08:35:52 UTC) #12
Rafał Chłodnicki
On 2013/10/03 08:35:52, Rafał Chłodnicki wrote: > There is a conflict when rebasing to master ...
7 years, 2 months ago (2013-10-03 08:59:00 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rchlodnicki@opera.com/24994002/45001
7 years, 2 months ago (2013-10-03 21:28:46 UTC) #14
commit-bot: I haz the power
7 years, 2 months ago (2013-10-04 01:25:20 UTC) #15
Message was sent while issue was closed.
Change committed as 226921

Powered by Google App Engine
This is Rietveld 408576698