ui: Remove ResourceBundle::LoadCommonResources().

ui: Remove ResourceBundle::LoadCommonResources().

BUG=176960
TEST=None
R=oshima@chromium.org,tony@chromium.org

[tfarina, 2014-07-24 22:31:59]

Just for discussion. Still totally untested.

[oshima, 2014-07-24 23:17:47]

I didn't get the point. What you're trying to solve?

[tfarina, 2014-07-24 23:34:43]

On 2014/07/24 23:17:47, oshima wrote:
> I didn't get the point. What you're trying to solve?

the dependency on chrome. Isn't that clear?

[oshima, 2014-07-29 08:24:42]

I think we should have ResourceBundle::InitParams with parameters for locale,
main (unscale resources) and scale resources, and have single

ResourceBundle::InitSharedInstance(InitParams);

and remove other unnecessary init methods. This should make the code consistent
across chrome/test/shells and cleaner.

Test should build & use separate main and scaled paks so that it can load
resources correctly for high dpi tests.

tony@, WDTY?

https://codereview.chromium.org/419093002/diff/1/ui/base/resource/resource_bu...
File ui/base/resource/resource_bundle.h (left):

https://codereview.chromium.org/419093002/diff/1/ui/base/resource/resource_bu...
ui/base/resource/resource_bundle.h:142: static void
InitSharedInstanceWithPakPath(const base::FilePath& path);
This should retire.

https://codereview.chromium.org/419093002/diff/1/ui/base/resource/resource_bu...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/419093002/diff/1/ui/base/resource/resource_bu...
ui/base/resource/resource_bundle.h:128: Delegate* delegate);
Is anyone using this delegate?

https://codereview.chromium.org/419093002/diff/1/ui/base/resource/resource_bu...
ui/base/resource/resource_bundle.h:137: // function in a sandbox without local
file access (as on Android).
I think we should ifdef for android as that's the only platform.

https://codereview.chromium.org/419093002/diff/1/ui/base/resource/resource_bu...
File ui/base/resource/resource_bundle_mac.mm (left):

https://codereview.chromium.org/419093002/diff/1/ui/base/resource/resource_bu...
ui/base/resource/resource_bundle_mac.mm:55: nil), SCALE_FACTOR_100P);
do you know why mac needs separate webkit_resources_xxx_percent?
this should already in chrome_xxx_percent.

[tony, 2014-07-29 16:34:06]

Oshima's suggestion sounds fine to me.  The caller to init should have to
specify what to load.  I would probably pass in a vector<pair<FilePath,
ScaleFactor> > or something.

Note that this is crbug.com/176960

https://codereview.chromium.org/419093002/diff/1/ui/base/resource/resource_bu...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/419093002/diff/1/ui/base/resource/resource_bu...
ui/base/resource/resource_bundle.h:128: Delegate* delegate);
On 2014/07/29 08:24:41, oshima wrote:
> Is anyone using this delegate?

Yes, the delegate is used by CEF.  We should add a comment to the class to make
this clear.

[oshima, 2014-07-29 16:43:39]

https://codereview.chromium.org/419093002/diff/1/ui/base/resource/resource_bu...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/419093002/diff/1/ui/base/resource/resource_bu...
ui/base/resource/resource_bundle.h:128: Delegate* delegate);
On 2014/07/29 16:34:05, tony wrote:
> On 2014/07/29 08:24:41, oshima wrote:
> > Is anyone using this delegate?
> 
> Yes, the delegate is used by CEF.  We should add a comment to the class to
make
> this clear.

Oh I see. I asked because all instances hit by cs passed NULL.
This should be a Init parameter as well.

[tfarina, 2014-08-07 02:41:01]

Tony, Oshima, please, take a look at patch set 3, just in resource_bundle.h,
where I put InitParams struct.

I would like to get your feedback on it, specially what kind of data you guys
had in mind to represent the resources. I picked a FilePath because I was more
familiar with InitSharedInstanceWithPakPath(), but it could be something else, a
string, you know.

Thank you for your patience.

[tony, 2014-08-07 16:29:34]

https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.h:123: };
In the future when might need to do something more complicated to differentiate
different scale factors, but this seems OK for now.

https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle_mac.mm (left):

https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
ui/base/resource/resource_bundle_mac.mm:55: nil), SCALE_FACTOR_100P);
Have you verified on OSX we don't need to load webkit_resources_100_percent? 
Maybe remove that in a separate patch?

[oshima, 2014-08-07 17:01:10]

https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.h:120: base::FilePath scaled_pak_path;
There can be two pak files for scaled images.

Are you going to use the pattern like scaled_pak_path + "_100_percent.pak",
scaled_pak_path + "_200_percent.pak"?

or have separate fields for 1x and 2x?

https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle_mac.mm (left):

