Select theme resources from ResourceBundle at requested scale factor.

Select theme resources from ResourceBundle at requested scale factor.

Return the best match for a requested scale factor when fetching raw image data from ResourceBundle.

TBR=aa,abodenha

BUG=123611
TEST=ResourceBundle.LoadImageResourceBytes


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

[flackr, 2012-05-07 18:01:10]

Hi Sailesh, please take a look. This parses a request path of
IDR_RESOURCE_NAME@<scale>x. We will also need to be able to fetch resources at
different scale factors from webkit (see
http://codereview.chromium.org/10383006/). What do you think of this approach?

Thanks!

[flackr, 2012-05-09 13:57:13]

friendly ping just in case this got lost in a mound of email :-).

[sail, 2012-05-09 16:09:41]

overall looks good.
is there a way to test the changes to the resource bundle?

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

http://codereview.chromium.org/10387010/diff/1/ui/base/resource/resource_bund...
ui/base/resource/resource_bundle.cc:309: if
(data_packs_[i]->GetStringPiece(resource_id, &best_match))
Worse case this will cause StringPiece to be overwritten 3 times (1.0, 1.4,
1.8). Can we just store the best match index instead?

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

http://codereview.chromium.org/10387010/diff/1/ui/base/resource/resource_bund...
ui/base/resource/resource_bundle.h:137: base::RefCountedStaticMemory*
LoadImageResourceBytes(
Instead of adding additional APIs we should just add the scale factor parameter
to the existing API.

Otherwise the ResourceBundle API becomes huge and hard to manage.

http://codereview.chromium.org/10387010/diff/1/ui/base/resource/resource_bund...
ui/base/resource/resource_bundle.h:143: base::StringPiece
GetRawImageResource(int resource_id,
same

[flackr, 2012-05-09 19:57:01]

Added unit test to verify resource selection.

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

http://codereview.chromium.org/10387010/diff/1/ui/base/resource/resource_bund...
ui/base/resource/resource_bundle.cc:309: if
(data_packs_[i]->GetStringPiece(resource_id, &best_match))
On 2012/05/09 16:09:41, sail wrote:
> Worse case this will cause StringPiece to be overwritten 3 times (1.0, 1.4,
> 1.8). Can we just store the best match index instead?

Done.

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

http://codereview.chromium.org/10387010/diff/1/ui/base/resource/resource_bund...
ui/base/resource/resource_bundle.h:137: base::RefCountedStaticMemory*
LoadImageResourceBytes(
On 2012/05/09 16:09:41, sail wrote:
> Instead of adding additional APIs we should just add the scale factor
parameter
> to the existing API.
> 
> Otherwise the ResourceBundle API becomes huge and hard to manage.

We could do this but the scale factor doesn't make sense for non-image resources
(html, css, js, etc), or locale resources I think. This only applies to image
resources.

[sail, 2012-05-09 20:08:19]

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

http://codereview.chromium.org/10387010/diff/1/ui/base/resource/resource_bund...
ui/base/resource/resource_bundle.h:137: base::RefCountedStaticMemory*
LoadImageResourceBytes(
On 2012/05/09 19:57:01, flackr wrote:
> On 2012/05/09 16:09:41, sail wrote:
> > Instead of adding additional APIs we should just add the scale factor
> parameter
> > to the existing API.
> > 
> > Otherwise the ResourceBundle API becomes huge and hard to manage.
> 
> We could do this but the scale factor doesn't make sense for non-image
resources
> (html, css, js, etc), or locale resources I think. This only applies to image
> resources.

For for loading images we'll now have two APIs, one that has scale factor and
one that doesn't.

Is there a way to assert that the API that doesn't take a scale factor won't be
called for non-image resources?

[flackr, 2012-05-09 20:15:42]

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

http://codereview.chromium.org/10387010/diff/1/ui/base/resource/resource_bund...
ui/base/resource/resource_bundle.h:137: base::RefCountedStaticMemory*
LoadImageResourceBytes(
On 2012/05/09 20:08:19, sail wrote:
> On 2012/05/09 19:57:01, flackr wrote:
> > On 2012/05/09 16:09:41, sail wrote:
> > > Instead of adding additional APIs we should just add the scale factor
> > parameter
> > > to the existing API.
> > > 
> > > Otherwise the ResourceBundle API becomes huge and hard to manage.
> > 
> > We could do this but the scale factor doesn't make sense for non-image
> resources
> > (html, css, js, etc), or locale resources I think. This only applies to
image
> > resources.
> 
> For for loading images we'll now have two APIs, one that has scale factor and
> one that doesn't.
> 
> Is there a way to assert that the API that doesn't take a scale factor won't
be
> called for non-image resources?

I assume you were tongue twisted and meant to say assert that the API that
doesn't take a scale factor won't be called for image resources :-). We could
add a type to DataPack or have a separate vector for image packs, and only
search image packs from the *Image* functions and only search file packs from
the other functions.

[flackr, 2012-05-09 20:16:53]

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

http://codereview.chromium.org/10387010/diff/1/ui/base/resource/resource_bund...
ui/base/resource/resource_bundle.h:137: base::RefCountedStaticMemory*
LoadImageResourceBytes(
On 2012/05/09 20:15:42, flackr wrote:
> On 2012/05/09 20:08:19, sail wrote:
> > On 2012/05/09 19:57:01, flackr wrote:
> > > On 2012/05/09 16:09:41, sail wrote:
> > > > Instead of adding additional APIs we should just add the scale factor
> > > parameter
> > > > to the existing API.
> > > > 
> > > > Otherwise the ResourceBundle API becomes huge and hard to manage.
> > > 
> > > We could do this but the scale factor doesn't make sense for non-image
> > resources
> > > (html, css, js, etc), or locale resources I think. This only applies to
> image
> > > resources.
> > 
> > For for loading images we'll now have two APIs, one that has scale factor
and
> > one that doesn't.
> > 
> > Is there a way to assert that the API that doesn't take a scale factor won't
> be
> > called for non-image resources?
> 
> I assume you were tongue twisted and meant to say assert that the API that
> doesn't take a scale factor won't be called for image resources :-). We could
> add a type to DataPack or have a separate vector for image packs, and only
> search image packs from the *Image* functions and only search file packs from
> the other functions.

Alternately as a low road approach we could assert that only one data pack
contains the requested resource when using the non *Image* functions.

[sail, 2012-05-09 20:19:16]

On 2012/05/09 20:15:42, flackr wrote:
>
http://codereview.chromium.org/10387010/diff/1/ui/base/resource/resource_bund...
> File ui/base/resource/resource_bundle.h (right):
> 
>
http://codereview.chromium.org/10387010/diff/1/ui/base/resource/resource_bund...
> ui/base/resource/resource_bundle.h:137: base::RefCountedStaticMemory*
> LoadImageResourceBytes(
> On 2012/05/09 20:08:19, sail wrote:
> > On 2012/05/09 19:57:01, flackr wrote:
> > > On 2012/05/09 16:09:41, sail wrote:
> > > > Instead of adding additional APIs we should just add the scale factor
> > > parameter
> > > > to the existing API.
> > > > 
> > > > Otherwise the ResourceBundle API becomes huge and hard to manage.
> > > 
> > > We could do this but the scale factor doesn't make sense for non-image
> > resources
> > > (html, css, js, etc), or locale resources I think. This only applies to
> image
> > > resources.
> > 
> > For for loading images we'll now have two APIs, one that has scale factor
and
> > one that doesn't.
> > 
> > Is there a way to assert that the API that doesn't take a scale factor won't
> be
> > called for non-image resources?
> 
> I assume you were tongue twisted and meant to say assert that the API that
> doesn't take a scale factor won't be called for image resources :-). We could
> add a type to DataPack or have a separate vector for image packs, and only
> search image packs from the *Image* functions and only search file packs from
> the other functions.

Data packs have a encoding type field. We could assert that if no scale factor
was passed then the encoding type must not be binary.

[flackr, 2012-05-09 20:29:32]

On 2012/05/09 20:19:16, sail wrote:
> On 2012/05/09 20:15:42, flackr wrote:
> >
>
http://codereview.chromium.org/10387010/diff/1/ui/base/resource/resource_bund...
> > File ui/base/resource/resource_bundle.h (right):
> > 
> >
>
http://codereview.chromium.org/10387010/diff/1/ui/base/resource/resource_bund...
> > ui/base/resource/resource_bundle.h:137: base::RefCountedStaticMemory*
> > LoadImageResourceBytes(
> > On 2012/05/09 20:08:19, sail wrote:
> > > On 2012/05/09 19:57:01, flackr wrote:
> > > > On 2012/05/09 16:09:41, sail wrote:
> > > > > Instead of adding additional APIs we should just add the scale factor
> > > > parameter
> > > > > to the existing API.
> > > > > 
> > > > > Otherwise the ResourceBundle API becomes huge and hard to manage.
> > > > 
> > > > We could do this but the scale factor doesn't make sense for non-image
> > > resources
> > > > (html, css, js, etc), or locale resources I think. This only applies to
> > image
> > > > resources.
> > > 
> > > For for loading images we'll now have two APIs, one that has scale factor
> and
> > > one that doesn't.
> > > 
> > > Is there a way to assert that the API that doesn't take a scale factor
won't
> > be
> > > called for non-image resources?
> > 
> > I assume you were tongue twisted and meant to say assert that the API that
> > doesn't take a scale factor won't be called for image resources :-). We
could
> > add a type to DataPack or have a separate vector for image packs, and only
> > search image packs from the *Image* functions and only search file packs
from
> > the other functions.
> 
> Data packs have a encoding type field. We could assert that if no scale factor
> was passed then the encoding type must not be binary.

We specify type="BINDATA" for many non-image data files (See
shared_resources.grd for example). That being said we could change all of these
non-image references but it doesn't feel right to assert that binary => image.
If we had/added an encoding type that was a binary image then this would
definitely make sense.

[flackr, 2012-05-09 22:15:30]

+sky, I've chatted with sail about how to avoid having two API's which can both
be used for requesting image resource data. I think the two approaches we are
down to are having GetRawResourceData only search non-image packs and
GetRawImageData only search image packs or passing a special scale_factor
constant when requesting non-image data and having a single method.

I prefer the former approach since we will do less searching and not have to
pass a conceptually meaningless  scale_factor when fetching non-image data. sail
suggested that we can add a meta tag on DataPack to identify these image packs.
Your thoughts? Thanks!

[sky, 2012-05-09 22:55:05]

I'm adding Tony to the review list. He should really be an owner here.

I agree with Sailesh, I prefer a smaller API.

[flackr, 2012-05-11 18:38:55]

Okay, I've extended the existing data access methods of ResourceBundle to take a
scale factor and passed the new constant kScaleFactorNone for resources where
this doesn't make sense. I'll also patch in my other CL which just landed to
handle 2x resource requests from webkit so that these work end to end.

[sail, 2012-05-11 20:23:36]

LGTM!
I think this is pretty good.

It seems like there's not much of a difference between LoadDataResourceBytes()
and GetRawDataResoruce().

I wonder if later on we could simplify it to something like this:
  base::StringPiece LoadRawStringResource(res_id);
      // assert that there's only a single copy of this resource in all the data
packs.

  base::RefCountedStaticMemory* LoadRawImageResourcePack(res_id, scale);
     // assert that the resource is never found in a data pack with utf8/utf16
encoding

[tony, 2012-05-11 21:22:03]

https://chromiumcodereview.appspot.com/10387010/diff/8087/chrome/browser/chro...
File chrome/browser/chromeos/offline/offline_load_page.cc (right):

https://chromiumcodereview.appspot.com/10387010/diff/8087/chrome/browser/chro...
chrome/browser/chromeos/offline/offline_load_page.cc:37: #include
"ui/base/resource/resource_handle.h"
Double include?  It looks like this is in most of the files you touched.

https://chromiumcodereview.appspot.com/10387010/diff/8087/ui/base/resource/da...
File ui/base/resource/data_pack.cc (right):

https://chromiumcodereview.appspot.com/10387010/diff/8087/ui/base/resource/da...
ui/base/resource/data_pack.cc:149: return !!bsearch(&resource_id, mmap_->data()
+ kHeaderLength, resource_count_,
Nit: The !! isn't necessary unless you think it improves readability.

https://chromiumcodereview.appspot.com/10387010/diff/8087/ui/base/resource/re...
File ui/base/resource/resource_bundle.cc (right):

https://chromiumcodereview.appspot.com/10387010/diff/8087/ui/base/resource/re...
ui/base/resource/resource_bundle.cc:310: if (best_match == data_packs_.size() ||
This seems unnecessarily complex considering the possible values are 2x, 1x and
None.  It seems like you could do:

if (scale_factor is 2x)
  look for resource in 2x data packs
  if found, return.

look for resource in 1x data packs (this is a fall through from above and for 1x
and None)
if found, return.

Would we have cases where we ask for something as 1x and only a 2x resource
exists?

https://chromiumcodereview.appspot.com/10387010/diff/8087/ui/base/resource/re...
File ui/base/resource/resource_handle.h (right):

https://chromiumcodereview.appspot.com/10387010/diff/8087/ui/base/resource/re...
ui/base/resource/resource_handle.h:33: static const float kScaleFactorNone;
Can we combine the scale factors into an enum?  The only valid values seem to be
are 1, 2 and None.  Also, I find it confusing that the variable names are 100x
and 200x since that sounds like 100 and 200 times bigger. 1x and 2x would make
more sense and match the other code.

[sail, 2012-05-11 21:28:50]

https://chromiumcodereview.appspot.com/10387010/diff/8087/ui/base/resource/re...
File ui/base/resource/resource_handle.h (right):

https://chromiumcodereview.appspot.com/10387010/diff/8087/ui/base/resource/re...
ui/base/resource/resource_handle.h:33: static const float kScaleFactorNone;
On 2012/05/11 21:22:03, tony wrote:
> Can we combine the scale factors into an enum?  The only valid values seem to
be
> are 1, 2 and None.  Also, I find it confusing that the variable names are 100x
> and 200x since that sounds like 100 and 200 times bigger. 1x and 2x would make
> more sense and match the other code.

This used to be 1x and 2x but Joi made a good point that we need to support 1.4x
and 1.8x for Windows 8 Metro. Using a percentage system makes that easier to
ready.

[Jói, 2012-05-11 21:33:55]

+oshima as this is related to work he is planning to do

I think the right naming would actually be _100p, _200p, _140p etc. rather than
_100x, _140x.

Cheers,
Jói

[tony, 2012-05-11 21:49:02]

On 2012/05/11 21:33:55, Jói wrote:
> +oshima as this is related to work he is planning to do
> 
> I think the right naming would actually be _100p, _200p, _140p etc. rather
than
> _100x, _140x.

I see, 100p, 140p, 180p and 200p sound fine to me.  Would it be possible to use
an enum (maybe they map to 100, 140, 180, and 200) to make sure people don't
accidentally pass an invalid value for scale_factor?

[flackr, 2012-05-11 21:59:07]

https://chromiumcodereview.appspot.com/10387010/diff/8087/chrome/browser/chro...
File chrome/browser/chromeos/offline/offline_load_page.cc (right):

https://chromiumcodereview.appspot.com/10387010/diff/8087/chrome/browser/chro...
chrome/browser/chromeos/offline/offline_load_page.cc:37: #include
"ui/base/resource/resource_handle.h"
On 2012/05/11 21:22:03, tony wrote:
> Double include?  It looks like this is in most of the files you touched.

They are resource_handle and resource_bundle, not the same :-).

https://chromiumcodereview.appspot.com/10387010/diff/8087/ui/base/resource/da...
File ui/base/resource/data_pack.cc (right):

https://chromiumcodereview.appspot.com/10387010/diff/8087/ui/base/resource/da...
ui/base/resource/data_pack.cc:149: return !!bsearch(&resource_id, mmap_->data()
+ kHeaderLength, resource_count_,
On 2012/05/11 21:22:03, tony wrote:
> Nit: The !! isn't necessary unless you think it improves readability.

The win_rel try bot complained about casting void* to bool:
e:\b\build\slave\win\build\src\ui\base\resource\data_pack.cc(150) :error C2220:
warning treated as error - no 'object' file generated
e:\b\build\slave\win\build\src\ui\base\resource\data_pack.cc(150) : warning
C4800: 'void *' : forcing value to bool 'true' or 'false' (performance warning)

https://chromiumcodereview.appspot.com/10387010/diff/8087/ui/base/resource/re...
File ui/base/resource/resource_bundle.cc (right):

https://chromiumcodereview.appspot.com/10387010/diff/8087/ui/base/resource/re...
ui/base/resource/resource_bundle.cc:310: if (best_match == data_packs_.size() ||
On 2012/05/11 21:22:03, tony wrote:
> This seems unnecessarily complex considering the possible values are 2x, 1x
and
> None.  It seems like you could do:
> 
> if (scale_factor is 2x)
>   look for resource in 2x data packs
>   if found, return.
> 
> look for resource in 1x data packs (this is a fall through from above and for
1x
> and None)
> if found, return.
> 
> Would we have cases where we ask for something as 1x and only a 2x resource
> exists?

We should always have a 1x asset. I was mostly worried about float equality and
handling the unlikely event that we request something for an unplanned for scale
factor. That being said I don't believe we would ever actually request a
non-supported scale fcator this so it could fall into a NOTREACHED clause.

I was also trying to optimize for the case where we had a nearby scale factor
asset but not the exact scale factor. This also is probably not going to happen
in actuality, so I think this would be fine.

[flackr, 2012-05-15 02:16:33]

https://chromiumcodereview.appspot.com/10387010/diff/8087/ui/base/resource/re...
File ui/base/resource/resource_bundle.cc (right):

https://chromiumcodereview.appspot.com/10387010/diff/8087/ui/base/resource/re...
ui/base/resource/resource_bundle.cc:310: if (best_match == data_packs_.size() ||
On 2012/05/11 21:22:03, tony wrote:
> This seems unnecessarily complex considering the possible values are 2x, 1x
and
> None.  It seems like you could do:
> 
> if (scale_factor is 2x)
>   look for resource in 2x data packs
>   if found, return.
> 
> look for resource in 1x data packs (this is a fall through from above and for
1x
> and None)
> if found, return.
> 
> Would we have cases where we ask for something as 1x and only a 2x resource
> exists?

Done.

[flackr, 2012-05-15 13:45:58]

tony, can you take another look? I've converted the scale factor to an enum as
you suggested.

[tony, 2012-05-15 20:19:56]

LGTM, some style nits below.

http://codereview.chromium.org/10387010/diff/16002/ui/base/layout.h
File ui/base/layout.h (right):

http://codereview.chromium.org/10387010/diff/16002/ui/base/layout.h#newcode37
ui/base/layout.h:37: static const ScaleFactor kScaleFactorNone =
SCALE_FACTOR_100P;
I think you can add this to the enum.
SCALE_FACTOR_NONE = SCALE_FACTOR_100P.

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

http://codereview.chromium.org/10387010/diff/16002/ui/base/resource/resource_...
ui/base/resource/resource_bundle_unittest.cc:29: extern const char
kSamplePakContents2x[];
Nit: Maybe calls these 200p as well?

http://codereview.chromium.org/10387010/diff/16002/ui/base/resource/resource_...
File ui/base/resource/resource_data_dll_win.cc (right):

http://codereview.chromium.org/10387010/diff/16002/ui/base/resource/resource_...
ui/base/resource/resource_data_dll_win.cc:23: if
(base::win::GetDataResourceFromModule(module_,
Can you just return the results of this function call?

[flackr, 2012-05-16 13:39:23]

https://chromiumcodereview.appspot.com/10387010/diff/16002/ui/base/layout.h
File ui/base/layout.h (right):

https://chromiumcodereview.appspot.com/10387010/diff/16002/ui/base/layout.h#n...
ui/base/layout.h:37: static const ScaleFactor kScaleFactorNone =
SCALE_FACTOR_100P;
On 2012/05/15 20:19:56, tony wrote:
> I think you can add this to the enum.
> SCALE_FACTOR_NONE = SCALE_FACTOR_100P.

Done.

https://chromiumcodereview.appspot.com/10387010/diff/16002/ui/base/resource/r...
File ui/base/resource/resource_data_dll_win.cc (right):

https://chromiumcodereview.appspot.com/10387010/diff/16002/ui/base/resource/r...
ui/base/resource/resource_data_dll_win.cc:23: if
(base::win::GetDataResourceFromModule(module_,
On 2012/05/15 20:19:56, tony wrote:
> Can you just return the results of this function call?

Done.

[flackr, 2012-05-16 13:54:24]

sky, can you approve for chrome, ui?
ben, can you approve the changes in content?

Thanks!

[sky, 2012-05-16 14:21:21]

http://codereview.chromium.org/10387010/diff/17002/chrome/browser/ui/webui/th...
File chrome/browser/ui/webui/theme_source.cc (right):

http://codereview.chromium.org/10387010/diff/17002/chrome/browser/ui/webui/th...
chrome/browser/ui/webui/theme_source.cc:49: // Our path may include cachebuster
arguments, so trim them off.
Set *scale_factor up here so that its always set.

http://codereview.chromium.org/10387010/diff/17002/ui/ui.gyp
File ui/ui.gyp (left):

http://codereview.chromium.org/10387010/diff/17002/ui/ui.gyp#oldcode243
ui/ui.gyp:243: 'base/resource/resource_handle.cc',
Is this intentional?

[flackr, 2012-05-16 14:37:12]

http://codereview.chromium.org/10387010/diff/17002/chrome/browser/ui/webui/th...
File chrome/browser/ui/webui/theme_source.cc (right):

http://codereview.chromium.org/10387010/diff/17002/chrome/browser/ui/webui/th...
chrome/browser/ui/webui/theme_source.cc:49: // Our path may include cachebuster
arguments, so trim them off.
On 2012/05/16 14:21:21, sky wrote:
> Set *scale_factor up here so that its always set.

Done.

http://codereview.chromium.org/10387010/diff/17002/ui/ui.gyp
File ui/ui.gyp (left):

http://codereview.chromium.org/10387010/diff/17002/ui/ui.gyp#oldcode243
ui/ui.gyp:243: 'base/resource/resource_handle.cc',
On 2012/05/16 14:21:21, sky wrote:
> Is this intentional?

Yes, this became an empty file.

[sky, 2012-05-16 14:38:17]

The patch doesn't show the file as a delete. Make sure it gets deleted.

LGTM

[Ben Goodger (Google), 2012-05-17 18:25:55]

lgtm

[commit-bot: I haz the power, 2012-05-17 18:57:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/10387010/9170

[Aaron Boodman, 2012-05-17 20:54:01]

http://codereview.chromium.org/10387010/diff/9170/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_protocols.cc (right):

http://codereview.chromium.org/10387010/diff/9170/chrome/browser/extensions/e...
chrome/browser/extensions/extension_protocols.cc:78: resource_id_,
ui::SCALE_FACTOR_NONE).as_string();
It seems kinda silly to have to pass a scale factor to get a resource that is
not an image. Did you consider having an overload that assumed
SCALE_FACTOR_NONE?

[commit-bot: I haz the power, 2012-05-17 21:05:45]

Change committed as 137734