Add new ResourceBundle::Delegate interface.

Add new ResourceBundle::Delegate interface.

BUG=125351
TEST=ResourceBundle.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=136039

[Marshall, 2012-04-30 20:36:56]

@sail: Please review. If you agree with the API changes I'll proceed with
updating the call sites.

@ben, @tony: FYI as OWNERS (see the bug report for details).

Thanks.

[Marshall, 2012-04-30 20:38:09]

On 2012/04/30 20:36:56, Marshall wrote:
> @sail: Please review. If you agree with the API changes I'll proceed with
> updating the call sites.
> 
> @ben, @tony: FYI as OWNERS (see the bug report for details).
  ^^ @sky
> 
> Thanks.

[tfarina, 2012-04-30 20:42:33]

http://codereview.chromium.org/10270023/diff/1/ui/base/resource/resource_bund...
File ui/base/resource/resource_bundle.h (right):

http://codereview.chromium.org/10270023/diff/1/ui/base/resource/resource_bund...
ui/base/resource/resource_bundle.h:66: class Delegate {
nit: please, move this to a separate class.

ui/base/resource/resource_bundle_delegate.h

Thanks.

http://codereview.chromium.org/10270023/diff/1/ui/base/resource/resource_bund...
ui/base/resource/resource_bundle.h:68: virtual ~Delegate() {}
nit: could you make this virtual destructor protected instead?

[Marshall, 2012-04-30 20:55:43]

http://codereview.chromium.org/10270023/diff/1/ui/base/resource/resource_bund...
File ui/base/resource/resource_bundle.h (right):

http://codereview.chromium.org/10270023/diff/1/ui/base/resource/resource_bund...
ui/base/resource/resource_bundle.h:66: class Delegate {
On 2012/04/30 20:42:33, tfarina wrote:
> nit: please, move this to a separate class.
> 
> ui/base/resource/resource_bundle_delegate.h
> 
> Thanks.

What advantage do you see in having a separate resource_bundle_delegate.h? Where
would the enums (FontStyle and ImageRTL) needed by both ResourceBundle and
ResourceBundleDelegate live in that case?

http://codereview.chromium.org/10270023/diff/1/ui/base/resource/resource_bund...
ui/base/resource/resource_bundle.h:68: virtual ~Delegate() {}
On 2012/04/30 20:42:33, tfarina wrote:
> nit: could you make this virtual destructor protected instead?

Done.

[sail, 2012-04-30 21:14:43]

http://codereview.chromium.org/10270023/diff/2003/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle.h (right):

http://codereview.chromium.org/10270023/diff/2003/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.h:84: virtual gfx::Image* GetImageNamed(int
resource_id) = 0;
can you have this return a scoped_ptr. Then you can get rid of the comment about
ownership. Same below.

http://codereview.chromium.org/10270023/diff/2003/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.h:211: void AddCommonDataPack(const
std::string& pack_name, const FilePath& path);
do you really need AddDataPack and AddCommonDataPack? Can we just have one?

If both are necessary then can you add a comment in both places about when one
should be used and when the other should be used.

http://codereview.chromium.org/10270023/diff/2003/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle_gtk.cc (right):

http://codereview.chromium.org/10270023/diff/2003/ui/base/resource/resource_b...
ui/base/resource/resource_bundle_gtk.cc:88: delete image;
instead of doing a delete here, can you use a scoped_ptr, and do images_[key] =
image.release() below.

http://codereview.chromium.org/10270023/diff/2003/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle_mac.mm (right):

http://codereview.chromium.org/10270023/diff/2003/ui/base/resource/resource_b...
ui/base/resource/resource_bundle_mac.mm:145: delete image;
same, should avoid this if possible

[Marshall, 2012-04-30 21:50:10]

http://codereview.chromium.org/10270023/diff/2003/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle.h (right):

http://codereview.chromium.org/10270023/diff/2003/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.h:84: virtual gfx::Image* GetImageNamed(int
resource_id) = 0;
On 2012/04/30 21:14:43, sail wrote:
> can you have this return a scoped_ptr. Then you can get rid of the comment
about
> ownership. Same below.

Done.

http://codereview.chromium.org/10270023/diff/2003/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.h:211: void AddCommonDataPack(const
std::string& pack_name, const FilePath& path);
On 2012/04/30 21:14:43, sail wrote:
> do you really need AddDataPack and AddCommonDataPack? Can we just have one?
> 
> If both are necessary then can you add a comment in both places about when one
> should be used and when the other should be used.

