Introduce ui.dll / libui.so for the component build.

Introduce ui.dll / libui.so for the component build.

R=ben@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=92819

[darin (slow to review), 2011-07-07 23:40:01]

NOTE:

1) I had to make a bunch of targets explicitly depend on ui_base in order to
ensure that the UI_BASE_DLL macro gets defined properly.  If we had a global
COMPONENT_BUILD macro, then we could avoid this.  I may do that as a follow-up.

2) ResourceBundle was using _AtlBaseModule.Get{Module,Resource}Instance() to
access resources from chrome.dll.  Now that this code does not always live in
chrome.dll, we need a way for the embedder to specify the module where resources
can be found.  I chose to make the code failover to using the currently running
EXE so as to minimize impact on unit tests.

[darin (slow to review), 2011-07-07 23:40:57]

One more note:  interestingly, there are some functions that did not require a
UI_BASE_API tag.  This means that those functions are unused, at least in the
Windows build.  Maybe this will suggest some code to delete =)

[Ben Goodger (Google), 2011-07-07 23:49:17]

Cool beans! LGTM.

http://codereview.chromium.org/7328011/diff/1/ui/base/dragdrop/drag_source.h
File ui/base/dragdrop/drag_source.h (right):

http://codereview.chromium.org/7328011/diff/1/ui/base/dragdrop/drag_source.h#...
ui/base/dragdrop/drag_source.h:22: public base::RefCountedThreadSafe<DragSource>
{
indentation

[tfarina, 2011-07-08 00:25:54]

http://codereview.chromium.org/7328011/diff/4003/ui/base/ui_base_api.h
File ui/base/ui_base_api.h (right):

http://codereview.chromium.org/7328011/diff/4003/ui/base/ui_base_api.h#newcode19
ui/base/ui_base_api.h:19: #endif  // defined(UI_IMPLEMENTATION)
nit: UI_IMPLEMENTATION -> UI_BASE_IMPLEMENTATION

http://codereview.chromium.org/7328011/diff/4003/ui/base/ui_base_api.h#newcode25
ui/base/ui_base_api.h:25: #else  /// defined(UI_DLL)
nit: /// defined(UI_DLL) -> // defined(UI_BASE_DLL)

[darin (slow to review), 2011-07-08 16:20:28]

Thanks!

On Thu, Jul 7, 2011 at 5:25 PM, <tfarina@chromium.org> wrote:

>
>
http://codereview.chromium.**org/7328011/diff/4003/ui/base/**ui_base_api.h<ht...
> File ui/base/ui_base_api.h (right):
>
> http://codereview.chromium.**org/7328011/diff/4003/ui/base/**
>
ui_base_api.h#newcode19<http://codereview.chromium.org/7328011/diff/4003/ui/base/ui_base_api.h#newcode19>
> ui/base/ui_base_api.h:19: #endif  // defined(UI_IMPLEMENTATION)
> nit: UI_IMPLEMENTATION -> UI_BASE_IMPLEMENTATION
>
> http://codereview.chromium.**org/7328011/diff/4003/ui/base/**
>
ui_base_api.h#newcode25<http://codereview.chromium.org/7328011/diff/4003/ui/base/ui_base_api.h#newcode25>
> ui/base/ui_base_api.h:25: #else  /// defined(UI_DLL)
> nit: /// defined(UI_DLL) -> // defined(UI_BASE_DLL)
>
>
>
http://codereview.chromium.**org/7328011/<http://codereview.chromium.org/7328...
>

[rvargas (doing something else), 2011-07-12 21:40:19]

Looks good!

http://codereview.chromium.org/7328011/diff/8001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/7328011/diff/8001/build/common.gypi#newcode681
build/common.gypi:681: 'defines': ['COMPONENT_BUILD=1'],
The consensus of the code review that defined xx_dll was to drop the "=1" part.

http://codereview.chromium.org/7328011/diff/8001/ui/base/l10n/l10n_font_util.h
File ui/base/l10n/l10n_font_util.h (right):

http://codereview.chromium.org/7328011/diff/8001/ui/base/l10n/l10n_font_util....
ui/base/l10n/l10n_font_util.h:9: #include "ui/ui_api.h"
nit: goes after the next include.

http://codereview.chromium.org/7328011/diff/8001/ui/gfx/image/image.h
File ui/gfx/image/image.h (right):

http://codereview.chromium.org/7328011/diff/8001/ui/gfx/image/image.h#newcode26
ui/gfx/image/image.h:26: #include "ui/gfx/native_widget_types.h"  //
Forward-declares GdkPixbuf and NSImage.
nit: do you mind fixing this comment?

http://codereview.chromium.org/7328011/diff/8001/ui/ui.gyp
File ui/ui.gyp (left):

http://codereview.chromium.org/7328011/diff/8001/ui/ui.gyp#oldcode28
ui/ui.gyp:28: 'ui_base.gypi',
note that Rietveld is not flagging this file with 'D' (although it's empty).

http://codereview.chromium.org/7328011/diff/8001/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/7328011/diff/8001/ui/ui.gyp#newcode32
ui/ui.gyp:32: #  'target_name': 'ui_base',
I guess you want to delete this.

http://codereview.chromium.org/7328011/diff/8001/ui/ui.gyp#newcode277
ui/ui.gyp:277: # TODO(darin): Support component build on all platforms.
Given that COMPONENT_BUILD is global, I'm not sure if there are side effects to
UI_API being active after this CL for other platforms.

It's probably OK, but you may want to make it void for other platforms on
ui_api.h

http://codereview.chromium.org/7328011/diff/8001/webkit/support/webkit_suppor...
File webkit/support/webkit_support.gypi (right):

http://codereview.chromium.org/7328011/diff/8001/webkit/support/webkit_suppor...
webkit/support/webkit_support.gypi:114: 'webkit_support_gfx.cc',
This file is missing

[darin (slow to review), 2011-07-12 22:21:39]

Thanks Ricardo!  I broke off the portions of this that define COMPONENT_BUILD
into a separate CL:  http://codereview.chromium.org/7344022

[darin (slow to review), 2011-07-15 22:36:17]

rvargas: please take another look

[rvargas (doing something else), 2011-07-16 01:37:35]

http://codereview.chromium.org/7328011/diff/47013/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/7328011/diff/47013/build/common.gypi#newcode898
build/common.gypi:898: [ 'OS=="win" and component=="shared_library"', {
Why do we need this for non chromium code?

http://codereview.chromium.org/7328011/diff/47013/webkit/support/webkit_suppo...
File webkit/support/webkit_support_gfx.cc (right):

http://codereview.chromium.org/7328011/diff/47013/webkit/support/webkit_suppo...
webkit/support/webkit_support_gfx.cc:12: // Decode a PNG into an RGBA pixel
array.
The comments are already on the header. (and below)

[darin (slow to review), 2011-07-16 16:41:04]

On Jul 15, 2011 6:37 PM, <rvargas@chromium.org> wrote:
>
>
> http://codereview.chromium.org/7328011/diff/47013/build/common.gypi
> File build/common.gypi (right):
>
>
http://codereview.chromium.org/7328011/diff/47013/build/common.gypi#newcode898
> build/common.gypi:898: [ 'OS=="win" and component=="shared_library"', {
> Why do we need this for non chromium code?

Boo, hiss.... because some third party code is depending on base.  Leveldb
is the latest offender.  I'll add a TODO about cleaning that up.

>
>
http://codereview.chromium.org/7328011/diff/47013/webkit/support/webkit_suppo...
> File webkit/support/webkit_support_gfx.cc (right):
>
>
http://codereview.chromium.org/7328011/diff/47013/webkit/support/webkit_suppo...
> webkit/support/webkit_support_gfx.cc:12: // Decode a PNG into an RGBA
> pixel array.
> The comments are already on the header. (and below)
>

Will remove.  That resulted from copy/pasting the header :-/

>
> http://codereview.chromium.org/7328011/

[rvargas (doing something else), 2011-07-17 07:16:28]

LGTM

[M-A Ruel, 2011-11-23 03:08:15]

http://codereview.chromium.org/7328011/diff/47026/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/7328011/diff/47026/ui/ui.gyp#newcode448
ui/ui.gyp:448: 'python', '-c',
Don't do that, please use gyp's 'copies' action instead.

[rvargas (doing something else), 2011-11-23 03:21:56]

http://codereview.chromium.org/7328011/diff/47026/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/7328011/diff/47026/ui/ui.gyp#newcode448
ui/ui.gyp:448: 'python', '-c',
On 2011/11/23 03:08:16, Marc-Antoine Ruel wrote:
> Don't do that, please use gyp's 'copies' action instead.

see http://codereview.chromium.org/7328011/diff/47026/ui/ui_gfx.gypi

[M-A Ruel, 2011-11-23 03:23:41]

http://codereview.chromium.org/7328011/diff/47026/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/7328011/diff/47026/ui/ui.gyp#newcode448
ui/ui.gyp:448: 'python', '-c',
On 2011/11/23 03:21:56, rvargas wrote:
> On 2011/11/23 03:08:16, Marc-Antoine Ruel wrote:
> > Don't do that, please use gyp's 'copies' action instead.
> 
> see http://codereview.chromium.org/7328011/diff/47026/ui/ui_gfx.gypi

Fine it was code move but it still should be fixed.

[rvargas (doing something else), 2011-11-23 03:26:05]

http://codereview.chromium.org/7328011/diff/47026/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/7328011/diff/47026/ui/ui.gyp#newcode448
ui/ui.gyp:448: 'python', '-c',
On 2011/11/23 03:21:56, rvargas wrote:
> On 2011/11/23 03:08:16, Marc-Antoine Ruel wrote:
> > Don't do that, please use gyp's 'copies' action instead.
> 

And by that, I really meant this one:
http://codereview.chromium.org/6688007/diff/32/ui/ui_gfx.gypi

(I'm just saying that you may want to talk to rsesek)