Remove antialiasing from views bounds rects.

Remove antialiasing from views bounds rects.

A views object clips drawing to its rectangular bounds. Those bounds are
in DIPs, and have antialiasing turned on. At non-integral scale factors
those bounds might not be pixel-aligned. When that happens it can cause
artifacting, so it needs antialiasing turned off. Since Blink appears to
need antialiasing turned on, this patch adds a field to ClipDisplayItem
so that each callsite can do the thing it wants.

BUG=615153
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[Bret, 2016-06-22 22:21:07]

Description was changed from

==========
Remove antialiasing from views bounds rects.

A views object clips drawing to its rectangular bounds. Those bounds are
in DIPs, and have antialiasing turned on. At non-integral scale factors
those bounds might not be pixel-aligned. When that happens it can cause
artifacting, so it needs antialiasing turned off. Since Blink appears to
need antialiasing turned on, this patch adds a field to ClipDisplayItem
so that each callsite can do the thing it wants.

BUG=615153
==========

to

==========
Remove antialiasing from views bounds rects.

A views object clips drawing to its rectangular bounds. Those bounds are
in DIPs, and have antialiasing turned on. At non-integral scale factors
those bounds might not be pixel-aligned. When that happens it can cause
artifacting, so it needs antialiasing turned off. Since Blink appears to
need antialiasing turned on, this patch adds a field to ClipDisplayItem
so that each callsite can do the thing it wants.

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

[Bret, 2016-06-22 22:22:03]

Description was changed from

==========
Remove antialiasing from views bounds rects.

A views object clips drawing to its rectangular bounds. Those bounds are
in DIPs, and have antialiasing turned on. At non-integral scale factors
those bounds might not be pixel-aligned. When that happens it can cause
artifacting, so it needs antialiasing turned off. Since Blink appears to
need antialiasing turned on, this patch adds a field to ClipDisplayItem
so that each callsite can do the thing it wants.

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

to

==========
Remove antialiasing from views bounds rects.

A views object clips drawing to its rectangular bounds. Those bounds are
in DIPs, and have antialiasing turned on. At non-integral scale factors
those bounds might not be pixel-aligned. When that happens it can cause
artifacting, so it needs antialiasing turned off. Since Blink appears to
need antialiasing turned on, this patch adds a field to ClipDisplayItem
so that each callsite can do the thing it wants.

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

[Bret, 2016-06-22 22:22:03]

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

[danakj, 2016-06-22 22:49:57]

https://codereview.chromium.org/2090203002/diff/1/cc/playback/clip_display_it...
File cc/playback/clip_display_item.cc (right):

https://codereview.chromium.org/2090203002/diff/1/cc/playback/clip_display_it...
cc/playback/clip_display_item.cc:70: bool antialiased = antialias_;
just use antialias_ throughout, drop the local variable?

https://codereview.chromium.org/2090203002/diff/1/cc/proto/display_item.proto
File cc/proto/display_item.proto (right):

https://codereview.chromium.org/2090203002/diff/1/cc/proto/display_item.proto...
cc/proto/display_item.proto:59: optional bool antialias = 3;
why not at the bottom of the message?

https://codereview.chromium.org/2090203002/diff/1/ui/compositor/clip_recorder.cc
File ui/compositor/clip_recorder.cc (right):

https://codereview.chromium.org/2090203002/diff/1/ui/compositor/clip_recorder...
ui/compositor/clip_recorder.cc:50: bool anti_alias = false;
nit: antialias

[Bret, 2016-06-22 23:39:42]

https://codereview.chromium.org/2090203002/diff/1/cc/playback/clip_display_it...
File cc/playback/clip_display_item.cc (right):

https://codereview.chromium.org/2090203002/diff/1/cc/playback/clip_display_it...
cc/playback/clip_display_item.cc:70: bool antialiased = antialias_;
On 2016/06/22 22:49:57, danakj wrote:
> just use antialias_ throughout, drop the local variable?

Done.

https://codereview.chromium.org/2090203002/diff/1/cc/proto/display_item.proto
File cc/proto/display_item.proto (right):

https://codereview.chromium.org/2090203002/diff/1/cc/proto/display_item.proto...
cc/proto/display_item.proto:59: optional bool antialias = 3;
On 2016/06/22 22:49:57, danakj wrote:
> why not at the bottom of the message?

I was trying to match ClipPathDisplayItem below, and it seemed like the original
author wanted the repeated field visually separated. Changed to just order the
fields by number though because that seems to follow the rest of the file.

