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

Issue 2623143005: Only depend on child sources from app in both target (Closed)

Created:
3 years, 11 months ago by scottmg
Modified:
3 years, 11 months ago
Reviewers:
Dirk Pranke, brettw
CC:
chromium-reviews, jam, darin-cc_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Only depend on child sources from app in both target Added here: https://codereview.chromium.org/1817903002. This makes chrome.dll depend on all of WebKit at build time. This changes restores c:\src\cr\src>gn path //out/Release //chrome:main_dll //third_party/WebKit/public:blink No non-data paths found between these two targets. Before this change, public_app_shared_sources was included into multi-dll browser, causing all the child deps to be linked in there. (crbug.com/680772 is about why assert_no_deps didn't catch this.) R=brettw@chromium.org BUG=581766, 680772 Review-Url: https://codereview.chromium.org/2623143005 Cr-Commit-Position: refs/heads/master@{#443636} Committed: https://chromium.googlesource.com/chromium/src/+/6b2011f69d69405750fef23c7fdc374735fbe160

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : attempt for non-win #

Total comments: 1

Patch Set 4 : . #

Patch Set 5 : . #

Patch Set 6 : . #

Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -3 lines) Patch
M content/public/app/BUILD.gn View 1 2 3 4 5 3 chunks +6 lines, -3 lines 0 comments Download

Messages

Total messages: 48 (38 generated)
scottmg
3 years, 11 months ago (2017-01-12 00:31:53 UTC) #3
brettw
lgtm https://codereview.chromium.org/2623143005/diff/40001/content/public/app/BUILD.gn File content/public/app/BUILD.gn (right): https://codereview.chromium.org/2623143005/diff/40001/content/public/app/BUILD.gn#newcode44 content/public/app/BUILD.gn:44: public_app_shared_deps += [ Can you add a comment ...
3 years, 11 months ago (2017-01-12 22:51:37 UTC) #19
scottmg
On 2017/01/12 22:51:37, brettw (ping after 24h) wrote: > lgtm > > https://codereview.chromium.org/2623143005/diff/40001/content/public/app/BUILD.gn > File ...
3 years, 11 months ago (2017-01-12 22:59:07 UTC) #20
scottmg
How do you feel about this approach? Does seem to fix the problem, but I ...
3 years, 11 months ago (2017-01-13 02:52:16 UTC) #32
scottmg
Dirk, I believe Brett is out, would you be able to review this one?
3 years, 11 months ago (2017-01-13 19:19:01 UTC) #38
Dirk Pranke
lgtm, but I'm not an owner. It's probably fine to tbr brett or jam, though.
3 years, 11 months ago (2017-01-13 19:27:55 UTC) #39
scottmg
On 2017/01/13 19:27:55, Dirk Pranke wrote: > lgtm, but I'm not an owner. It's probably ...
3 years, 11 months ago (2017-01-13 19:28:39 UTC) #40
scottmg
On 2017/01/13 19:27:55, Dirk Pranke wrote: > lgtm, but I'm not an owner. It's probably ...
3 years, 11 months ago (2017-01-13 19:28:53 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2623143005/110001
3 years, 11 months ago (2017-01-13 19:29:56 UTC) #45
commit-bot: I haz the power
3 years, 11 months ago (2017-01-13 19:37:23 UTC) #48
Message was sent while issue was closed.
Committed patchset #6 (id:110001) as
https://chromium.googlesource.com/chromium/src/+/6b2011f69d69405750fef23c7fdc...

Powered by Google App Engine
This is Rietveld 408576698