Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2118853002:
Fix pixelation of tab borders when device scale changes (abandoned) (Closed)
|
Created:
4 years, 5 months ago by Greg Levin Modified:
3 years, 10 months ago CC:
chromium-reviews, tfarina, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix pixelation of tab borders when device scale changes
BUG=621139
TEST=Open browser, change device resolution (CTRL+SHIFT+ +/-), observe
quality of inactive tab borders.
NOTE: After some discussion in the comments, this CL was abandoned in favor of a simpler approach:
https://codereview.chromium.org/2246213003/
Patch Set 1 #Patch Set 2 : Use CanvasImageSource to draw at different scales #
Total comments: 11
Patch Set 3 : Move PaintBackgroundParams to cc file #Patch Set 4 : Address review #
Total comments: 12
Patch Set 5 : Address review 2 #
Total comments: 8
Patch Set 6 : Address review 3 #
Total comments: 2
Patch Set 7 : Merge #Patch Set 8 : Remove hc arg #
Total comments: 15
Patch Set 9 : Merge with recent fill_/stroke_canvas CL #
Total comments: 2
Patch Set 10 : Address review 4 #Patch Set 11 : Merge #
Messages
Total messages: 45 (24 generated)
glevin@chromium.org changed reviewers: + pkasting@chromium.org
Please have a look!
On 2016/07/01 15:47:53, Greg Levin wrote: > Please have a look! pkasting@ - Actually, hold off till the next patch. oshima indicates that the current version won't work on multiple monitors with different scale factors, and needs some modification.
glevin@chromium.org changed reviewers: + oshima@chromium.org
oshima@, please have another look! https://codereview.chromium.org/2118853002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2118853002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:431: SkColor stroke_color; I've removed cache as a differentiator of cache entries. Am I correct that one CanvasImageSource will handle all relevant scales? https://codereview.chromium.org/2118853002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1332: image_cache_->emplace_front(metadata, this, fill_id, y_offset); Previously, the image would be rendered here, then passed to the cache. Now, we only give the cache the needed parameters to construct a TabImageSource; the rendering will be done on demand by TabImageSource::Draw. https://codereview.chromium.org/2118853002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1439: PaintTabFill(canvas, fill_image, params.x_offset, params.y_offset, I'm a little concerned about this use of x_offset. As I understand the code, this block is triggered for non-MD, inactive, unhovered tabs. In that case, this function is being called by TabImageSource::Draw, where x_offset = 0, not a tab-specific value. On the other hand, it kinda looks like the image painted and cached here by the old version of the code was using an x_offset specific to the originating tab, so maybe it's fine.
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2118853002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2118853002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1368: gfx::ImageSkia* fill_image = params.tp->GetImageSkiaNamed(params.fill_id); instead of passing tp/fill_id etc, can you just pass fill_image, tab_color, and hover_color? https://codereview.chromium.org/2118853002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1374: !params.is_active && params.hc && params.hc->ShouldDraw(); you can also just pas nullptr if !ShouldDraw() https://codereview.chromium.org/2118853002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.h (right): https://codereview.chromium.org/2118853002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.h:58: struct PaintBackgroundParams { can you move this to private & .cc file? https://codereview.chromium.org/2118853002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.h:279: bool is_active); move private static member function to anonymous namespace in .cc
Please have another look! https://codereview.chromium.org/2118853002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2118853002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1368: gfx::ImageSkia* fill_image = params.tp->GetImageSkiaNamed(params.fill_id); On 2016/07/13 20:55:28, oshima wrote: > instead of passing tp/fill_id etc, can you just pass > > fill_image, tab_color, and hover_color? I'm still constructing TabImageSource with tab and fill_id, but I'm converting those into fill_image, toolbar_color, and background_color in the TabImageSource constructor, so that ThemeProvider* isn't stored in the struct. https://codereview.chromium.org/2118853002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1374: !params.is_active && params.hc && params.hc->ShouldDraw(); On 2016/07/13 20:55:28, oshima wrote: > you can also just pas nullptr if !ShouldDraw() Done. https://codereview.chromium.org/2118853002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.h (right): https://codereview.chromium.org/2118853002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.h:58: struct PaintBackgroundParams { On 2016/07/13 20:55:28, oshima wrote: > can you move this to private & .cc file? Done in Patch #3 ; moved to namespace in Patch #4. https://codereview.chromium.org/2118853002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.h:279: bool is_active); On 2016/07/13 20:55:28, oshima wrote: > move private static member function to anonymous namespace in .cc Done. Moved PaintTabBackgroundUsingFillId() in Patch #4, moved other stuff in in https://codereview.chromium.org/2150013002/. https://codereview.chromium.org/2118853002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (left): https://codereview.chromium.org/2118853002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:277: url.host() != chrome::kChromeUIAppLauncherPageHost; Removed by merge https://codereview.chromium.org/2118853002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2118853002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:126: gfx::ImageSkia* fill_image; Note sure if this should be a pointer, but was having trouble sorting out const's and constructors otherwise.
On 2016/07/01 16:04:05, Greg Levin (OOO 7.18-7.22) wrote: > On 2016/07/01 15:47:53, Greg Levin wrote: > > Please have a look! > > pkasting@ - Actually, hold off till the next patch. oshima indicates that the > current version won't work on multiple monitors with different scale factors, > and needs some modification. Are you wanting me to start looking at this now?
On 2016/07/18 18:05:29, Peter Kasting (slow) wrote: > On 2016/07/01 16:04:05, Greg Levin (OOO 7.18-7.22) wrote: > > On 2016/07/01 15:47:53, Greg Levin wrote: > > > Please have a look! > > > > pkasting@ - Actually, hold off till the next patch. oshima indicates that the > > current version won't work on multiple monitors with different scale factors, > > and needs some modification. > > Are you wanting me to start looking at this now? Let's let oshima@ have another look first, and make sure all the CanvasImageSource stuff is in order. Also, I'm OOO until next Monday, so there's no hurry.
https://codereview.chromium.org/2118853002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2118853002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:124: views::GlowHoverController* hc; I was hoping that we can resolve this w/o having a pointer here. I think what you need is * hover_location * hover_color * default_background_color You can have a bool flag to draw hover, or you can use the hover color's opacity to decide if it should draw it (but please ask owner) https://codereview.chromium.org/2118853002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:126: gfx::ImageSkia* fill_image; On 2016/07/15 22:24:48, Greg Levin wrote: > Note sure if this should be a pointer, but was having trouble sorting out > const's and constructors otherwise. You should be able to just use gfx::ImageSkia instance, not a pointer. ImageSkia uses ref-counted storage internally, so you don't have to worry about the memory cost. https://codereview.chromium.org/2118853002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:127: bool has_custom_image; can you use if the fill_image is null? https://codereview.chromium.org/2118853002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:129: int y_offset; gfx::Point offset;
oshima@- Please have another look! pkasting@- Things seem fairly stable, could you take a look now? Thanks! https://codereview.chromium.org/2118853002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2118853002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:124: views::GlowHoverController* hc; On 2016/07/26 21:11:26, oshima wrote: > I was hoping that we can resolve this w/o having a pointer here. > > I think what you need is > > * hover_location > * hover_color > * default_background_color > > You can have a bool flag to draw hover, or you can use the hover color's opacity > to decide if it should draw it (but please ask owner) As per offline discussion, made hc a separate arg for PaintTabBackgroundUsingFillId(). https://codereview.chromium.org/2118853002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:126: gfx::ImageSkia* fill_image; On 2016/07/26 21:11:26, oshima wrote: > On 2016/07/15 22:24:48, Greg Levin wrote: > > Note sure if this should be a pointer, but was having trouble sorting out > > const's and constructors otherwise. > > You should be able to just use gfx::ImageSkia instance, not a pointer. ImageSkia > uses ref-counted storage internally, so you don't have to worry about the memory > cost. Done. (Got the const issues worked out.) https://codereview.chromium.org/2118853002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:127: bool has_custom_image; On 2016/07/26 21:11:26, oshima wrote: > can you use if the fill_image is null? I don't think so. If I'm reading the code right, fill_image may be a built in resource, and has_custom_image == false because there's no theme. https://codereview.chromium.org/2118853002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:129: int y_offset; On 2016/07/26 21:11:26, oshima wrote: > gfx::Point offset; Done.
https://codereview.chromium.org/2118853002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2118853002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:127: bool has_custom_image; On 2016/07/28 15:48:04, Greg Levin wrote: > On 2016/07/26 21:11:26, oshima wrote: > > can you use if the fill_image is null? > > I don't think so. If I'm reading the code right, fill_image may be a built in > resource, and has_custom_image == false because there's no theme. can't you leave ImageSkia empty (isNull() return true) if has_custom_image is false? https://codereview.chromium.org/2118853002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2118853002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:128: gfx::Size size; Sorry I should have noticed this before, but looks like these two are for tab bounds? If so, we can just use gfx::Rect. https://codereview.chromium.org/2118853002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1620: params.fill_image = *GetThemeProvider()->GetImageSkiaNamed(fill_id); you can set this only if has_custom_image || !ui::MaterialDesignController::IsModeMaterial()
Please have another look! https://codereview.chromium.org/2118853002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2118853002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:127: bool has_custom_image; On 2016/07/28 16:56:06, oshima wrote: > On 2016/07/28 15:48:04, Greg Levin wrote: > > On 2016/07/26 21:11:26, oshima wrote: > > > can you use if the fill_image is null? > > > > I don't think so. If I'm reading the code right, fill_image may be a built in > > resource, and has_custom_image == false because there's no theme. > > can't you leave ImageSkia empty (isNull() return true) if has_custom_image is > false? I don't think so. fill_id, which is used to get fill_image, looks like it has a value regardless of has_custom_image: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab.cc?type... And here, the image gets used regardless of the value of has_custom_image: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab.cc?type... This might work once the non-MD code is gone, but for now I think these two values should be kept separate. We could ask pkasting@ for a more informed opinion than mine. https://codereview.chromium.org/2118853002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2118853002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:128: gfx::Size size; On 2016/07/28 16:56:06, oshima wrote: > Sorry I should have noticed this before, but looks like these two are for tab > bounds? If so, we can just use gfx::Rect. Is this a strong preference? I don't have a sense of how close the logical connection of these two are, and replacing these with "offset_rect" is going to wrap several more lines. Also, in some places where it's used, generic "size()" seems to make more sense than "offset_rect.size()". https://codereview.chromium.org/2118853002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1620: params.fill_image = *GetThemeProvider()->GetImageSkiaNamed(fill_id); On 2016/07/28 16:56:06, oshima wrote: > you can set this only if > has_custom_image || !ui::MaterialDesignController::IsModeMaterial() Done. (I see this is good from a resource saving perspective, but wasn't sure, from a code design perspective, if it was okay to tweak the parameters this specifically based on the internal workings of the function, rather than treating the function more as a black box.)
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
these are just my preference, and I'll defer to owners. https://codereview.chromium.org/2118853002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2118853002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:128: gfx::Size size; On 2016/07/28 20:03:54, Greg Levin wrote: > On 2016/07/28 16:56:06, oshima wrote: > > Sorry I should have noticed this before, but looks like these two are for tab > > bounds? If so, we can just use gfx::Rect. > > Is this a strong preference? I don't have a sense of how close the logical > connection of these two are, and replacing these with "offset_rect" is going to > wrap several more lines. Also, in some places where it's used, generic "size()" > seems to make more sense than "offset_rect.size()". It just looked to me that drawing code uses it as bounds. (DrawXxx(x, y, width, height)). https://codereview.chromium.org/2118853002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1620: params.fill_image = *GetThemeProvider()->GetImageSkiaNamed(fill_id); On 2016/07/28 20:03:54, Greg Levin wrote: > On 2016/07/28 16:56:06, oshima wrote: > > you can set this only if > > has_custom_image || !ui::MaterialDesignController::IsModeMaterial() > > Done. > (I see this is good from a resource saving perspective, but wasn't sure, from a > code design perspective, if it was okay to tweak the parameters this > specifically based on the internal workings of the function, rather than > treating the function more as a black box.) It wasn't for saving resource. It's creating a image even though we know it will not be used, and that's what I feel odd about. https://codereview.chromium.org/2118853002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2118853002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1623: params.has_custom_image = has_custom_image; you should be able to use fill_image.isNull() to check if it should be drawn.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #7 (id:120001) has been deleted
Here's the latest version, please have another look! Patch Set 7: Merged with changes from https://codereview.chromium.org/2197613002/ Patch Set 8: pkasting@ - apologies, in the last revision of that other CL, I totally missed this comment and the related change. This next patch fixes that. >>> Given that, I suggest making the hover controller part of the params, and >>> adding a constructor for the params struct, which you can use here too. >>> I don't think we lose a lot of readability converting the separate named >>> field assignments into arguments to a constructor call, since the list of >>> argument names is above in this same file to reference. > > [...] > > So I would either put it back in the params, or I would not pass it at all > and find a way to pass the relevant info to the function. (I don't know > whether that's terribly feasible, and I wouldn't spend a lot of time and > effort trying to do it.) I pulled info out of hc, and made it part of params. https://codereview.chromium.org/2118853002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2118853002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:128: gfx::Size size; On 2016/07/28 20:45:48, oshima wrote: > On 2016/07/28 20:03:54, Greg Levin wrote: > > On 2016/07/28 16:56:06, oshima wrote: > > > Sorry I should have noticed this before, but looks like these two are for > tab > > > bounds? If so, we can just use gfx::Rect. > > > > Is this a strong preference? I don't have a sense of how close the logical > > connection of these two are, and replacing these with "offset_rect" is going > to > > wrap several more lines. Also, in some places where it's used, generic > "size()" > > seems to make more sense than "offset_rect.size()". > > It just looked to me that drawing code uses it as bounds. (DrawXxx(x, y, width, > height)). Done. https://codereview.chromium.org/2118853002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1620: params.fill_image = *GetThemeProvider()->GetImageSkiaNamed(fill_id); On 2016/07/28 20:45:48, oshima wrote: > On 2016/07/28 20:03:54, Greg Levin wrote: > > On 2016/07/28 16:56:06, oshima wrote: > > > you can set this only if > > > has_custom_image || !ui::MaterialDesignController::IsModeMaterial() > > > > Done. > > (I see this is good from a resource saving perspective, but wasn't sure, from > a > > code design perspective, if it was okay to tweak the parameters this > > specifically based on the internal workings of the function, rather than > > treating the function more as a black box.) > > It wasn't for saving resource. It's creating a image even though we know it will > not be used, and that's what I feel odd about. > Acknowledged. https://codereview.chromium.org/2118853002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2118853002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1623: params.has_custom_image = has_custom_image; On 2016/07/28 20:45:48, oshima wrote: > you should be able to use fill_image.isNull() to check if it should be drawn. Done.
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Basically LGTM, my biggest question is simply the first one below about why the existing code was buggy. https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (left): https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1576: ui::GetSupportedScaleFactor(canvas->image_scale()), size()); Fundamentally, why didn't this prevent the bug from occurring? It seems like when the scale factor changed, this value should have changed, and we should have thrown out the cached image. https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:156: const PaintBackgroundParams& params); Nit: Maybe this function should have gone here?... https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:162: // once and rendered correctly for all scale factors. Nit: Maybe "...inactive tab image to be cached once for each requested scale factor."? Since we don't really cache the image "once" across all scale factors. https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:168: // gfx::CanvasImageSource override. I think you should also override HasRepresentationAtAllScales() to return true? (I'm not terribly familiar with ImageSkiaSource.) https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:260: image(new TabImageSource(tab, fill_id, y_offset), tab->size()) {} I don't suppose you'd like to write a followup cleanup CL that makes ImageSkia (and ImageSkiaStorage) take a unique_ptr instead of a raw pointer here, so reviewers didn't have to worry about whether this was a memory leak? https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:556: DrawHighlight(canvas, params.hover_location * SkFloatToScalar(scale), I think you want to omit the SkFloatToScalar() call here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pkasting@, could you have a look at this merge with https://codereview.chromium.org/2210033002/? I want to see if what I'm doing here makes sense to you. There are TabImageSources for fill and stroke images (when separate), and TabImageSource keeps track of which one(s) it is. TabImageSource::Draw() then passes its canvas arg to the correct canvas(es) in PaintTabBackgroundUsingParams(). What's the easiest way to put the Linux desktop version of Chrome OS into tab stacking mode? My merge works fine with normal tabs, but I haven't tested it yet with stacked tabs. I'll address your latest round of comments in the next patch.
I think this approach works.
I wonder if we should split the tab background-painting function into two
pieces, one for fill and one for stroke. Then instead of ?:s on the caller side
and ifs inside the function, we'd have something more like
if (use_canvas_ != STROKE)
PaintTabFill(canvas);
if (use_canvas_ != FILL)
PaintTabStroke(canvas);
We would only put the pre-MD code into one of these two functions. Even though
right now it's drawing to the stroke canvas, I would probably put it in the fill
function, because the MD fill code needs to compute the same hover params as the
pre-MD code. This doesn't pose a problem; we can just make the fill canvas and
fill image be the one that gets used in the cases where we're only using one.
On the whole this wouldn't be a very big change, but I think it would reduce
indenting a bit, shorten function lengths, and make things slightly easier to
reason about for a reader.
https://codereview.chromium.org/2118853002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab.cc (right):
https://codereview.chromium.org/2118853002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab.cc:200:
PaintTabBackgroundUsingParams(use_canvas_ != STROKE ? canvas : nullptr,
Nit: Prefer "use_canvas_ == STROKE ? nullptr : canvas", so the "else" portion of
the ?: doesn't read mentally like a double-negative.
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addressed all recent comments, please have another look! Also, could you let me know what flags, etc I need to set in order to put the Linux desktop version of Chrome OS into tab stacking mode? I haven't yet tested the case where fill and stroke are drawn on different canvases, because I don't know how to access it. > I wonder if we should split the tab background-painting function into two > pieces, one for fill and one for stroke. Then instead of ?:s on the caller side > and ifs inside the function, we'd have something more like > > if (use_canvas_ != STROKE) > PaintTabFill(canvas); > if (use_canvas_ != FILL) > PaintTabStroke(canvas); Done (named PaintTabFill/StrokeUsingParams since we already have a PaintTabFill()). https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (left): https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1576: ui::GetSupportedScaleFactor(canvas->image_scale()), size()); On 2016/08/10 21:02:03, Peter Kasting wrote: > Fundamentally, why didn't this prevent the bug from occurring? It seems like > when the scale factor changed, this value should have changed, and we should > have thrown out the cached image. In the case where the device / native scale factor is 1.25 (e.g., --ash-host-window-bounds=1920x1080*1.25), canvas->image_scale() returns 1.25 at default resolution, and 1 the rest of the time (under all other CTRL+SHIFT+ +/- induced zooms). So most of the time, zooming doesn't change the canvas's scale. More to the point, ui::GetSupportedScaleFactor(1.25) = SCALE_FACTOR_100P, so this quantity was constant under all zooms, and had no effect on metadata differentiation. (I don't know why we don't get ui::GetSupportedScaleFactor(1.25) = SCALE_FACTOR_125P I don't know much about scaling code, and didn't dig any further. oshima@ knows a lot more about this stuff.) https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:156: const PaintBackgroundParams& params); On 2016/08/10 21:02:03, Peter Kasting wrote: > Nit: Maybe this function should have gone here?... I'll move it (well, "them" now) if you think that't better. I put it where I did for consistency... to keep structs and classes together above, and non-member paint utility functions together below. I felt the extra declaration was worth keeping like content grouped in the file. https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:162: // once and rendered correctly for all scale factors. On 2016/08/10 21:02:03, Peter Kasting wrote: > Nit: Maybe "...inactive tab image to be cached once for each requested scale > factor."? Since we don't really cache the image "once" across all scale > factors. Done. https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:168: // gfx::CanvasImageSource override. On 2016/08/10 21:02:03, Peter Kasting wrote: > I think you should also override HasRepresentationAtAllScales() to return true? > (I'm not terribly familiar with ImageSkiaSource.) Done. https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:260: image(new TabImageSource(tab, fill_id, y_offset), tab->size()) {} On 2016/08/10 21:02:03, Peter Kasting wrote: > I don't suppose you'd like to write a followup cleanup CL that makes ImageSkia > (and ImageSkiaStorage) take a unique_ptr instead of a raw pointer here, so > reviewers didn't have to worry about whether this was a memory leak? I'll ask oshima@ about it... https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:556: DrawHighlight(canvas, params.hover_location * SkFloatToScalar(scale), On 2016/08/10 21:02:03, Peter Kasting wrote: > I think you want to omit the SkFloatToScalar() call here. Done. https://codereview.chromium.org/2118853002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2118853002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:200: PaintTabBackgroundUsingParams(use_canvas_ != STROKE ? canvas : nullptr, On 2016/08/11 22:51:15, Peter Kasting wrote: > Nit: Prefer "use_canvas_ == STROKE ? nullptr : canvas", so the "else" portion of > the ?: doesn't read mentally like a double-negative. Obsolete due to other change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
To test stacked tab mode if you don't have a touch input device, you need to hack up the code. I think all you need to do is make TabStrip::SetStackedLayout() a no-op and force |stacked_layout_| in that class to true. Then just add enough tabs to the strip and/or shrink the strip width until the browser begins stacking them. As soon as that happens, you'll be in split-canvas mode. https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (left): https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1576: ui::GetSupportedScaleFactor(canvas->image_scale()), size()); On 2016/08/12 19:48:15, Greg Levin wrote: > On 2016/08/10 21:02:03, Peter Kasting wrote: > > Fundamentally, why didn't this prevent the bug from occurring? It seems like > > when the scale factor changed, this value should have changed, and we should > > have thrown out the cached image. > > In the case where the device / native scale factor is 1.25 (e.g., > --ash-host-window-bounds=1920x1080*1.25), > canvas->image_scale() returns 1.25 at default resolution, and 1 the rest of the > time (under all other CTRL+SHIFT+ +/- induced zooms). So most of the time, > zooming doesn't change the canvas's scale. More to the point, > ui::GetSupportedScaleFactor(1.25) = SCALE_FACTOR_100P, so this quantity was > constant under all zooms, and had no effect on metadata differentiation. Would just removing the call to GetSupportedScaleFactor() (and thus using image_scale() directly) fix all the problems, or would we still be broken (or be broken in a new way) when zooming with ctrl-shift-+/-? (And if broken, how many other parts of the browser chrome are broken in that case?) > (I don't know why we don't get > ui::GetSupportedScaleFactor(1.25) = SCALE_FACTOR_125P > I don't know much about scaling code, and didn't dig any further. oshima@ knows > a lot more about this stuff.) It might be good to get his answer on this if the route above doesn't work as a solution here, since maybe making that change would help?
pkasting@- let me know if my most recent replies sufficiently address your
concerns, or if there's any further work you'd like done.
regarding:
260 image(new TabImageSource(tab, fill_id, y_offset), tab->size()) {}
Peter Kasting 2016/08/10 21:02:03
> I don't suppose you'd like to write a followup cleanup CL that makes ImageSkia
> (and ImageSkiaStorage) take a unique_ptr instead of a raw pointer here, so
> reviewers didn't have to worry about whether this was a memory leak?
I filed https://bugs.chromium.org/p/chromium/issues/detail?id=637898 to address
this, and will start on it as soon as this CL lands.
https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab.cc (left):
https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab.cc:1576:
ui::GetSupportedScaleFactor(canvas->image_scale()), size());
On 2016/08/13 06:38:33, Peter Kasting wrote:
> On 2016/08/12 19:48:15, Greg Levin wrote:
> > On 2016/08/10 21:02:03, Peter Kasting wrote:
> > > Fundamentally, why didn't this prevent the bug from occurring? It seems
> like
> > > when the scale factor changed, this value should have changed, and we
should
> > > have thrown out the cached image.
> >
> > In the case where the device / native scale factor is 1.25 (e.g.,
> > --ash-host-window-bounds=1920x1080*1.25),
> > canvas->image_scale() returns 1.25 at default resolution, and 1 the rest of
> the
> > time (under all other CTRL+SHIFT+ +/- induced zooms). So most of the time,
> > zooming doesn't change the canvas's scale. More to the point,
> > ui::GetSupportedScaleFactor(1.25) = SCALE_FACTOR_100P, so this quantity was
> > constant under all zooms, and had no effect on metadata differentiation.
>
> Would just removing the call to GetSupportedScaleFactor() (and thus using
> image_scale() directly) fix all the problems, or would we still be broken (or
be
> broken in a new way) when zooming with ctrl-shift-+/-? (And if broken, how
many
> other parts of the browser chrome are broken in that case?)
GetSupportedScaleFactor() is somewhat misleadingly named, and I misunderstood
its purpose. It doesn't refer to device scales, but to image resource scales.
That is, we have image resources at 100% and 200%, and those are the supported
scales. Since our pre-rendered resources only come in those two scales, it
seems we want to stick to those two for these dynamically generated resources as
well. So ui::GetSupportedScaleFactor(1.25) = SCALE_FACTOR_100P just means that
100% is a better match than 200% for device scaling of 125%.
> > (I don't know why we don't get
> > ui::GetSupportedScaleFactor(1.25) = SCALE_FACTOR_125P
> > I don't know much about scaling code, and didn't dig any further. oshima@
> knows
> > a lot more about this stuff.)
>
> It might be good to get his answer on this if the route above doesn't work as
a
> solution here, since maybe making that change would help?
Providing images at different scales is the purpose of CanvasImageSource, and
was oshima@'s recommendation for fixing this bug.
Does the above satisfy your concerns here? Is there anything further you'd like
me to ask or do?
The CQ bit was checked by glevin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (left): https://codereview.chromium.org/2118853002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1576: ui::GetSupportedScaleFactor(canvas->image_scale()), size()); On 2016/08/15 18:40:18, Greg Levin wrote: > On 2016/08/13 06:38:33, Peter Kasting wrote: > > On 2016/08/12 19:48:15, Greg Levin wrote: > > > On 2016/08/10 21:02:03, Peter Kasting wrote: > > > > Fundamentally, why didn't this prevent the bug from occurring? It seems > > like > > > > when the scale factor changed, this value should have changed, and we > should > > > > have thrown out the cached image. > > > > > > In the case where the device / native scale factor is 1.25 (e.g., > > > --ash-host-window-bounds=1920x1080*1.25), > > > canvas->image_scale() returns 1.25 at default resolution, and 1 the rest of > > the > > > time (under all other CTRL+SHIFT+ +/- induced zooms). So most of the time, > > > zooming doesn't change the canvas's scale. More to the point, > > > ui::GetSupportedScaleFactor(1.25) = SCALE_FACTOR_100P, so this quantity was > > > constant under all zooms, and had no effect on metadata differentiation. > > > > Would just removing the call to GetSupportedScaleFactor() (and thus using > > image_scale() directly) fix all the problems, or would we still be broken (or > be > > broken in a new way) when zooming with ctrl-shift-+/-? (And if broken, how > many > > other parts of the browser chrome are broken in that case?) > > GetSupportedScaleFactor() is somewhat misleadingly named, and I misunderstood > its purpose. It doesn't refer to device scales, but to image resource scales. > That is, we have image resources at 100% and 200%, and those are the supported > scales. Since our pre-rendered resources only come in those two scales, it > seems we want to stick to those two for these dynamically generated resources as > well. So ui::GetSupportedScaleFactor(1.25) = SCALE_FACTOR_100P just means that > 100% is a better match than 200% for device scaling of 125%. Ah! That's interesting. That means at least for MD, we should definitely drop the GetSupportedScaleFactor call, since we're not using image resources to draw the tab edges. I would try just doing that and seeing if it fixes this bug. If so, we should maybe undo parts of your previous change to introduce the params struct. For pre-MD we maybe should keep using this, but honestly pre-MD is now dead and we don't care about fixing bugs in it, so I wouldn't worry about trying to special-case it.
Description was changed from ========== Fix pixelation of tab borders when device scale changes BUG=621139 TEST=Open browser, change device resolution (CTRL+SHIFT+ +/-), observe quality of inactive tab borders. ========== to ========== Fix pixelation of tab borders when device scale changes BUG=621139 TEST=Open browser, change device resolution (CTRL+SHIFT+ +/-), observe quality of inactive tab borders. NOTE: After some discussion in the comments, this CL was abandoned in favor of a simpler approach: https://codereview.chromium.org/2246213003/ ==========
Closing. FYI, if you want to abandon a CL, be sure to explicitly close it (with the (X) button next to the issue title) so it doesn't stay in people's queues. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
