Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(84)

Issue 419093002: ui: Remove ResourceBundle::LoadCommonResources().

Created:
6 years, 5 months ago by tfarina
Modified:
4 years, 7 months ago
Reviewers:
tony, oshima
CC:
chromium-reviews, sail, Sergey Ulanov
Project:
chromium
Visibility:
Public.

Description

ui: Remove ResourceBundle::LoadCommonResources(). BUG=176960 TEST=None R=oshima@chromium.org,tony@chromium.org

Patch Set 1 #

Total comments: 6

Patch Set 2 : REBASE #

Patch Set 3 : InitParams #

Total comments: 7

Patch Set 4 : InitParams changes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -116 lines) Patch
M ui/base/resource/resource_bundle.h View 1 2 3 3 chunks +15 lines, -5 lines 0 comments Download
M ui/base/resource/resource_bundle.cc View 1 2 3 3 chunks +9 lines, -7 lines 0 comments Download
M ui/base/resource/resource_bundle_android.cc View 1 2 chunks +1 line, -13 lines 0 comments Download
M ui/base/resource/resource_bundle_auralinux.cc View 1 chunk +0 lines, -34 lines 0 comments Download
M ui/base/resource/resource_bundle_ios.mm View 1 chunk +0 lines, -15 lines 0 comments Download
M ui/base/resource/resource_bundle_mac.mm View 1 chunk +0 lines, -16 lines 0 comments Download
M ui/base/resource/resource_bundle_win.cc View 1 chunk +0 lines, -26 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
tfarina
Just for discussion. Still totally untested.
6 years, 5 months ago (2014-07-24 22:31:59 UTC) #1
oshima
I didn't get the point. What you're trying to solve?
6 years, 5 months ago (2014-07-24 23:17:47 UTC) #2
tfarina
On 2014/07/24 23:17:47, oshima wrote: > I didn't get the point. What you're trying to ...
6 years, 5 months ago (2014-07-24 23:34:43 UTC) #3
oshima
I think we should have ResourceBundle::InitParams with parameters for locale, main (unscale resources) and scale ...
6 years, 4 months ago (2014-07-29 08:24:42 UTC) #4
tony
Oshima's suggestion sounds fine to me. The caller to init should have to specify what ...
6 years, 4 months ago (2014-07-29 16:34:06 UTC) #5
oshima
https://codereview.chromium.org/419093002/diff/1/ui/base/resource/resource_bundle.h File ui/base/resource/resource_bundle.h (right): https://codereview.chromium.org/419093002/diff/1/ui/base/resource/resource_bundle.h#newcode128 ui/base/resource/resource_bundle.h:128: Delegate* delegate); On 2014/07/29 16:34:05, tony wrote: > On ...
6 years, 4 months ago (2014-07-29 16:43:39 UTC) #6
tfarina
Tony, Oshima, please, take a look at patch set 3, just in resource_bundle.h, where I ...
6 years, 4 months ago (2014-08-07 02:41:01 UTC) #7
tony
https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resource_bundle.h File ui/base/resource/resource_bundle.h (right): https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resource_bundle.h#newcode123 ui/base/resource/resource_bundle.h:123: }; In the future when might need to do ...
6 years, 4 months ago (2014-08-07 16:29:34 UTC) #8
oshima
https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resource_bundle.h File ui/base/resource/resource_bundle.h (right): https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resource_bundle.h#newcode120 ui/base/resource/resource_bundle.h:120: base::FilePath scaled_pak_path; There can be two pak files for ...
6 years, 4 months ago (2014-08-07 17:01:10 UTC) #9
tfarina
Thanks for the comments! I will try to change InitParams later tonight. https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resource_bundle.h File ui/base/resource/resource_bundle.h ...
6 years, 4 months ago (2014-08-07 17:12:33 UTC) #10
oshima
On 2014/08/07 17:12:33, tfarina wrote: > Thanks for the comments! > > I will try ...
6 years, 4 months ago (2014-08-07 17:15:16 UTC) #11
tony
On 2014/08/07 17:12:33, tfarina wrote: > https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resource_bundle_mac.mm > File ui/base/resource/resource_bundle_mac.mm (left): > > https://codereview.chromium.org/419093002/diff/40001/ui/base/resource/resource_bundle_mac.mm#oldcode55 > ...
6 years, 4 months ago (2014-08-07 17:18:30 UTC) #12
tfarina
6 years, 4 months ago (2014-08-09 16:22:42 UTC) #13
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.

Powered by Google App Engine
This is Rietveld 408576698