Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(4)

Issue 16881011: Remove WEBKIT_DLL and replace it with COMPONENT_BUILD (Closed)

Created:
7 years, 1 month ago by jochen (gone - plz use gerrit)
Modified:
7 years, 1 month ago
CC:
blink-reviews, eae+blinkwatch
Visibility:
Public.

Description

Remove WEBKIT_DLL and replace it with COMPONENT_BUILD COMPONENT_BUILD is already defined, so we don't need to define WEBKIT_DLL all over the place R=abarth@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152661

Patch Set 1 #

Total comments: 2

Patch Set 2 : updates #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -20 lines) Patch
M Source/WebKit/chromium/WebKit.gyp View 1 2 chunks +0 lines, -8 lines 0 comments Download
M Source/WebKit/chromium/WebKitUnitTests.gyp View 1 1 chunk +1 line, -5 lines 3 comments Download
M Source/core/core.gyp View 1 1 chunk +0 lines, -5 lines 0 comments Download
M public/platform/WebCommon.h View 1 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 16 (0 generated)
jochen (gone - plz use gerrit)
7 years, 1 month ago (2013-06-17 12:32:02 UTC) #1
abarth-chromium
https://codereview.chromium.org/16881011/diff/1/Source/modules/modules.gyp File Source/modules/modules.gyp (right): https://codereview.chromium.org/16881011/diff/1/Source/modules/modules.gyp#newcode53 Source/modules/modules.gyp:53: 'WEBKIT_DLL', This sort of thing should be in config.gyp.
7 years, 1 month ago (2013-06-17 17:33:02 UTC) #2
jochen (gone - plz use gerrit)
On 2013/06/17 17:33:02, abarth wrote: > https://codereview.chromium.org/16881011/diff/1/Source/modules/modules.gyp > File Source/modules/modules.gyp (right): > > https://codereview.chromium.org/16881011/diff/1/Source/modules/modules.gyp#newcode53 > ...
7 years, 1 month ago (2013-06-17 20:11:44 UTC) #3
jamesr
Why do we use WEBKIT_DLL instead of COMPONENT_BUILD which is defined by build/common.gypi ?
7 years, 1 month ago (2013-06-17 20:23:49 UTC) #4
jochen (gone - plz use gerrit)
On 2013/06/17 20:23:49, jamesr wrote: > Why do we use WEBKIT_DLL instead of COMPONENT_BUILD which ...
7 years, 1 month ago (2013-06-17 20:28:05 UTC) #5
abarth-chromium
https://codereview.chromium.org/16881011/diff/1/Source/modules/modules.gyp File Source/modules/modules.gyp (right): https://codereview.chromium.org/16881011/diff/1/Source/modules/modules.gyp#newcode44 Source/modules/modules.gyp:44: 'WEBKIT_IMPLEMENTATION=1', I thought WEBKIT_IMPLEMENTATION controlled whether we import or ...
7 years, 1 month ago (2013-06-17 20:30:04 UTC) #6
jamesr
On 2013/06/17 20:28:05, jochen wrote: > On 2013/06/17 20:23:49, jamesr wrote: > > Why do ...
7 years, 1 month ago (2013-06-17 20:30:51 UTC) #7
jochen (gone - plz use gerrit)
On Mon, Jun 17, 2013 at 10:30 PM, <jamesr@chromium.org> wrote: > On 2013/06/17 20:28:05, jochen ...
7 years, 1 month ago (2013-06-17 20:35:27 UTC) #8
jochen (gone - plz use gerrit)
I've updated the CL to replace WEBKIT_DLL with COMPONENT_BUILD thanks again for the catch, James ...
7 years, 1 month ago (2013-06-18 08:35:11 UTC) #9
abarth-chromium
This looks great to me, but jamesr is the expert here.
7 years, 1 month ago (2013-06-18 09:18:36 UTC) #10
jamesr
lgtm. cool
7 years, 1 month ago (2013-06-18 19:46:23 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jochen@chromium.org/16881011/12001
7 years, 1 month ago (2013-06-18 19:47:50 UTC) #12
commit-bot: I haz the power
Change committed as 152661
7 years, 1 month ago (2013-06-18 20:18:31 UTC) #13
darin (slow to review)
https://chromiumcodereview.appspot.com/16881011/diff/12001/Source/WebKit/chromium/WebKitUnitTests.gyp File Source/WebKit/chromium/WebKitUnitTests.gyp (left): https://chromiumcodereview.appspot.com/16881011/diff/12001/Source/WebKit/chromium/WebKitUnitTests.gyp#oldcode68 Source/WebKit/chromium/WebKitUnitTests.gyp:68: 'WEBKIT_DLL_UNITTEST', I think this change nuked webkit_unit_tests in the ...
7 years, 1 month ago (2013-06-19 21:05:46 UTC) #14
jamesr
https://chromiumcodereview.appspot.com/16881011/diff/12001/Source/WebKit/chromium/WebKitUnitTests.gyp File Source/WebKit/chromium/WebKitUnitTests.gyp (left): https://chromiumcodereview.appspot.com/16881011/diff/12001/Source/WebKit/chromium/WebKitUnitTests.gyp#oldcode68 Source/WebKit/chromium/WebKitUnitTests.gyp:68: 'WEBKIT_DLL_UNITTEST', On 2013/06/19 21:05:47, darin wrote: > I think ...
7 years, 1 month ago (2013-06-19 21:06:52 UTC) #15
jochen (gone - plz use gerrit)
7 years, 1 month ago (2013-06-20 10:49:12 UTC) #16
Message was sent while issue was closed.
https://codereview.chromium.org/16881011/diff/12001/Source/WebKit/chromium/We...
File Source/WebKit/chromium/WebKitUnitTests.gyp (left):

https://codereview.chromium.org/16881011/diff/12001/Source/WebKit/chromium/We...
Source/WebKit/chromium/WebKitUnitTests.gyp:68: 'WEBKIT_DLL_UNITTEST',
On 2013/06/19 21:06:52, jamesr wrote:
> On 2013/06/19 21:05:47, darin wrote:
> > I think this change nuked webkit_unit_tests in the component=shared_library
> > build.  Now, webkit_unit_tests has no tests!  The WEBKIT_DLL_UNITTEST define
> was
> > needed so that RunAllTests.cpp would know to find the tests in webkit.dll
> > instead of inside webkit_unit_tests.exe.
> 
> Yikes, that's right!  Why did you remove this, Jochen?  It doesn't seem
related
> to the rest of the patch.

OMG, that was a careless mistake :-/

Sorry about that and thanks for fixing

Powered by Google App Engine
This is Rietveld 408576698