I think both are needed. AddDataPack() is used external to ResourceBundle to
load pack files after the ResourceBundle is created and does not trigger the
delegate. AddCommonDataPack() is used in ResourceBundle::LoadCommonResources()
and allows the delegate to override. The comments should make that clearer now.

http://codereview.chromium.org/10270023/diff/2003/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle_gtk.cc (right):

http://codereview.chromium.org/10270023/diff/2003/ui/base/resource/resource_b...
ui/base/resource/resource_bundle_gtk.cc:88: delete image;
On 2012/04/30 21:14:43, sail wrote:
> instead of doing a delete here, can you use a scoped_ptr, and do images_[key]
=
> image.release() below.

Done.

http://codereview.chromium.org/10270023/diff/2003/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle_mac.mm (right):

http://codereview.chromium.org/10270023/diff/2003/ui/base/resource/resource_b...
ui/base/resource/resource_bundle_mac.mm:145: delete image;
On 2012/04/30 21:14:43, sail wrote:
> same, should avoid this if possible

Done.

[Marshall, 2012-04-30 22:03:26]

@sail: How would you feel if we added a new static method
(InitSharedInstanceWithLocaleAndDelegate?) instead of changing the 18 existing
call sites?

[sail, 2012-04-30 22:31:27]

On 2012/04/30 22:03:26, Marshall wrote:
> @sail: How would you feel if we added a new static method
> (InitSharedInstanceWithLocaleAndDelegate?) instead of changing the 18 existing
> call sites?

I think we normally avoid this if possible. I know it sucks but at least it
keeps the size of the ResourceBundle API down.

[sail, 2012-04-30 22:33:26]

Looks good to me so far. The only thing I'm not sure about is the addition of
AddDataPackCommon().

http://codereview.chromium.org/10270023/diff/2003/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle.h (right):

http://codereview.chromium.org/10270023/diff/2003/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.h:211: void AddCommonDataPack(const
std::string& pack_name, const FilePath& path);
On 2012/04/30 21:50:10, Marshall wrote:
> On 2012/04/30 21:14:43, sail wrote:
> > do you really need AddDataPack and AddCommonDataPack? Can we just have one?
> > 
> > If both are necessary then can you add a comment in both places about when
one
> > should be used and when the other should be used.
> 
> I think both are needed. AddDataPack() is used external to ResourceBundle to
> load pack files after the ResourceBundle is created and does not trigger the
> delegate. AddCommonDataPack() is used in ResourceBundle::LoadCommonResources()
> and allows the delegate to override. The comments should make that clearer
now.

But why does external vs internal matter? Why wouldn't external AddDataPack()
requests get routed through the delegate?

[tony, 2012-05-01 15:56:48]

I'm still working on this review, but won't have time to finish until tomorrow. 
Sorry!

http://codereview.chromium.org/10270023/diff/4003/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle.cc (right):

http://codereview.chromium.org/10270023/diff/4003/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:209: DCHECK(!data_packs_.empty()) <<
"Missing call to SetResourcesDataDLL?";
Nit: This could also mean that the delegate blocked the image load, right?  I'm
not sure if we want to keep this check, although in the past it has been useful
in debugging why a new unittest is failing.

http://codereview.chromium.org/10270023/diff/4003/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle.h (right):

http://codereview.chromium.org/10270023/diff/4003/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.h:132: // Registers additional data pack files
with the global ResourceBundle. This
Nit: Remove the word global since this operates on an instance.

http://codereview.chromium.org/10270023/diff/4003/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.h:209: // Registers additional data pack files
with the global ResourceBundle. This
Nit: Remove global here too?

[Marshall, 2012-05-01 17:29:41]

Also updated the patch set to Chromium revision 134688.

http://codereview.chromium.org/10270023/diff/2003/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle.h (right):

http://codereview.chromium.org/10270023/diff/2003/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.h:211: void AddCommonDataPack(const
std::string& pack_name, const FilePath& path);
On 2012/04/30 22:33:26, sail wrote:
> But why does external vs internal matter? Why wouldn't external AddDataPack()
> requests get routed through the delegate?

Removed the additional method.

http://codereview.chromium.org/10270023/diff/4003/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle.cc (right):

http://codereview.chromium.org/10270023/diff/4003/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.cc:209: DCHECK(!data_packs_.empty()) <<
"Missing call to SetResourcesDataDLL?";
On 2012/05/01 15:56:48, tony wrote:
> Nit: This could also mean that the delegate blocked the image load, right? 
I'm
> not sure if we want to keep this check, although in the past it has been
useful
> in debugging why a new unittest is failing.

Good question. I think these checks can be useful. Maybe we should change the
GetImageNamed delegate method to return a bool, or make GetEmptyImage() public
so that the delegate can explicitly return the empty image.

