Load the resources for max scale factor first.

Load the resources for max scale factor first.
- made SCALE_FACTOR_NONE != SCALE_FACTOR_100P
- updated a few places that used SCALE_FACTOR_NONE to load
  100P images
- Don't include 200P to supported scale factor unless
  we have 200P assets (on chromeos)

BUG=156569
TEST=covered by test.

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

[oshima, 2012-10-28 00:59:21]

I still need to go through all use of SCALE_FACTOR_NONE,
but sending to you for preview. Let me know what you think.

[pkotwicz, 2012-10-29 20:06:19]

Cool!

[oshima, 2012-11-05 23:31:12]

sail -> ui/base/resource_bundle*
pkotwicz -> everything (especially icon changes)
flackr -> webui/web_ui_util*

[sail, 2012-11-05 23:46:39]

https://codereview.chromium.org/11301007/diff/8013/ui/base/resource/resource_...
File ui/base/resource/resource_bundle_unittest.cc (right):

https://codereview.chromium.org/11301007/diff/8013/ui/base/resource/resource_...
ui/base/resource/resource_bundle_unittest.cc:384: FilePath locale_path =
dir.path().Append(FILE_PATH_LITERAL("empty.pak"));
use AppendASCII instead of FILE_PATH_LITERAL? same below

https://codereview.chromium.org/11301007/diff/8013/ui/base/resource/resource_...
ui/base/resource/resource_bundle_unittest.cc:388: {
do you really need this?

https://codereview.chromium.org/11301007/diff/8013/ui/base/resource/resource_...
ui/base/resource/resource_bundle_unittest.cc:394: // Load the regular and 2x pak
files.
I can't tell how this code loads both regular and 2x pak files

[flackr, 2012-11-06 00:19:03]

webui/web_ui_util* lgtm

[oshima, 2012-11-06 00:36:51]

https://codereview.chromium.org/11301007/diff/8013/ui/base/resource/resource_...
File ui/base/resource/resource_bundle_unittest.cc (right):

https://codereview.chromium.org/11301007/diff/8013/ui/base/resource/resource_...
ui/base/resource/resource_bundle_unittest.cc:384: FilePath locale_path =
dir.path().Append(FILE_PATH_LITERAL("empty.pak"));
On 2012/11/05 23:46:39, sail wrote:
> use AppendASCII instead of FILE_PATH_LITERAL? same below

Done. changed all instance of this.

https://codereview.chromium.org/11301007/diff/8013/ui/base/resource/resource_...
ui/base/resource/resource_bundle_unittest.cc:388: {
On 2012/11/05 23:46:39, sail wrote:
> do you really need this?
removed here and above.

https://codereview.chromium.org/11301007/diff/8013/ui/base/resource/resource_...
ui/base/resource/resource_bundle_unittest.cc:394: // Load the regular and 2x pak
files.
On 2012/11/05 23:46:39, sail wrote:
> I can't tell how this code loads both regular and 2x pak files
fixed the comment.

[sail, 2012-11-06 16:33:29]

LGTM

[pkotwicz, 2012-11-07 06:40:44]

LGTM with nits

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

http://codereview.chromium.org/11301007/diff/8014/ui/base/layout.h#newcode36
ui/base/layout.h:36: // string, html/js files or an image that can be used for
any scales
Nit: /any scale/all scale factors/g

http://codereview.chromium.org/11301007/diff/8014/ui/base/layout.h#newcode43
ui/base/layout.h:43: // factor data packs.
Nit: remove the comment above as it is stale

http://codereview.chromium.org/11301007/diff/8014/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle.h (right):

http://codereview.chromium.org/11301007/diff/8014/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.h:269: FRIEND_TEST_ALL_PREFIXES(ResourceBundle,
FallbackToNone);
Nit: When you rebase, you will probably find this unnecessary

http://codereview.chromium.org/11301007/diff/8014/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle_unittest.cc (right):

http://codereview.chromium.org/11301007/diff/8014/ui/base/resource/resource_b...
ui/base/resource/resource_bundle_unittest.cc:358:
image_skia->image_reps()[0].scale_factor());
Shouldn't you call ui::test::SetSupportedScaleFactos() so that this works on
retina mac?
This may be out of date, but it seems as if ui::GetMaxScaleFactor() checks for
the maximum supported scale factor on non Chromeos.

[oshima, 2012-11-07 22:35:30]

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

