tabstrip: Use a PaintRecorder to access the Canvas for painting.

tabstrip: Use a PaintRecorder to access the Canvas for painting.

Don't use the canvas() from the PaintContext so that we can have the
PaintContext be backed by a DisplayItemList instead in the future.
Access to the Canvas should be done through a PaintRecorder.

For compositing effects (opacity) we use a CompositingRecorder to
provide a scoped opacity change.

R=pkasting
BUG=466426
Committed: https://crrev.com/5729f982048935b2b702181d0b349589d607f34b
Cr-Commit-Position: refs/heads/master@{#324173}

[danakj, 2015-04-07 20:41:05]

danakj@chromium.org changed reviewers:
+ pkasting@chromium.org
- pkasting@google.com

[danakj, 2015-04-07 20:49:39]

Patchset #2 (id:20001) has been deleted

[danakj, 2015-04-07 20:49:43]

Patchset #1 (id:1) has been deleted

[danakj, 2015-04-07 20:49:49]

Patchset #1 (id:40001) has been deleted

[danakj, 2015-04-07 20:49:55]

Patchset #1 (id:60001) has been deleted

[danakj, 2015-04-07 20:51:43]

Hi Peter, PTAL

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (left):

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:1306: canvas->Restore();
This goes away thanks to scoped recorder.

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:1264: ui::CompositingRecorder
opacity_recorder(context, inactive_tab_alpha);
This looks like a big change but it's mostly just re-indenting. I'll point out
what I've done:

This is a scoped CompositingRecorder instead of SaveLayerAlpha()/Restore()
calls.

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:1266: PaintClosingTabs(tab_count(),
context);
This is all just reformatting...

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:1309: ui::PaintRecorder
recorder(context);
A paint recorder to draw to a canvas

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:1343: ui::CompositingRecorder
opacity_recorder(context, inactive_tab_alpha);
Another scoped recorder for alpha

[Peter Kasting, 2015-04-07 21:32:39]

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc (right):

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:466: int alpha =
static_cast<int>(255 * opacity_);
Probably not very important, but it seems like this should round rather than
truncating.  (Use gfx::ToRoundedInt().)

Perhaps even better would be to simply have |opacity_| be integral instead of
floating-point.  I think this could be trivially done by using
CurrentValueBetween() with the animation on the caller side to translate the
underlying animation state into an opacity.

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:1261: :
kInactiveTabAndNewTabButtonAlpha;
It would have been nice not to reformat this or the one below, since they were
legal as-is...

https://codereview.chromium.org/1062293003/diff/80001/ui/compositor/compositi...
File ui/compositor/compositing_recorder.cc (right):

https://codereview.chromium.org/1062293003/diff/80001/ui/compositor/compositi...
ui/compositor/compositing_recorder.cc:17: canvas_->SaveLayerAlpha(alpha);
Because SaveLayerAlpha() takes a uint8, I think |alpha| should also be a uint8
(well, probably a uint8_t).  This eliminates the need to do DCHECKs as well.

[danakj, 2015-04-07 21:36:26]

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc (right):

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:466: int alpha =
static_cast<int>(255 * opacity_);
On 2015/04/07 21:32:39, Peter Kasting wrote:
> Probably not very important, but it seems like this should round rather than
> truncating.  (Use gfx::ToRoundedInt().)
> 
> Perhaps even better would be to simply have |opacity_| be integral instead of
> floating-point.  I think this could be trivially done by using
> CurrentValueBetween() with the animation on the caller side to translate the
> underlying animation state into an opacity.

Sure, I was just maintaining what this did before, am happy to make it better.
Can I do that in another CL though? I want to try not make changes at the same
time as refactoring.

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:1261: :
kInactiveTabAndNewTabButtonAlpha;
On 2015/04/07 21:32:39, Peter Kasting wrote:
> It would have been nice not to reformat this or the one below, since they were
> legal as-is...

git cl format did it, cuz I touched them, but I then undid my changes. I can
revert it. They'll just get reformatted the next time someone touches them
instead.

https://codereview.chromium.org/1062293003/diff/80001/ui/compositor/compositi...
File ui/compositor/compositing_recorder.cc (right):

https://codereview.chromium.org/1062293003/diff/80001/ui/compositor/compositi...
ui/compositor/compositing_recorder.cc:17: canvas_->SaveLayerAlpha(alpha);
On 2015/04/07 21:32:39, Peter Kasting wrote:
> Because SaveLayerAlpha() takes a uint8, I think |alpha| should also be a uint8
> (well, probably a uint8_t).  This eliminates the need to do DCHECKs as well.

I did this at first (which is why the extra git cl format changes). But the
style guide pretty clearly says to use int not unsigned, and use DCHECKs for
bounds checking instead:

"Do not use unsigned types to mean "this value should never be < 0". For that,
use assertions or run-time checks (as appropriate)."

That feels like it probably applies to "should never be > 255" just the same?
And the existing code is all ints.

WDYT?

[danakj, 2015-04-07 21:43:38]

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc (right):

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:466: int alpha =
static_cast<int>(255 * opacity_);
On 2015/04/07 21:36:25, danakj wrote:
> On 2015/04/07 21:32:39, Peter Kasting wrote:
> > Probably not very important, but it seems like this should round rather than
> > truncating.  (Use gfx::ToRoundedInt().)
> > 
> > Perhaps even better would be to simply have |opacity_| be integral instead
of
> > floating-point.  I think this could be trivially done by using
> > CurrentValueBetween() with the animation on the caller side to translate the
> > underlying animation state into an opacity.
> 
> Sure, I was just maintaining what this did before, am happy to make it better.
> Can I do that in another CL though? I want to try not make changes at the same
> time as refactoring.

Here's a followup: https://codereview.chromium.org/1064213002

[Peter Kasting, 2015-04-07 21:45:59]

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc (right):

https://codereview.chromium.org/1062293003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:466: int alpha =
static_cast<int>(255 * opacity_);
On 2015/04/07 21:36:25, danakj wrote:
> On 2015/04/07 21:32:39, Peter Kasting wrote:
> > Probably not very important, but it seems like this should round rather than
> > truncating.  (Use gfx::ToRoundedInt().)
> > 
> > Perhaps even better would be to simply have |opacity_| be integral instead
of
> > floating-point.  I think this could be trivially done by using
> > CurrentValueBetween() with the animation on the caller side to translate the
> > underlying animation state into an opacity.
> 
> Sure, I was just maintaining what this did before, am happy to make it better.
> Can I do that in another CL though? I want to try not make changes at the same
> time as refactoring.

You're welcome to do it either way, though I think the changes are so simple
that it would be less work for both of us to just do it in one CL.  But I see
you've already made a new one :)

https://codereview.chromium.org/1062293003/diff/80001/ui/compositor/compositi...
File ui/compositor/compositing_recorder.cc (right):

https://codereview.chromium.org/1062293003/diff/80001/ui/compositor/compositi...
ui/compositor/compositing_recorder.cc:17: canvas_->SaveLayerAlpha(alpha);
On 2015/04/07 21:36:26, danakj wrote:
> On 2015/04/07 21:32:39, Peter Kasting wrote:
> > Because SaveLayerAlpha() takes a uint8, I think |alpha| should also be a
uint8
> > (well, probably a uint8_t).  This eliminates the need to do DCHECKs as well.
> 
> I did this at first (which is why the extra git cl format changes). But the
> style guide pretty clearly says to use int not unsigned, and use DCHECKs for
> bounds checking instead:
> 
> "Do not use unsigned types to mean "this value should never be < 0". For that,
> use assertions or run-time checks (as appropriate)."
> 
> That feels like it probably applies to "should never be > 255" just the same?
> And the existing code is all ints.
> 
> WDYT?

I would prefer to be consistent with the existing API there.  If someone later
wants to change all levels of the API to use int at once they can.

Even in that case, I actually think this is a case where use of uint8_t is
arguably correct.  It's not just "can't be less than zero", it's a very specific
range of values that corresponds exactly to a particular type.  The specific
sized-ness of the type is relevant.

[danakj, 2015-04-07 21:52:52]

PTAL using uint8_t

> You're welcome to do it either way, though I think the changes are so
> simple that it would be less work for both of us to just do it in one
> CL.  But I see you've already made a new one :)

I'm more worried about weird regressions for blaming, so I like to keep a no-op
refactor separate.

[Peter Kasting, 2015-04-07 21:58:08]

LGTM

[danakj, 2015-04-07 21:58:22]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-04-07 21:59:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1062293003/140001

[commit-bot: I haz the power, 2015-04-08 01:51:39]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2015-04-08 01:52:31]

Patchset 4 (id:??) landed as
https://crrev.com/5729f982048935b2b702181d0b349589d607f34b
Cr-Commit-Position: refs/heads/master@{#324173}