https://codereview.chromium.org/2090203002/diff/1/ui/compositor/clip_recorder.cc
File ui/compositor/clip_recorder.cc (right):

https://codereview.chromium.org/2090203002/diff/1/ui/compositor/clip_recorder...
ui/compositor/clip_recorder.cc:50: bool anti_alias = false;
On 2016/06/22 22:49:57, danakj wrote:
> nit: antialias

Changed everywhere in the file.

[danakj, 2016-06-22 23:54:53]

LGTM

+sky FYI this changes all clips in the ui.

[danakj, 2016-06-22 23:54:56]

LGTM

+sky FYI this changes all clips in the ui.

[sky, 2016-06-22 23:58:39]

sky@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2016-06-22 23:58:40]

Does this impact how we schedule damage regions? By that I mean if views is now
painting to a different area do we need to change how views schedules damage?

[Bret, 2016-06-22 23:59:06]

On 2016/06/22 23:54:56, danakj wrote:
> LGTM
> 
> +sky FYI this changes all clips in the ui.

For reference, I believe UI clips have only been antialiased since January, due
to this patch:
https://chromium.googlesource.com/chromium/src/+/ce3dce9ffa916d60c9b6d70f427a...

[Bret, 2016-06-23 00:16:55]

On 2016/06/22 23:58:40, sky wrote:
> Does this impact how we schedule damage regions? By that I mean if views is
now
> painting to a different area do we need to change how views schedules damage?

I know basically nothing about how damage works so I don't think I can answer
that question. A cursory search doesn't reveal any view-specific code related to
calculating damaged regions. The only other information I can point to is the
fact that the earlier patch that enabled antialiasing didn't seem to change
anything like that, so even if it did need to be updated I'm probably just
putting it back to way it was before.

[Bret, 2016-06-23 00:35:14]

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

[commit-bot: I haz the power, 2016-06-23 00:36:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2090203002/20001

[commit-bot: I haz the power, 2016-06-23 00:49:26]

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

[commit-bot: I haz the power, 2016-06-23 00:49:27]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sky, 2016-06-23 15:23:52]

Ok, great.

On Wed, Jun 22, 2016 at 5:16 PM,  <bsep@chromium.org> wrote:
> On 2016/06/22 23:58:40, sky wrote:
>> Does this impact how we schedule damage regions? By that I mean if views
>> is
> now
>> painting to a different area do we need to change how views schedules
>> damage?
>
> I know basically nothing about how damage works so I don't think I can
> answer
> that question. A cursory search doesn't reveal any view-specific code
> related to
> calculating damaged regions. The only other information I can point to is
> the
> fact that the earlier patch that enabled antialiasing didn't seem to change
> anything like that, so even if it did need to be updated I'm probably just
> putting it back to way it was before.
>
> https://codereview.chromium.org/2090203002/

-- 
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.

[Bret, 2016-06-23 17:19:15]

The CQ bit was checked by bsep@chromium.org

[Bret, 2016-06-23 17:19:16]

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/2090203002/#ps40001
(title: "fix display_item_list_unittest")

[commit-bot: I haz the power, 2016-06-23 17:19:50]

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

[commit-bot: I haz the power, 2016-06-23 17:37:53]

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

[commit-bot: I haz the power, 2016-06-23 17:37:54]

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

[Bret, 2016-06-23 18:37:14]

The CQ bit was checked by bsep@chromium.org

[Bret, 2016-06-23 18:37:14]

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/2090203002/#ps60001
(title: "ClipDisplayItem now largest on Linux64")

[commit-bot: I haz the power, 2016-06-23 18:37:59]

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

[commit-bot: I haz the power, 2016-06-23 18:51:59]

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

[commit-bot: I haz the power, 2016-06-23 18:52:00]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Bret, 2016-06-23 19:52:49]

The CQ bit was checked by bsep@chromium.org

[Bret, 2016-06-23 19:52:50]

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/2090203002/#ps80001
(title: "take the max of ClipDisplayItem and TransformDisplayItem")

[commit-bot: I haz the power, 2016-06-23 19:53:15]

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

[commit-bot: I haz the power, 2016-06-23 20:02:09]

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

[commit-bot: I haz the power, 2016-06-23 20:02:10]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Bret, 2016-06-23 20:15:40]

The CQ bit was checked by bsep@chromium.org

[Bret, 2016-06-23 20:15:40]

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/2090203002/#ps100001
(title: "std::max is bad apparently")

[commit-bot: I haz the power, 2016-06-23 20:16:08]

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

