Fix ResourceBundleImageSource

Fix ResourceBundleImageSource so that it does not crash when 1.4x resources are requested.

BUG=148841
Test=ResourceBundle.GetImageNamed

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

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

[pkotwicz, 2012-09-17 15:02:10]

This is a subset of Xianzhu's CL.
It prevents a crash when a resource for 1.4x is requested from the resource
bundle. I think this is landable for M23
I will upstream the rest of the CL in a separate patch for M24.

[sky, 2012-09-17 16:56:11]

LGTM

[commit-bot: I haz the power, 2012-09-17 17:03:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10928231/1

[huangs, 2012-09-17 17:53:45]

Sorry for taking this long...

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

http://codereview.chromium.org/10928231/diff/1/ui/base/resource/resource_bund...
ui/base/resource/resource_bundle.cc:66: // An ImageSkiaSource that loads bitmaps
for given scale factor from
This block of comments should be updated.
(using "--highlight-missing-scaled-resources", etc.)

http://codereview.chromium.org/10928231/diff/1/ui/base/resource/resource_bund...
ui/base/resource/resource_bundle.cc:96: // size and
--highlight-missing-2x-resources is specified, logs
Comments need update.

http://codereview.chromium.org/10928231/diff/1/ui/base/resource/resource_bund...
ui/base/resource/resource_bundle.cc:115: static_cast<int>(ceil(bitmap1x->width()
* scale)),
Reuse size_in_pixel, for consistent rounding.

[commit-bot: I haz the power, 2012-09-17 18:32:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10928231/10002

[commit-bot: I haz the power, 2012-09-17 21:21:37]

Change committed as 157201

[mmenke, 2012-09-17 22:29:40]

This CL seems to have broken ui_unittests (BytesFormattingTest.FormatBytes). 
All runs prior to this CL passed, all ones after it was applied fail.

[mmenke, 2012-09-17 22:30:38]

On 2012/09/17 22:29:40, Matt Menke wrote:
> This CL seems to have broken ui_unittests (BytesFormattingTest.FormatBytes). 
> All runs prior to this CL passed, all ones after it was applied fail.

Sorry, didn't see the revert.

[pkotwicz, 2012-09-18 15:26:40]

Scott, can you please have another look? I updated the unittests such that the
ResourceBundle.GetImageNamed() test does not operate on
ResourceBundle::GetSharedInstance(), thus preventing undesired side effects in
other tests.
The most notable change is that the ResourceBundle class now supports weak ptrs.

[sky, 2012-09-18 18:10:36]

I'm swapping myself with Tony. Arbitrarily making RB implement WeakPtr makes me
nervous.

[tony, 2012-09-18 18:43:42]

I agree with sky.

In what situations would the gfx::Image returned by GetImageNamed() outlive the
resource bundle?

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

http://codereview.chromium.org/10928231/diff/16001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.h:252: FRIEND_TEST_ALL_PREFIXES(ResourceBundle,
ImageLifetime);
Nit: In a separate change, it would probably be worth switching this to 'friend
class ResourceBundleTest;' rather than continuing to enumerate all the test
names.

[pkotwicz, 2012-09-18 21:10:21]

The ResourceBundle is destroyed after the BrowserMainLoop has finished running,
so it does not seem as if it is possible for the gfx::Image returned by
GetImageNamed() to outlive the ResourceBundle. (Outside of tests).
Removed the test which tested this case and updated the comment in
ResourceBundle::GetImageNamed().

[tony, 2012-09-18 22:44:45]

LGTM

[Xianzhu, 2012-09-18 22:58:04]

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

http://codereview.chromium.org/10928231/diff/29001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:130: // The scaled resource pack may have
the 1x image if its grd file
On Android we won't have the 140%, 180% and 200% resources soon, so this will
still break the web pages on Android because when the scale is above 140% the
images will not be displayed. So besides this change we still need
Layout::GetScaleFactorForScale() not to return unsupported scales so that we can
scale the resources by ourselves during rendering.

In addition, we should have the flexibility not to support some scale factors.
For example, it seems not to make sense to support both 180% and 200%. But the
current logic requires both of them. Note that on Android we may scale the page
in a wide range of scales.

This change itself lgtm.

[oshima, 2012-09-18 23:41:48]

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

http://codereview.chromium.org/10928231/diff/29001/ui/base/resource/resource_...
ui/base/resource/resource_bundle.cc:130: // The scaled resource pack may have
the 1x image if its grd file
On 2012/09/18 22:58:04, Xianzhu wrote:
> On Android we won't have the 140%, 180% and 200% resources soon, so this will
> still break the web pages on Android because when the scale is above 140% the
> images will not be displayed. So besides this change we still need
> Layout::GetScaleFactorForScale() not to return unsupported scales so that we
can
> scale the resources by ourselves during rendering.
> 
> In addition, we should have the flexibility not to support some scale factors.
> For example, it seems not to make sense to support both 180% and 200%. But the
> current logic requires both of them. Note that on Android we may scale the
page
> in a wide range of scales.
> 
> This change itself lgtm.

This particular code is a workaround for the issue where we had a lot of
missing 2x resources. In (hopefully) near future, I'd like to remove this
once all necessary resources are in and grit is updated to automatically
generate scaled image if it's missing (or fail).

I think Android's scale factor is static (sorry if I'm wrong) and
assuming browser UI is java + android framework, the requirement can be quite
different and it may make sense to use different ImageSource for android.

[Xianzhu, 2012-09-19 00:11:25]

On 2012/09/18 23:41:48, oshima wrote:
> I think Android's scale factor is static (sorry if I'm wrong) and
> assuming browser UI is java + android framework, the requirement can be quite
> different and it may make sense to use different ImageSource for android.

On Android the page scale is arbitrary because we support pinch zoom. Having
140% and 180% pre-scaled resources can help improve the display quality of the
images, but we still need to almost always scale them again to display. The
current Layout::GetScaleFactorFromScale() can return any value in ScaleFactor
because of the arbitrary scale. I'll make a change to add
Layout::GetSupportedScaleFactorFromScale() and call it on Android. However, I'm
wondering if other callers of Layout::GetScaleFactorFromScale() are expecting
unsupported ScaleFactor.

[oshima, 2012-09-19 00:28:09]

(bcc'ing other reviews)

On Tue, Sep 18, 2012 at 5:11 PM, <wangxianzhu@chromium.org> wrote:

> On 2012/09/18 23:41:48, oshima wrote:
>
>> I think Android's scale factor is static (sorry if I'm wrong) and
>> assuming browser UI is java + android framework, the requirement can be
>> quite
>> different and it may make sense to use different ImageSource for android.
>>
>
> On Android the page scale is arbitrary because we support pinch zoom.
> Having
> 140% and 180% pre-scaled resources can help improve the display quality of
> the
> images, but we still need to almost always scale them again to display. The
> current Layout::**GetScaleFactorFromScale() can return any value in
> ScaleFactor
> because of the arbitrary scale. I'll make a change to add
> Layout::**GetSupportedScaleFactorFromSca**le() and call it on Android.
> However, I'm
> wondering if other callers of Layout::**GetScaleFactorFromScale() are
> expecting
> unsupported ScaleFactor.
>
>
http://codereview.chromium.**org/10928231/<http://codereview.chromium.org/109...
>

Ah i see. maybe we should meet and talk about how to proceedd. I have a few
plan
to improve this and want to make sure that it covers or at least doesn't
break
your requirements/

- oshima

[commit-bot: I haz the power, 2012-09-19 16:54:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10928231/26006

[commit-bot: I haz the power, 2012-09-19 19:20:21]

Change committed as 157583