Allow background tab titles to use LCD AA.

Allow background tab titles to use LCD AA.

This involves plumbing an additional flag through CompositingRecorder and
CompositingDisplayItem to use the new Skia functionality just added for this.

BUG=none
TEST=Titles on background tabs should use LCD antialiasing.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/03447cc3e0e71bb1b85ce6871794f887fc313fb7
Cr-Commit-Position: refs/heads/master@{#364903}

[Peter Kasting, 2015-12-10 01:44:55]

Description was changed from

==========
Allow background tab titles to use LCD AA.

BUG=
TEST=
==========

to

==========
Allow background tab titles to use LCD AA.

BUG=
TEST=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Peter Kasting, 2015-12-11 02:16:50]

Description was changed from

==========
Allow background tab titles to use LCD AA.

BUG=
TEST=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Allow background tab titles to use LCD AA.

This involves plumbing an additional flag through CompositingRecorder and
CompositingDisplayItem to use the new Skia functionality just added for this.

BUG=none
TEST=Titles on background tabs should use LCD antialiasing.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Peter Kasting, 2015-12-11 02:16:50]

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

[danakj, 2015-12-11 19:07:00]

Thanks Peter! A few comments, looks great though.

https://codereview.chromium.org/1506203008/diff/20001/cc/blink/web_display_it...
File cc/blink/web_display_item_list_impl.cc (right):

https://codereview.chromium.org/1506203008/diff/20001/cc/blink/web_display_it...
cc/blink/web_display_item_list_impl.cc:174: xfermode, bounds,
skia::SharePtr(color_filter), false);
can you use a temp var to give the boolean a name rather than passing a bool
literal directly?

https://codereview.chromium.org/1506203008/diff/20001/cc/playback/compositing...
File cc/playback/compositing_display_item.cc (right):

https://codereview.chromium.org/1506203008/diff/20001/cc/playback/compositing...
cc/playback/compositing_display_item.cc:51: SetNew(alpha, xfermode,
bounds.get(), std::move(filter), false);
Can you add this flag to the proto?

https://codereview.chromium.org/1506203008/diff/20001/cc/playback/compositing...
File cc/playback/compositing_display_item.h (right):

https://codereview.chromium.org/1506203008/diff/20001/cc/playback/compositing...
cc/playback/compositing_display_item.h:27: bool allow_lcd_text);
can you rename this a bit? currently it sounds like false would disallow lcd
text, but this is more of an override to force allow, and false just does the
default behaviour. perhaps always_allow_lcd_text?

https://codereview.chromium.org/1506203008/diff/20001/cc/playback/display_ite...
File cc/playback/display_item_list_unittest.cc (right):

https://codereview.chromium.org/1506203008/diff/20001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:202: kVisualRect, 150,
SkXfermode::Mode::kDst_Mode, nullptr, filter, false);
can you pass true here, false is easier to get right by accident?

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

https://codereview.chromium.org/1506203008/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:513:
ui::CompositingRecorder recorder(context, size(), alpha_, false);
I'd appreciate a named temp var for this as well, instead of passing |false|
directly. Ditto at other call sites.

https://codereview.chromium.org/1506203008/diff/20001/ui/compositor/compositi...
File ui/compositor/compositing_recorder.h (right):

https://codereview.chromium.org/1506203008/diff/20001/ui/compositor/compositi...
ui/compositor/compositing_recorder.h:35: bool allow_lcd_text);
same comment about the name here

[Peter Kasting, 2015-12-11 22:07:47]

* Added flag to proto.
* Renamed flag "lcd_text_requires_opaque_layer" in hopes of greater clarity. 
This also meant I reversed the sense of the flag.
* Flipped flag value in test; as discussed over IM, this doesn't actually affect
the test.
* Added const bool temps in various places.  (Not in tab_strip.cc because I
wanted to write a comment about the values chosen there anyway and the comment
seems clear enough alone.)

I don't love this last bullet.  In particular, the temp in
display_item_list_unittest.cc can either be named the same as the param name --
in which case it's named as if LCD text requires an opaque layer, which is
opposite its meaning -- or it can be named what its value actually does, in
which case the actual assignment statement is confusing to read:
"kLcdTextAlwaysAllowed = false".  I did the latter + a comment, but honestly, I
would find this clearer without the temp at all.

[danakj, 2015-12-11 22:44:02]

LGTM

https://codereview.chromium.org/1506203008/diff/40001/cc/blink/web_display_it...
File cc/blink/web_display_item_list_impl.cc (right):