http://codereview.chromium.org/11301007/diff/8014/ui/base/layout.h#newcode36
ui/base/layout.h:36: // string, html/js files or an image that can be used for
any scales
On 2012/11/07 06:40:44, pkotwicz wrote:
> Nit: /any scale/all scale factors/g

Done.

http://codereview.chromium.org/11301007/diff/8014/ui/base/layout.h#newcode43
ui/base/layout.h:43: // factor data packs.
On 2012/11/07 06:40:44, pkotwicz wrote:
> Nit: remove the comment above as it is stale

Done.

http://codereview.chromium.org/11301007/diff/8014/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle.h (right):

http://codereview.chromium.org/11301007/diff/8014/ui/base/resource/resource_b...
ui/base/resource/resource_bundle.h:269: FRIEND_TEST_ALL_PREFIXES(ResourceBundle,
FallbackToNone);
On 2012/11/07 06:40:44, pkotwicz wrote:
> Nit: When you rebase, you will probably find this unnecessary

Done.

http://codereview.chromium.org/11301007/diff/8014/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle_unittest.cc (right):

http://codereview.chromium.org/11301007/diff/8014/ui/base/resource/resource_b...
ui/base/resource/resource_bundle_unittest.cc:358:
image_skia->image_reps()[0].scale_factor());
On 2012/11/07 06:40:44, pkotwicz wrote:
> Shouldn't you call ui::test::SetSupportedScaleFactos() so that this works on
> retina mac?
> This may be out of date, but it seems as if ui::GetMaxScaleFactor() checks for
> the maximum supported scale factor on non Chromeos.

Current code is a bit broken as we included 200P even if the device
doesn't support it. I fixed it so that SetSupportedScaleFactor works for all
platforms, and added explicit 200P.

[oshima, 2012-11-07 22:35:47]

sky -> OWNERS review

[sky, 2012-11-07 22:51:44]

https://codereview.chromium.org/11301007/diff/17002/ui/base/layout.h
File ui/base/layout.h (right):

https://codereview.chromium.org/11301007/diff/17002/ui/base/layout.h#newcode39
ui/base/layout.h:39: SCALE_FACTOR_NONE = 0,
Instead of NONE should it be INDEPENDENT?

[sky, 2012-11-07 23:50:17]

LGTM

[oshima, 2012-11-07 23:51:45]

https://codereview.chromium.org/11301007/diff/17002/ui/base/layout.h
File ui/base/layout.h (right):

https://codereview.chromium.org/11301007/diff/17002/ui/base/layout.h#newcode39
ui/base/layout.h:39: SCALE_FACTOR_NONE = 0,
On 2012/11/07 22:51:44, sky wrote:
> Instead of NONE should it be INDEPENDENT?

Discussed offline. I'll do this as a part of separate CL which converts this
enum to a class.

[oshima, 2012-11-09 22:32:37]

Scott, I have to change the CL a bit:
I couldn't use ResourceBundle in GetSupportedScaleFactorsInternal because
it's not available in some test cases, and also ResourceBundleAuraX11 now
uses it to determine if it should load 200P.
 I also decided to load 200P first, as this exercises more code path.
(and I could find the issue crbug.com/160074 this way)

Important new changes are only in layout.cc and resource_bundle.cc.
Can you take another look?

[sky, 2012-11-09 23:02:17]

LGTM

https://codereview.chromium.org/11301007/diff/10017/ui/base/layout.cc
File ui/base/layout.cc (right):

https://codereview.chromium.org/11301007/diff/10017/ui/base/layout.cc#newcode32
ui/base/layout.cc:32: #include "base/debug/stack_trace.h"
remove this

[oshima, 2012-11-09 23:49:37]

https://codereview.chromium.org/11301007/diff/10017/ui/base/layout.cc
File ui/base/layout.cc (right):

https://codereview.chromium.org/11301007/diff/10017/ui/base/layout.cc#newcode32
ui/base/layout.cc:32: #include "base/debug/stack_trace.h"
On 2012/11/09 23:02:17, sky wrote:
> remove this
Done.

[commit-bot: I haz the power, 2012-11-13 18:34:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/11301007/17005

[commit-bot: I haz the power, 2012-11-13 18:42:53]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-13 21:44:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/11301007/28041

[commit-bot: I haz the power, 2012-11-14 05:49:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/11301007/5045

[commit-bot: I haz the power, 2012-11-14 07:28:42]

Change committed as 167622