Fix pixelation of tab borders when device scale changes (alternate approach)

Fix pixelation of tab borders when device scale changes (alternate approach)

BUG=621139
TEST=Open browser, change device resolution (CTRL+SHIFT+ +/-), observe
quality of inactive tab borders.

Note that this is a simpler fix than
https://codereview.chromium.org/2118853002/
(assuming it's valid)

Committed: https://crrev.com/e99b9c69a63d7857dd397ae2e488a16cc8e7c48d
Cr-Commit-Position: refs/heads/master@{#412234}

[Greg Levin, 2016-08-15 23:04:53]

glevin@chromium.org changed reviewers:
+ oshima@chromium.org, pkasting@chromium.org

[Greg Levin, 2016-08-15 23:04:53]

@oshima- pkasting@ was (I believe) suggesting something along these lines in
this comment:

https://codereview.chromium.org/2118853002/#msg43

The first patch in that CL was abandoned (I believe) because it wouldn't support
multiple monitors at different scales.  This CL does cache multiple image scales
simultaneously, but I don't have a sense (without testing) of whether it might
solve the multiple monitor situation.

Could you please have a look, and provide some commentary about the relative
merits of these two approaches?  Thanks!

[Peter Kasting, 2016-08-15 23:07:39]

LGTM

https://codereview.chromium.org/2246213003/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2246213003/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab.cc:187: DCHECK_NE(0.0, scale_factor);
Nit: Technically 0.0 is a double.  Honestly I'd rather just ditch this DCHECK
since I believe scale 0 won't actually cause any problems in called code.

[oshima, 2016-08-15 23:21:46]

On 2016/08/15 23:04:53, Greg Levin wrote:
> @oshima- pkasting@ was (I believe) suggesting something along these lines in
> this comment:
> 
> https://codereview.chromium.org/2118853002/#msg43
> 
> The first patch in that CL was abandoned (I believe) because it wouldn't
support
> multiple monitors at different scales.
>  This CL does cache multiple image scales
> simultaneously, but I don't have a sense (without testing) of whether it might
> solve the multiple monitor situation.
> 
> Could you please have a look, and provide some commentary about the relative
> merits of these two approaches?  Thanks!

Using ImageSource is the standard way to handle scale factors in ImageSkia. I
still recommend
to follow the same pattern because that's how it's done in many other places and
will likely
to prevent a regression in the future. That's being said, I'm not the owner, so
if pkasting@
feels strongly about it, then I'm fine.

[Peter Kasting, 2016-08-15 23:33:28]

On 2016/08/15 23:21:46, oshima wrote:
> On 2016/08/15 23:04:53, Greg Levin wrote:
> > @oshima- pkasting@ was (I believe) suggesting something along these lines in
> > this comment:
> > 
> > https://codereview.chromium.org/2118853002/#msg43
> > 
> > The first patch in that CL was abandoned (I believe) because it wouldn't
> support
> > multiple monitors at different scales.
> >  This CL does cache multiple image scales
> > simultaneously, but I don't have a sense (without testing) of whether it
might
> > solve the multiple monitor situation.
> > 
> > Could you please have a look, and provide some commentary about the relative
> > merits of these two approaches?  Thanks!
> 
> Using ImageSource is the standard way to handle scale factors in ImageSkia. I
> still recommend
> to follow the same pattern because that's how it's done in many other places
and
> will likely
> to prevent a regression in the future. That's being said, I'm not the owner,
so
> if pkasting@
> feels strongly about it, then I'm fine.

ImageSource is not a good fit for how this code works.  Basically, we're trying
to programmatically draw directly to the screen.  Trying to shoehorn in
ImageSource makes the code significantly more complicated and isn't going to
make any kind of regression less likely.

[oshima, 2016-08-16 00:05:32]

On 2016/08/15 23:33:28, Peter Kasting wrote:
> On 2016/08/15 23:21:46, oshima wrote:
> > On 2016/08/15 23:04:53, Greg Levin wrote:
> > > @oshima- pkasting@ was (I believe) suggesting something along these lines
in
> > > this comment:
> > > 
> > > https://codereview.chromium.org/2118853002/#msg43
> > > 
> > > The first patch in that CL was abandoned (I believe) because it wouldn't
> > support
> > > multiple monitors at different scales.
> > >  This CL does cache multiple image scales
> > > simultaneously, but I don't have a sense (without testing) of whether it
> might
> > > solve the multiple monitor situation.
> > > 
> > > Could you please have a look, and provide some commentary about the
relative
> > > merits of these two approaches?  Thanks!
> > 
> > Using ImageSource is the standard way to handle scale factors in ImageSkia.
I
> > still recommend
> > to follow the same pattern because that's how it's done in many other places
> and
> > will likely
> > to prevent a regression in the future. That's being said, I'm not the owner,
> so
> > if pkasting@
> > feels strongly about it, then I'm fine.
> 
> ImageSource is not a good fit for how this code works.  Basically, we're
trying
> to programmatically draw directly to the screen.  Trying to shoehorn in
> ImageSource makes the code significantly more complicated and isn't going to
> make any kind of regression less likely.

CanvasImageSource is exactly for that purpose. (ex:
https://cs.chromium.org/chromium/src/ui/chromeos/network/network_icon.cc?rcl=...)
I'm still fine if you prefer this way (and I understand that this requires less
change).

[Peter Kasting, 2016-08-16 00:15:52]

On 2016/08/16 00:05:32, oshima wrote:
> On 2016/08/15 23:33:28, Peter Kasting wrote:
> > On 2016/08/15 23:21:46, oshima wrote:
> > > On 2016/08/15 23:04:53, Greg Levin wrote:
> > > > @oshima- pkasting@ was (I believe) suggesting something along these
lines
> in
> > > > this comment:
> > > > 
> > > > https://codereview.chromium.org/2118853002/#msg43
> > > > 
> > > > The first patch in that CL was abandoned (I believe) because it wouldn't
> > > support
> > > > multiple monitors at different scales.
> > > >  This CL does cache multiple image scales
> > > > simultaneously, but I don't have a sense (without testing) of whether it
> > might
> > > > solve the multiple monitor situation.
> > > > 
> > > > Could you please have a look, and provide some commentary about the
> relative
> > > > merits of these two approaches?  Thanks!
> > > 
> > > Using ImageSource is the standard way to handle scale factors in
ImageSkia.
> I
> > > still recommend
> > > to follow the same pattern because that's how it's done in many other
places
> > and
> > > will likely
> > > to prevent a regression in the future. That's being said, I'm not the
owner,
> > so
> > > if pkasting@
> > > feels strongly about it, then I'm fine.
> > 
> > ImageSource is not a good fit for how this code works.  Basically, we're
> trying
> > to programmatically draw directly to the screen.  Trying to shoehorn in
> > ImageSource makes the code significantly more complicated and isn't going to
> > make any kind of regression less likely.
> 
> CanvasImageSource is exactly for that purpose.

Right, and that's what Greg used.  But this isn't a simple matter of
programmatically drawing one image per scale factor.  We have a whole
constellation of different conditions that mean sometimes we're drawing into one
canvas and sometimes into two, sometimes we need to throw away or redraw the
cached value even when the scale factor doesn't change, etc.

If the only thing changing was the scale, and thus CanvasImageSource was
managing all the caching itself, it'd be right.  But basically we have to build
out our own complicated drawing and caching system anyway, so it is better not
to also be using another caching system for one piece of that.

[oshima, 2016-08-16 01:00:19]

On 2016/08/16 00:15:52, Peter Kasting wrote:
> On 2016/08/16 00:05:32, oshima wrote:
> > On 2016/08/15 23:33:28, Peter Kasting wrote:
> > > On 2016/08/15 23:21:46, oshima wrote:
> > > > On 2016/08/15 23:04:53, Greg Levin wrote:
> > > > > @oshima- pkasting@ was (I believe) suggesting something along these
> lines
> > in
> > > > > this comment:
> > > > > 
> > > > > https://codereview.chromium.org/2118853002/#msg43
> > > > > 
> > > > > The first patch in that CL was abandoned (I believe) because it
wouldn't
> > > > support
> > > > > multiple monitors at different scales.
> > > > >  This CL does cache multiple image scales
> > > > > simultaneously, but I don't have a sense (without testing) of whether
it
> > > might
> > > > > solve the multiple monitor situation.
> > > > > 
> > > > > Could you please have a look, and provide some commentary about the
> > relative
> > > > > merits of these two approaches?  Thanks!
> > > > 
> > > > Using ImageSource is the standard way to handle scale factors in
> ImageSkia.
> > I
> > > > still recommend
> > > > to follow the same pattern because that's how it's done in many other
> places
> > > and
> > > > will likely
> > > > to prevent a regression in the future. That's being said, I'm not the
> owner,
> > > so
> > > > if pkasting@
> > > > feels strongly about it, then I'm fine.
> > > 
> > > ImageSource is not a good fit for how this code works.  Basically, we're
> > trying
> > > to programmatically draw directly to the screen.  Trying to shoehorn in
> > > ImageSource makes the code significantly more complicated and isn't going
to
> > > make any kind of regression less likely.
> > 
> > CanvasImageSource is exactly for that purpose.
> 
> Right, and that's what Greg used.  But this isn't a simple matter of
> programmatically drawing one image per scale factor.  We have a whole
> constellation of different conditions that mean sometimes we're drawing into
one
> canvas and sometimes into two, sometimes we need to throw away or redraw the
> cached value even when the scale factor doesn't change, etc.
> 
> If the only thing changing was the scale, and thus CanvasImageSource was
> managing all the caching itself, it'd be right.  But basically we have to
build
> out our own complicated drawing and caching system anyway, so it is better not
> to also be using another caching system for one piece of that.

The example above does somewhat similar thing (it creates different images based
on parameter passed,
and they're cached in the client side for the key they defined).

In any case, itt's not important enough to block this, so lgtm (in case you're
waiting, Greg).

[Greg Levin, 2016-08-16 14:05:11]

The CQ bit was checked by glevin@chromium.org

[Greg Levin, 2016-08-16 14:05:12]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2246213003/#ps20001
(title: "Remove DCHECK")

[commit-bot: I haz the power, 2016-08-16 14:05:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 14:41:42]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-16 14:43:15]

Description was changed from

==========
Fix pixelation of tab borders when device scale changes (alternate approach)

BUG=621139
TEST=Open browser, change device resolution (CTRL+SHIFT+ +/-), observe
quality of inactive tab borders.

Note that this is a simpler fix than
https://codereview.chromium.org/2118853002/
(assuming it's valid)
==========

to

==========
Fix pixelation of tab borders when device scale changes (alternate approach)

BUG=621139
TEST=Open browser, change device resolution (CTRL+SHIFT+ +/-), observe
quality of inactive tab borders.

Note that this is a simpler fix than
https://codereview.chromium.org/2118853002/
(assuming it's valid)

Committed: https://crrev.com/e99b9c69a63d7857dd397ae2e488a16cc8e7c48d
Cr-Commit-Position: refs/heads/master@{#412234}
==========

[commit-bot: I haz the power, 2016-08-16 14:43:16]

Patchset 2 (id:??) landed as
https://crrev.com/e99b9c69a63d7857dd397ae2e488a16cc8e7c48d
Cr-Commit-Position: refs/heads/master@{#412234}