Add scale factor tag to data packs

Add scale factor tag to data packs

This CL sets the image scale factor for data packs.

This will be used by the DOM UI code to look up encoded PNG data for images with a specific scale factor.

BUG=114311
TEST=


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

[sail, 2012-04-25 19:56:32]

Hi Robert, let me  know if this seems useful.

[flackr, 2012-04-25 20:16:07]

LGTM, this is exactly what I need for WebUI asset fetching.

[sail, 2012-04-25 20:17:02]

On 2012/04/25 20:16:07, flackr wrote:
> LGTM, this is exactly what I need for WebUI asset fetching.

Woot, right on the first try.

[sail, 2012-04-25 20:17:46]

+sky: ui OWNERS approval
+jhawkins: webui OWNERS approval

[James Hawkins, 2012-04-25 20:24:36]

https://chromiumcodereview.appspot.com/10151025/diff/2001/chrome/browser/ui/w...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10151025/diff/2001/chrome/browser/ui/w...
chrome/browser/ui/webui/web_ui_browsertest.cc:324:
ResourceBundle::GetSharedInstance().AddDataPack(resources_pack_path, 1.0);
This 1.0 should really be in a constant.

[sail, 2012-04-25 20:51:26]

On 2012/04/25 20:24:36, James Hawkins wrote:
>
https://chromiumcodereview.appspot.com/10151025/diff/2001/chrome/browser/ui/w...
> File chrome/browser/ui/webui/web_ui_browsertest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10151025/diff/2001/chrome/browser/ui/w...
> chrome/browser/ui/webui/web_ui_browsertest.cc:324:
> ResourceBundle::GetSharedInstance().AddDataPack(resources_pack_path, 1.0);
> This 1.0 should really be in a constant.

Hi James, should this be a general constant, like
ResourceHandle::SCALE_FACTOR_DEFAULT? Or should this be specific to the chrome
resources.pak file. Something like chrome::RESOURCES_PAK_SCALE_FACTOR.

Also, if this should be generic should it be _DEFAULT or _1X?

[James Hawkins, 2012-04-25 20:56:56]

On 2012/04/25 20:51:26, sail wrote:
> On 2012/04/25 20:24:36, James Hawkins wrote:
> >
>
https://chromiumcodereview.appspot.com/10151025/diff/2001/chrome/browser/ui/w...
> > File chrome/browser/ui/webui/web_ui_browsertest.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10151025/diff/2001/chrome/browser/ui/w...
> > chrome/browser/ui/webui/web_ui_browsertest.cc:324:
> > ResourceBundle::GetSharedInstance().AddDataPack(resources_pack_path, 1.0);
> > This 1.0 should really be in a constant.
> 
> Hi James, should this be a general constant, like
> ResourceHandle::SCALE_FACTOR_DEFAULT? Or should this be specific to the chrome
> resources.pak file. Something like chrome::RESOURCES_PAK_SCALE_FACTOR.
> 
> Also, if this should be generic should it be _DEFAULT or _1X?

It probably should be global if we're going to start using this more often (in
more places), and if we are going to have _2X or what have you, this should
probably be _1X (_DEFAULT is too ambiguous I think).

[sky, 2012-04-25 21:11:42]

https://chromiumcodereview.appspot.com/10151025/diff/2001/ui/base/resource/da...
File ui/base/resource/data_pack.h (right):

https://chromiumcodereview.appspot.com/10151025/diff/2001/ui/base/resource/da...
ui/base/resource/data_pack.h:57: void set_scale_factor(float scale_factor) {
scale_factor_ = scale_factor; }
Do you really need a setter here? A setter implies you can change after the
fact, which I don't think we want, right? How about passing to constructor and
making const.

nit: non-virtual before virtual.

https://chromiumcodereview.appspot.com/10151025/diff/2001/ui/base/resource/da...
ui/base/resource/data_pack.h:69: // The scale factor of image resources.
This isn't really clear. It reads like all images are scaled by this, which I
don't think is what it does.

https://chromiumcodereview.appspot.com/10151025/diff/2001/ui/base/resource/re...
File ui/base/resource/resource_bundle.h (right):

https://chromiumcodereview.appspot.com/10151025/diff/2001/ui/base/resource/re...
ui/base/resource/resource_bundle.h:92: void AddDataPack(const FilePath& path,
float scale_factor);
Document scale_factor.

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

https://chromiumcodereview.appspot.com/10151025/diff/2001/ui/base/resource/re...
ui/base/resource/resource_handle.h:44: // Get the scale factor of any image
resources.
Same comment about improving this description.

[sail, 2012-04-25 22:18:35]

https://chromiumcodereview.appspot.com/10151025/diff/2001/chrome/browser/ui/w...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10151025/diff/2001/chrome/browser/ui/w...
chrome/browser/ui/webui/web_ui_browsertest.cc:324:
ResourceBundle::GetSharedInstance().AddDataPack(resources_pack_path, 1.0);
On 2012/04/25 20:24:36, James Hawkins wrote:
> This 1.0 should really be in a constant.

Done.