http://codereview.chromium.org/10270023/diff/4003/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle.h (right):

http://codereview.chromium.org/10270023/diff/4003/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.h:132: // Registers additional data pack files
with the global ResourceBundle. This
On 2012/05/01 15:56:48, tony wrote:
> Nit: Remove the word global since this operates on an instance.

Done.

http://codereview.chromium.org/10270023/diff/4003/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.h:209: // Registers additional data pack files
with the global ResourceBundle. This
On 2012/05/01 15:56:48, tony wrote:
> Nit: Remove global here too?

Done.

[sail, 2012-05-01 17:31:38]

LGTM!

[Marshall, 2012-05-01 22:54:17]

Added unit tests. Waiting for the rest of tony's comments and then I'll update
call sites.

[tony, 2012-05-02 20:41:20]

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
File ui/base/resource/resource_bundle.cc (right):

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:94:
!delegate_->GetPathForResourcePack(pack_name, &pack_path, scale_factor)) {
i.e., change this to
if (delegate_) {
  split path into pack_name and directory
  if !delegate->GetPathForResourcePack(...)
    return
  pack_path = pack_path.Append(pack_name);
}

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:220: DCHECK(!data_packs_.empty()) <<
"Missing call to SetResourcesDataDLL?";
Let's change this to (!delegate_ && !data_packs_.empty()) since it may be
intentional to not have an image.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:266: bytes =
data_packs_[i]->GetStaticMemory(resource_id);
This is wrong. We want to return the first match we find.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:383:
base_font_.reset(delegate_->GetFont(BaseFont).release());
Can we put all the calls to delegate_->GetFont in a single if (delegate_) block?

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
File ui/base/resource/resource_bundle.h (right):

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.h:63: class Delegate {
Are delegates expected to deal with thread safety on their own?  In Chrome, we
allow calls to load resources/strings from any thread.  I'm not sure if this is
an issue for content_shell or ash.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.h:71: float scale_factor) = 0;
I would keep pack_name as a FilePath.  What if we wanted to load a pack_name
with non-ascii in it? Out parameters (pack_path) should come last.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.h:78: FilePath* pack_path) = 0;
I would just return a FilePath (empty to cancel the load).  If the delegate
doesn't want to change it, it returns the pack_path passed in.