[chrishtr, 2016-06-23 20:33:40]

chrishtr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[chrishtr, 2016-06-23 20:33:41]

FYI Blink is solving the same kind of issue by snapping positions to actual
device pixels,
not DIPs, after layout (crbug.com/485650). Does that approach make sense for UI
also?

[Bret, 2016-06-23 20:48:20]

On 2016/06/23 20:33:41, chrishtr wrote:
> FYI Blink is solving the same kind of issue by snapping positions to actual
> device pixels,
> not DIPs, after layout (crbug.com/485650). Does that approach make sense for
UI
> also?

It might be worth considering, but is certainly out of scope for this patch. The
non-integral dsf issues I see in the UI are usually pretty minor anyway.

[sky, 2016-06-23 20:57:27]

I think that would be hard when the ui code uses integer dips.

  -Scott

On Thu, Jun 23, 2016 at 1:33 PM,  <chrishtr@chromium.org> wrote:
> FYI Blink is solving the same kind of issue by snapping positions to actual
> device pixels,
> not DIPs, after layout (crbug.com/485650). Does that approach make sense for
> UI
> also?
>
> https://codereview.chromium.org/2090203002/

-- 
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.

[chrishtr, 2016-06-23 21:11:02]

On 2016/06/23 at 20:57:27, sky wrote:
> I think that would be hard when the ui code uses integer dips.

Right. The UI code would have to do something like the pixel snapping that Blink
does (lay out in one coordinate space, then snap to device pixels later in some
consistent way. In our case, calling pixelSnappedIntRect().

Anyway, just wanted to point that out for future work.

> 
>   -Scott
> 
> On Thu, Jun 23, 2016 at 1:33 PM,  <chrishtr@chromium.org> wrote:
> > FYI Blink is solving the same kind of issue by snapping positions to actual
> > device pixels,
> > not DIPs, after layout (crbug.com/485650). Does that approach make sense for
> > UI
> > also?
> >
> > https://codereview.chromium.org/2090203002/
> 
> -- 
> 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.
> 
>

[commit-bot: I haz the power, 2016-06-23 21:29:08]

Description was changed from

==========
Remove antialiasing from views bounds rects.

A views object clips drawing to its rectangular bounds. Those bounds are
in DIPs, and have antialiasing turned on. At non-integral scale factors
those bounds might not be pixel-aligned. When that happens it can cause
artifacting, so it needs antialiasing turned off. Since Blink appears to
need antialiasing turned on, this patch adds a field to ClipDisplayItem
so that each callsite can do the thing it wants.

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

to

==========
Remove antialiasing from views bounds rects.

A views object clips drawing to its rectangular bounds. Those bounds are
in DIPs, and have antialiasing turned on. At non-integral scale factors
those bounds might not be pixel-aligned. When that happens it can cause
artifacting, so it needs antialiasing turned off. Since Blink appears to
need antialiasing turned on, this patch adds a field to ClipDisplayItem
so that each callsite can do the thing it wants.

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

[commit-bot: I haz the power, 2016-06-23 21:29:09]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-23 21:31:58]

Description was changed from

==========
Remove antialiasing from views bounds rects.

A views object clips drawing to its rectangular bounds. Those bounds are
in DIPs, and have antialiasing turned on. At non-integral scale factors
those bounds might not be pixel-aligned. When that happens it can cause
artifacting, so it needs antialiasing turned off. Since Blink appears to
need antialiasing turned on, this patch adds a field to ClipDisplayItem
so that each callsite can do the thing it wants.

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

to

==========
Remove antialiasing from views bounds rects.

A views object clips drawing to its rectangular bounds. Those bounds are
in DIPs, and have antialiasing turned on. At non-integral scale factors
those bounds might not be pixel-aligned. When that happens it can cause
artifacting, so it needs antialiasing turned off. Since Blink appears to
need antialiasing turned on, this patch adds a field to ClipDisplayItem
so that each callsite can do the thing it wants.

BUG=615153
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[commit-bot: I haz the power, 2016-06-23 21:31:59]

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

[brucedawson, 2016-06-23 22:23:33]

On 2016/06/23 21:31:59, commit-bot: I haz the power wrote:
> Patchset 6 (id:??) landed as
> https://crrev.com/e11a8ea32baa62619f66c2e1104786ad861e9c05
> Cr-Commit-Position: refs/heads/master@{#401726}

FWIW, once https://bugs.chromium.org/p/chromium/issues/detail?id=554717 is
resolved then std::max can be used in constexpr, and then patch set 5 would
work.