https://codereview.chromium.org/1506203008/diff/40001/cc/blink/web_display_it...
cc/blink/web_display_item_list_impl.cc:171: const bool
kLcdTextRequiresOpaqueLayer = true;
fwiw, i would normally write this as

bool lcd_text_requires_opaque_layer = true;

i mean, this is fine and all, just saying in case you like that.

https://codereview.chromium.org/1506203008/diff/40001/cc/playback/compositing...
File cc/playback/compositing_display_item.h (right):

https://codereview.chromium.org/1506203008/diff/40001/cc/playback/compositing...
cc/playback/compositing_display_item.h:47: bool allow_lcd_text);
missed this name

https://codereview.chromium.org/1506203008/diff/40001/cc/playback/display_ite...
File cc/playback/display_item_list_unittest.cc (right):

https://codereview.chromium.org/1506203008/diff/40001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:204: kLcdTextAlwaysAllowed);
in tests I'm less strict about naming boolean literals, you can just pass true
here if you want (i on purpose didn't mention this site in the last round of
review)

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

https://codereview.chromium.org/1506203008/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:1435: // We pass false for
|lcd_text_requires_opaque_layer| so that background
Ya this works :)

[Peter Kasting, 2015-12-12 02:51:49]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2015-12-12 02:51:49]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1506203008/#ps60001
(title: "Review comments")

[Peter Kasting, 2015-12-12 02:51:55]

https://codereview.chromium.org/1506203008/diff/40001/cc/blink/web_display_it...
File cc/blink/web_display_item_list_impl.cc (right):

https://codereview.chromium.org/1506203008/diff/40001/cc/blink/web_display_it...
cc/blink/web_display_item_list_impl.cc:171: const bool
kLcdTextRequiresOpaqueLayer = true;
On 2015/12/11 22:44:02, danakj (behind on reviews) wrote:
> fwiw, i would normally write this as
> 
> bool lcd_text_requires_opaque_layer = true;
> 
> i mean, this is fine and all, just saying in case you like that.

Yeah, an argument can be made for each -- is this a boolean variable whose value
doesn't change?  Or is it a named constant?  I went with the latter.

https://codereview.chromium.org/1506203008/diff/40001/cc/playback/compositing...
File cc/playback/compositing_display_item.h (right):

https://codereview.chromium.org/1506203008/diff/40001/cc/playback/compositing...
cc/playback/compositing_display_item.h:47: bool allow_lcd_text);
On 2015/12/11 22:44:02, danakj (behind on reviews) wrote:
> missed this name

Done.

https://codereview.chromium.org/1506203008/diff/40001/cc/playback/display_ite...
File cc/playback/display_item_list_unittest.cc (right):

https://codereview.chromium.org/1506203008/diff/40001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:204: kLcdTextAlwaysAllowed);
On 2015/12/11 22:44:02, danakj (behind on reviews) wrote:
> in tests I'm less strict about naming boolean literals, you can just pass true
> here if you want (i on purpose didn't mention this site in the last round of
> review)

Done.

[commit-bot: I haz the power, 2015-12-12 02:54:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1506203008/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1506203008/60001

[commit-bot: I haz the power, 2015-12-12 04:27:50]

Description was changed from

==========
Allow background tab titles to use LCD AA.

This involves plumbing an additional flag through CompositingRecorder and
CompositingDisplayItem to use the new Skia functionality just added for this.

BUG=none
TEST=Titles on background tabs should use LCD antialiasing.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Allow background tab titles to use LCD AA.

This involves plumbing an additional flag through CompositingRecorder and
CompositingDisplayItem to use the new Skia functionality just added for this.

BUG=none
TEST=Titles on background tabs should use LCD antialiasing.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2015-12-12 04:27:50]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-12 04:28:34]

Description was changed from

==========
Allow background tab titles to use LCD AA.

This involves plumbing an additional flag through CompositingRecorder and
CompositingDisplayItem to use the new Skia functionality just added for this.

BUG=none
TEST=Titles on background tabs should use LCD antialiasing.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Allow background tab titles to use LCD AA.

This involves plumbing an additional flag through CompositingRecorder and
CompositingDisplayItem to use the new Skia functionality just added for this.

BUG=none
TEST=Titles on background tabs should use LCD antialiasing.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/03447cc3e0e71bb1b85ce6871794f887fc313fb7
Cr-Commit-Position: refs/heads/master@{#364903}
==========

[commit-bot: I haz the power, 2015-12-12 04:28:34]

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