Build Content as a shared library even when using incremental linking.

Build Content as a shared library even when using incremental linking.

[vangelis, 2012-04-19 02:03:19]

Ready for review.

[Dirk Pranke, 2012-04-19 02:16:23]

lgtm!

[vangelis, 2012-04-19 06:03:49]

On 2012/04/19 02:16:23, Dirk Pranke wrote:
> lgtm!

Added a couple more changes that were necessary to build and link. PTAL.

[Dirk Pranke, 2012-04-20 23:29:12]

So, there's two things in this patch that look fishy (as noted below). I'm doing
a win build now to figure out what's going on before I can LGTM this (or not).

https://chromiumcodereview.appspot.com/10121004/diff/4001/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (left):

https://chromiumcodereview.appspot.com/10121004/diff/4001/chrome/chrome_exe.g...
chrome/chrome_exe.gypi:492: }]
I think this change is bad, it'll cause chrome.exe to depend on content.dll at
link time and that's not allowed ... I'm doing a win build to verify this now
and investigate further.

https://chromiumcodereview.appspot.com/10121004/diff/4001/content/public/comm...
File content/public/common/content_switches.h (right):

https://chromiumcodereview.appspot.com/10121004/diff/4001/content/public/comm...
content/public/common/content_switches.h:18: CONTENT_EXPORT extern const char
kAllowWebUICompositing[];
You shouldn't have had to export these symbols as a part of this patch ... this
tells me that we're linking content-switches.cc into chrome.dll directly, and
IIRC we're not supposed to be (it looks like it's getting included via
installer_util, which contains a copy and is now getting linked into
chrome.dll). So I need to dig into how and why that happened and whether it's
okay

(I also don't yet know why incremental linking disabled this such that now your
link failed ...)

[Dirk Pranke, 2012-04-20 23:56:46]

Also, have you actually run this patch through the try servers? You should run
both win_rel and win to make sure we don't break the static build or the
non-incremental shared build.

[Dirk Pranke, 2012-04-21 00:05:01]

https://chromiumcodereview.appspot.com/10121004/diff/4001/content/public/comm...
File content/public/common/content_switches.h (right):

https://chromiumcodereview.appspot.com/10121004/diff/4001/content/public/comm...
content/public/common/content_switches.h:18: CONTENT_EXPORT extern const char
kAllowWebUICompositing[];
On 2012/04/20 23:29:13, Dirk Pranke wrote:
> You shouldn't have had to export these symbols as a part of this patch ...
this
> tells me that we're linking content-switches.cc into chrome.dll directly, and
> IIRC we're not supposed to be (it looks like it's getting included via
> installer_util, which contains a copy and is now getting linked into
> chrome.dll). So I need to dig into how and why that happened and whether it's
> okay
> 
> (I also don't yet know why incremental linking disabled this such that now
your
> link failed ...)

Following up on my own comment, it looks like I'm responsible for this and it's
been broken since the initial content-as-component effort :) I'm not sure how I
didn't notice this before, and haven't yet decided on the best way to fix it,
but it seems like we shouldn't hold up fixing incremental linking as a part of
this. I'm continuing to dig, though ...

[Dirk Pranke, 2012-04-21 01:22:45]

Okay, I think we didn't need either the changes in content_switches.cc or
chrome_exe.gypi. I've uploaded a patch to
https://chromiumcodereview.appspot.com/10166021/ that seems to work for me
locally and I'm running it through the try servers now.