https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
ui/base/resource/resource_bundle_mac.mm:55: nil), SCALE_FACTOR_100P);
On 2014/08/07 16:29:34, tony wrote:
> Have you verified on OSX we don't need to load webkit_resources_100_percent? 
> Maybe remove that in a separate patch?

yes, please check if this is needed. If this is necessary, then InitParams needs
to take two files for each scale (which I'd like to avoid)

[tfarina, 2014-08-07 17:12:33]

Thanks for the comments!

I will try to change InitParams later tonight.

https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.h:120: base::FilePath scaled_pak_path;
On 2014/08/07 17:01:10, oshima wrote:
> There can be two pak files for scaled images.
> 
> Are you going to use the pattern like scaled_pak_path + "_100_percent.pak",
> scaled_pak_path + "_200_percent.pak"?
>
Nope.
 
> or have separate fields for 1x and 2x?
Right.

I will change it to:

base::FilePath scaled_1x_pak_path;
base::FilePath scaled_2x_pak_path;

I think that is clearer.

https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle_mac.mm (left):

https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
ui/base/resource/resource_bundle_mac.mm:55: nil), SCALE_FACTOR_100P);
On 2014/08/07 17:01:10, oshima wrote:
> On 2014/08/07 16:29:34, tony wrote:
> > Have you verified on OSX we don't need to load webkit_resources_100_percent?

> > Maybe remove that in a separate patch?
> 
> yes, please check if this is needed. If this is necessary, then InitParams
needs
> to take two files for each scale (which I'd like to avoid)

The only way I can check is with trybots, as I don't have a Mac to try locally
myself.

[oshima, 2014-08-07 17:15:16]

On 2014/08/07 17:12:33, tfarina wrote:
> Thanks for the comments!
> 
> I will try to change InitParams later tonight.
> 
>
https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
> File ui/base/resource/resource_bundle.h (right):
> 
>
https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
> ui/base/resource/resource_bundle.h:120: base::FilePath scaled_pak_path;
> On 2014/08/07 17:01:10, oshima wrote:
> > There can be two pak files for scaled images.
> > 
> > Are you going to use the pattern like scaled_pak_path + "_100_percent.pak",
> > scaled_pak_path + "_200_percent.pak"?
> >
> Nope.
>  
> > or have separate fields for 1x and 2x?
> Right.
> 
> I will change it to:
> 
> base::FilePath scaled_1x_pak_path;
> base::FilePath scaled_2x_pak_path;
> 
> I think that is clearer.
> 
>
https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
> File ui/base/resource/resource_bundle_mac.mm (left):
> 
>
https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
> ui/base/resource/resource_bundle_mac.mm:55: nil), SCALE_FACTOR_100P);
> On 2014/08/07 17:01:10, oshima wrote:
> > On 2014/08/07 16:29:34, tony wrote:
> > > Have you verified on OSX we don't need to load
webkit_resources_100_percent?
> 
> > > Maybe remove that in a separate patch?
> > 
> > yes, please check if this is needed. If this is necessary, then InitParams
> needs
> > to take two files for each scale (which I'd like to avoid)
> 
> The only way I can check is with trybots, as I don't have a Mac to try locally
> myself.

you can ask the author of this change why he/she added them.

[tony, 2014-08-07 17:18:30]

On 2014/08/07 17:12:33, tfarina wrote:
>
https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
> File ui/base/resource/resource_bundle_mac.mm (left):
> 
>
https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
> ui/base/resource/resource_bundle_mac.mm:55: nil), SCALE_FACTOR_100P);
> On 2014/08/07 17:01:10, oshima wrote:
> > On 2014/08/07 16:29:34, tony wrote:
> > > Have you verified on OSX we don't need to load
webkit_resources_100_percent?
> 
> > > Maybe remove that in a separate patch?
> > 
> > yes, please check if this is needed. If this is necessary, then InitParams
> needs
> > to take two files for each scale (which I'd like to avoid)
> 
> The only way I can check is with trybots, as I don't have a Mac to try locally
> myself.

That should work.  You can also try downloading Mac builds, unzipping the file
and inspecting the pak files manually.  You can import
tools/grit/grit/format/data_pack.py in a python interpreter and load the pak
file to see if there are overlapping values.

[tfarina, 2014-08-09 16:22:42]

I'm splitting some changes here to make this landable.

https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resourc...
ui/base/resource/resource_bundle.h:120: base::FilePath scaled_pak_path;
On 2014/08/07 17:12:33, tfarina wrote:
> On 2014/08/07 17:01:10, oshima wrote:
> > There can be two pak files for scaled images.
> > 
> > Are you going to use the pattern like scaled_pak_path + "_100_percent.pak",
> > scaled_pak_path + "_200_percent.pak"?
> >
> Nope.
>  
> > or have separate fields for 1x and 2x?
> Right.
> 
> I will change it to:
> 
> base::FilePath scaled_1x_pak_path;
> base::FilePath scaled_2x_pak_path;
> 
> I think that is clearer.

Done.