Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10831207:
Make ThumbnailGenerator support high DPI. (Closed)
|
Created:
8 years, 4 months ago by mazda Modified:
8 years, 4 months ago CC:
chromium-reviews, Avi (use Gerrit), ajwong+watch_chromium.org, creis+watch_chromium.org, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake ThumbnailGenerator support high DPI.
- Store 2x thumbnail when the primary monitor has ui::SCALE_FACTOR_200P.
- Copy the constant size of pixels from RWHV regardless of the view's device scale factor.
BUG=125043
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152450
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : address comment #
Total comments: 2
Patch Set 4 : change if-defs for win #Patch Set 5 : #
Total comments: 4
Patch Set 6 : change LOG to DLOG #Patch Set 7 : add TODO comment #Messages
Total messages: 14 (0 generated)
http://codereview.chromium.org/10831207/diff/1002/chrome/browser/tab_contents... File chrome/browser/tab_contents/thumbnail_generator.cc (right): http://codereview.chromium.org/10831207/diff/1002/chrome/browser/tab_contents... chrome/browser/tab_contents/thumbnail_generator.cc:130: gfx::Display primary_display = gfx::Screen::GetPrimaryDisplay(); Can you add comment TODO(oshima): Use device's default scale factor. This may not give the right scale factor. For example, you can mirror the display on Arrow, which will most likely give you 1x scale factor. I'm planning to add "device's default scale factor" to address this. http://codereview.chromium.org/10831207/diff/1002/chrome/browser/tab_contents... chrome/browser/tab_contents/thumbnail_generator.cc:136: #if defined(OS_WIN) Is it better to have "&& !defined(USE_AURA) "? I know we don't officially support 2x scale factor on windows, but we don't have to break it either.
Thank you for the review. Please take another look. http://codereview.chromium.org/10831207/diff/1002/chrome/browser/tab_contents... File chrome/browser/tab_contents/thumbnail_generator.cc (right): http://codereview.chromium.org/10831207/diff/1002/chrome/browser/tab_contents... chrome/browser/tab_contents/thumbnail_generator.cc:130: gfx::Display primary_display = gfx::Screen::GetPrimaryDisplay(); On 2012/08/09 02:11:02, oshima wrote: > Can you add comment > > TODO(oshima): Use device's default scale factor. > > This may not give the right scale factor. For example, > you can mirror the display on Arrow, which will most likely give you 1x scale > factor. I'm planning to add "device's default scale factor" to address this. Done. http://codereview.chromium.org/10831207/diff/1002/chrome/browser/tab_contents... chrome/browser/tab_contents/thumbnail_generator.cc:136: #if defined(OS_WIN) On 2012/08/09 02:11:02, oshima wrote: > Is it better to have "&& !defined(USE_AURA) "? > I know we don't officially support 2x scale factor on windows, but we don't have > to break it either. Done. Even with Aura, gfx::scrollbar_size() returns the size taking into account system DPI, so the returned size may be incorrect. Probably it's better to fix gfx::scrollbar_size() to handle system DPI and chrome's scale factor properly, but that can break other parts that use this function. I added a comment to revisit Win Aura case.
lgtm for non windows part. On 2012/08/09 17:07:38, mazda wrote: > Thank you for the review. Please take another look. > > http://codereview.chromium.org/10831207/diff/1002/chrome/browser/tab_contents... > File chrome/browser/tab_contents/thumbnail_generator.cc (right): > > http://codereview.chromium.org/10831207/diff/1002/chrome/browser/tab_contents... > chrome/browser/tab_contents/thumbnail_generator.cc:130: gfx::Display > primary_display = gfx::Screen::GetPrimaryDisplay(); > On 2012/08/09 02:11:02, oshima wrote: > > Can you add comment > > > > TODO(oshima): Use device's default scale factor. > > > > This may not give the right scale factor. For example, > > you can mirror the display on Arrow, which will most likely give you 1x scale > > factor. I'm planning to add "device's default scale factor" to address this. > > Done. > > http://codereview.chromium.org/10831207/diff/1002/chrome/browser/tab_contents... > chrome/browser/tab_contents/thumbnail_generator.cc:136: #if defined(OS_WIN) > On 2012/08/09 02:11:02, oshima wrote: > > Is it better to have "&& !defined(USE_AURA) "? > > I know we don't officially support 2x scale factor on windows, but we don't > have > > to break it either. > > Done. > Even with Aura, gfx::scrollbar_size() returns the size taking into account > system DPI, so the returned size may be incorrect. > Probably it's better to fix gfx::scrollbar_size() to handle system DPI and > chrome's scale factor properly, but that can break other parts that use this > function. > I added a comment to revisit Win Aura case. I see. If that's the case, I'm not sure what's the right thing to do on Windows. You probably should ask Ben or Scott. It also looks to me that this CL changes the behavior of Windows port when DPI scale is > 1.0, no? If so, please add sail@ to reviewers list.
lgtm w/nits (forgot to send other comments) http://codereview.chromium.org/10831207/diff/1002/chrome/browser/tab_contents... File chrome/browser/tab_contents/thumbnail_generator.cc (right): http://codereview.chromium.org/10831207/diff/1002/chrome/browser/tab_contents... chrome/browser/tab_contents/thumbnail_generator.cc:83: // factor in order to copy the pixels with the size above. I'm a bit confused by this comment. It thought the size in pixel is always 2 x kThumbnailWidth/Size in pixels, and that's what the following code is doing? http://codereview.chromium.org/10831207/diff/2003/chrome/browser/tab_contents... File chrome/browser/tab_contents/thumbnail_generator.cc (left): http://codereview.chromium.org/10831207/diff/2003/chrome/browser/tab_contents... chrome/browser/tab_contents/thumbnail_generator.cc:88: const gfx::Size& desired_size) { so this wasn't used? Just want to confirm.
Sorry for the belated reply. It turned out I didn't need to add if-defs for scrollbar size on Win. RenderWidgetHostViewWin::GetViewBounds returns view size in pixel and RenderWidgetHostViewWin::CopyFromBackingStore takes rect and size in pixel as arguments. On Win Aura, RenderWidgetHostViewAura::GetViewBounds also returns view size in pixel, and as DIP is not enabled as of now, RenderWidgetHostViewAura::CopyFromBackingStore takes rect and size in pixel as arguments. So, unless DIP gets enabled on Win Aura, we should use scrollbar size in pixel in all cases, which is returned by gfx::scrollbar_size(). I removed the code that depends on Windows system DPI settings in the latest patch set. > It also looks to me that this CL changes the behavior of Windows port when DPI scale is > 1.0, no? The latest patch set does not change the behavior of Windows port any longer. http://codereview.chromium.org/10831207/diff/1002/chrome/browser/tab_contents... File chrome/browser/tab_contents/thumbnail_generator.cc (right): http://codereview.chromium.org/10831207/diff/1002/chrome/browser/tab_contents... chrome/browser/tab_contents/thumbnail_generator.cc:83: // factor in order to copy the pixels with the size above. On 2012/08/10 02:01:15, oshima wrote: > I'm a bit confused by this comment. It thought the size in pixel is always 2 x > kThumbnailWidth/Size in pixels, and that's what the following code is doing? Yes, that's correct. RenderWidgetHost::CopyFromBackingStore takes the size argument in DIP, but here I'd like to ensure the data is copied in constant size in pixels (2 x kThumbnailWidth/Size) for the reasons written below in the comment. http://codereview.chromium.org/10831207/diff/2003/chrome/browser/tab_contents... File chrome/browser/tab_contents/thumbnail_generator.cc (left): http://codereview.chromium.org/10831207/diff/2003/chrome/browser/tab_contents... chrome/browser/tab_contents/thumbnail_generator.cc:88: const gfx::Size& desired_size) { On 2012/08/10 02:01:15, oshima wrote: > so this wasn't used? Just want to confirm. This function has not been used tentatively since the following CL http://src.chromium.org/viewvc/chrome?view=rev&revision=150398, but that's only until I'm adding the high DPI support in this CL.
+brettw Could you do an OWNERS review for chrome/browser/tab_contents?
http://codereview.chromium.org/10831207/diff/1006/chrome/browser/tab_contents... File chrome/browser/tab_contents/thumbnail_generator.cc (right): http://codereview.chromium.org/10831207/diff/1006/chrome/browser/tab_contents... chrome/browser/tab_contents/thumbnail_generator.cc:101: // display alleviates the aliasing. One thing that's not clear to me: In the case of a 100% scale, is this function a hack around the "scaling is poor" bug? Or do we want this behavior for other reasons? Should we change this when we resample better? http://codereview.chromium.org/10831207/diff/1006/chrome/browser/tab_contents... chrome/browser/tab_contents/thumbnail_generator.cc:115: LOG(WARNING) << "Unsupported scale factor. Use the same copy size as " Can you make this a DLOG to avoid shipping release builds with this code & string?
Thank you for the review. http://codereview.chromium.org/10831207/diff/1006/chrome/browser/tab_contents... File chrome/browser/tab_contents/thumbnail_generator.cc (right): http://codereview.chromium.org/10831207/diff/1006/chrome/browser/tab_contents... chrome/browser/tab_contents/thumbnail_generator.cc:101: // display alleviates the aliasing. On 2012/08/19 23:36:55, brettw wrote: > One thing that's not clear to me: In the case of a 100% scale, is this function > a hack around the "scaling is poor" bug? Or do we want this behavior for other > reasons? Should we change this when we resample better? Yes, that a hack around the "scaling is poor" bug. Ideally, I just want to read back 212x132 pixels in the case of a 100% scale and 424x264 pixels in the case of a 200% scale. Once http://crbug.com/141235 is fixed, I'll change this. http://codereview.chromium.org/10831207/diff/1006/chrome/browser/tab_contents... chrome/browser/tab_contents/thumbnail_generator.cc:115: LOG(WARNING) << "Unsupported scale factor. Use the same copy size as " On 2012/08/19 23:36:55, brettw wrote: > Can you make this a DLOG to avoid shipping release builds with this code & > string? Done.
Okay, can you clarify with a todo that this behavior should be changed once that other bug is fixed?
lgtm with that
On 2012/08/20 17:25:18, brettw wrote: > Okay, can you clarify with a todo that this behavior should be changed once that > other bug is fixed? I added a TODO comment for GetCopySizeForThumbnail. Thanks.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mazda@chromium.org/10831207/1008
Change committed as 152450 |
