cros: Fix browser header wrongly drawn frame image

cros: Fix browser header wrongly drawn frame image

Changes:
Check the code before https://codereview.chromium.org/2770943002, found that when frame_overlay_image_.isNull(), TileImageInt should be called.

BUG=704636
TEST=test that bug doesn't happen with theme installed

Review-Url: https://codereview.chromium.org/2770943004
Cr-Commit-Position: refs/heads/master@{#460265}
Committed: https://chromium.googlesource.com/chromium/src/+/b30c149bc6b38870ca74cb8dcd5fb34e90e75480

[Qiang(Joe) Xu, 2017-03-23 23:04:33]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 23:05:22]

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

[Qiang(Joe) Xu, 2017-03-23 23:23:51]

Description was changed from

==========
cros: Fix browser header wrongly drawn frame image

BUG=
==========

to

==========
cros: Fix browser header wrongly drawn frame image

BUG=704636
TEST=test that bug doesn't happen with theme installed
==========

[Qiang(Joe) Xu, 2017-03-23 23:23:52]

warx@chromium.org changed reviewers:
+ danakj@chromium.org, pkasting@chromium.org

[Qiang(Joe) Xu, 2017-03-23 23:32:47]

Description was changed from

==========
cros: Fix browser header wrongly drawn frame image

BUG=704636
TEST=test that bug doesn't happen with theme installed
==========

to

==========
cros: Fix browser header wrongly drawn frame image

Changes:
Check the code before https://codereview.chromium.org/2770943002, found that
when frame_overlay_image_.isNull(), TileImageInt should be called. I think we
don't need the middle frame_overlay_image.isNull() condition, it should be
covered within else{}.

BUG=704636
TEST=test that bug doesn't happen with theme installed
==========

[Qiang(Joe) Xu, 2017-03-23 23:33:27]

Hi all, PTAL, thanks!

[commit-bot: I haz the power, 2017-03-23 23:42:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 23:42:17]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-03-23 23:51:22]

LGTM.  The key here is that DrawImageInPath(), which was used before, looks like
a "draw" call but is actually a "tile" call.

I sorta wish we had a good way to regression test some of this stuff.  I wonder
if it would make sense to do a pixel test for window frame drawing with some
different custom themes.

[danakj, 2017-03-24 14:59:50]

On 2017/03/23 23:51:22, Peter Kasting wrote:
> LGTM.  The key here is that DrawImageInPath(), which was used before, looks
like
> a "draw" call but is actually a "tile" call.
> 
> I sorta wish we had a good way to regression test some of this stuff.  I
wonder
> if it would make sense to do a pixel test for window frame drawing with some
> different custom themes.

I think this would be a good idea. Kinda like layout tests for UI. Ideally with
a low bar for rebaselining but at least you can acknowledge what's changing esp
in review.

[danakj, 2017-03-24 14:59:54]

https://codereview.chromium.org/2770943004/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_header_painter_ash.cc (right):

https://codereview.chromium.org/2770943004/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:84:
canvas->SaveLayerWithFlags(flags);
SaveLayer isn't free, we should keep the middle branch IMO and and change it to
TileImageInt ?

[Qiang(Joe) Xu, 2017-03-24 16:17:52]

Description was changed from

==========
cros: Fix browser header wrongly drawn frame image

Changes:
Check the code before https://codereview.chromium.org/2770943002, found that
when frame_overlay_image_.isNull(), TileImageInt should be called. I think we
don't need the middle frame_overlay_image.isNull() condition, it should be
covered within else{}.

BUG=704636
TEST=test that bug doesn't happen with theme installed
==========

to

==========
cros: Fix browser header wrongly drawn frame image

Changes:
Check the code before https://codereview.chromium.org/2770943002, found that
when frame_overlay_image_.isNull(), TileImageInt should be called.

BUG=704636
TEST=test that bug doesn't happen with theme installed
==========

[Qiang(Joe) Xu, 2017-03-24 16:22:05]

Hi all, please recheck the new patch. About the test coverage, we can file a bug
to track it.

https://codereview.chromium.org/2770943004/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_header_painter_ash.cc (right):

https://codereview.chromium.org/2770943004/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:84:
canvas->SaveLayerWithFlags(flags);
On 2017/03/24 14:59:54, danakj wrote:
> SaveLayer isn't free, we should keep the middle branch IMO and and change it
to
> TileImageInt ?

Done.

[danakj, 2017-03-24 16:29:31]

https://codereview.chromium.org/2770943004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_header_painter_ash.cc (right):

