Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
263253003:
Allows arbitrary scale factor in ImageSkia. (Closed)
|
Created:
6 years, 7 months ago by Jun Mukai Modified:
6 years, 7 months ago CC:
chromium-reviews, rsesek+watch_chromium.org, tbarzic Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAllows arbitrary scale factor in ImageSkia.
Previously ImageSkia requests the current device scale factor
and then pray everything is okay. Now it fetches the best fitting
scale factor and then scale the image on ImageSkia side.
BUG=372212
R=oshima@chromium.org, ananta@chromium.org
TBR=msw@chromium.org, jyasskin@chromium.org
TEST=ui_unittests --gtest_filter='ImageSkiaTest.*'
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270582
Patch Set 1 #
Total comments: 5
Patch Set 2 : half fix #Patch Set 3 : fix #
Total comments: 7
Patch Set 4 : fix #Patch Set 5 : fix #
Total comments: 2
Patch Set 6 : fix #Patch Set 7 : fix #Patch Set 8 : with unscaled #Patch Set 9 : reset upstream #
Total comments: 4
Patch Set 10 : rebase #Patch Set 11 : fix #
Total comments: 8
Patch Set 12 : fix #Patch Set 13 : cleanup #
Messages
Total messages: 47 (0 generated)
ananta, can you try this patch and see if this works on win or cause issue? https://codereview.chromium.org/263253003/diff/1/ui/gfx/image/image_skia.cc File ui/gfx/image/image_skia.cc (right): https://codereview.chromium.org/263253003/diff/1/ui/gfx/image/image_skia.cc#n... ui/gfx/image/image_skia.cc:34: bool IsImageSkiaScaling() { how about IsDSFScalingInImageSkiaEnabled ? https://codereview.chromium.org/263253003/diff/1/ui/gfx/image/image_skia.cc#n... ui/gfx/image/image_skia.cc:156: if (IsImageSkiaScaling() && scale != 1.0f && scale != 2.0f) { can you use g_supported_scales?
On 2014/05/06 18:52:27, oshima wrote: > ananta, can you try this patch and see if this works on win or cause issue? > > https://codereview.chromium.org/263253003/diff/1/ui/gfx/image/image_skia.cc > File ui/gfx/image/image_skia.cc (right): > > https://codereview.chromium.org/263253003/diff/1/ui/gfx/image/image_skia.cc#n... > ui/gfx/image/image_skia.cc:34: bool IsImageSkiaScaling() { > how about IsDSFScalingInImageSkiaEnabled ? > > https://codereview.chromium.org/263253003/diff/1/ui/gfx/image/image_skia.cc#n... > ui/gfx/image/image_skia.cc:156: if (IsImageSkiaScaling() && scale != 1.0f && > scale != 2.0f) { > can you use g_supported_scales? ResourceBundle has code for Windows which does automatic scaling. That needs to be taken out?
On 2014/05/06 19:12:56, ananta wrote: > On 2014/05/06 18:52:27, oshima wrote: > > ananta, can you try this patch and see if this works on win or cause issue? > > > > https://codereview.chromium.org/263253003/diff/1/ui/gfx/image/image_skia.cc > > File ui/gfx/image/image_skia.cc (right): > > > > > https://codereview.chromium.org/263253003/diff/1/ui/gfx/image/image_skia.cc#n... > > ui/gfx/image/image_skia.cc:34: bool IsImageSkiaScaling() { > > how about IsDSFScalingInImageSkiaEnabled ? > > > > > https://codereview.chromium.org/263253003/diff/1/ui/gfx/image/image_skia.cc#n... > > ui/gfx/image/image_skia.cc:156: if (IsImageSkiaScaling() && scale != 1.0f && > > scale != 2.0f) { > > can you use g_supported_scales? > > ResourceBundle has code for Windows which does automatic scaling. That needs to > be taken out? Eventually yes, but there may be a code that feeds scale factor manually, we still may need it for now. PlatformScaleImage should always get the same scales so it *should* work even with that code.
https://codereview.chromium.org/263253003/diff/1/ui/gfx/image/image_skia.cc File ui/gfx/image/image_skia.cc (right): https://codereview.chromium.org/263253003/diff/1/ui/gfx/image/image_skia.cc#n... ui/gfx/image/image_skia.cc:34: bool IsImageSkiaScaling() { On 2014/05/06 18:52:27, oshima wrote: > how about IsDSFScalingInImageSkiaEnabled ? Done. https://codereview.chromium.org/263253003/diff/1/ui/gfx/image/image_skia.cc#n... ui/gfx/image/image_skia.cc:156: if (IsImageSkiaScaling() && scale != 1.0f && scale != 2.0f) { On 2014/05/06 18:52:27, oshima wrote: > can you use g_supported_scales? Well, that's still unclear to me. What is g_supported_scales? Isn't it the DSFs supported by ImageSkia? Then, it sounds like it should contain 1.25 or something. Here means the scale factor supported by the resource bundle (more strictly ImageSkiaSource but that doesn't matter right now), right? How does it come?
https://codereview.chromium.org/263253003/diff/1/ui/gfx/image/image_skia.cc File ui/gfx/image/image_skia.cc (right): https://codereview.chromium.org/263253003/diff/1/ui/gfx/image/image_skia.cc#n... ui/gfx/image/image_skia.cc:156: if (IsImageSkiaScaling() && scale != 1.0f && scale != 2.0f) { On 2014/05/06 19:56:49, Jun Mukai wrote: > On 2014/05/06 18:52:27, oshima wrote: > > can you use g_supported_scales? > > Well, that's still unclear to me. What is g_supported_scales? Isn't it the DSFs > supported by ImageSkia? Then, it sounds like it should contain 1.25 or > something. > > Here means the scale factor supported by the resource bundle (more strictly > ImageSkiaSource but that doesn't matter right now), right? How does it come? Chatted offline and I was wrong. g_supported_scales is the scale factors of resources. Changed the code to look up g_supported_scales. Also revised the test.
https://codereview.chromium.org/263253003/diff/40001/ui/gfx/image/image_skia.cc File ui/gfx/image/image_skia.cc (right): https://codereview.chromium.org/263253003/diff/40001/ui/gfx/image/image_skia.... ui/gfx/image/image_skia.cc:147: float request_scale = scale; can you use "resource_scale" ? https://codereview.chromium.org/263253003/diff/40001/ui/gfx/image/image_skia.... ui/gfx/image/image_skia.cc:154: request_scale = (*g_supported_scales)[i]; can you make it so that this picks 2.0 for 1.25 and up (and so forth)? https://codereview.chromium.org/263253003/diff/40001/ui/gfx/image/image_skia_... File ui/gfx/image/image_skia_unittest.cc (right): https://codereview.chromium.org/263253003/diff/40001/ui/gfx/image/image_skia_... ui/gfx/image/image_skia_unittest.cc:89: class ScopedScaleFactorSetter { can you use ScopedSetSupportedScaleFactors in ui/base/layout.h?
https://codereview.chromium.org/263253003/diff/40001/ui/gfx/image/image_skia.cc File ui/gfx/image/image_skia.cc (right): https://codereview.chromium.org/263253003/diff/40001/ui/gfx/image/image_skia.... ui/gfx/image/image_skia.cc:147: float request_scale = scale; On 2014/05/06 20:59:49, oshima wrote: > can you use "resource_scale" ? Done. https://codereview.chromium.org/263253003/diff/40001/ui/gfx/image/image_skia.... ui/gfx/image/image_skia.cc:154: request_scale = (*g_supported_scales)[i]; On 2014/05/06 20:59:49, oshima wrote: > can you make it so that this picks 2.0 for 1.25 and up (and so forth)? I think this does so already, this for-loop attempted to pick up the least scales which is greater than or equals to the specified scale. The test case covers such case. https://codereview.chromium.org/263253003/diff/40001/ui/gfx/image/image_skia_... File ui/gfx/image/image_skia_unittest.cc (right): https://codereview.chromium.org/263253003/diff/40001/ui/gfx/image/image_skia_... ui/gfx/image/image_skia_unittest.cc:89: class ScopedScaleFactorSetter { On 2014/05/06 20:59:49, oshima wrote: > can you use ScopedSetSupportedScaleFactors in ui/base/layout.h? well, DEPS prohibit to include ui/base/layout.h from this file. Can I add it?
https://codereview.chromium.org/263253003/diff/40001/ui/gfx/image/image_skia.cc File ui/gfx/image/image_skia.cc (right): https://codereview.chromium.org/263253003/diff/40001/ui/gfx/image/image_skia.... ui/gfx/image/image_skia.cc:154: request_scale = (*g_supported_scales)[i]; On 2014/05/06 21:06:27, Jun Mukai wrote: > On 2014/05/06 20:59:49, oshima wrote: > > can you make it so that this picks 2.0 for 1.25 and up (and so forth)? > > I think this does so already, this for-loop attempted to pick up the least > scales which is greater than or equals to the specified scale. > The test case covers such case. Fixed the code so that 1.25 falls back to 1.0. Added a test case for this. PTAL.
lgtm, but please wait for ananta's lgtm
lgtm
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/263253003/80001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg_triggered_tests on tryserver.chromium chromium_presubmit on tryserver.chromium win_chromium_rel on tryserver.chromium win_chromium_x64_rel on tryserver.chromium
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/263253003/80001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: chromium_presubmit on tryserver.chromium win_chromium_rel on tryserver.chromium win_chromium_x64_rel on tryserver.chromium
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
TBR=msw for trivial change of ui/gfx/switches
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/263253003/80001
LGTM with a nit; also, will you add an about:flags entry? https://codereview.chromium.org/263253003/diff/80001/ui/gfx/switches.h File ui/gfx/switches.h (right): https://codereview.chromium.org/263253003/diff/80001/ui/gfx/switches.h#newcode16 ui/gfx/switches.h:16: GFX_EXPORT extern const char kAllowArbitraryScaleFactorInImageSkia[]; nit: I think this list is generally kept in alphabetical order, but it's short enough that it probably doesn't matter.
On 2014/05/08 17:56:06, msw wrote: > LGTM with a nit; also, will you add an about:flags entry? I don't want users to play with it (It can make bug reports confusing), so let's not have it for now. > > https://codereview.chromium.org/263253003/diff/80001/ui/gfx/switches.h > File ui/gfx/switches.h (right): > > https://codereview.chromium.org/263253003/diff/80001/ui/gfx/switches.h#newcode16 > ui/gfx/switches.h:16: GFX_EXPORT extern const char > kAllowArbitraryScaleFactorInImageSkia[]; > nit: I think this list is generally kept in alphabetical order, but it's short > enough that it probably doesn't matter.
The CQ bit was unchecked by mukai@chromium.org
https://codereview.chromium.org/263253003/diff/80001/ui/gfx/switches.h File ui/gfx/switches.h (right): https://codereview.chromium.org/263253003/diff/80001/ui/gfx/switches.h#newcode16 ui/gfx/switches.h:16: GFX_EXPORT extern const char kAllowArbitraryScaleFactorInImageSkia[]; On 2014/05/08 17:56:07, msw wrote: > nit: I think this list is generally kept in alphabetical order, but it's short > enough that it probably doesn't matter. Fortunately any other flags don't start with 'D', so I just moved them at the top. I expect it's short, but it would be right after the branch cut of M38 in the earliest. It's better to keep the order.
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/263253003/100001
Turns out this doesn't handle unscaled images well. Added a fix which is based on https://codereview.chromium.org/277773002/ PTAL (not urgent -- I believe this will be landed after the branch cut).
https://codereview.chromium.org/263253003/diff/150001/ui/gfx/image/image_skia.cc File ui/gfx/image/image_skia.cc (right): https://codereview.chromium.org/263253003/diff/150001/ui/gfx/image/image_skia... ui/gfx/image/image_skia.cc:120: // will fallback to closest image rep. can you add description for new mode and TODO to cleanup? https://codereview.chromium.org/263253003/diff/150001/ui/gfx/image/image_skia... ui/gfx/image/image_skia.cc:187: } This currently works without the following because of the fallback process, but I *think* you'll need it if we removed the fallback process, which we don't need once we fully moved to the new scheme. You can add it now, or add TODO. else { image = *iter; }
slgtm On 2014/05/09 16:10:29, oshima wrote: > https://codereview.chromium.org/263253003/diff/150001/ui/gfx/image/image_skia.cc > File ui/gfx/image/image_skia.cc (right): > > https://codereview.chromium.org/263253003/diff/150001/ui/gfx/image/image_skia... > ui/gfx/image/image_skia.cc:120: // will fallback to closest image rep. > can you add description for new mode and TODO to cleanup? > > https://codereview.chromium.org/263253003/diff/150001/ui/gfx/image/image_skia... > ui/gfx/image/image_skia.cc:187: } > This currently works without the following because of the fallback process, but > I *think* you'll need it if we removed the fallback process, which we don't need > once we fully moved to the new scheme. > > You can add it now, or add TODO. > > else { > image = *iter; > }
https://codereview.chromium.org/263253003/diff/150001/ui/gfx/image/image_skia.cc File ui/gfx/image/image_skia.cc (right): https://codereview.chromium.org/263253003/diff/150001/ui/gfx/image/image_skia... ui/gfx/image/image_skia.cc:120: // will fallback to closest image rep. On 2014/05/09 16:10:30, oshima wrote: > can you add description for new mode and TODO to cleanup? Done. https://codereview.chromium.org/263253003/diff/150001/ui/gfx/image/image_skia... ui/gfx/image/image_skia.cc:187: } On 2014/05/09 16:10:30, oshima wrote: > This currently works without the following because of the fallback process, but > I *think* you'll need it if we removed the fallback process, which we don't need > once we fully moved to the new scheme. > > You can add it now, or add TODO. > > else { > image = *iter; > } > Done.
by the way, do we have some issues for this task? I noticed that BUG= line is still blank.
On 2014/05/12 05:24:00, Jun Mukai wrote: > by the way, do we have some issues for this task? I noticed that BUG= line is > still blank. I filed crbug.com/372212
not lgtm. found a bug https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia.cc File ui/gfx/image/image_skia.cc (right): https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia... ui/gfx/image/image_skia.cc:177: float resource_scale = scale; this should be 1.0f (or minimum supported scale factor) https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia... ui/gfx/image/image_skia.cc:179: if (g_supported_scales->back() <= scale) { and this logic pass through 1.5 when 1.0f and 2.0f is supported because scale is < = 2.0f and 1.25 < 1.5.
https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia.cc File ui/gfx/image/image_skia.cc (right): https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia... ui/gfx/image/image_skia.cc:177: float resource_scale = scale; On 2014/05/14 00:33:49, oshima wrote: > this should be 1.0f (or minimum supported scale factor) why? https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia... ui/gfx/image/image_skia.cc:179: if (g_supported_scales->back() <= scale) { On 2014/05/14 00:33:49, oshima wrote: > and this logic pass through 1.5 when 1.0f and 2.0f is supported because scale is > < = 2.0f and 1.25 < 1.5. hmm? Sorry, I don't get your point. Your explanation is what I expected. For 1.5f, then enter the for-loop below and finally resource_scale will become 2.0f.
https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia.cc File ui/gfx/image/image_skia.cc (right): https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia... ui/gfx/image/image_skia.cc:179: if (g_supported_scales->back() <= scale) { On 2014/05/14 01:05:18, Jun Mukai wrote: > On 2014/05/14 00:33:49, oshima wrote: > > and this logic pass through 1.5 when 1.0f and 2.0f is supported because scale > is > > < = 2.0f and 1.25 < 1.5. > > hmm? Sorry, I don't get your point. Your explanation is what I expected. > For 1.5f, then enter the for-loop below and finally resource_scale will become > 2.0f. Sorry, what I've seen was different (It looked like it's passing through). I think we need to disable this in renderer process because renderer takes care of fractional scale factor already. Can you keep the flag and enabled only in browser (say c/b/ui/aura/chrome_browser_main_extra_parts_aura.cc) ? https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia... ui/gfx/image/image_skia.cc:184: resource_scale) { can you compare to scale ?
https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia.cc File ui/gfx/image/image_skia.cc (right): https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia... ui/gfx/image/image_skia.cc:179: if (g_supported_scales->back() <= scale) { On 2014/05/14 02:11:57, oshima wrote: > On 2014/05/14 01:05:18, Jun Mukai wrote: > > On 2014/05/14 00:33:49, oshima wrote: > > > and this logic pass through 1.5 when 1.0f and 2.0f is supported because > scale > > is > > > < = 2.0f and 1.25 < 1.5. > > > > hmm? Sorry, I don't get your point. Your explanation is what I expected. > > For 1.5f, then enter the for-loop below and finally resource_scale will become > > 2.0f. > > Sorry, what I've seen was different (It looked like it's passing through). > > I think we need to disable this in renderer process because renderer takes care > of fractional > scale factor already. > > Can you keep the flag and enabled only in browser (say > c/b/ui/aura/chrome_browser_main_extra_parts_aura.cc) ? You mean that we'll keep the two code paths (scaling on ImageSkia side and ResourceBundle side) and ImageSkia side scaling is only for browser process? That is different from my understanding. Do we really do that? I am also wondering how many files in renderer process actually use ImageSkia class. As I quickly searched, that is only used to pass image resources to the PDF plugin, not the renderer process itself. https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia... ui/gfx/image/image_skia.cc:184: resource_scale) { On 2014/05/14 02:11:57, oshima wrote: > can you compare to scale ? Done.
On 2014/05/14 05:08:41, Jun Mukai wrote: > https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia.cc > File ui/gfx/image/image_skia.cc (right): > > https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia... > ui/gfx/image/image_skia.cc:179: if (g_supported_scales->back() <= scale) { > On 2014/05/14 02:11:57, oshima wrote: > > On 2014/05/14 01:05:18, Jun Mukai wrote: > > > On 2014/05/14 00:33:49, oshima wrote: > > > > and this logic pass through 1.5 when 1.0f and 2.0f is supported because > > scale > > > is > > > > < = 2.0f and 1.25 < 1.5. > > > > > > hmm? Sorry, I don't get your point. Your explanation is what I expected. > > > For 1.5f, then enter the for-loop below and finally resource_scale will > become > > > 2.0f. > > > > Sorry, what I've seen was different (It looked like it's passing through). > > > > I think we need to disable this in renderer process because renderer takes > care > > of fractional > > scale factor already. > > > > Can you keep the flag and enabled only in browser (say > > c/b/ui/aura/chrome_browser_main_extra_parts_aura.cc) ? > > You mean that we'll keep the two code paths (scaling on ImageSkia side and > ResourceBundle side) and ImageSkia side scaling is only for browser process? > That is different from my understanding. Do we really do that? > > I am also wondering how many files in renderer process actually use ImageSkia > class. As I quickly searched, that is only used to pass image resources to the > PDF plugin, not the renderer process itself. Looks like it's used t paint UI components. See https://code.google.com/p/chromium/codesearch#chromium/src/content/child/webt... > > https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia... > ui/gfx/image/image_skia.cc:184: resource_scale) { > On 2014/05/14 02:11:57, oshima wrote: > > can you compare to scale ? > > Done.
On 2014/05/14 05:59:02, oshima wrote: > On 2014/05/14 05:08:41, Jun Mukai wrote: > > > https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia.cc > > File ui/gfx/image/image_skia.cc (right): > > > > > https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia... > > ui/gfx/image/image_skia.cc:179: if (g_supported_scales->back() <= scale) { > > On 2014/05/14 02:11:57, oshima wrote: > > > On 2014/05/14 01:05:18, Jun Mukai wrote: > > > > On 2014/05/14 00:33:49, oshima wrote: > > > > > and this logic pass through 1.5 when 1.0f and 2.0f is supported because > > > scale > > > > is > > > > > < = 2.0f and 1.25 < 1.5. > > > > > > > > hmm? Sorry, I don't get your point. Your explanation is what I expected. > > > > For 1.5f, then enter the for-loop below and finally resource_scale will > > become > > > > 2.0f. > > > > > > Sorry, what I've seen was different (It looked like it's passing through). > > > > > > I think we need to disable this in renderer process because renderer takes > > care > > > of fractional > > > scale factor already. > > > > > > Can you keep the flag and enabled only in browser (say > > > c/b/ui/aura/chrome_browser_main_extra_parts_aura.cc) ? > > > > You mean that we'll keep the two code paths (scaling on ImageSkia side and > > ResourceBundle side) and ImageSkia side scaling is only for browser process? > > That is different from my understanding. Do we really do that? > > > > I am also wondering how many files in renderer process actually use ImageSkia > > class. As I quickly searched, that is only used to pass image resources to > the > > PDF plugin, not the renderer process itself. > > Looks like it's used t paint UI components. See > > https://code.google.com/p/chromium/codesearch#chromium/src/content/child/webt... > Still don't get why the flag has to be off in renderers. I don't know how renderer takes care of the arbitrary scale factors. Do we keep the scaling in the resource bundle in renderer? If the renderer taking care of arbitrary scale factor means requesting 1.0f or 2.0f DSF, we don't have to disable the flag, because nothing has to be changed. > > > > > > https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia... > > ui/gfx/image/image_skia.cc:184: resource_scale) { > > On 2014/05/14 02:11:57, oshima wrote: > > > can you compare to scale ? > > > > Done.
On 2014/05/14 06:16:19, Jun Mukai wrote: > On 2014/05/14 05:59:02, oshima wrote: > > On 2014/05/14 05:08:41, Jun Mukai wrote: > > > > > > https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia.cc > > > File ui/gfx/image/image_skia.cc (right): > > > > > > > > > https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia... > > > ui/gfx/image/image_skia.cc:179: if (g_supported_scales->back() <= scale) { > > > On 2014/05/14 02:11:57, oshima wrote: > > > > On 2014/05/14 01:05:18, Jun Mukai wrote: > > > > > On 2014/05/14 00:33:49, oshima wrote: > > > > > > and this logic pass through 1.5 when 1.0f and 2.0f is supported > because > > > > scale > > > > > is > > > > > > < = 2.0f and 1.25 < 1.5. > > > > > > > > > > hmm? Sorry, I don't get your point. Your explanation is what I > expected. > > > > > For 1.5f, then enter the for-loop below and finally resource_scale will > > > become > > > > > 2.0f. > > > > > > > > Sorry, what I've seen was different (It looked like it's passing through). > > > > > > > > I think we need to disable this in renderer process because renderer takes > > > care > > > > of fractional > > > > scale factor already. > > > > > > > > Can you keep the flag and enabled only in browser (say > > > > c/b/ui/aura/chrome_browser_main_extra_parts_aura.cc) ? > > > > > > You mean that we'll keep the two code paths (scaling on ImageSkia side and > > > ResourceBundle side) and ImageSkia side scaling is only for browser process? > > > That is different from my understanding. Do we really do that? > > > > > > I am also wondering how many files in renderer process actually use > ImageSkia > > > class. As I quickly searched, that is only used to pass image resources to > > the > > > PDF plugin, not the renderer process itself. > > > > Looks like it's used t paint UI components. See > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/child/webt... > > > > Still don't get why the flag has to be off in renderers. > I don't know how renderer takes care of the arbitrary scale factors. > Do we keep the scaling in the resource bundle in renderer? > > If the renderer taking care of arbitrary scale factor means > requesting 1.0f or 2.0f DSF, we don't have to disable the flag, > because nothing has to be changed. > > > > > > > > > > > > https://codereview.chromium.org/263253003/diff/190001/ui/gfx/image/image_skia... > > > ui/gfx/image/image_skia.cc:184: resource_scale) { > > > On 2014/05/14 02:11:57, oshima wrote: > > > > can you compare to scale ? > > > > > > Done. I looked into more and found a bug (actually a regression) in ResourceBundle that make it looks like renderer is generating 1.4 scale images for 1.4. My reason was that If it can really generate 1.4 image for 1.4, we should not request 2.0f, then scale down, but this is not happening so this CL lgtm. Sorry for confusion.
TBR=jyasskin for the test change of chrome/browser/extensions/extension_icon_image_unittest.cc Confirmed with the original author (tbarzic) that this change still keeps the expectation.
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/263253003/230001
Message was sent while issue was closed.
Change committed as 270582 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