Maybe you would do the same with GetPathForResourcePack, but it's a bit
different since the "do nothing" case means appending pack_name to pack_path.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.h:131: bool LocaleDataPakExists(const
std::string& locale);
Does this need to be public?

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.h:140: void AddDataPack(const std::string&
pack_name,
I would leave this interface unchanged and and split the path into filename and
directory pieces in the function itself.  Should be less code churn.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.h:251: // This pointer is guaranteed to outlive
the ResourceBundle instance.
I would add that it can be NULL.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
File ui/base/resource/resource_bundle_unittest.cc (right):

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle_unittest.cc:19: using ::testing::AtLeast;
Nit: I don't see AtLeast used in this file.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle_unittest.cc:60: base::StringPiece* value)
OVERRIDE {
Nit: indented 2 spaces too far?

[Marshall, 2012-05-03 19:10:54]

rohitrao@ and rsesek@ have requested that we use gfx::Image instead of
scoped_ptr<gfx::Image> because the underlying image is already refcounted. The
updated patch set includes these changes as well.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
File ui/base/resource/resource_bundle.cc (right):

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:94:
!delegate_->GetPathForResourcePack(pack_name, &pack_path, scale_factor)) {
On 2012/05/02 20:41:20, tony wrote:
> i.e., change this to
> if (delegate_) {
>   split path into pack_name and directory
>   if !delegate->GetPathForResourcePack(...)
>     return
>   pack_path = pack_path.Append(pack_name);
> }

I've changed the API to take a single FilePath argument and return a FilePath.
If the system path does not resolve (for example, GetResourcesPakFilePath in
resource_bundle_mac.mm) then just the file name will be passed in.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:220: DCHECK(!data_packs_.empty()) <<
"Missing call to SetResourcesDataDLL?";
On 2012/05/02 20:41:20, tony wrote:
> Let's change this to (!delegate_ && !data_packs_.empty()) since it may be
> intentional to not have an image.

Done.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:266: bytes =
data_packs_[i]->GetStaticMemory(resource_id);
On 2012/05/02 20:41:20, tony wrote:
> This is wrong. We want to return the first match we find.

It will return the first match. See the "&& !bytes" in the for() loop
conditional.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:383:
base_font_.reset(delegate_->GetFont(BaseFont).release());
On 2012/05/02 20:41:20, tony wrote:
> Can we put all the calls to delegate_->GetFont in a single if (delegate_)
block?

Done.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
File ui/base/resource/resource_bundle.h (right):

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.h:63: class Delegate {
On 2012/05/02 20:41:20, tony wrote:
> Are delegates expected to deal with thread safety on their own?  In Chrome, we
> allow calls to load resources/strings from any thread.  I'm not sure if this
is
> an issue for content_shell or ash.

Yes, the delegate will need to deal with thread safety issues. I've added a
comment in the head to that effect.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.h:71: float scale_factor) = 0;
On 2012/05/02 20:41:20, tony wrote:
> I would keep pack_name as a FilePath.  What if we wanted to load a pack_name
> with non-ascii in it? Out parameters (pack_path) should come last.

Done.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.h:78: FilePath* pack_path) = 0;
On 2012/05/02 20:41:20, tony wrote:
> I would just return a FilePath (empty to cancel the load).  If the delegate
> doesn't want to change it, it returns the pack_path passed in.
> 
> Maybe you would do the same with GetPathForResourcePack, but it's a bit
> different since the "do nothing" case means appending pack_name to pack_path.

Done.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.h:131: bool LocaleDataPakExists(const
std::string& locale);
On 2012/05/02 20:41:20, tony wrote:
> Does this need to be public?

It's used by l10n_util.cc currently.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.h:140: void AddDataPack(const std::string&
pack_name,
On 2012/05/02 20:41:20, tony wrote:
> I would leave this interface unchanged and and split the path into filename
and
> directory pieces in the function itself.  Should be less code churn.

Done.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.h:251: // This pointer is guaranteed to outlive
the ResourceBundle instance.
On 2012/05/02 20:41:20, tony wrote:
> I would add that it can be NULL.

Done.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
File ui/base/resource/resource_bundle_unittest.cc (right):

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle_unittest.cc:19: using ::testing::AtLeast;
On 2012/05/02 20:41:20, tony wrote:
> Nit: I don't see AtLeast used in this file.

Done.

http://codereview.chromium.org/10270023/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_bundle_unittest.cc:60: base::StringPiece* value)
OVERRIDE {
On 2012/05/02 20:41:20, tony wrote:
> Nit: indented 2 spaces too far?

Done.

[sail, 2012-05-03 19:34:48]

LGTM
changes look good to me

[Robert Sesek, 2012-05-03 19:53:57]

LGTM

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
File ui/base/resource/resource_bundle.cc (right):

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:92: DCHECK(!path.empty());
Good! Callers were doing this and it was causing logspew.

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
File ui/base/resource/resource_bundle_gtk.cc (right):

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
ui/base/resource/resource_bundle_gtk.cc:75: gfx::Image&
ResourceBundle::GetNativeImageNamed(int resource_id, ImageRTL rtl) {
const?

[tony, 2012-05-03 20:35:41]

LGTM2

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
File ui/base/resource/resource_bundle.cc (right):

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:92: DCHECK(!path.empty());
On 2012/05/03 19:53:57, rsesek wrote:
> Good! Callers were doing this and it was causing logspew.

Seems like a bug that they're trying to load a pak file that doesn't exist. 
Would be nice to fix as a follow up patch.

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:462: static gfx::Image empty_image;
This should be a pointer, otherwise an exit time destructor is needed.

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
File ui/base/resource/resource_bundle.h (right):

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.h:22: #include "ui/gfx/font.h"
Nit: I think we can still forward declare gfx::Font.

[Marshall, 2012-05-04 14:41:03]

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
File ui/base/resource/resource_bundle.cc (right):

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:462: static gfx::Image empty_image;
On 2012/05/03 20:35:42, tony wrote:
> This should be a pointer, otherwise an exit time destructor is needed.

Taking a closer look I don't think there's an advantage in making the empty
gfx::Image static because in Chromium there's only the single ResourceBundle
instance. However, there is some risk in having the static value because the
lock protecting its creation is owned by the ResourceBundle instance. It could
potentially result in a race condition if we ever allow multiple ResourceBundle
instances to exist and the GetEmptyImage method is called on different threads.
I've changed it to store the empty image in a member variable instead.

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
File ui/base/resource/resource_bundle.h (right):

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.h:22: #include "ui/gfx/font.h"
On 2012/05/03 20:35:42, tony wrote:
> Nit: I think we can still forward declare gfx::Font.

When using scoped_ptr with a forward declared type on Windows I get errors in
other header files that include resource_bundle.h but not font.h:

scoped_ptr.h(161): error C2027: use of undefined type 'gfx::Font'

So I think it's probably better to include font.h in resource_bundle.h instead
of modifying all of the other header files.

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
File ui/base/resource/resource_bundle_gtk.cc (right):

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
ui/base/resource/resource_bundle_gtk.cc:75: gfx::Image&
ResourceBundle::GetNativeImageNamed(int resource_id, ImageRTL rtl) {
On 2012/05/03 19:53:57, rsesek wrote:
> const?

Done.

[tony, 2012-05-04 17:10:51]

Still LGTM, but please get a green try run.

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
File ui/base/resource/resource_bundle.h (right):

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.h:22: #include "ui/gfx/font.h"
On 2012/05/04 14:41:03, Marshall wrote:
> On 2012/05/03 20:35:42, tony wrote:
> > Nit: I think we can still forward declare gfx::Font.
> 
> When using scoped_ptr with a forward declared type on Windows I get errors in
> other header files that include resource_bundle.h but not font.h:
> 
> scoped_ptr.h(161): error C2027: use of undefined type 'gfx::Font'
> 
> So I think it's probably better to include font.h in resource_bundle.h instead
> of modifying all of the other header files.

Which header files did you see this in?  Should they be including font.h
directly?  We should fix that in a separate change, but it's OK to include
font.h here for now.

[Marshall, 2012-05-04 17:28:41]

Working on getting a green try run :-).

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
File ui/base/resource/resource_bundle.h (right):

http://codereview.chromium.org/10270023/diff/25001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.h:22: #include "ui/gfx/font.h"
On 2012/05/04 17:10:51, tony wrote:
> On 2012/05/04 14:41:03, Marshall wrote:
> > On 2012/05/03 20:35:42, tony wrote:
> > > Nit: I think we can still forward declare gfx::Font.
> > 
> > When using scoped_ptr with a forward declared type on Windows I get errors
in
> > other header files that include resource_bundle.h but not font.h:
> > 
> > scoped_ptr.h(161): error C2027: use of undefined type 'gfx::Font'
> > 
> > So I think it's probably better to include font.h in resource_bundle.h
instead
> > of modifying all of the other header files.
> 
> Which header files did you see this in?  Should they be including font.h
> directly?  We should fix that in a separate change, but it's OK to include
> font.h here for now.

Here is the complete error message:

1>C:\svn\chromium_writable\src\base/memory/scoped_ptr.h(161): error C2027: use
of undefined type 'gfx::Font'
1>          C:\svn\chromium_writable\src\ui/base/resource/resource_bundle.h(33)
: see declaration of 'gfx::Font'
1>          C:\svn\chromium_writable\src\base/memory/scoped_ptr.h(160) : while
compiling class template member function 'scoped_ptr<C>::~scoped_ptr(void)'
1>          with
1>          [
1>              C=gfx::Font
1>          ]
1>          C:\svn\chromium_writable\src\ui/base/resource/resource_bundle.h(274)
: see reference to class template instantiation 'scoped_ptr<C>' being compiled
1>          with
1>          [
1>              C=gfx::Font
1>          ]
1>C:\svn\chromium_writable\src\base/memory/scoped_ptr.h(162): warning C4150:
deletion of pointer to incomplete type 'gfx::Font'; no destructor called
1>          C:\svn\chromium_writable\src\ui/base/resource/resource_bundle.h(33)
: see declaration of 'gfx::Font'
1>

So I misspoke earlier. It appears to be an error reported in resource_bundle.h
itself when using a forward declaration of gfx::Font.

[tony, 2012-05-04 17:44:59]

On 2012/05/04 17:28:41, Marshall wrote:
> So I misspoke earlier. It appears to be an error reported in resource_bundle.h
> itself when using a forward declaration of gfx::Font.

I see, it looks like we have to include font.h for its use in scoped_ptr.  I
guess it's needed for a scoped_ptr return type but not for a member variable. 
Seems fine.

[commit-bot: I haz the power, 2012-05-07 14:11:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marshall@chromium.org/10270023/40042

[commit-bot: I haz the power, 2012-05-07 14:11:40]

Presubmit check for 10270023-40042 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ui/views
    ash
    ui
    ui/aura

Presubmit checks took 2.5s to calculate.

[Marshall, 2012-05-07 14:14:34]

@sky: Can you approve as ui & ash OWNER? Thanks.

[sky, 2012-05-09 14:33:17]

LGTM

[commit-bot: I haz the power, 2012-05-09 14:37:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marshall@chromium.org/10270023/40042

[commit-bot: I haz the power, 2012-05-09 15:57:52]

Change committed as 136039