https://codereview.chromium.org/2770943004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:84:
canvas->TileImageInt(frame_image, image_inset_x, 0, 0, 0, bounds.width(),
pkasting should confirm the -inset to +inset is right.. but LGTM in spirit.

[Peter Kasting, 2017-03-24 20:08:13]

https://codereview.chromium.org/2770943004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_header_painter_ash.cc (right):

https://codereview.chromium.org/2770943004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:84:
canvas->TileImageInt(frame_image, image_inset_x, 0, 0, 0, bounds.width(),
On 2017/03/24 16:29:30, danakj wrote:
> pkasting should confirm the -inset to +inset is right.. but LGTM in spirit.

The coord change is fine (changes from dest to source coords), but the
functionality isn't: TileImageInt() doesn't take a flags arg, which is why we
had to SaveLayer in the other arm to begin with.

Either we can go back to the simpler code, or we can try to work around this,
maybe by manually implementing the tile call here, or by mucking with the canvas
APIs.

Unless we know that SaveLayer is really costly and this has a measurable perf or
battery effect, my preference would be the former.

[danakj, 2017-03-24 20:22:52]

https://codereview.chromium.org/2770943004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_header_painter_ash.cc (right):

https://codereview.chromium.org/2770943004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:84:
canvas->TileImageInt(frame_image, image_inset_x, 0, 0, 0, bounds.width(),
On 2017/03/24 20:08:12, Peter Kasting wrote:
> On 2017/03/24 16:29:30, danakj wrote:
> > pkasting should confirm the -inset to +inset is right.. but LGTM in spirit.
> 
> The coord change is fine (changes from dest to source coords), but the
> functionality isn't: TileImageInt() doesn't take a flags arg, which is why we
> had to SaveLayer in the other arm to begin with.

Oh oops. Using the underlying PaintCanvas directly would be fine (that's where
we're trying to head anyway).

> Either we can go back to the simpler code, or we can try to work around this,
> maybe by manually implementing the tile call here, or by mucking with the
canvas
> APIs.
> 
> Unless we know that SaveLayer is really costly and this has a measurable perf
or
> battery effect, my preference would be the former.

save layer is similar to what this did before, it draws to an intermediate
target then draws that to the primary target. So it requires extra memory
bandwidth, and possible allocations if not using a cached memory chunk.

I suggest we add PaintFlags to (an override of?) TileImageInt. In
Canvas::TileImageInt we use that and add the shader to it. The setBlendMode
should be removed (it's the default blend mode anyway so that's a no-op)
https://cs.chromium.org/chromium/src/ui/gfx/canvas.cc?rcl=a2d5d8259ad5462f1df...

[Peter Kasting, 2017-03-24 20:26:38]

https://codereview.chromium.org/2770943004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_header_painter_ash.cc (right):

https://codereview.chromium.org/2770943004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:84:
canvas->TileImageInt(frame_image, image_inset_x, 0, 0, 0, bounds.width(),
On 2017/03/24 20:22:52, danakj wrote:
> On 2017/03/24 20:08:12, Peter Kasting wrote:
> > On 2017/03/24 16:29:30, danakj wrote:
> > > pkasting should confirm the -inset to +inset is right.. but LGTM in
spirit.
> > 
> > The coord change is fine (changes from dest to source coords), but the
> > functionality isn't: TileImageInt() doesn't take a flags arg, which is why
we
> > had to SaveLayer in the other arm to begin with.
> 
> Oh oops. Using the underlying PaintCanvas directly would be fine (that's where
> we're trying to head anyway).
> 
> > Either we can go back to the simpler code, or we can try to work around
this,
> > maybe by manually implementing the tile call here, or by mucking with the
> canvas
> > APIs.
> > 
> > Unless we know that SaveLayer is really costly and this has a measurable
perf
> or
> > battery effect, my preference would be the former.
> 
> save layer is similar to what this did before, it draws to an intermediate
> target then draws that to the primary target. So it requires extra memory
> bandwidth, and possible allocations if not using a cached memory chunk.
> 
> I suggest we add PaintFlags to (an override of?) TileImageInt. In
> Canvas::TileImageInt we use that and add the shader to it. The setBlendMode
> should be removed (it's the default blend mode anyway so that's a no-op)
>
https://cs.chromium.org/chromium/src/ui/gfx/canvas.cc?rcl=a2d5d8259ad5462f1df...

If TileImageInt() got a with-flags variant, and DrawImageInt already has one,
then the only thing in the else arm that doesn't have one is DrawColor.  Can we
easily rewrite that to some similar call that uses flags?  If so, we can avoid
introducing a third conditional arm here AND eliminate the existing SaveLayer
call.

[danakj, 2017-03-24 20:33:55]

https://codereview.chromium.org/2770943004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_header_painter_ash.cc (right):

https://codereview.chromium.org/2770943004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:84:
canvas->TileImageInt(frame_image, image_inset_x, 0, 0, 0, bounds.width(),
On 2017/03/24 20:26:38, Peter Kasting wrote:
> On 2017/03/24 20:22:52, danakj wrote:
> > On 2017/03/24 20:08:12, Peter Kasting wrote:
> > > On 2017/03/24 16:29:30, danakj wrote:
> > > > pkasting should confirm the -inset to +inset is right.. but LGTM in
> spirit.
> > > 
> > > The coord change is fine (changes from dest to source coords), but the
> > > functionality isn't: TileImageInt() doesn't take a flags arg, which is why
> we
> > > had to SaveLayer in the other arm to begin with.
> > 
> > Oh oops. Using the underlying PaintCanvas directly would be fine (that's
where
> > we're trying to head anyway).
> > 
> > > Either we can go back to the simpler code, or we can try to work around
> this,
> > > maybe by manually implementing the tile call here, or by mucking with the
> > canvas
> > > APIs.
> > > 
> > > Unless we know that SaveLayer is really costly and this has a measurable
> perf
> > or
> > > battery effect, my preference would be the former.
> > 
> > save layer is similar to what this did before, it draws to an intermediate
> > target then draws that to the primary target. So it requires extra memory
> > bandwidth, and possible allocations if not using a cached memory chunk.
> > 
> > I suggest we add PaintFlags to (an override of?) TileImageInt. In
> > Canvas::TileImageInt we use that and add the shader to it. The setBlendMode
> > should be removed (it's the default blend mode anyway so that's a no-op)
> >
>
https://cs.chromium.org/chromium/src/ui/gfx/canvas.cc?rcl=a2d5d8259ad5462f1df...
> 
> If TileImageInt() got a with-flags variant, and DrawImageInt already has one,
> then the only thing in the else arm that doesn't have one is DrawColor.  Can
we
> easily rewrite that to some similar call that uses flags?  If so, we can avoid
> introducing a third conditional arm here AND eliminate the existing SaveLayer
> call.

I thought you need the SaveLayer to draw the contents of the whole layer with
kPlus blend mode together, instead of each piece as kPlus on top of the other.

[Peter Kasting, 2017-03-24 20:39:40]

https://codereview.chromium.org/2770943004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_header_painter_ash.cc (right):

https://codereview.chromium.org/2770943004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:84:
canvas->TileImageInt(frame_image, image_inset_x, 0, 0, 0, bounds.width(),
On 2017/03/24 20:33:54, danakj wrote:
> On 2017/03/24 20:26:38, Peter Kasting wrote:
> > On 2017/03/24 20:22:52, danakj wrote:
> > > On 2017/03/24 20:08:12, Peter Kasting wrote:
> > > > On 2017/03/24 16:29:30, danakj wrote:
> > > > > pkasting should confirm the -inset to +inset is right.. but LGTM in
> > spirit.
> > > > 
> > > > The coord change is fine (changes from dest to source coords), but the
> > > > functionality isn't: TileImageInt() doesn't take a flags arg, which is
why
> > we
> > > > had to SaveLayer in the other arm to begin with.
> > > 
> > > Oh oops. Using the underlying PaintCanvas directly would be fine (that's
> where
> > > we're trying to head anyway).
> > > 
> > > > Either we can go back to the simpler code, or we can try to work around
> > this,
> > > > maybe by manually implementing the tile call here, or by mucking with
the
> > > canvas
> > > > APIs.
> > > > 
> > > > Unless we know that SaveLayer is really costly and this has a measurable
> > perf
> > > or
> > > > battery effect, my preference would be the former.
> > > 
> > > save layer is similar to what this did before, it draws to an intermediate
> > > target then draws that to the primary target. So it requires extra memory
> > > bandwidth, and possible allocations if not using a cached memory chunk.
> > > 
> > > I suggest we add PaintFlags to (an override of?) TileImageInt. In
> > > Canvas::TileImageInt we use that and add the shader to it. The
setBlendMode
> > > should be removed (it's the default blend mode anyway so that's a no-op)
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/gfx/canvas.cc?rcl=a2d5d8259ad5462f1df...
> > 
> > If TileImageInt() got a with-flags variant, and DrawImageInt already has
one,
> > then the only thing in the else arm that doesn't have one is DrawColor.  Can
> we
> > easily rewrite that to some similar call that uses flags?  If so, we can
avoid
> > introducing a third conditional arm here AND eliminate the existing
SaveLayer
> > call.
> 
> I thought you need the SaveLayer to draw the contents of the whole layer with
> kPlus blend mode together, instead of each piece as kPlus on top of the other.

Oh.  Yeah, that's probably correct.  Argh.

I still worry that we're micro-optimizing something that maybe doesn't matter. 
But definitely don't micro-optimize by doing it the way I suggested.  :P

[danakj, 2017-03-24 20:40:51]

https://codereview.chromium.org/2770943004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_header_painter_ash.cc (right):

https://codereview.chromium.org/2770943004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_header_painter_ash.cc:84:
canvas->TileImageInt(frame_image, image_inset_x, 0, 0, 0, bounds.width(),
On 2017/03/24 20:39:40, Peter Kasting wrote:
> On 2017/03/24 20:33:54, danakj wrote:
> > On 2017/03/24 20:26:38, Peter Kasting wrote:
> > > On 2017/03/24 20:22:52, danakj wrote:
> > > > On 2017/03/24 20:08:12, Peter Kasting wrote:
> > > > > On 2017/03/24 16:29:30, danakj wrote:
> > > > > > pkasting should confirm the -inset to +inset is right.. but LGTM in
> > > spirit.
> > > > > 
> > > > > The coord change is fine (changes from dest to source coords), but the
> > > > > functionality isn't: TileImageInt() doesn't take a flags arg, which is
> why
> > > we
> > > > > had to SaveLayer in the other arm to begin with.
> > > > 
> > > > Oh oops. Using the underlying PaintCanvas directly would be fine (that's
> > where
> > > > we're trying to head anyway).
> > > > 
> > > > > Either we can go back to the simpler code, or we can try to work
around
> > > this,
> > > > > maybe by manually implementing the tile call here, or by mucking with
> the
> > > > canvas
> > > > > APIs.
> > > > > 
> > > > > Unless we know that SaveLayer is really costly and this has a
measurable
> > > perf
> > > > or
> > > > > battery effect, my preference would be the former.
> > > > 
> > > > save layer is similar to what this did before, it draws to an
intermediate
> > > > target then draws that to the primary target. So it requires extra
memory
> > > > bandwidth, and possible allocations if not using a cached memory chunk.
> > > > 
> > > > I suggest we add PaintFlags to (an override of?) TileImageInt. In
> > > > Canvas::TileImageInt we use that and add the shader to it. The
> setBlendMode
> > > > should be removed (it's the default blend mode anyway so that's a no-op)
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/gfx/canvas.cc?rcl=a2d5d8259ad5462f1df...
> > > 
> > > If TileImageInt() got a with-flags variant, and DrawImageInt already has
> one,
> > > then the only thing in the else arm that doesn't have one is DrawColor. 
Can
> > we
> > > easily rewrite that to some similar call that uses flags?  If so, we can
> avoid
> > > introducing a third conditional arm here AND eliminate the existing
> SaveLayer
> > > call.
> > 
> > I thought you need the SaveLayer to draw the contents of the whole layer
with
> > kPlus blend mode together, instead of each piece as kPlus on top of the
other.
> 
> Oh.  Yeah, that's probably correct.  Argh.
> 
> I still worry that we're micro-optimizing something that maybe doesn't matter.

> But definitely don't micro-optimize by doing it the way I suggested.  :P

Memory allocs/bandwidth are one of the best things to optimize and will easily
dominate the time spent in raster. We should definitely not savelayer
carelessly.

[Qiang(Joe) Xu, 2017-03-24 20:51:54]

On 2017/03/24 20:40:51, danakj wrote:
>
https://codereview.chromium.org/2770943004/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/frame/browser_header_painter_ash.cc (right):
> 
>
https://codereview.chromium.org/2770943004/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/frame/browser_header_painter_ash.cc:84:
> canvas->TileImageInt(frame_image, image_inset_x, 0, 0, 0, bounds.width(),
> On 2017/03/24 20:39:40, Peter Kasting wrote:
> > On 2017/03/24 20:33:54, danakj wrote:
> > > On 2017/03/24 20:26:38, Peter Kasting wrote:
> > > > On 2017/03/24 20:22:52, danakj wrote:
> > > > > On 2017/03/24 20:08:12, Peter Kasting wrote:
> > > > > > On 2017/03/24 16:29:30, danakj wrote:
> > > > > > > pkasting should confirm the -inset to +inset is right.. but LGTM
in
> > > > spirit.
> > > > > > 
> > > > > > The coord change is fine (changes from dest to source coords), but
the
> > > > > > functionality isn't: TileImageInt() doesn't take a flags arg, which
is
> > why
> > > > we
> > > > > > had to SaveLayer in the other arm to begin with.
> > > > > 
> > > > > Oh oops. Using the underlying PaintCanvas directly would be fine
(that's
> > > where
> > > > > we're trying to head anyway).
> > > > > 
> > > > > > Either we can go back to the simpler code, or we can try to work
> around
> > > > this,
> > > > > > maybe by manually implementing the tile call here, or by mucking
with
> > the
> > > > > canvas
> > > > > > APIs.
> > > > > > 
> > > > > > Unless we know that SaveLayer is really costly and this has a
> measurable
> > > > perf
> > > > > or
> > > > > > battery effect, my preference would be the former.
> > > > > 
> > > > > save layer is similar to what this did before, it draws to an
> intermediate
> > > > > target then draws that to the primary target. So it requires extra
> memory
> > > > > bandwidth, and possible allocations if not using a cached memory
chunk.
> > > > > 
> > > > > I suggest we add PaintFlags to (an override of?) TileImageInt. In
> > > > > Canvas::TileImageInt we use that and add the shader to it. The
> > setBlendMode
> > > > > should be removed (it's the default blend mode anyway so that's a
no-op)
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/gfx/canvas.cc?rcl=a2d5d8259ad5462f1df...
> > > > 
> > > > If TileImageInt() got a with-flags variant, and DrawImageInt already has
> > one,
> > > > then the only thing in the else arm that doesn't have one is DrawColor. 
> Can
> > > we
> > > > easily rewrite that to some similar call that uses flags?  If so, we can
> > avoid
> > > > introducing a third conditional arm here AND eliminate the existing
> > SaveLayer
> > > > call.
> > > 
> > > I thought you need the SaveLayer to draw the contents of the whole layer
> with
> > > kPlus blend mode together, instead of each piece as kPlus on top of the
> other.
> > 
> > Oh.  Yeah, that's probably correct.  Argh.
> > 
> > I still worry that we're micro-optimizing something that maybe doesn't
matter.
> 
> > But definitely don't micro-optimize by doing it the way I suggested.  :P
> 
> Memory allocs/bandwidth are one of the best things to optimize and will easily
> dominate the time spent in raster. We should definitely not savelayer
> carelessly.

hmm.. I am a little out reach of the discussion. Which suggestion I should take
or could land it as it is?
Or maybe this one should be assigned to danakj@ for a closer look?

Thanks!

[Peter Kasting, 2017-03-24 20:55:42]

On 2017/03/24 20:51:54, Qiang(Joe) Xu wrote:
> hmm.. I am a little out reach of the discussion. Which suggestion I should
take
> or could land it as it is?

Can't land as-is, it's functionally incorrect.

Take danakj's suggestion regarding adding a PaintFlags-taking version of
TileImageInt.

[danakj, 2017-03-24 20:55:44]

On Fri, Mar 24, 2017 at 4:51 PM, <warx@chromium.org> wrote:

> On 2017/03/24 20:40:51, danakj wrote:
> >
> https://codereview.chromium.org/2770943004/diff/20001/
> chrome/browser/ui/views/frame/browser_header_painter_ash.cc
> > File chrome/browser/ui/views/frame/browser_header_painter_ash.cc
> (right):
> >
> >
> https://codereview.chromium.org/2770943004/diff/20001/
> chrome/browser/ui/views/frame/browser_header_painter_ash.cc#newcode84
> > chrome/browser/ui/views/frame/browser_header_painter_ash.cc:84:
> > canvas->TileImageInt(frame_image, image_inset_x, 0, 0, 0,
> bounds.width(),
> > On 2017/03/24 20:39:40, Peter Kasting wrote:
> > > On 2017/03/24 20:33:54, danakj wrote:
> > > > On 2017/03/24 20:26:38, Peter Kasting wrote:
> > > > > On 2017/03/24 20:22:52, danakj wrote:
> > > > > > On 2017/03/24 20:08:12, Peter Kasting wrote:
> > > > > > > On 2017/03/24 16:29:30, danakj wrote:
> > > > > > > > pkasting should confirm the -inset to +inset is right.. but
> LGTM
> in
> > > > > spirit.
> > > > > > >
> > > > > > > The coord change is fine (changes from dest to source coords),
> but
> the
> > > > > > > functionality isn't: TileImageInt() doesn't take a flags arg,
> which
> is
> > > why
> > > > > we
> > > > > > > had to SaveLayer in the other arm to begin with.
> > > > > >
> > > > > > Oh oops. Using the underlying PaintCanvas directly would be fine
> (that's
> > > > where
> > > > > > we're trying to head anyway).
> > > > > >
> > > > > > > Either we can go back to the simpler code, or we can try to
> work
> > around
> > > > > this,
> > > > > > > maybe by manually implementing the tile call here, or by
> mucking
> with
> > > the
> > > > > > canvas
> > > > > > > APIs.
> > > > > > >
> > > > > > > Unless we know that SaveLayer is really costly and this has a
> > measurable
> > > > > perf
> > > > > > or
> > > > > > > battery effect, my preference would be the former.
> > > > > >
> > > > > > save layer is similar to what this did before, it draws to an
> > intermediate
> > > > > > target then draws that to the primary target. So it requires
> extra
> > memory
> > > > > > bandwidth, and possible allocations if not using a cached memory
> chunk.
> > > > > >
> > > > > > I suggest we add PaintFlags to (an override of?) TileImageInt. In
> > > > > > Canvas::TileImageInt we use that and add the shader to it. The
> > > setBlendMode
> > > > > > should be removed (it's the default blend mode anyway so that's a
> no-op)
> > > > > >
> > > > >
> > > >
> > >
> >
> https://cs.chromium.org/chromium/src/ui/gfx/canvas.cc?rcl=
> a2d5d8259ad5462f1dfcb56949472be2ab296474&l=539
> > > > >
> > > > > If TileImageInt() got a with-flags variant, and DrawImageInt
> already has
> > > one,
> > > > > then the only thing in the else arm that doesn't have one is
> DrawColor.
> > Can
> > > > we
> > > > > easily rewrite that to some similar call that uses flags? If so,
> we can
> > > avoid
> > > > > introducing a third conditional arm here AND eliminate the existing
> > > SaveLayer
> > > > > call.
> > > >
> > > > I thought you need the SaveLayer to draw the contents of the whole
> layer
> > with
> > > > kPlus blend mode together, instead of each piece as kPlus on top of
> the
> > other.
> > >
> > > Oh. Yeah, that's probably correct. Argh.
> > >
> > > I still worry that we're micro-optimizing something that maybe doesn't
> matter.
> >
> > > But definitely don't micro-optimize by doing it the way I suggested. :P
> >
> > Memory allocs/bandwidth are one of the best things to optimize and will
> easily
> > dominate the time spent in raster. We should definitely not savelayer
> > carelessly.
>
> hmm.. I am a little out reach of the discussion. Which suggestion I should
> take
> or could land it as it is?
>

I think you should add a PaintFlags to TileImageInt() like DrawImageInt
has, and pass the flags here to it, and have TileImageInt add the shader to
a copy of your flags like DrawImageInt does.


> Or maybe this one should be assigned to danakj@ for a closer look?
>

I would just do that :P


>
> Thanks!
>
> https://codereview.chromium.org/2770943004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Qiang(Joe) Xu, 2017-03-24 20:58:50]

On 2017/03/24 20:55:44, danakj wrote:
> On Fri, Mar 24, 2017 at 4:51 PM, <mailto:warx@chromium.org> wrote:
> 
> > On 2017/03/24 20:40:51, danakj wrote:
> > >
> > https://codereview.chromium.org/2770943004/diff/20001/
> > chrome/browser/ui/views/frame/browser_header_painter_ash.cc
> > > File chrome/browser/ui/views/frame/browser_header_painter_ash.cc
> > (right):
> > >
> > >
> > https://codereview.chromium.org/2770943004/diff/20001/
> > chrome/browser/ui/views/frame/browser_header_painter_ash.cc#newcode84
> > > chrome/browser/ui/views/frame/browser_header_painter_ash.cc:84:
> > > canvas->TileImageInt(frame_image, image_inset_x, 0, 0, 0,
> > bounds.width(),
> > > On 2017/03/24 20:39:40, Peter Kasting wrote:
> > > > On 2017/03/24 20:33:54, danakj wrote:
> > > > > On 2017/03/24 20:26:38, Peter Kasting wrote:
> > > > > > On 2017/03/24 20:22:52, danakj wrote:
> > > > > > > On 2017/03/24 20:08:12, Peter Kasting wrote:
> > > > > > > > On 2017/03/24 16:29:30, danakj wrote:
> > > > > > > > > pkasting should confirm the -inset to +inset is right.. but
> > LGTM
> > in
> > > > > > spirit.
> > > > > > > >
> > > > > > > > The coord change is fine (changes from dest to source coords),
> > but
> > the
> > > > > > > > functionality isn't: TileImageInt() doesn't take a flags arg,
> > which
> > is
> > > > why
> > > > > > we
> > > > > > > > had to SaveLayer in the other arm to begin with.
> > > > > > >
> > > > > > > Oh oops. Using the underlying PaintCanvas directly would be fine
> > (that's
> > > > > where
> > > > > > > we're trying to head anyway).
> > > > > > >
> > > > > > > > Either we can go back to the simpler code, or we can try to
> > work
> > > around
> > > > > > this,
> > > > > > > > maybe by manually implementing the tile call here, or by
> > mucking
> > with
> > > > the
> > > > > > > canvas
> > > > > > > > APIs.
> > > > > > > >
> > > > > > > > Unless we know that SaveLayer is really costly and this has a
> > > measurable
> > > > > > perf
> > > > > > > or
> > > > > > > > battery effect, my preference would be the former.
> > > > > > >
> > > > > > > save layer is similar to what this did before, it draws to an
> > > intermediate
> > > > > > > target then draws that to the primary target. So it requires
> > extra
> > > memory
> > > > > > > bandwidth, and possible allocations if not using a cached memory
> > chunk.
> > > > > > >
> > > > > > > I suggest we add PaintFlags to (an override of?) TileImageInt. In
> > > > > > > Canvas::TileImageInt we use that and add the shader to it. The
> > > > setBlendMode
> > > > > > > should be removed (it's the default blend mode anyway so that's a
> > no-op)
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> > https://cs.chromium.org/chromium/src/ui/gfx/canvas.cc?rcl=
> > a2d5d8259ad5462f1dfcb56949472be2ab296474&l=539
> > > > > >
> > > > > > If TileImageInt() got a with-flags variant, and DrawImageInt
> > already has
> > > > one,
> > > > > > then the only thing in the else arm that doesn't have one is
> > DrawColor.
> > > Can
> > > > > we
> > > > > > easily rewrite that to some similar call that uses flags? If so,
> > we can
> > > > avoid
> > > > > > introducing a third conditional arm here AND eliminate the existing
> > > > SaveLayer
> > > > > > call.
> > > > >
> > > > > I thought you need the SaveLayer to draw the contents of the whole
> > layer
> > > with
> > > > > kPlus blend mode together, instead of each piece as kPlus on top of
> > the
> > > other.
> > > >
> > > > Oh. Yeah, that's probably correct. Argh.
> > > >
> > > > I still worry that we're micro-optimizing something that maybe doesn't
> > matter.
> > >
> > > > But definitely don't micro-optimize by doing it the way I suggested. :P
> > >
> > > Memory allocs/bandwidth are one of the best things to optimize and will
> > easily
> > > dominate the time spent in raster. We should definitely not savelayer
> > > carelessly.
> >
> > hmm.. I am a little out reach of the discussion. Which suggestion I should
> > take
> > or could land it as it is?
> >
> 
> I think you should add a PaintFlags to TileImageInt() like DrawImageInt
> has, and pass the flags here to it, and have TileImageInt add the shader to
> a copy of your flags like DrawImageInt does.
> 
> 
> > Or maybe this one should be assigned to danakj@ for a closer look?
> >
> 
> I would just do that :P
> 
> 
> >
> > Thanks!
> >
> > https://codereview.chromium.org/2770943004/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

OK, got it!

[Qiang(Joe) Xu, 2017-03-24 21:55:33]

Hi all, PTAL, thanks!

https://codereview.chromium.org/2770943004/diff/40001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/2770943004/diff/40001/ui/gfx/canvas.h#newcode440
ui/gfx/canvas.h:440: void TileImageInt(const ImageSkia& image,
I still keep the no-flag version since there are some (but not many) callers of
it.

[Peter Kasting, 2017-03-24 22:09:11]

LGTM with an API cleanup suggestion

https://codereview.chromium.org/2770943004/diff/40001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/2770943004/diff/40001/ui/gfx/canvas.h#newcode440
ui/gfx/canvas.h:440: void TileImageInt(const ImageSkia& image,
On 2017/03/24 21:55:33, Qiang(Joe) Xu wrote:
> I still keep the no-flag version since there are some (but not many) callers
of
> it.

It looks like the three existing functions here have something like 19, 6, and 1
callers, respectively (per codesearch xrefs, which may be inaccurate, but a
glance through the text search results suggests this is close).  I suggest the
following:

* Eliminate the very last function (that takes tile_scale_x/y) as it's just the
internal implementation, and roll its functionality into the second-to-last
function
* Rather than add a new copy here, just add the flags argument to the existing
7-arg function, with a default "= nullptr"

[Qiang(Joe) Xu, 2017-03-24 22:23:16]

Patchset #4 (id:60001) has been deleted

[Qiang(Joe) Xu, 2017-03-24 22:31:45]

On 2017/03/24 22:09:11, Peter Kasting wrote:
> LGTM with an API cleanup suggestion
> 
> https://codereview.chromium.org/2770943004/diff/40001/ui/gfx/canvas.h
> File ui/gfx/canvas.h (right):
> 
>
https://codereview.chromium.org/2770943004/diff/40001/ui/gfx/canvas.h#newcode440
> ui/gfx/canvas.h:440: void TileImageInt(const ImageSkia& image,
> On 2017/03/24 21:55:33, Qiang(Joe) Xu wrote:
> > I still keep the no-flag version since there are some (but not many) callers
> of
> > it.
> 
> It looks like the three existing functions here have something like 19, 6, and
1
> callers, respectively (per codesearch xrefs, which may be inaccurate, but a
> glance through the text search results suggests this is close).  I suggest the
> following:
> 
> * Eliminate the very last function (that takes tile_scale_x/y) as it's just
the
> internal implementation, and roll its functionality into the second-to-last
> function
> * Rather than add a new copy here, just add the flags argument to the existing
> 7-arg function, with a default "= nullptr"

The last function is eliminated.
But we cannot pass default parameter nullptr. Because it has to be non-nullptr
and I remember chromium style doesn't encourage using default parameter?
We would better leave it or I can cleanup the callers in another patch. WDYT?

[Peter Kasting, 2017-03-24 22:41:14]

Note: Please try to reply to comments using the per-comment reply interface,
rather than "send message"/emails, so it keeps the comment thread localized.

On 2017/03/24 22:31:45, Qiang(Joe) Xu wrote:
> On 2017/03/24 22:09:11, Peter Kasting wrote:
> > LGTM with an API cleanup suggestion
> > 
> > https://codereview.chromium.org/2770943004/diff/40001/ui/gfx/canvas.h
> > File ui/gfx/canvas.h (right):
> > 
> >
>
https://codereview.chromium.org/2770943004/diff/40001/ui/gfx/canvas.h#newcode440
> > ui/gfx/canvas.h:440: void TileImageInt(const ImageSkia& image,
> > On 2017/03/24 21:55:33, Qiang(Joe) Xu wrote:
> > > I still keep the no-flag version since there are some (but not many)
callers
> > of
> > > it.
> > 
> > It looks like the three existing functions here have something like 19, 6,
and
> 1
> > callers, respectively (per codesearch xrefs, which may be inaccurate, but a
> > glance through the text search results suggests this is close).  I suggest
the
> > following:
> > 
> > * Eliminate the very last function (that takes tile_scale_x/y) as it's just
> the
> > internal implementation, and roll its functionality into the second-to-last
> > function
> > * Rather than add a new copy here, just add the flags argument to the
existing
> > 7-arg function, with a default "= nullptr"
> 
> The last function is eliminated.
> But we cannot pass default parameter nullptr. Because it has to be non-nullptr
> and I remember chromium style doesn't encourage using default parameter?
> We would better leave it or I can cleanup the callers in another patch. WDYT?

Default parameters are fine:
http://google.github.io/styleguide/cppguide.html#Default_Arguments

It doesn't have to be non-null, either; have the function implementation use the
address of a local temp internally when the arg is null.

[Qiang(Joe) Xu, 2017-03-24 22:51:06]

OK, thanks for the suggestion. Done.

https://codereview.chromium.org/2770943004/diff/40001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/2770943004/diff/40001/ui/gfx/canvas.h#newcode440
ui/gfx/canvas.h:440: void TileImageInt(const ImageSkia& image,
On 2017/03/24 22:09:10, Peter Kasting wrote:
> On 2017/03/24 21:55:33, Qiang(Joe) Xu wrote:
> > I still keep the no-flag version since there are some (but not many) callers
> of
> > it.
> 
> It looks like the three existing functions here have something like 19, 6, and
1
> callers, respectively (per codesearch xrefs, which may be inaccurate, but a
> glance through the text search results suggests this is close).  I suggest the
> following:
> 
> * Eliminate the very last function (that takes tile_scale_x/y) as it's just
the
> internal implementation, and roll its functionality into the second-to-last
> function
> * Rather than add a new copy here, just add the flags argument to the existing
> 7-arg function, with a default "= nullptr"

Done.

[Peter Kasting, 2017-03-24 22:52:35]

https://codereview.chromium.org/2770943004/diff/100001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/2770943004/diff/100001/ui/gfx/canvas.cc#newco...
ui/gfx/canvas.cc:504: flags = &paint_flags;
This doesn't work -- you've declared your temp inside the conditional, so as
soon as you cross the } below, |flags| points to garbage.

You need to declare the temp above the conditional.

https://codereview.chromium.org/2770943004/diff/100001/ui/gfx/canvas.cc#newco...
ui/gfx/canvas.cc:507: flags)) {
Nit: Don't add {}

[Qiang(Joe) Xu, 2017-03-24 22:57:00]

https://codereview.chromium.org/2770943004/diff/100001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/2770943004/diff/100001/ui/gfx/canvas.cc#newco...
ui/gfx/canvas.cc:504: flags = &paint_flags;
On 2017/03/24 22:52:35, Peter Kasting wrote:
> This doesn't work -- you've declared your temp inside the conditional, so as
> soon as you cross the } below, |flags| points to garbage.
> 
> You need to declare the temp above the conditional.

oh, right! sorry for the iterations...

https://codereview.chromium.org/2770943004/diff/100001/ui/gfx/canvas.cc#newco...
ui/gfx/canvas.cc:507: flags)) {
On 2017/03/24 22:52:35, Peter Kasting wrote:
> Nit: Don't add {}

Done.

[Peter Kasting, 2017-03-24 22:58:01]

LGTM

[danakj, 2017-03-28 14:34:37]

LGTM

https://codereview.chromium.org/2770943004/diff/120001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/2770943004/diff/120001/ui/gfx/canvas.cc#newco...
ui/gfx/canvas.cc:514: float tile_scale_x,
These tile_scales are never anything but 1.f anymore, I would remove the
arguments

[Qiang(Joe) Xu, 2017-03-28 17:11:15]

https://codereview.chromium.org/2770943004/diff/120001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/2770943004/diff/120001/ui/gfx/canvas.cc#newco...
ui/gfx/canvas.cc:514: float tile_scale_x,
On 2017/03/28 14:34:37, danakj wrote:
> These tile_scales are never anything but 1.f anymore, I would remove the
> arguments

seems
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_strip.c...
is still using them.

[Qiang(Joe) Xu, 2017-03-28 17:11:23]

The CQ bit was checked by warx@chromium.org

[commit-bot: I haz the power, 2017-03-28 17:11:48]

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

[danakj, 2017-03-28 17:49:41]

https://codereview.chromium.org/2770943004/diff/120001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/2770943004/diff/120001/ui/gfx/canvas.cc#newco...
ui/gfx/canvas.cc:514: float tile_scale_x,
On 2017/03/28 17:11:15, Qiang(Joe) Xu wrote:
> On 2017/03/28 14:34:37, danakj wrote:
> > These tile_scales are never anything but 1.f anymore, I would remove the
> > arguments
> 
> seems
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_strip.c...
> is still using them.

Oh.. I assumed it is a private method, kinda surprised it isn't but okay.
Thanks!

[commit-bot: I haz the power, 2017-03-28 17:57:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 17:57:57]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Qiang(Joe) Xu, 2017-03-28 19:55:24]

The CQ bit was checked by warx@chromium.org

[commit-bot: I haz the power, 2017-03-28 19:56:06]

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

[Qiang(Joe) Xu, 2017-03-28 20:24:12]

The CQ bit was checked by warx@chromium.org

[Qiang(Joe) Xu, 2017-03-28 20:24:12]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2770943004/#ps140001
(title: "rebase for compile error")

[commit-bot: I haz the power, 2017-03-28 20:24:35]

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, 2017-03-28 21:04:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 21:04:20]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Qiang(Joe) Xu, 2017-03-28 21:13:31]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 21:14:10]

Dry run: 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, 2017-03-28 21:16:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 21:16:35]

Dry run: CQ has no permission to schedule in bucket
master.tryserver.chromium.linux

[Qiang(Joe) Xu, 2017-03-28 21:37:01]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 21:37:14]

Dry run: 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, 2017-03-28 22:36:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 22:36:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Qiang(Joe) Xu, 2017-03-28 23:21:07]

https://codereview.chromium.org/2770943004/diff/40001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/2770943004/diff/40001/ui/gfx/canvas.h#newcode440
ui/gfx/canvas.h:440: void TileImageInt(const ImageSkia& image,
On 2017/03/24 22:09:10, Peter Kasting wrote:
> On 2017/03/24 21:55:33, Qiang(Joe) Xu wrote:
> > I still keep the no-flag version since there are some (but not many) callers
> of
> > it.
> 
> It looks like the three existing functions here have something like 19, 6, and
1
> callers, respectively (per codesearch xrefs, which may be inaccurate, but a
> glance through the text search results suggests this is close).  I suggest the
> following:
> 
> * Eliminate the very last function (that takes tile_scale_x/y) as it's just
the
> internal implementation, and roll its functionality into the second-to-last
> function
> * Rather than add a new copy here, just add the flags argument to the existing
> 7-arg function, with a default "= nullptr"

We shall not eliminate the last function because of:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/glass_brow...,
it is compiled for OS_WIN. Do you want a recheck, pkasting@?

[Qiang(Joe) Xu, 2017-03-29 00:37:00]

The CQ bit was checked by warx@chromium.org

[Qiang(Joe) Xu, 2017-03-29 00:37:01]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2770943004/#ps160001
(title: "do not eliminate the last TileImageInt")

[commit-bot: I haz the power, 2017-03-29 00:37:40]

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

[Peter Kasting, 2017-03-29 01:37:06]

On 2017/03/28 23:21:07, Qiang(Joe) Xu wrote:
> https://codereview.chromium.org/2770943004/diff/40001/ui/gfx/canvas.h
> File ui/gfx/canvas.h (right):
> 
>
https://codereview.chromium.org/2770943004/diff/40001/ui/gfx/canvas.h#newcode440
> ui/gfx/canvas.h:440: void TileImageInt(const ImageSkia& image,
> On 2017/03/24 22:09:10, Peter Kasting wrote:
> > On 2017/03/24 21:55:33, Qiang(Joe) Xu wrote:
> > > I still keep the no-flag version since there are some (but not many)
callers
> > of
> > > it.
> > 
> > It looks like the three existing functions here have something like 19, 6,
and
> 1
> > callers, respectively (per codesearch xrefs, which may be inaccurate, but a
> > glance through the text search results suggests this is close).  I suggest
the
> > following:
> > 
> > * Eliminate the very last function (that takes tile_scale_x/y) as it's just
> the
> > internal implementation, and roll its functionality into the second-to-last
> > function
> > * Rather than add a new copy here, just add the flags argument to the
existing
> > 7-arg function, with a default "= nullptr"
> 
> We shall not eliminate the last function because of:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/glass_brow...,
> it is compiled for OS_WIN. Do you want a recheck, pkasting@?

If that's the lone caller, then rather than readd this function, I'd just make
the tile scale args be passed before the paint flags so they can be defaulted as
well.

This will also let you collapse the separate X and Y scales into a single scale
factor arg since it looks like the callers all want to pass the same scale for
both.

[commit-bot: I haz the power, 2017-03-29 03:21:12]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490747820306650,
"parent_rev": "3f3ab99b9846cffbaab5ff2b807e9fd9d92c07ed", "commit_rev":
"b30c149bc6b38870ca74cb8dcd5fb34e90e75480"}

[commit-bot: I haz the power, 2017-03-29 03:22:57]

Description was changed from

==========
cros: Fix browser header wrongly drawn frame image

Changes:
Check the code before https://codereview.chromium.org/2770943002, found that
when frame_overlay_image_.isNull(), TileImageInt should be called.

BUG=704636
TEST=test that bug doesn't happen with theme installed
==========

to

==========
cros: Fix browser header wrongly drawn frame image

Changes:
Check the code before https://codereview.chromium.org/2770943002, found that
when frame_overlay_image_.isNull(), TileImageInt should be called.

BUG=704636
TEST=test that bug doesn't happen with theme installed

Review-Url: https://codereview.chromium.org/2770943004
Cr-Commit-Position: refs/heads/master@{#460265}
Committed:
https://chromium.googlesource.com/chromium/src/+/b30c149bc6b38870ca74cb8dcd5f...
==========

[commit-bot: I haz the power, 2017-03-29 03:22:59]

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/b30c149bc6b38870ca74cb8dcd5f...

[Qiang(Joe) Xu, 2017-03-29 16:41:10]

On 2017/03/29 01:37:06, Peter Kasting wrote:
> On 2017/03/28 23:21:07, Qiang(Joe) Xu wrote:
> > https://codereview.chromium.org/2770943004/diff/40001/ui/gfx/canvas.h
> > File ui/gfx/canvas.h (right):
> > 
> >
>
https://codereview.chromium.org/2770943004/diff/40001/ui/gfx/canvas.h#newcode440
> > ui/gfx/canvas.h:440: void TileImageInt(const ImageSkia& image,
> > On 2017/03/24 22:09:10, Peter Kasting wrote:
> > > On 2017/03/24 21:55:33, Qiang(Joe) Xu wrote:
> > > > I still keep the no-flag version since there are some (but not many)
> callers
> > > of
> > > > it.
> > > 
> > > It looks like the three existing functions here have something like 19, 6,
> and
> > 1
> > > callers, respectively (per codesearch xrefs, which may be inaccurate, but
a
> > > glance through the text search results suggests this is close).  I suggest
> the
> > > following:
> > > 
> > > * Eliminate the very last function (that takes tile_scale_x/y) as it's
just
> > the
> > > internal implementation, and roll its functionality into the
second-to-last
> > > function
> > > * Rather than add a new copy here, just add the flags argument to the
> existing
> > > 7-arg function, with a default "= nullptr"
> > 
> > We shall not eliminate the last function because of:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/glass_brow...,
> > it is compiled for OS_WIN. Do you want a recheck, pkasting@?
> 
> If that's the lone caller, then rather than readd this function, I'd just make
> the tile scale args be passed before the paint flags so they can be defaulted
as
> well.
> 
> This will also let you collapse the separate X and Y scales into a single
scale
> factor arg since it looks like the callers all want to pass the same scale for
> both.

I filed crbug.com/706441 to track this cleanup.