CHR-458: Remove DCHECK which prevents from using custom ResourceBundle::Delegate

CHR-458: Remove DCHECK which prevents from using custom ResourceBundle::Delegate

I need to create a Delegate for ResourceBundle. The only purpose of it is to provide our own fonts. Other resources should be handled by ResourceBundle. A method GetImageNamed() should also be handled by ResourceBundle. It works fine in release builds, but triggers an assert in debug builds.
DCHECK is triggered if a delegate does not create an image. This is wrong since the code can handle such case.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217726

[Przemek Kudla, 2013-08-06 14:02:08]

@reviewer: does this patch look OK to you?

[tony, 2013-08-06 17:11:26]

This doesn't look correct.  Why is code trying to load an image that doesn't
exist?  It should not request the image in the first place.

The code handles the case by returning a red square because we don't want to
crash, but it normally means something is seriously wrong.

[oshima, 2013-08-06 20:32:00]

Can't you simply define another ResourceHandler for fonts?

[Przemek Kudla, 2013-08-07 09:09:06]

On 2013/08/06 20:32:00, oshima wrote:
> Can't you simply define another ResourceHandler for fonts?

Defining another ResourceHandler would force me to modify a lot of code where it
is used.

[Przemek Kudla, 2013-08-07 09:12:17]

On 2013/08/06 17:11:26, tony wrote:
> This doesn't look correct.  Why is code trying to load an image that doesn't
> exist?  It should not request the image in the first place.
> 
> The code handles the case by returning a red square because we don't want to
> crash, but it normally means something is seriously wrong.

If the delegate is not defined then standard procedure is used to retrieve an
image from resource file (line 337). AFAIK, if the image is not available then
the red square is returned there. This is fine for me and I don't want to change
it.
I just want to change behaviour in case of fonts, the rest should be handled
standard way.

[Przemek Kudla, 2013-08-07 09:12:17]

On 2013/08/06 17:11:26, tony wrote:
> This doesn't look correct.  Why is code trying to load an image that doesn't
> exist?  It should not request the image in the first place.
> 
> The code handles the case by returning a red square because we don't want to
> crash, but it normally means something is seriously wrong.

If the delegate is not defined then standard procedure is used to retrieve an
image from resource file (line 337). AFAIK, if the image is not available then
the red square is returned there. This is fine for me and I don't want to change
it.
I just want to change behaviour in case of fonts, the rest should be handled
standard way.

[oshima_google, 2013-08-07 11:25:12]

On 2013/08/07 09:09:06, Przemek Kudla wrote:
> On 2013/08/06 20:32:00, oshima wrote:
> > Can't you simply define another ResourceHandler for fonts?
> 
> Defining another ResourceHandler would force me to modify a lot of code where
it
> is used.

This is how Windows handles native resources for example.
Can you explain what forces you to modify a lot of code?

[Przemek Kudla, 2013-08-07 12:43:24]

On 2013/08/07 11:25:12, oshima1 wrote:
> On 2013/08/07 09:09:06, Przemek Kudla wrote:
> > On 2013/08/06 20:32:00, oshima wrote:
> > > Can't you simply define another ResourceHandler for fonts?
> > 
> > Defining another ResourceHandler would force me to modify a lot of code
where
> it
> > is used.
> 
> This is how Windows handles native resources for example.
> Can you explain what forces you to modify a lot of code?

Following code is used in many places in my application:
ResourceBundle& rb = ResourceBundle::GetSharedInstance();
gfx::Font new_font = rb.GetFont(ResourceBundle::BaseFont);

If I create something new for fonts, I will have to modify above code. Right?

[tony, 2013-08-07 16:55:02]

Now I understand what you are saying.  We load the image after this DCHECK.  I
would rewrite the DCHECK to DCHECK(!data_packs_.empty()) << "No ResourceHandles
found.";.

[oshima, 2013-08-07 17:07:48]

Thank you for clarification.
I agree with tony's suggestion.

[Przemek Kudla, 2013-08-12 12:26:51]

Could you review my patch again?

[oshima, 2013-08-14 20:45:58]

lgtm, although I'm not owner.

[tony, 2013-08-14 21:37:04]

Sorry, I missed this from earlier.  LGTM2.  I'll try the commit queue.

[commit-bot: I haz the power, 2013-08-14 21:38:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkudla@opera.com/22418003/4002

[commit-bot: I haz the power, 2013-08-15 01:35:09]

Change committed as 217726