https://chromiumcodereview.appspot.com/10151025/diff/2001/ui/base/resource/da...
File ui/base/resource/data_pack.h (right):

https://chromiumcodereview.appspot.com/10151025/diff/2001/ui/base/resource/da...
ui/base/resource/data_pack.h:57: void set_scale_factor(float scale_factor) {
scale_factor_ = scale_factor; }
On 2012/04/25 21:11:42, sky wrote:
> Do you really need a setter here? A setter implies you can change after the
> fact, which I don't think we want, right? How about passing to constructor and
> making const.
> 
> nit: non-virtual before virtual.

Done.
Removed setter.

https://chromiumcodereview.appspot.com/10151025/diff/2001/ui/base/resource/da...
ui/base/resource/data_pack.h:69: // The scale factor of image resources.
On 2012/04/25 21:11:42, sky wrote:
> This isn't really clear. It reads like all images are scaled by this, which I
> don't think is what it does. 

Done.

https://chromiumcodereview.appspot.com/10151025/diff/2001/ui/base/resource/re...
File ui/base/resource/resource_bundle.h (right):

https://chromiumcodereview.appspot.com/10151025/diff/2001/ui/base/resource/re...
ui/base/resource/resource_bundle.h:92: void AddDataPack(const FilePath& path,
float scale_factor);
On 2012/04/25 21:11:42, sky wrote:
> Document scale_factor.

Done.

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

https://chromiumcodereview.appspot.com/10151025/diff/2001/ui/base/resource/re...
ui/base/resource/resource_handle.h:44: // Get the scale factor of any image
resources.
On 2012/04/25 21:11:42, sky wrote:
> Same comment about improving this description.

Done.

[sky, 2012-04-25 22:46:01]

LGTM

[James Hawkins, 2012-04-25 22:47:32]

lgtm

[sail, 2012-04-26 00:28:38]

webkit: darin, tkent
chroem_frame: ananta,robertshield

[tkent, 2012-04-26 02:05:14]

LGTM for webkit/support/

[Jói, 2012-04-26 11:02:05]

Drive-by:

https://chromiumcodereview.appspot.com/10151025/diff/15001/ui/base/resource/r...
File ui/base/resource/resource_handle.h (right):

https://chromiumcodereview.appspot.com/10151025/diff/15001/ui/base/resource/r...
ui/base/resource/resource_handle.h:29: static const float kScaleFactor1x;
Idea:  Using percentage-based naming might make for better constant names for
various non-integral scale factors, since we can't put a . in identifiers.  That
would name these constants as kScaleFactor100 and kScaleFactor200.

[oshima, 2012-04-26 15:27:55]

drive-by

http://codereview.chromium.org/10151025/diff/15001/ui/test/test_suite.cc
File ui/test/test_suite.cc (right):

http://codereview.chromium.org/10151025/diff/15001/ui/test/test_suite.cc#newc...
ui/test/test_suite.cc:11: #include "ui/base/resource/resource_handle.h"
duplicate?

[oshima, 2012-04-26 15:32:10]

another drive-by

http://codereview.chromium.org/10151025/diff/15001/ui/base/resource/resource_...
File ui/base/resource/resource_handle.h (right):

http://codereview.chromium.org/10151025/diff/15001/ui/base/resource/resource_...
ui/base/resource/resource_handle.h:30: static const float kScaleFactor2x;
I *think* you can just do static const float kScaleFactor2x = 1.0f;

[jamesr, 2012-04-27 21:05:50]

webkit/ LGTM

[robertshield, 2012-04-27 22:05:49]

lgtm

[sail, 2012-05-01 04:16:01]

https://chromiumcodereview.appspot.com/10151025/diff/15001/ui/base/resource/r...
File ui/base/resource/resource_handle.h (right):

https://chromiumcodereview.appspot.com/10151025/diff/15001/ui/base/resource/r...
ui/base/resource/resource_handle.h:29: static const float kScaleFactor1x;
On 2012/04/26 11:02:05, Jói wrote:
> Idea:  Using percentage-based naming might make for better constant names for
> various non-integral scale factors, since we can't put a . in identifiers. 
That
> would name these constants as kScaleFactor100 and kScaleFactor200.

Done.

https://chromiumcodereview.appspot.com/10151025/diff/15001/ui/base/resource/r...
ui/base/resource/resource_handle.h:30: static const float kScaleFactor2x;
On 2012/04/26 15:32:10, oshima wrote:
> I *think* you can just do static const float kScaleFactor2x = 1.0f;

Unfortunately this only works for ints. For floats you need to put it in the CC
file.

https://chromiumcodereview.appspot.com/10151025/diff/15001/ui/test/test_suite.cc
File ui/test/test_suite.cc (right):

https://chromiumcodereview.appspot.com/10151025/diff/15001/ui/test/test_suite...
ui/test/test_suite.cc:11: #include "ui/base/resource/resource_handle.h"
On 2012/04/26 15:27:55, oshima wrote:
> duplicate?

oh no, handle vs bundle

[commit-bot: I haz the power, 2012-05-01 04:33:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10151025/25002

[commit-bot: I haz the power, 2012-05-01 06:11:55]

Change committed as 134682