Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11028064:
Resize images for hi-dpi based on a custom PNG chunk added by GRIT r78, and roll GRIT r78 (Closed)
|
Created:
8 years, 2 months ago by benrg Modified:
8 years, 2 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionResize images for hi-dpi based on a custom PNG chunk added by GRIT r78, and roll GRIT r78
Doing both at once to avoid a checkin-and-egg problem.
BUG=153892
TEST=manual with --force-device-scale-factor=2, with and without --highlight-missing-scaled-resources
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162587
Patch Set 1 : . #
Total comments: 20
Patch Set 2 : scale factor instead of % #Patch Set 3 : don't store source scale in csCl, add unit test #Patch Set 4 : code adapted from issue 10914092 #
Total comments: 13
Patch Set 5 : comments #
Total comments: 14
Patch Set 6 : rebase (no other changes) #Patch Set 7 : comments #
Total comments: 6
Patch Set 8 : back out ChromeBrowserMainPartsLinux::PreProfileInit change, add RoundToInt, move image loading to … #
Total comments: 5
Patch Set 9 : comments #
Total comments: 5
Patch Set 10 : only enable user chunk handling for resource bundle images #Patch Set 11 : back out libpng/PNGCodec changes, scan for special chunks by hand (and rebase) #
Total comments: 6
Patch Set 12 : address comments #
Total comments: 3
Patch Set 13 : nits #Patch Set 14 : roll grit 78 #
Messages
Total messages: 54 (0 generated)
http://codereview.chromium.org/11028064/diff/4001/ui/gfx/codec/png_codec.cc File ui/gfx/codec/png_codec.cc (right): http://codereview.chromium.org/11028064/diff/4001/ui/gfx/codec/png_codec.cc#n... ui/gfx/codec/png_codec.cc:443: png_set_error_fn(png_ptr, NULL, LogLibPNGDecodeError, LogLibPNGDecodeWarning); This call wasn't in the 4-argument version of PNGCodec::Decode before the refactoring. I assume that's okay. http://codereview.chromium.org/11028064/diff/4001/ui/gfx/codec/png_codec.cc#n... ui/gfx/codec/png_codec.cc:467: output->clear(); Before the refactoring the output was cleared only when png_process_data completed without calling DecodeEndCallback.
I'm not right person to review ui/gfx/codec. Can you find someone from blame list? http://codereview.chromium.org/11028064/diff/4001/third_party/libpng/pngusr.h File third_party/libpng/pngusr.h (right): http://codereview.chromium.org/11028064/diff/4001/third_party/libpng/pngusr.h... third_party/libpng/pngusr.h:69: #undef PNG_NO_READ_USER_CHUNKS I think we probably should double check with someone in chrome team who knows/owns png library if this is ok. (like if there is any security related risks etc) http://codereview.chromium.org/11028064/diff/4001/ui/base/resource/resource_b... File ui/base/resource/resource_bundle.cc (right): http://codereview.chromium.org/11028064/diff/4001/ui/base/resource/resource_b... ui/base/resource/resource_bundle.cc:81: resource_id_, scale_factor, &image, &image_percent); We need scale info only in the images that fell back, don't we? http://codereview.chromium.org/11028064/diff/4001/ui/base/resource/resource_b... ui/base/resource/resource_bundle.cc:611: int* percent) const { I think it's better to use ScaleFactor rather than int, and rejects scales that we don't support. We can use scale_factor in/out parameter too. http://codereview.chromium.org/11028064/diff/4001/ui/gfx/codec/png_codec.cc File ui/gfx/codec/png_codec.cc (right): http://codereview.chromium.org/11028064/diff/4001/ui/gfx/codec/png_codec.cc#n... ui/gfx/codec/png_codec.cc:463: if (ok) { I think if (Decode...) { return true; } ... return false is preferred in chrome. http://codereview.chromium.org/11028064/diff/4001/ui/gfx/codec/png_codec.cc#n... ui/gfx/codec/png_codec.cc:477: bool ok = DecodeInternal(input, input_size, &state); ditto.
+erg for png_codec changes.
http://codereview.chromium.org/11028064/diff/4001/ui/base/resource/resource_b... File ui/base/resource/resource_bundle.cc (right): http://codereview.chromium.org/11028064/diff/4001/ui/base/resource/resource_b... ui/base/resource/resource_bundle.cc:81: resource_id_, scale_factor, &image, &image_percent); On 2012/10/05 20:33:11, oshima wrote: > We need scale info only in the images that fell back, don't we? And in that case, all we need is a simple marker instead of scale info. http://codereview.chromium.org/11028064/diff/4001/ui/base/resource/resource_b... ui/base/resource/resource_bundle.cc:336: // more efficient. I wanted to know if the image has fallen back to 1x in. http://codereview.chromium.org/10914092 Let's keep other resouce_bundle.cc change separate, and finish marker change first.
https://chromiumcodereview.appspot.com/11028064/diff/4001/ui/base/resource/re... File ui/base/resource/resource_bundle.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/4001/ui/base/resource/re... ui/base/resource/resource_bundle.cc:81: resource_id_, scale_factor, &image, &image_percent); On 2012/10/07 02:42:21, oshima wrote: > On 2012/10/05 20:33:11, oshima wrote: > > We need scale info only in the images that fell back, don't we? > > And in that case, all we need is a simple marker instead of > scale info. Yes, when we add a csCl chunk the scale is always 100% because we only fall back to 100%. I chose to store the scale anyway, but I could change that if you want. https://chromiumcodereview.appspot.com/11028064/diff/4001/ui/base/resource/re... ui/base/resource/resource_bundle.cc:336: // more efficient. On 2012/10/07 02:42:21, oshima wrote: > I wanted to know if the image has fallen back to 1x in. > http://codereview.chromium.org/10914092 > > Let's keep other resouce_bundle.cc change separate, > and finish marker change first. Are you saying that my changes here are okay as is? https://chromiumcodereview.appspot.com/11028064/diff/4001/ui/base/resource/re... ui/base/resource/resource_bundle.cc:611: int* percent) const { On 2012/10/05 20:33:11, oshima wrote: > I think it's better to use ScaleFactor rather than int, and rejects scales that > we don't support. > We can use scale_factor in/out parameter too. Done.
https://chromiumcodereview.appspot.com/11028064/diff/4001/ui/base/resource/re... File ui/base/resource/resource_bundle.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/4001/ui/base/resource/re... ui/base/resource/resource_bundle.cc:81: resource_id_, scale_factor, &image, &image_percent); On 2012/10/07 04:36:11, benrg wrote: > On 2012/10/07 02:42:21, oshima wrote: > > On 2012/10/05 20:33:11, oshima wrote: > > > We need scale info only in the images that fell back, don't we? > > > > And in that case, all we need is a simple marker instead of > > scale info. > > Yes, when we add a csCl chunk the scale is always 100% because we only fall back > to 100%. I chose to store the scale anyway, but I could change that if you want. Since we'll never store scale other than 100, let's keep it simple and store a bit. https://chromiumcodereview.appspot.com/11028064/diff/4001/ui/base/resource/re... ui/base/resource/resource_bundle.cc:85: // image with the correct scale factor. you can just return empty ImageSkiaReps and it will fallback to 100P https://chromiumcodereview.appspot.com/11028064/diff/4001/ui/base/resource/re... ui/base/resource/resource_bundle.cc:89: DCHECK(found); I don't think this DCHECK is useful. https://chromiumcodereview.appspot.com/11028064/diff/4001/ui/base/resource/re... ui/base/resource/resource_bundle.cc:336: // more efficient. On 2012/10/07 04:36:11, benrg wrote: > On 2012/10/07 02:42:21, oshima wrote: > > I wanted to know if the image has fallen back to 1x in. > > http://codereview.chromium.org/10914092 > > > > Let's keep other resouce_bundle.cc change separate, > > and finish marker change first. > > Are you saying that my changes here are okay as is? I want to change so that it will load at the target scale here, (as in http://codereview.chromium.org/10914092), so I'd like to keep this method as is for now.
Elliot: png ping. Oshima, read on: https://chromiumcodereview.appspot.com/11028064/diff/4001/ui/base/resource/re... File ui/base/resource/resource_bundle.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/4001/ui/base/resource/re... ui/base/resource/resource_bundle.cc:81: resource_id_, scale_factor, &image, &image_percent); On 2012/10/08 17:32:15, oshima wrote: > Since we'll never store scale other than 100, let's keep it simple and store a > bit. Done. https://chromiumcodereview.appspot.com/11028064/diff/4001/ui/base/resource/re... ui/base/resource/resource_bundle.cc:85: // image with the correct scale factor. On 2012/10/08 17:32:15, oshima wrote: > you can just return empty ImageSkiaReps and it will fallback to 100P I left this as is for now because the new code below depends on the current behavior. https://chromiumcodereview.appspot.com/11028064/diff/4001/ui/base/resource/re... ui/base/resource/resource_bundle.cc:336: // more efficient. On 2012/10/08 17:32:15, oshima wrote: > I want to change so that it will load at the target scale here, (as in > http://codereview.chromium.org/10914092), so I'd like to keep this method > as is for now. I folded some of the code from that CL into this one. Let me know what you think. The call to GetPrimaryDisplay() is commented out because it crashes at startup and I'm not sure how to fix that, but the code works as written and should be easier to fix than what I had before.
https://chromiumcodereview.appspot.com/11028064/diff/4001/ui/base/resource/re... File ui/base/resource/resource_bundle.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/4001/ui/base/resource/re... ui/base/resource/resource_bundle.cc:336: // more efficient. On 2012/10/09 00:07:06, benrg wrote: > On 2012/10/08 17:32:15, oshima wrote: > > I want to change so that it will load at the target scale here, (as in > > http://codereview.chromium.org/10914092), so I'd like to keep this method > > as is for now. > > I folded some of the code from that CL into this one. Let me know what you > think. The call to GetPrimaryDisplay() is commented out because it crashes at > startup and I'm not sure how to fix that, but the code works as written and > should be easier to fix than what I had before. looks ok. Can you send me stack trace of the crash you had? https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/base/resource/r... File ui/base/resource/resource_bundle.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/base/resource/r... ui/base/resource/resource_bundle.cc:59: // will show the scaled image blended with red instead. Update the comment https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/base/resource/r... ui/base/resource/resource_bundle.cc:327: // gfx::Screen::GetPrimaryDisplay().device_scale_factor()); can you show me stack trace? https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/base/resource/r... ui/base/resource/resource_bundle.cc:332: image_source->GetImageForScale(primary_scale_factor); Can't we just do gfx::ImageSkia image_skia(..); if (image_skia.GetRepresentation(scale_factor).is_null() { .... return GetEmptyImage(); } https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/base/resource/r... ui/base/resource/resource_bundle.cc:584: return true; you need {} in this case https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/gfx/codec/png_c... File ui/gfx/codec/png_codec.h (right): https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/gfx/codec/png_c... ui/gfx/codec/png_codec.h:126: // Most clients don't need the |scale_fallback| argument. // Same as Decode above, but scale_fallback is NULL. // Should be used this instead if you don't need to know // if the image has csCl chunk. (in better English)
On 2012/10/09 00:07:06, benrg wrote: > Elliot: png ping. Oshima, read on: I'm not an owner of ui/gfx/.
https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/base/resource/r... File ui/base/resource/resource_bundle.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/base/resource/r... ui/base/resource/resource_bundle.cc:59: // will show the scaled image blended with red instead. On 2012/10/09 18:28:35, oshima wrote: > Update the comment Done. https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/base/resource/r... ui/base/resource/resource_bundle.cc:327: // gfx::Screen::GetPrimaryDisplay().device_scale_factor()); On 2012/10/09 18:28:35, oshima wrote: > can you show me stack trace? (Done) https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/base/resource/r... ui/base/resource/resource_bundle.cc:332: image_source->GetImageForScale(primary_scale_factor); On 2012/10/09 18:28:35, oshima wrote: > Can't we just do > gfx::ImageSkia image_skia(..); > if (image_skia.GetRepresentation(scale_factor).is_null() { > .... > return GetEmptyImage(); > } > ImageSkia wants size_in_dip at construction time, and I don't know how to get it except by loading the image first. https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/base/resource/r... ui/base/resource/resource_bundle.cc:584: return true; On 2012/10/09 18:28:35, oshima wrote: > you need {} in this case Done. https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/gfx/codec/png_c... File ui/gfx/codec/png_codec.h (right): https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/gfx/codec/png_c... ui/gfx/codec/png_codec.h:126: // Most clients don't need the |scale_fallback| argument. On 2012/10/09 18:28:35, oshima wrote: > // Same as Decode above, but scale_fallback is NULL. > // Should be used this instead if you don't need to know > // if the image has csCl chunk. > (in better English) Done.
Sailesh, can you look at the PNG codec changes? Do we need a security review for this?
the resource/* stuff mostly looks good to me is there anyone else that can review the PNG stuff? https://chromiumcodereview.appspot.com/11028064/diff/29003/ui/base/resource/r... File ui/base/resource/resource_bundle.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/29003/ui/base/resource/r... ui/base/resource/resource_bundle.cc:69: bool scale_fallback = false; same, naming could be better https://chromiumcodereview.appspot.com/11028064/diff/29003/ui/base/resource/r... ui/base/resource/resource_bundle.cc:76: DCHECK_NE(scale_factor, SCALE_FACTOR_100P); This DCHECK says that we shouldn't be missing images at 1x scale factory. Then in the very next line we check if the image is missing and return an empty image. Since the calling code will return a red image if anything is missing I think this DCHECK is unnecessary. https://chromiumcodereview.appspot.com/11028064/diff/29003/ui/base/resource/r... ui/base/resource/resource_bundle.cc:88: static_cast<int>(image.width() * scale + 0.5f), the cast and + 0.5 is for rounding right? can you put that in a helper function? even better can we move that to a util file, specially since we use this everywhere Hm.. this CL might be interesting: https://chromiumcodereview.appspot.com/11081007/ It uses ToFlooredSize() https://chromiumcodereview.appspot.com/11028064/diff/29003/ui/base/resource/r... ui/base/resource/resource_bundle.cc:325: // ScaleFactor primary_scale_factor = GetScaleFactorFromScale( there's no point leaving commented out code here. You can put a TODO if you like and explain that the code is not ready yet due to crash at startup https://chromiumcodereview.appspot.com/11028064/diff/29003/ui/base/resource/r... File ui/base/resource/resource_bundle.h (right): https://chromiumcodereview.appspot.com/11028064/diff/29003/ui/base/resource/r... ui/base/resource/resource_bundle.h:294: bool* scale_fallback) const; scale_fallback is not very descriptive. maybe did_fallback_to_1x_scale? https://chromiumcodereview.appspot.com/11028064/diff/29003/ui/gfx/codec/png_c... File ui/gfx/codec/png_codec.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/29003/ui/gfx/codec/png_c... ui/gfx/codec/png_codec.cc:344: DCHECK(memcmp(chunk->name, kPngScaleChunk, 5) == 0); // guaranteed by libpng sentence fragment
https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/base/resource/r... File ui/base/resource/resource_bundle.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/base/resource/r... ui/base/resource/resource_bundle.cc:332: image_source->GetImageForScale(primary_scale_factor); On 2012/10/09 21:37:18, benrg wrote: > On 2012/10/09 18:28:35, oshima wrote: > > Can't we just do > > gfx::ImageSkia image_skia(..); > > if (image_skia.GetRepresentation(scale_factor).is_null() { > > .... > > return GetEmptyImage(); > > } > > > > ImageSkia wants size_in_dip at construction time, and I don't know how to get it > except by loading the image first. How about ImageSkia(source, init_scale_factor) ? You should be able to get size_in_dip, or create an empty ImageSkia.
https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/base/resource/r... File ui/base/resource/resource_bundle.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/base/resource/r... ui/base/resource/resource_bundle.cc:327: // gfx::Screen::GetPrimaryDisplay().device_scale_factor()); On 2012/10/09 21:37:18, benrg wrote: > On 2012/10/09 18:28:35, oshima wrote: > > can you show me stack trace? > > (Done) Thanks. Doing any UI related stuff before aura/ash are initialized can be problematic. Please update the comment to something like // TODO: This should use the currently used scale factor, but // scale factor information is not yet available when this is first // accessed. Fix initialization order and use the correct device scale // factor. and remove the code.
brettw, can you review the PNG codec changes or suggest someone?
+stevenjb for chrome_browser_main_chromeos.cc. https://codereview.chromium.org/11028064/diff/29003/ui/base/resource/resource... File ui/base/resource/resource_bundle.cc (right): https://codereview.chromium.org/11028064/diff/29003/ui/base/resource/resource... ui/base/resource/resource_bundle.cc:69: bool scale_fallback = false; On 2012/10/09 22:01:37, sail wrote: > same, naming could be better Done. https://codereview.chromium.org/11028064/diff/29003/ui/base/resource/resource... ui/base/resource/resource_bundle.cc:76: DCHECK_NE(scale_factor, SCALE_FACTOR_100P); On 2012/10/09 22:01:37, sail wrote: > This DCHECK says that we shouldn't be missing images at 1x scale factory. > Then in the very next line we check if the image is missing and return an empty > image. > > Since the calling code will return a red image if anything is missing I think > this DCHECK is unnecessary. You're right; rewritten. https://codereview.chromium.org/11028064/diff/29003/ui/base/resource/resource... ui/base/resource/resource_bundle.cc:88: static_cast<int>(image.width() * scale + 0.5f), On 2012/10/09 22:01:37, sail wrote: > the cast and + 0.5 is for rounding right? can you put that in a helper > function? > even better can we move that to a util file, specially since we use this > everywhere > > Hm.. this CL might be interesting: > https://chromiumcodereview.appspot.com/11081007/ > It uses ToFlooredSize() I'm not sure I even want to make a helper function for this because it is fundamentally a hack. The nearest integer is just a guess, not necessarily the right answer. A helper function would need a name and description implying that it did something that made sense. There should be a round-to-int function in util, but it's pretty hard to do right (floorf(x+0.5) fails for x = 2^23+1 for example). I think it's beyond the scope of this CL. C99 and C++11 have lroundf(). It's used in a couple of files in cc/. Does that mean it's fair game? Probably not. https://codereview.chromium.org/11028064/diff/29003/ui/base/resource/resource... ui/base/resource/resource_bundle.cc:325: // ScaleFactor primary_scale_factor = GetScaleFactorFromScale( On 2012/10/09 22:01:37, sail wrote: > there's no point leaving commented out code here. > You can put a TODO if you like and explain that the code is not ready yet due > to crash at startup Fixed by fixing the crash (courtesy Oshima). https://codereview.chromium.org/11028064/diff/29003/ui/base/resource/resource... File ui/base/resource/resource_bundle.h (right): https://codereview.chromium.org/11028064/diff/29003/ui/base/resource/resource... ui/base/resource/resource_bundle.h:294: bool* scale_fallback) const; On 2012/10/09 22:01:37, sail wrote: > scale_fallback is not very descriptive. maybe did_fallback_to_1x_scale? How about fell_back_to_1x? https://codereview.chromium.org/11028064/diff/29003/ui/gfx/codec/png_codec.cc File ui/gfx/codec/png_codec.cc (right): https://codereview.chromium.org/11028064/diff/29003/ui/gfx/codec/png_codec.cc... ui/gfx/codec/png_codec.cc:344: DCHECK(memcmp(chunk->name, kPngScaleChunk, 5) == 0); // guaranteed by libpng On 2012/10/09 22:01:37, sail wrote: > sentence fragment Done.
resource/* LGTM https://codereview.chromium.org/11028064/diff/29003/ui/base/resource/resource... File ui/base/resource/resource_bundle.cc (right): https://codereview.chromium.org/11028064/diff/29003/ui/base/resource/resource... ui/base/resource/resource_bundle.cc:88: static_cast<int>(image.width() * scale + 0.5f), On 2012/10/10 02:18:13, benrg wrote: > On 2012/10/09 22:01:37, sail wrote: > > the cast and + 0.5 is for rounding right? can you put that in a helper > > function? > > even better can we move that to a util file, specially since we use this > > everywhere > > > > Hm.. this CL might be interesting: > > https://chromiumcodereview.appspot.com/11081007/ > > It uses ToFlooredSize() > > I'm not sure I even want to make a helper function for this because it is > fundamentally a hack. The nearest integer is just a guess, not necessarily the > right answer. A helper function would need a name and description implying that > it did something that made sense. > > There should be a round-to-int function in util, but it's pretty hard to do > right (floorf(x+0.5) fails for x = 2^23+1 for example). I think it's beyond the > scope of this CL. > > C99 and C++11 have lroundf(). It's used in a couple of files in cc/. Does that > mean it's fair game? Probably not. This is still a rounding function. Duplicated. Even if you don't put in a public file you should still move it to a function at the top of the file.
I think pkasting is a good reviewer for the image decoder stuff.
On 2012/10/10 03:12:31, brettw wrote: > I think pkasting is a good reviewer for the image decoder stuff. Nope, I've never looked at anything PNG-related. SVN log for png_codec.cc suggests maybe francoisk777@gmail.com or tony@chromium.org. noel@chromium.org might be the best bet, though, as he's our resident PNG expert and has done lots on the WebKit PNG decoders.
noel, can you review the PNG codec changes or suggest someone (maybe francoisk777@gmail.com or tony@chromium.org)?
On 2012/10/09 22:06:07, oshima wrote: > https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/base/resource/r... > File ui/base/resource/resource_bundle.cc (right): > > https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/base/resource/r... > ui/base/resource/resource_bundle.cc:332: > image_source->GetImageForScale(primary_scale_factor); > On 2012/10/09 21:37:18, benrg wrote: > > On 2012/10/09 18:28:35, oshima wrote: > > > Can't we just do > > > gfx::ImageSkia image_skia(..); > > > if (image_skia.GetRepresentation(scale_factor).is_null() { > > > .... > > > return GetEmptyImage(); > > > } > > > > > > > ImageSkia wants size_in_dip at construction time, and I don't know how to get > it > > except by loading the image first. > > How about ImageSkia(source, init_scale_factor) ? > You should be able to get size_in_dip, or create an empty ImageSkia. In case you missed this message. And please run try jobs.
http://codereview.chromium.org/11028064/diff/26008/chrome/browser/chromeos/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): http://codereview.chromium.org/11028064/diff/26008/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:354: // the device scale factor is available when loading images. // Call it first before performing UI related tasks. For // example, the device scale factor must be available before // loading images. Please correct Engrish as needed :)
Sorry to further delay this, but I am nervous about reordering the ash::Shell initialization. Can we extract that as a separate CL so that we can more easily revert it if there are problems? Thanks! https://codereview.chromium.org/11028064/diff/26008/chrome/browser/chromeos/c... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11028064/diff/26008/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:354: // the device scale factor is available when loading images. On 2012/10/10 16:21:28, oshima wrote: > // Call it first before performing UI related tasks. For > // example, the device scale factor must be available before > // loading images. > > Please correct Engrish as needed :) How this initializes Ash is not entirely obvious, I would recommend: // This will call ChromeBrowserMainParts::PreProfileInit(), which calls ChromeBrowserMainExtraPartsAsh::PreProfileInit() which instantiates ash::Shell. This needs to happen before performing any UI related tasks, e.g. the device scale factor must be available before loading images. https://codereview.chromium.org/11028064/diff/26008/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:358: chromeos::BootTimesLoader::Get()->RecordChromeMainStats(); This should probably still be first. https://codereview.chromium.org/11028064/diff/26008/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:376: g_browser_process->profile_manager(); I assume this has been carefully tested? I'd worry that Ash::Shell initialization may expect the profile manager to be instantiated. If I recall correctly, it was added to the end of this function because of dependencies on profile and/or login or session information.
http://codereview.chromium.org/11028064/diff/26008/ui/base/resource/resource_... File ui/base/resource/resource_bundle.cc (right): http://codereview.chromium.org/11028064/diff/26008/ui/base/resource/resource_... ui/base/resource/resource_bundle.cc:323: gfx::Screen::GetPrimaryDisplay().device_scale_factor()); Looks this this need more work (see bot failures). Can you keep the change (that you incorporated from my change) separated? I'll look into them. (then you don't need PreProfileInit change).
On 2012/10/10 17:07:27, stevenjb (chromium) wrote: > Sorry to further delay this, but I am nervous about reordering the ash::Shell > initialization. Can we extract that as a separate CL so that we can more easily > revert it if there are problems? Thanks! > > https://codereview.chromium.org/11028064/diff/26008/chrome/browser/chromeos/c... > File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): > > https://codereview.chromium.org/11028064/diff/26008/chrome/browser/chromeos/c... > chrome/browser/chromeos/chrome_browser_main_chromeos.cc:354: // the device scale > factor is available when loading images. > On 2012/10/10 16:21:28, oshima wrote: > > // Call it first before performing UI related tasks. For > > // example, the device scale factor must be available before > > // loading images. > > > > Please correct Engrish as needed :) > > How this initializes Ash is not entirely obvious, I would recommend: > > // This will call ChromeBrowserMainParts::PreProfileInit(), which calls > ChromeBrowserMainExtraPartsAsh::PreProfileInit() which instantiates ash::Shell. > This needs to happen before performing any UI related tasks, e.g. the device > scale factor must be available before loading images. > > https://codereview.chromium.org/11028064/diff/26008/chrome/browser/chromeos/c... > chrome/browser/chromeos/chrome_browser_main_chromeos.cc:358: > chromeos::BootTimesLoader::Get()->RecordChromeMainStats(); > This should probably still be first. > > https://codereview.chromium.org/11028064/diff/26008/chrome/browser/chromeos/c... > chrome/browser/chromeos/chrome_browser_main_chromeos.cc:376: > g_browser_process->profile_manager(); > I assume this has been carefully tested? I'd worry that Ash::Shell > initialization may expect the profile manager to be instantiated. If I recall > correctly, it was added to the end of this function because of dependencies on > profile and/or login or session information. I tested it on linux desktop with login manager, and looks to be working correctly. That doesn't mean it won't have issue and this CL still have to be tested on bots etc though. That's being said, if ash initialization depends on profile stuff, it needs to be refactored so that we can initialize aura/ash first before doing any UI related work. aura/ash is like native platform (win/gtk) and has to be initialized first. I'll look into this separately.
oshima, PTAL https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/base/resource/r... File ui/base/resource/resource_bundle.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/22004/ui/base/resource/r... ui/base/resource/resource_bundle.cc:332: image_source->GetImageForScale(primary_scale_factor); On 2012/10/09 22:06:07, oshima wrote: > On 2012/10/09 21:37:18, benrg wrote: > > On 2012/10/09 18:28:35, oshima wrote: > > > Can't we just do > > > gfx::ImageSkia image_skia(..); > > > if (image_skia.GetRepresentation(scale_factor).is_null() { > > > .... > > > return GetEmptyImage(); > > > } > > > > > > > ImageSkia wants size_in_dip at construction time, and I don't know how to get > it > > except by loading the image first. > > How about ImageSkia(source, init_scale_factor) ? > You should be able to get size_in_dip, or create an empty ImageSkia. Done. https://chromiumcodereview.appspot.com/11028064/diff/29003/ui/base/resource/r... File ui/base/resource/resource_bundle.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/29003/ui/base/resource/r... ui/base/resource/resource_bundle.cc:88: static_cast<int>(image.width() * scale + 0.5f), On 2012/10/10 03:09:55, sail wrote: > On 2012/10/10 02:18:13, benrg wrote: > > On 2012/10/09 22:01:37, sail wrote: > > > the cast and + 0.5 is for rounding right? can you put that in a helper > > > function? > > > even better can we move that to a util file, specially since we use this > > > everywhere > > > > > > Hm.. this CL might be interesting: > > > https://chromiumcodereview.appspot.com/11081007/ > > > It uses ToFlooredSize() > > > > I'm not sure I even want to make a helper function for this because it is > > fundamentally a hack. The nearest integer is just a guess, not necessarily the > > right answer. A helper function would need a name and description implying > that > > it did something that made sense. > > > > There should be a round-to-int function in util, but it's pretty hard to do > > right (floorf(x+0.5) fails for x = 2^23+1 for example). I think it's beyond > the > > scope of this CL. > > > > C99 and C++11 have lroundf(). It's used in a couple of files in cc/. Does that > > mean it's fair game? Probably not. > > This is still a rounding function. Duplicated. > Even if you don't put in a public file you should still move it to a function at > the top of the file. Done. https://chromiumcodereview.appspot.com/11028064/diff/26008/ui/base/resource/r... File ui/base/resource/resource_bundle.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/26008/ui/base/resource/r... ui/base/resource/resource_bundle.cc:323: gfx::Screen::GetPrimaryDisplay().device_scale_factor()); On 2012/10/10 18:13:47, oshima wrote: > Looks this this need more work (see bot failures). Can you keep the change (that > you incorporated from my change) separated? I'll look into them. (then you don't > need PreProfileInit change). I went back to using SCALE_FACTOR_100P instead of the correct scale factor, which matches the behavior of the code before this CL. Is the comment okay or should I go into more detail?
https://chromiumcodereview.appspot.com/11028064/diff/20006/ui/base/resource/r... File ui/base/resource/resource_bundle.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/20006/ui/base/resource/r... ui/base/resource/resource_bundle.cc:98: RoundToInt(image.height() * scale)); Exactly how conversion should work depends on how conversion is performed in layout code, right? Since layout code does floor, This doesn't really matter right now as we only supports 1x and 2x (not sure exactly how Win8 is going to support other scales), but I think this should do the same, using gfx::ToFlooredSize. Thoughts? https://chromiumcodereview.appspot.com/11028064/diff/20006/ui/gfx/image/image... File ui/gfx/image/image_skia.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/20006/ui/gfx/image/image... ui/gfx/image/image_skia.cc:193: storage_ = new internal::ImageSkiaStorage(source, size_in_dip); Can you add new constructor ImageSkiaStorage(source, scale_factor), and call FindRepresentation() in there, and initialize size. (you need to remove const from size_). Then you can just return ImageSkiaRep() in ResourceBundleImageSource when the image for given scale factor is not found. https://chromiumcodereview.appspot.com/11028064/diff/20006/ui/gfx/image/image... ui/gfx/image/image_skia.cc:195: DetachStorageFromThread(); DetachStorageFromThread should be called after AddRepresentation
https://chromiumcodereview.appspot.com/11028064/diff/20006/ui/base/resource/r... File ui/base/resource/resource_bundle.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/20006/ui/base/resource/r... ui/base/resource/resource_bundle.cc:98: RoundToInt(image.height() * scale)); On 2012/10/10 21:43:29, oshima wrote: > Exactly how conversion should work depends on how conversion is performed in > layout code, right? Since layout code does floor, > This doesn't really matter right now as we only supports 1x and 2x (not sure > exactly how Win8 is going to support other scales), but I think this should do > the same, using gfx::ToFlooredSize. Thoughts? That's fine with me. Everything is going to have to be changed anyway for 1.4x and 1.8x; I don't see how we can reliably get the 1.4x/1.8x pixel size from the 1x size or vice versa. https://chromiumcodereview.appspot.com/11028064/diff/20006/ui/gfx/image/image... File ui/gfx/image/image_skia.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/20006/ui/gfx/image/image... ui/gfx/image/image_skia.cc:193: storage_ = new internal::ImageSkiaStorage(source, size_in_dip); On 2012/10/10 21:43:29, oshima wrote: > Can you add new constructor ImageSkiaStorage(source, scale_factor), > and call FindRepresentation() in there, and initialize size. > (you need to remove const from size_). > Then you can just return ImageSkiaRep() in ResourceBundleImageSource > when the image for given scale factor is not found. Like this? It's a bit awkward because of the failure handling.
https://chromiumcodereview.appspot.com/11028064/diff/19011/ui/base/resource/r... File ui/base/resource/resource_bundle.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/19011/ui/base/resource/r... ui/base/resource/resource_bundle.cc:80: if (!found) and you should be able to return just ImageSkiaRep() here. https://chromiumcodereview.appspot.com/11028064/diff/19011/ui/gfx/image/image... File ui/gfx/image/image_skia.cc (right): https://chromiumcodereview.appspot.com/11028064/diff/19011/ui/gfx/image/image... ui/gfx/image/image_skia.cc:66: source_.reset(); no need to reset here. You can just check image_reps().empty() below.
On 2012/10/10 15:25:31, benrg wrote: > noel, can you review the PNG codec changes or suggest someone (maybe > mailto:francoisk777@gmail.com or tony@chromium.org)? Sure. I'm amazed we need to modify libpng to support this. How about this: make GRIT create the rescaled resource at build time.
On 2012/10/11 04:36:56, noel chromium wrote: > On 2012/10/10 15:25:31, benrg wrote: > > noel, can you review the PNG codec changes or suggest someone (maybe > > mailto:francoisk777@gmail.com or tony@chromium.org)? > > Sure. I'm amazed we need to modify libpng to support this. How about this: > make GRIT create the rescaled resource at build time. That seems very wasteful. Not only would this increase binary size, it would also increase time to load the pak file.
> That seems very wasteful. Not only would this increase binary size, it would > also increase time to load the pak file. We already ship both 1x and 2x, right? Is that wasteful? Do we load both?
On 2012/10/11 05:00:58, noel chromium wrote: > We already ship both 1x and 2x, right? Is that wasteful? Do we load both? I sure hope we don't. pak file load time is in the critical path for application startup.
On 2012/10/11 05:00:58, noel chromium wrote: > > That seems very wasteful. Not only would this increase binary size, it would > > also increase time to load the pak file. > > We already ship both 1x and 2x, right? Is that wasteful? Do we load both? On ChromeOS, we ship 2x only when needed. On Mac, we do ship both because you can't tell if it will be needed. And yes, it is wasteful to ship something that a user doesn't need and we should fix it. We did consider the option to scale them in grit. We decided to go this route for following reasons: a) We believe this is simpler overall (doing this in C++ is simpler than doing it in grit/python + external program. Also easy to maintain because this is in main trunk) b) It uses less space/memory. c) We also like to use more custom chunks to improve maintainability of our resource files of our image data. (9 patch and pointer hot point)
On Wed, Oct 10, 2012 at 10:21 PM, <pkasting@chromium.org> wrote: > On 2012/10/11 05:00:58, noel chromium wrote: > >> We already ship both 1x and 2x, right? Is that wasteful? Do we load >> both? >> > > I sure hope we don't. pak file load time is in the critical path for > application startup. > On ChromeOS/link, we do load both unfortunately. We're working on fixing this (crbug.com/125756) and this CL is part of it. - oshima > > https://chromiumcodereview.**appspot.com/11028064/<https://chromiumcodereview... >
On 2012/10/11 05:45:43, oshima wrote: > On Wed, Oct 10, 2012 at 10:21 PM, <mailto:pkasting@chromium.org> wrote: > > > On 2012/10/11 05:00:58, noel chromium wrote: > > > >> We already ship both 1x and 2x, right? Is that wasteful? Do we load > >> both? > >> > > > > I sure hope we don't. pak file load time is in the critical path for > > application startup. > > > > On ChromeOS/link, we do load both unfortunately. We're working on fixing > this (crbug.com/125756) and this CL is part of it. > > - oshima Ok if pak file loading is on the critical path, then perhaps fix the PNG file in the pak files. I noticed many PNGs with rdf comments, and example of which is http://code.google.com/p/chromium/issues/detail?id=153892#c9 aka, 778 bytes of unneeded XMP rdf comment in a png of 1365 bytes total -- not good.
On 2012/10/11 05:29:13, oshima wrote: > On 2012/10/11 05:00:58, noel chromium wrote: > > > That seems very wasteful. Not only would this increase binary size, it would > > > also increase time to load the pak file. > > > > We already ship both 1x and 2x, right? Is that wasteful? Do we load both? > > On ChromeOS, we ship 2x only when needed. On Mac, we do ship both because you > can't tell if it will be needed. And yes, it is wasteful to ship something that > a user doesn't need and we should fix it. Thanks for the explanation. Windows also has a HDPI story from Vista onwards, and I might have HDPI monitor plugged into my windows box for all you know :)
On 2012/10/11 06:57:56, noel chromium wrote: > On 2012/10/11 05:45:43, oshima wrote: > > On Wed, Oct 10, 2012 at 10:21 PM, <mailto:pkasting@chromium.org> wrote: > > > > > On 2012/10/11 05:00:58, noel chromium wrote: > > > > > >> We already ship both 1x and 2x, right? Is that wasteful? Do we load > > >> both? > > >> > > > > > > I sure hope we don't. pak file load time is in the critical path for > > > application startup. > > > > > > > On ChromeOS/link, we do load both unfortunately. We're working on fixing > > this (crbug.com/125756) and this CL is part of it. > > > > - oshima > > > Ok if pak file loading is on the critical path, then perhaps fix the PNG file in > the pak files. I noticed many PNGs with rdf comments, and example of which is > http://code.google.com/p/chromium/issues/detail?id=153892#c9 aka, 778 bytes of > unneeded XMP rdf comment in a png of 1365 bytes total -- not good. Yes, this is know issue (crbug.com/146632) and we run pngcrush once in a while as documented in chrome/app/theme/README to reduce the size. We like to automate this, but don't have specific milestone tho.
On 2012/10/11 05:29:13, oshima wrote: > We did consider the option to scale them in grit. We decided to go this route > for following reasons: > > a) We believe this is simpler overall (doing this in C++ is simpler than doing > it in grit/python + external program. Also easy to maintain because this is in > main trunk) It would be simpler to have the image designers provide them in 1x and 2x sizes: perhaps that's too much to ask. > b) It uses less space/memory. Did you compare the cost of rescaling vs decoding? If PNG image decoding costs X in time, then image rescaling can cost as much as 6X as a rule of thumb. > c) We also like to use more custom chunks to improve maintainability of our > resource files of our image data. (9 patch and pointer hot point) The maintainability of resource files seems like a problem GRIT should solve. Can we think of some simple way to add a new attribute to GRIT resource XML and provide code to read it? I say this because adding custom chunks to PNG is a security issue.
> Did you compare the cost of rescaling vs decoding? If PNG image > decoding costs X in time, then image rescaling can cost as much as 6X > as a rule of thumb. I haven't tested it. Paging time would also need to be counted. I normally expect that to dominate processing time, but maybe it isn't with a fast SSD and a slow CPU. > The maintainability of resource files seems like a problem GRIT should > solve. Can we think of some simple way to add a new attribute to GRIT > resource XML and provide code to read it? I say this because adding > custom chunks to PNG is a security issue. I don't understand why custom chunks are more of a problem than any other aspect of PNG decoding. Can you point me to an explanation? Making GRIT dump this information in a C++ header file instead of a PNG chunk may be a good idea. I don't have time to work on it in the near future. In the short term I could avoid the problem by reading this chunk in the same way I add it: as a known sequence of bytes at a known point in the file. That would avoid any changes to the PNG codec.
On 2012/10/11 16:19:04, benrg wrote: > > Did you compare the cost of rescaling vs decoding? If PNG image > > decoding costs X in time, then image rescaling can cost as much as 6X > > as a rule of thumb. > > I haven't tested it. Paging time would also need to be counted. I normally > expect that to dominate processing time, but maybe it isn't with a fast SSD and > a slow CPU. > > > The maintainability of resource files seems like a problem GRIT should > > solve. Can we think of some simple way to add a new attribute to GRIT > > resource XML and provide code to read it? I say this because adding > > custom chunks to PNG is a security issue. > > I don't understand why custom chunks are more of a problem than any other aspect > of PNG decoding. Can you point me to an explanation? It's probably good idea to limit chunk processing only to images in resource bundle. > Making GRIT dump this information in a C++ header file instead of a PNG chunk > may be a good idea. I thought about this option as well. While this works for this particular scaling issue, this doesn't solve maintainability issue. 9patch/cursor hot point are the property of each image data that UX designer controls. They can (and do) differ between 100P and 200P, so in order to handle it in grd, we need to re-introduce different grd definitions that we just eliminated. In addition, UX designer only provides images and they do not modify grd files. Android uses npTc chunk to store 9 patch information, and it'd be really nice if we can use the same chunk to store 9 patch as we can use their tool to create 9 patch images. > I don't have time to work on it in the near future. In the > short term I could avoid the problem by reading this chunk in the same way I add > it: as a known sequence of bytes at a known point in the file. That would avoid > any changes to the PNG codec.
On 2012/10/11 17:24:29, oshima wrote: > On 2012/10/11 16:19:04, benrg wrote: > > > The maintainability of resource files seems like a problem GRIT should > > > solve. Can we think of some simple way to add a new attribute to GRIT > > > resource XML and provide code to read it? I say this because adding > > > custom chunks to PNG is a security issue. > > > > I don't understand why custom chunks are more of a problem than any other > > aspect of PNG decoding. Can you point me to an explanation? > > It's probably good idea to limit chunk processing only to images in resource > bundle. Note the lib being modified here is also used in the chrome renderer (WebCore). And the use ancillary png chunks has had a checkered history http://libpng.sourceforge.net/decompression_bombs.html for example.
On 2012/10/11 16:19:04, benrg wrote: > > Did you compare the cost of rescaling vs decoding? If PNG image > > decoding costs X in time, then image rescaling can cost as much as 6X > > as a rule of thumb. > > I haven't tested it. Paging time would also need to be counted. I normally > expect that to dominate processing time, but maybe it isn't with a fast SSD and > a slow CPU. Measurements of software performance often differ from my expectations due to those neglected "other factors". A tape measure is the first thing I put in my tool box :)
On 2012/10/11 16:19:04, benrg wrote: > > The maintainability of resource files seems like a problem GRIT should > > solve. Can we think of some simple way to add a new attribute to GRIT > > resource XML and provide code to read it? I say this because adding > > custom chunks to PNG is a security issue. > > Making GRIT dump this information in a C++ header file instead of a PNG chunk > may be a good idea. I don't have time to work on it in the near future. In the > short term I could avoid the problem by reading this chunk in the same way I add > it: as a known sequence of bytes at a known point in the file. "I add it: as a known sequence of bytes at a known point in the file" - I see two approaches: 1) In the .pak file, we have an offset to the resource data and a resource length for each resource therein. On creation of a pak file PNG entry, write arbitrary data containing whatever magic metadata you need prepended to the PNG data. On reading, detect that prepended data and do whatever it says to do. Pass the remaining data to the PNG decoder. 2) WebCore allows the png TEXT (comment) chunk. Assume the PNG is already pngcrushed. Write your magic metadata into a TEXT chunk. The PNG header size is know apriori and is small for crushed PNG. Find your magic metadata somewhere within the header length and do whatever it says to do if found. > That would avoid any changes to the PNG codec. See above.
On 2012/10/12 03:19:12, noel chromium wrote: > Note the lib being modified here is also used in the chrome renderer (WebCore). > And the use ancillary png chunks has had a checkered history > http://libpng.sourceforge.net/decompression_bombs.html for example. libpng will not pass the contents of unknown chunks to zlib or process them in any other way. It will just pass the buffer verbatim to a user function. I still don't understand why you think there is a security risk here. Is it that the extra libpng code I enabled has not been audited? Can it be audited? I don't think it's very complicated.
oshima, PTAL. I removed all libpng/PNGCodec changes. https://codereview.chromium.org/11028064/diff/19011/ui/base/resource/resource... File ui/base/resource/resource_bundle.cc (right): https://codereview.chromium.org/11028064/diff/19011/ui/base/resource/resource... ui/base/resource/resource_bundle.cc:80: if (!found) On 2012/10/11 01:31:54, oshima wrote: > and you should be able to return just ImageSkiaRep() here. Done. https://codereview.chromium.org/11028064/diff/19011/ui/gfx/image/image_skia.cc File ui/gfx/image/image_skia.cc (right): https://codereview.chromium.org/11028064/diff/19011/ui/gfx/image/image_skia.c... ui/gfx/image/image_skia.cc:66: source_.reset(); On 2012/10/11 01:31:54, oshima wrote: > no need to reset here. You can just check image_reps().empty() below. It's not empty -- FindRepresentation inserts an empty image so that it can short-circuit the next search.
thanks, looks better. https://codereview.chromium.org/11028064/diff/19011/ui/gfx/image/image_skia.cc File ui/gfx/image/image_skia.cc (right): https://codereview.chromium.org/11028064/diff/19011/ui/gfx/image/image_skia.c... ui/gfx/image/image_skia.cc:66: source_.reset(); On 2012/10/15 21:58:54, benrg wrote: > On 2012/10/11 01:31:54, oshima wrote: > > no need to reset here. You can just check image_reps().empty() below. > > It's not empty -- FindRepresentation inserts an empty image so that it can > short-circuit the next search. ok, you can still use image_reps().begin()->is_null(), but i'm also OK as is. https://codereview.chromium.org/11028064/diff/54001/ui/base/resource/resource... File ui/base/resource/resource_bundle.cc (right): https://codereview.chromium.org/11028064/diff/54001/ui/base/resource/resource... ui/base/resource/resource_bundle.cc:68: if (size - pos < 12) can you define constants for these literals? (12, "csCl", etc?) https://codereview.chromium.org/11028064/diff/54001/ui/base/resource/resource... ui/base/resource/resource_bundle.cc:77: found_cscl = true; why not just *fell_back_to_1x = true? https://codereview.chromium.org/11028064/diff/54001/ui/base/resource/resource... File ui/base/resource/resource_bundle.h (right): https://codereview.chromium.org/11028064/diff/54001/ui/base/resource/resource... ui/base/resource/resource_bundle.h:291: // If the call succeeds and |fell_back_to_1x| is not NULL, it is set to true fell_back_to_1x must not be NULL now, correct?
oshima, PTAL. https://codereview.chromium.org/11028064/diff/54001/ui/base/resource/resource... File ui/base/resource/resource_bundle.cc (right): https://codereview.chromium.org/11028064/diff/54001/ui/base/resource/resource... ui/base/resource/resource_bundle.cc:68: if (size - pos < 12) On 2012/10/16 23:12:09, oshima wrote: > can you define constants for these literals? (12, "csCl", etc?) Done. https://codereview.chromium.org/11028064/diff/54001/ui/base/resource/resource... ui/base/resource/resource_bundle.cc:77: found_cscl = true; On 2012/10/16 23:12:09, oshima wrote: > why not just *fell_back_to_1x = true? Done. https://codereview.chromium.org/11028064/diff/54001/ui/base/resource/resource... File ui/base/resource/resource_bundle.h (right): https://codereview.chromium.org/11028064/diff/54001/ui/base/resource/resource... ui/base/resource/resource_bundle.h:291: // If the call succeeds and |fell_back_to_1x| is not NULL, it is set to true On 2012/10/16 23:12:09, oshima wrote: > fell_back_to_1x must not be NULL now, correct? Fixed.
lgtm with nits http://codereview.chromium.org/11028064/diff/58001/ui/base/resource/resource_... File ui/base/resource/resource_bundle.cc (right): http://codereview.chromium.org/11028064/diff/58001/ui/base/resource/resource_... ui/base/resource/resource_bundle.cc:75: uint32 length; = 0;, just in case http://codereview.chromium.org/11028064/diff/58001/ui/base/resource/resource_... ui/base/resource/resource_bundle.cc:604: bool* fell_back_to_1x) const { DCHECK(fell_back_to_1x) http://codereview.chromium.org/11028064/diff/58001/ui/base/resource/resource_... ui/base/resource/resource_bundle.cc:632: bool* fell_back_to_1x) const { DCHECK(fell_back_to_1x)
Ben, requesting owner approval for ui/gfx/image/*, also a question: There's a chicken-and-egg problem with this CL and 11187033, since either one by itself will break the UI. Is it okay to update DEPS in this CL?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benrg@chromium.org/11028064/62003
Change committed as 162587 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
